Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add treat default type as error #1654

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/developer/guide/defining-clients-swagger.md
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ Properties in SwaggerSchema do not contain a required field. Instead, Each defin
```

### Error Modeling
At runtime, if the server returns an unexpected status code, the generated client throws an exception of type `HttpOperationException`. The exception instance will contain the request of type `HttpRequestMessage` (in property `Request`), the response of type `HttpResponseMessage` (in property `Response`), and the error model (in property `Body`). The error model must be defined as the schema of the `default` response.
At runtime, if the server returns an unexpected status code, the generated client throws an exception of type `HttpOperationException`. The exception instance will contain the request of type `HttpRequestMessage` (in property `Request`), the response of type `HttpResponseMessage` (in property `Response`), and the error model (in property `Body`). The error model must be defined as the schema of the `default` response. You may specify any number of responses that return the error model, and they will all be treated as errors and throw.

**Example:**
A response of
Expand Down
189 changes: 189 additions & 0 deletions src/modeler/AutoRest.Swagger.Tests/Swagger/swagger-error-ignore.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
{
"swagger": "2.0",
"info": {
"title": "Microsoft Azure Redis Cache Management API",
"description": "Some cool documentation.",
"version": "2014-04-01-preview"
},
"host": "management.azure.com",
"schemes": [
"https"
],
"basePath": "/",
"produces": [ "application/json" ],
"consumes": [ "application/json" ],
"paths": {
"/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/Microsoft.Cache/Redis?api-version={apiVersion}": {
"get": {
"operationId": "list",
"summary": "Product Types",
"description": "The Products endpoint returns information about the Uber products offered at a given location. The response includes the display name and other details about each product, and lists the products in the proper display order.",
"parameters": [
{
"$ref": "#/parameters/SubscriptionIdParamterer"
},
{
"name": "resourceGroupName",
"in": "path",
"description": "Resource Group ID.",
"required": true,
"type": "string"
},
{
"$ref": "ApiVersionParameter"
}
],
"tags": [
"Redis"
],
"responses": {
"200": {
"description": "A list of caches",
"schema": {
"$ref": "Product"
}
},
"410": {
"description": "This product has been removed",
"schema": {
"$ref": "Error"
}
},
"default": {
"description": "Unexpected error",
"schema": {
"$ref": "Error"
}
}
}
},
"post": {
"operationId": "reset",
"summary": "Resets products",
"description": "Resets products.",
"parameters": [
{
"name": "subscriptionId",
"in": "path",
"description": "Subscription ID.",
"required": true,
"type": "string"
},
{
"name": "resourceGroupName",
"in": "path",
"description": "Resource Group ID.",
"required": true,
"type": "string"
},
{
"name": "apiVersion",
"in": "path",
"description": "API ID.",
"required": true,
"type": "string"
}
],
"tags": [
"Redis"
],
"responses": {
"204": {
"description": "A list of caches",
"examples": {
"application/json": {
"id": 9,
"category": {
"name": "domestic"
},
"name": "monster",
"tags": [
{
"name": "for sale"
}
],
"status": "alive"
}
}
},
"403": {
"description": "User is forbidden to reset the products.",
"schema": {
"$ref": "Error"
}
},
"default": {
"description": "Unexpected error",
"schema": {
"$ref": "Error"
}
}
}
}
}
},
"definitions": {
"Product": {
"title": "The product title.",
"description": "The product documentation.",
"properties": {
"product_id": {
"type": "string",
"title": "A product id.",
"description": "Unique identifier representing a specific product for a given latitude & longitude. For example, uberX in San Francisco will have a different product_id than uberX in Los Angeles."
},
"description": {
"type": "string",
"description": "Description of product."
},
"display_name": {
"type": "string",
"description": "Display name of product."
},
"capacity": {
"type": "string",
"description": "Capacity of product. For example, 4 people.",
"default": "100"
},
"image": {
"type": "string",
"description": "Image URL representing the product."
}
},
"example": {
"name": "Puma",
"id": 1
}
},
"Error": {
"properties": {
"code": {
"type": "integer",
"format": "int32"
},
"message": {
"type": "string"
},
"fields": {
"type": "string"
}
}
}
},
"parameters": {
"SubscriptionIdParamterer": {
"name": "subscriptionId",
"in": "path",
"description": "Subscription ID.",
"required": true,
"type": "string"
},
"ApiVersionParameter": {
"name": "apiVersion",
"in": "path",
"description": "API ID.",
"required": true,
"type": "string"
}
}
}
70 changes: 70 additions & 0 deletions src/modeler/AutoRest.Swagger.Tests/SwaggerModelerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -908,5 +908,75 @@ public void TestAdditionalProperties()
Assert.True(dictionaryProperty.SupportsAdditionalProperties);
}
}

[Fact]
public void TestTreatErrorAsDefault()
{
using (NewContext)
{
new Settings {
Namespace = "Test",
Input = Path.Combine("Swagger", "swagger-error-ignore.json")
};
Modeler modeler = new SwaggerModeler();
var codeModel = modeler.Build();

var description =
"The Products endpoint returns information about the Uber products offered at a given location. The response includes the display name and other details about each product, and lists the products in the proper display order.";
var summary = "Product Types";

Assert.NotNull(codeModel);
Assert.Equal(2, codeModel.Properties.Count);
Assert.True(
codeModel.Properties.Any(p => p.Name.EqualsIgnoreCase("subscriptionId")));
Assert.True(
codeModel.Properties.Any(p => p.Name.EqualsIgnoreCase("apiVersion")));
Assert.Equal("2014-04-01-preview", codeModel.ApiVersion);
Assert.Equal("https://management.azure.com/", codeModel.BaseUrl);
Assert.Equal("Some cool documentation.", codeModel.Documentation);
//var allMethods = codeModel.Operations.SelectMany(each => each.Methods);
Assert.Equal(2, codeModel.Methods.Count);
Assert.Equal("List", codeModel.Methods[0].Name);
Assert.NotEmpty(codeModel.Methods[0].Description);
Assert.Equal(description, codeModel.Methods[0].Description);
Assert.NotEmpty(codeModel.Methods[0].Summary);
Assert.Equal(summary, codeModel.Methods[0].Summary);
Assert.Equal(HttpMethod.Get, codeModel.Methods[0].HttpMethod);
Assert.Equal(3, codeModel.Methods[0].Parameters.Count);
Assert.Equal("subscriptionId", codeModel.Methods[0].Parameters[0].Name);
Assert.NotNull(codeModel.Methods[0].Parameters[0].ClientProperty);
Assert.Equal("resourceGroupName", codeModel.Methods[0].Parameters[1].Name);
Assert.Equal("resourceGroupName", codeModel.Methods[0].Parameters[1].SerializedName);
Assert.Equal("Resource Group ID.", codeModel.Methods[0].Parameters[1].Documentation);
Assert.Equal(true, codeModel.Methods[0].Parameters[0].IsRequired);
Assert.Equal(ParameterLocation.Path, codeModel.Methods[0].Parameters[0].Location);
Assert.Equal("String", codeModel.Methods[0].Parameters[0].ModelType.ToString());
Assert.Equal("Reset", codeModel.Methods[1].Name);
Assert.Equal("Product", codeModel.ModelTypes.First(m => m.Name == "Product").Name);
Assert.Equal("Product", codeModel.ModelTypes.First(m => m.Name == "Product").SerializedName);
Assert.Equal("The product title.", codeModel.ModelTypes.First(m => m.Name == "Product").Summary);
Assert.Equal("The product documentation.",
codeModel.ModelTypes.First(m => m.Name == "Product").Documentation);
Assert.Equal("A product id.",
codeModel.ModelTypes.First(m => m.Name == "Product").Properties[0].Summary);
Assert.Equal("ProductId", codeModel.ModelTypes.First(m => m.Name == "Product").Properties[0].Name);
Assert.Equal("product_id",
codeModel.ModelTypes.First(m => m.Name == "Product").Properties[0].SerializedName);
Assert.Null(codeModel.Methods[1].ReturnType.Body);
Assert.Null(codeModel.Methods[1].Responses[HttpStatusCode.NoContent].Body);
Assert.Equal(3, codeModel.Methods[1].Parameters.Count);
Assert.Equal("subscriptionId", codeModel.Methods[1].Parameters[0].Name);
Assert.Null(codeModel.Methods[1].Parameters[0].ClientProperty);
Assert.Equal("resourceGroupName", codeModel.Methods[1].Parameters[1].Name);
Assert.Equal("apiVersion", codeModel.Methods[1].Parameters[2].Name);

Assert.Equal("Capacity", codeModel.ModelTypes.First(m => m.Name == "Product").Properties[3].Name);
Assert.Equal("100", codeModel.ModelTypes.First(m => m.Name == "Product").Properties[3].DefaultValue);

Assert.DoesNotContain(HttpStatusCode.Gone, codeModel.Methods[0].Responses.Keys);
Assert.DoesNotContain(HttpStatusCode.Forbidden, codeModel.Methods[1].Responses.Keys);

}
}
}
}
57 changes: 38 additions & 19 deletions src/modeler/AutoRest.Swagger/OperationBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@
using System.Linq;
using System.Net;
using System.Text;
using AutoRest.Core.Model;
using AutoRest.Core.Logging;
using AutoRest.Core.Model;
using AutoRest.Core.Utilities;
using AutoRest.Swagger.Model;
using AutoRest.Swagger.Properties;
using ParameterLocation = AutoRest.Swagger.Model.ParameterLocation;

using static AutoRest.Core.Utilities.DependencyInjection;
using ParameterLocation = AutoRest.Swagger.Model.ParameterLocation;

namespace AutoRest.Swagger
{
Expand Down Expand Up @@ -51,14 +50,13 @@ public Method BuildMethod(HttpMethod httpMethod, string url, string methodName,
{
EnsureUniqueMethodName(methodName, methodGroup);

var method = New<Method>(new
{
var method = New<Method>(new {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While personally, I'd prefer using the 1TBS, the rest of the project uses the traditional ANSI-style braces, and I'd prefer to keep code changes in a single brace format.

If you could tweak the braces to be consistent that'd be great.

HttpMethod = httpMethod,
Url = url,
Name = methodName,
SerializedName = _operation.OperationId
});

method.RequestContentType = _effectiveConsumes.FirstOrDefault() ?? APP_JSON_MIME;
string produce = _effectiveConsumes.FirstOrDefault(s => s.StartsWith(APP_JSON_MIME, StringComparison.OrdinalIgnoreCase));
if (!string.IsNullOrEmpty(produce))
Expand Down Expand Up @@ -96,16 +94,12 @@ public Method BuildMethod(HttpMethod httpMethod, string url, string methodName,

var headerTypeName = string.Format(CultureInfo.InvariantCulture,
"{0}-{1}-Headers", methodGroup, methodName).Trim('-');
var headerType = New<CompositeType>(headerTypeName,new
{
var headerType = New<CompositeType>(headerTypeName, new {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to ANSI-style Braces

SerializedName = headerTypeName,
Documentation = string.Format(CultureInfo.InvariantCulture, "Defines headers for {0} operation.", methodName)
});
responseHeaders.ForEach(h =>
{

var property = New<Property>(new
{
responseHeaders.ForEach(h => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to ANSI-style Braces

var property = New<Property>(new {
Name = h.Key,
SerializedName = h.Key,
ModelType = h.Value.GetBuilder(this._swaggerModeler).BuildServiceType(h.Key),
Expand Down Expand Up @@ -142,8 +136,7 @@ public Method BuildMethod(HttpMethod httpMethod, string url, string methodName,
private static IEnumerable<SwaggerParameter> DeduplicateParameters(IEnumerable<SwaggerParameter> parameters)
{
return parameters
.Select(s =>
{
.Select(s => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to ANSI-style Braces

// if parameter with the same name exists in Body and Path/Query then we need to give it a unique name
if (s.In == ParameterLocation.Body)
{
Expand Down Expand Up @@ -222,10 +215,37 @@ private List<Stack<IModelType>> BuildResponses(Method method, CompositeType head
}
}
}

RemoveResponsesWithDefaultType(method, typesList);
return typesList;
}

private void RemoveResponsesWithDefaultType(Method method, List<Stack<IModelType>> typesList)
{
var errorModelType = method.DefaultResponse.Body as CompositeType;
if (errorModelType == null || method.Responses == null) return;
var keys = method.Responses.Keys;
var removeResponses = new List<HttpStatusCode>();
foreach (var key in keys)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response types for error conditions should be related (ie, default uses #/definitions/SomeError and all the other error responses should use something that either returns that type, or has an allOf: "#/definitions/SomeError" in it's type declaration.)

That way we can have very specific types for errors, and makes it very clear what's an acceptable error.

Of course that means we have to do three other things:

  • make sure that use of the allOf doesn't force type-inheritance, it's possible that we should be able to have two error types that bubble up to the same swagger model type (that doesn't even have any properties), but still generate individual classes without a hierarchy.

  • change the generator to throw the correct exception type, rather than living with the default class

  • and add method documentation for known possible exceptions

{
//Names have been disambiguated by this point.
if (method.Responses[key] == null || method.Responses[key].Body == null)
{
continue;
}
if (method.Responses[key].Body.Name == errorModelType.Name)
{
removeResponses.Add(key);
}
}

foreach (var key in removeResponses)
{
method.Responses.Remove(key);
}

typesList.RemoveAll(foo => foo.Peek().Name.Equals(errorModelType.Name));
}

private Response BuildMethodReturnType(List<Stack<IModelType>> types, IModelType headerType)
{
IModelType baseType = New<PrimaryType>(KnownPrimaryType.Object);
Expand All @@ -241,8 +261,7 @@ private Response BuildMethodReturnType(List<Stack<IModelType>> types, IModelType
}

// BuildParameter up type inheritance tree
types.ForEach(typeStack =>
{
types.ForEach(typeStack => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ANSI Braces Please.

IModelType type = typeStack.Peek();
while (!Equals(type, baseType))
{
Expand Down Expand Up @@ -445,4 +464,4 @@ private static string GenerateErrorModelName(string methodName)
"{0}ErrorModel", methodName);
}
}
}
}