Add option to model implicit default response #229
Conversation
|
Will update with new reference to Sway after https://github.com/amarzavery/sway/pull/25 gets merged. |
lib/util/utils.js
Outdated
| */ | ||
| exports.CloudError = { | ||
| description: "Error response describing why the operation failed.", | ||
| schema: { |
There was a problem hiding this comment.
I see what you are trying to do. However if possible do not nest properties. That will make sure that when we use oneOf for nullable types we do not do anything drastic.
There was a problem hiding this comment.
not sure what you mean. Also here would prefer to not have nullable types since there is no reason for that.
lib/validators/specResolver.js
Outdated
|
|
||
| // going through responses | ||
| if (operation.responses && !operation.responses.default) { | ||
| operation.responses.default = utils.CloudError; |
There was a problem hiding this comment.
operation.responses.default = utils.CloudErrorSchema;
if (!spec.definitions.CloudError) {
spec.definitions.CloudErrorWrapper = utils.CloudErrorWrapper;
spec.definitions.CloudError = utils.CloudError;
}There was a problem hiding this comment.
@amarzavery so you prefer to define the cloud error in the definitions and then just make a ref to it ? that works I think
lib/util/utils.js
Outdated
| /** | ||
| * Models an ARM cloud error. | ||
| */ | ||
| exports.CloudError = { |
There was a problem hiding this comment.
exports.CloudErrorSchema = {
description: "Error response describing why the operation failed.",
schema: {
"$ref": "#/definitions/CloudErrorWrapper"
}
};
exports.CloudErrorWrapper = {
"type": "object",
properties: {
error: {
"$ref": "#/definitions/CloudError"
}
},
additionalProperties: false
};
exports.CloudError = {
"type": "object",
properties: {
code: {
type: "string",
description: "An identifier for the error. Codes are invariant and are intended to be consumed programmatically."
},
message: {
type: "string",
description: "A message describing the error, intended to be suitable for display in a user interface."
},
target: {
type: "string",
description: "The target of the particular error. For example, the name of the property in error."
},
details: {
type: "array",
items: { type: "object" },
description: "A list of additional details about the error."
},
additionalInfo: {
type: "array",
items: { type: "object" },
description: "A list of additional info about an error."
}
},
additionalProperties: false
};There was a problem hiding this comment.
This should keep things in sync with our current design. Do not forget "type": "object" in the model definitions for CloudErrorWrapper and CloudError. Now you know the consequences of missing "type" on a model definitions. Couple of questions:
- CloudErrorWrapper has only one property "error". Shouldn't that be a required property?
- Are there any required properties in
"CloudError"model? I think at minimum there should be code and message. Otherwise what is the point of the error object.
There was a problem hiding this comment.
https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/common-api-details.md#error-response-content - code and message are required.
There was a problem hiding this comment.
thanks for the link, I was looking in the specs repo and those were not marked as required. Btw should we live the innerError and add the new additionalInfo ? (If you saw the email from Gaurav)
There was a problem hiding this comment.
Yes, I saw that email. I think we should have CloudErrorV1 and CloudErrorV2. Looks like some teams will be providing innerErrors for the already deployed api-versions. We can make our CloudError object a oneOf [V1, V2]. If we do that then we should make sure to not add CloudError during semantic validation, since oneOf is not supported in 2.0 version of the swagger spec.
There was a problem hiding this comment.
well are we sure those are required ?either way this change is not intended to catch invalid cloud errors, is mostly to avoid noise from those and potentially catch any other non-described errors. So maybe I would just leave both innerError and additionalInfo. We can refine in the future
5d9b066 to
1530142
Compare
| items: { type: "object" }, | ||
| description: "A list of additional details about the error." | ||
| }, | ||
| additionalInfo: { |
There was a problem hiding this comment.
wanna add innerError as well?
There was a problem hiding this comment.
yes just that the spec is a little confusing. It says the type is string but it seems to be an object. I'll just leave it without specifying any type.
There was a problem hiding this comment.
ah nevermind, it seems to be always an object
Fixes #224
Fixes #228