Skip to content

Commit

Permalink
route-helper: add responseMessages
Browse files Browse the repository at this point in the history
Add a default "success" response message, the status code is 200 or 204
depending on whether the method returns any data.

Append any error messages as specified in the `errors` property
of method's remoting metadata.

Move the description of operation's return type to the "success"
response message.

Include error message models in the API models.
  • Loading branch information
Miroslav Bajtoš committed Oct 16, 2014
1 parent 9a6bd35 commit d05dcb7
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 25 deletions.
33 changes: 26 additions & 7 deletions lib/route-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,21 +123,40 @@ var routeHelper = module.exports = {

debug('route %j', route);

var responseDoc = modelHelper.LDLPropToSwaggerDataType(returns);

// Note: Swagger Spec does not provide a way how to specify
// that the responseModel is "array of X". However,
// Swagger UI converts Arrays to the item types anyways,
// therefore it should be ok to do the same here.
var responseModel = responseDoc.type === 'array' ?
responseDoc.items.type : responseDoc.type;

var responseMessages = [{
code: route.returns && route.returns.length ? 200 : 204,
message: 'Request was successful',
responseModel: responseModel
}];

if (route.errors) {
responseMessages.push.apply(responseMessages, route.errors);
}

var apiDoc = {
path: routeHelper.convertPathFragments(route.path),
// Create the operation doc. Use `extendWithType` to add the necessary
// `items` and `format` fields.
operations: [routeHelper.extendWithType({
// Create the operation doc.
// Note that we are not calling `extendWithType`, as the response type

This comment has been minimized.

Copy link
@STRML

STRML Nov 10, 2014

Member

It appears that this is a spec violation.

From the 1.2 spec:

5.2.3 Operation Object

The Operation Object describes a single operation on a path.
...
This object includes the Data Type Fields in order to describe the return value of the operation. The type field MUST be used to link to other models.

When integrating with other tools (in this case, Restunited), I've seen breakage due to the missing type parameter.

It seems strange to use responseMessages for 200s. Isn't it just meant for errors?

This comment has been minimized.

Copy link
@STRML

STRML Nov 10, 2014

Member

For example, from the petstore example, the output is:

{
  "path": "/pet/findByStatus",
  "operations": [{
    "method": "GET",
    "summary": "Finds Pets by status",
    "notes": "Multiple status values can be provided with comma seperated strings",
    "type": "array",
    "items": {
      "$ref": "Pet"
    },
    "nickname": "findPetsByStatus",
    "authorizations": {},
    "parameters": [{
      "name": "status",
      "description": "Status values that need to be considered for filter",
      "defaultValue": "available",
      "required": true,
      "type": "string",
      "paramType": "query",
      "allowMultiple": true,
      "enum": ["available", "pending", "sold"]
    }],
    "responseMessages": [{
      "code": 400,
      "message": "Invalid status value"
    }]
  }]
}

This comment has been minimized.

Copy link
@bajtos

bajtos Nov 10, 2014

Member

@STRML see the discussion and screenshots in #61 for the rationale.

I don't have a strong opinion on what's the correct approach here, it seems to me that we have discovered a flaw in Swagger Spec and/or Swagger UI.

To keep the discussion actionable, how about adding a config option allowing GoDaddy to enable the behaviour they need (operation result is described in 200 response message), while keeping the old behaviour everywhere else?

This comment has been minimized.

Copy link
@STRML

STRML Nov 10, 2014

Member

Moved discussion to #75.

// is specified in the first response message.
operations: [{
method: routeHelper.convertVerb(route.verb),
// [rfeng] Swagger UI doesn't escape '.' for jQuery selector
nickname: route.method.replace(/\./g, '_'),
nickname: route.method.replace(/\./g, '_'),
parameters: accepts,
// TODO(schoon) - We don't have descriptions for this yet.
responseMessages: [],
responseMessages: responseMessages,
summary: typeConverter.convertText(route.description),
notes: typeConverter.convertText(route.notes),
deprecated: route.deprecated
}, returns)]
}]
};

return apiDoc;
Expand Down
4 changes: 3 additions & 1 deletion lib/swagger.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ function Swagger(loopbackApplication, swaggerApp, opts) {

addTypeToModels(routeDoc.type);

// TODO(bajtos) handle types used by responseMessages
routeDoc.responseMessages.forEach(function(msg) {
addTypeToModels(msg.responseModel);
});

function addTypeToModels(name) {
if (!name || name === 'void') return;
Expand Down
65 changes: 48 additions & 17 deletions test/route-helper.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ describe('route-helper', function() {
{ arg: 'avg', type: 'number' }
]
});
expect(doc.operations[0].type).to.equal('object');
expect(doc.operations[0].type).to.equal(undefined);
expect(getResponseType(doc.operations[0])).to.equal('object');
});

it('converts path params when they exist in the route name', function() {
Expand Down Expand Up @@ -60,19 +61,12 @@ describe('route-helper', function() {
]
});
var opDoc = doc.operations[0];
expect(opDoc.type).to.equal('array');
expect(opDoc.items).to.eql({type: 'customType'});
});
// Note: swagger-ui treat arrays of X the same way as object X
expect(getResponseType(opDoc)).to.equal('customType');

it('correctly converts return types (format)', function() {
var doc = createAPIDoc({
returns: [
{arg: 'data', type: 'buffer'}
]
});
var opDoc = doc.operations[0];
expect(opDoc.type).to.equal('string');
expect(opDoc.format).to.equal('byte');
// NOTE(bajtos) this would be the case if there was a single response type
// expect(opDoc.type).to.equal('array');
// expect(opDoc.items).to.eql({type: 'customType'});
});

it('includes `notes` metadata', function() {
Expand Down Expand Up @@ -151,12 +145,45 @@ describe('route-helper', function() {
.to.have.property('enum').eql([1,2,3]);
});

it('preserves `enum` returns arg metadata', function() {
it('includes the default response message with code 200', function() {
var doc = createAPIDoc({
returns: [{ name: 'arg', root: true, type: 'number', enum: [1,2,3] }]
returns: [{ name: 'result', type: 'object', root: true }]
});
expect(doc.operations[0].responseMessages).to.eql([
{
code: 200,
message: 'Request was successful',
responseModel: 'object'
}
]);
});

it('uses the response code 204 when `returns` is empty', function() {
var doc = createAPIDoc({
returns: []
});
expect(doc.operations[0].responseMessages).to.eql([
{
code: 204,
message: 'Request was successful',
responseModel: 'void'
}
]);
});

it('includes custom error response in `responseMessages`', function() {
var doc = createAPIDoc({
errors: [{
code: 422,
message: 'Validation failed',
responseModel: 'ValidationError'
}]
});
expect(doc.operations[0].responseMessages[1]).to.eql({
code: 422,
message: 'Validation failed',
responseModel: 'ValidationError'
});
expect(doc.operations[0])
.to.have.property('enum').eql([1,2,3]);
});
});

Expand All @@ -168,3 +195,7 @@ function createAPIDoc(def) {
method: 'test.get'
}));
}

function getResponseType(operationDoc) {
return operationDoc.responseMessages[0].responseModel;
}
18 changes: 18 additions & 0 deletions test/swagger.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,24 @@ describe('swagger definition', function() {
done();
});
});

it('includes `responseMessages` models', function(done) {
var app = createLoopbackAppWithModel();
loopback.createModel('ValidationError');
givenSharedMethod(app.models.Product, 'setImage', {
errors: [{
code: '422',
message: 'Validation failed',
responseModel: 'ValidationError'
}]
});
mountExplorer(app);

getAPIDeclaration(app, 'products').end(function(err, res) {
expect(Object.keys(res.body.models)).to.include('ValidationError');
done();
});
});
});

describe('Cross-origin resource sharing', function() {
Expand Down

0 comments on commit d05dcb7

Please sign in to comment.