Treat all 2xx status codes as success

[peombwa]

The generated commands should treat all 2xx HTTP status codes as success, or we should provide an option to specify this behavior via a configuration. With Microsoft Graph API, treating all 2xx status codes as success is generally recommended since some APIs can return any status code in the 2xx range to mean success. e.g., Security APIs can return 200 or 206 depending on response from the provider.

The OpenAPI files may not always contain all status codes in the response object.

Potential solution

Instead of handling each status code in a switch, we should check if the response message IsSuccessStatusCode and call an onSuccess callback method.

Instead of this (before)

switch (_response.StatusCode)
{
    case global::System.Net.HttpStatusCode.OK:
        {
            await eventListener.Signal(Microsoft.Graph.PowerShell.Runtime.Events.BeforeResponseDispatch, _response); if (eventListener.Token.IsCancellationRequested) { return; }
            await onOk(_response, _response.Content.ReadAsStringAsync().ContinueWith(body => Microsoft.Graph.PowerShell.Models.CollectionOfAlert.FromJson(Microsoft.Graph.PowerShell.Runtime.Json.JsonNode.Parse(body.Result))));
            break;
        }
    default:
        {
            await eventListener.Signal(Microsoft.Graph.PowerShell.Runtime.Events.BeforeResponseDispatch, _response); if (eventListener.Token.IsCancellationRequested) { return; }
            await onDefault(_response, _response.Content.ReadAsStringAsync().ContinueWith(body => Microsoft.Graph.PowerShell.Models.OdataError.FromJson(Microsoft.Graph.PowerShell.Runtime.Json.JsonNode.Parse(body.Result))));
            break;
        }
}

Lets to do this (Suggested change)

if (_response.IsSuccessStatusCode)
{
    await eventListener.Signal(Microsoft.Graph.PowerShell.Runtime.Events.BeforeResponseDispatch, _response); if (eventListener.Token.IsCancellationRequested) { return; }
    // We can then handle specific status code in the onSuccess call back if need be.
    // We should also deserialize the content based on the response schema of a 2xx status code in the OpenAPI file. 
    await onSuccess(_response, _response.Content.ReadAsStringAsync().ContinueWith(body => Microsoft.Graph.PowerShell.Models.CollectionOfAlert.FromJson(Microsoft.Graph.PowerShell.Runtime.Json.JsonNode.Parse(body.Result))));
} else
{
    await eventListener.Signal(Microsoft.Graph.PowerShell.Runtime.Events.BeforeResponseDispatch, _response); if (eventListener.Token.IsCancellationRequested) { return; }
    await onDefault(_response, _response.Content.ReadAsStringAsync().ContinueWith(body => Microsoft.Graph.PowerShell.Models.OdataError.FromJson(Microsoft.Graph.PowerShell.Runtime.Json.JsonNode.Parse(body.Result))));
}

[dolauli]

We have handled different 2XX if you have defined them in swagger.
For example, if you define following responses as

        "responses": {
          "200": {
            "description": "OK - Returns the workspace.",
            "schema": {
              "$ref": "#/definitions/Workspace"
            }
          },
          "206": {
            "description": "OK - Returns the workspace.",
            "schema": {
              "$ref": "#/definitions/Workspace"
            }
          },
          "204": {
            "description": "OK - Returns the workspace."
          },
          "default": {
            "description": "Error response describing why the operation failed.",
            "schema": {
              "$ref": "#/definitions/ErrorResponse"
            }
          }

We will generate code as below.

                    switch ( _response.StatusCode )
                    {
                        case global::System.Net.HttpStatusCode.OK:
                        {
                            await eventListener.Signal(Microsoft.Azure.PowerShell.Cmdlets.Databricks.Runtime.Events.BeforeResponseDispatch, _response); if( eventListener.Token.IsCancellationRequested ) { return; }
                            await onOk(_response,_response.Content.ReadAsStringAsync().ContinueWith( body => Microsoft.Azure.PowerShell.Cmdlets.Databricks.Models.Api20180401.Workspace.FromJson(Microsoft.Azure.PowerShell.Cmdlets.Databricks.Runtime.Json.JsonNode.Parse(body.Result)) ));
                            break;
                        }
                        case global::System.Net.HttpStatusCode.NoContent:
                        {
                            await eventListener.Signal(Microsoft.Azure.PowerShell.Cmdlets.Databricks.Runtime.Events.BeforeResponseDispatch, _response); if( eventListener.Token.IsCancellationRequested ) { return; }
                            await onNoContent(_response);
                            break;
                        }
                        case global::System.Net.HttpStatusCode.PartialContent:
                        {
                            await eventListener.Signal(Microsoft.Azure.PowerShell.Cmdlets.Databricks.Runtime.Events.BeforeResponseDispatch, _response); if( eventListener.Token.IsCancellationRequested ) { return; }
                            await onPartialContent(_response,_response.Content.ReadAsStringAsync().ContinueWith( body => Microsoft.Azure.PowerShell.Cmdlets.Databricks.Models.Api20180401.Workspace.FromJson(Microsoft.Azure.PowerShell.Cmdlets.Databricks.Runtime.Json.JsonNode.Parse(body.Result)) ));
                            break;
                        }
                        default:
                        {
                            await eventListener.Signal(Microsoft.Azure.PowerShell.Cmdlets.Databricks.Runtime.Events.BeforeResponseDispatch, _response); if( eventListener.Token.IsCancellationRequested ) { return; }
                            await onDefault(_response,_response.Content.ReadAsStringAsync().ContinueWith( body => Microsoft.Azure.PowerShell.Cmdlets.Databricks.Models.Api20180401.ErrorResponse.FromJson(Microsoft.Azure.PowerShell.Cmdlets.Databricks.Runtime.Json.JsonNode.Parse(body.Result)) ));
                            break;
                        }
                    }

[peombwa]

This assumes that all status codes will be present in the OpenAPI file. Here is what the spec says about this:

Responses Object
A container for the expected responses of an operation. The container maps a HTTP response code to the expected response.

The documentation is not necessarily expected to cover all possible HTTP response codes because they may not be known in advance. However, documentation is expected to cover a successful operation response and any known errors.

The default MAY be used as a default response object for all HTTP codes that are not covered individually by the specification.

The Responses Object MUST contain at least one response code, and it SHOULD be the response for a successful operation call.

Ref: https://spec.openapis.org/oas/latest.html#responses-object

cc @darrelmiller

[darrelmiller]

The OpenAPI specification is in aligned with the HTTP specification which says:

HTTP clients are not required to
understand the meaning of all registered status codes, though such
understanding is obviously desirable. However, a client MUST
understand the class of any status code, as indicated by the first
digit, and treat an unrecognized status code as being equivalent to
the x00 status code of that class, with the exception that a
recipient MUST NOT cache a response with an unrecognized status code.

https://datatracker.ietf.org/doc/html/rfc7231#section-6

To paraphrase, clients that make HTTP calls MUST never fail because they get back an unexpected status code.

[dolauli]

@peombwa @darrelmiller Share some of my points.

• For 4xx responses, I agree that we should not list all of them in the swagger. And errors normally share the same schema of response, and a good place for them is the default. But for 2xx responses, I do think we should list all of them in the schema. And there are two reasons for this.
  • Firstly, from service side, I do not think there should be some unknown 2xx responses in advance.
  • Secondly, Different responses of 2xx normally do not share the same schema of response. For your case, if 206 is not defined in swagger, How could we generate response model for 206, since there is no response schema defined for 206.

[darrelmiller]

@dolauli The HTTP specification is very clear on what a client MUST do. The OpenAPI specification is in agreement.

201 and 200 can often return the same schema. The content-length header will tell the client if there is a payload for the 201. If there is the shape will likely be the same as 200. 203 is not something an API provider may know about and therefore would not likely ever be listed in the OpenAPI description. 204 has a well defined empty response payload which a client can return a null value. Same for 205.

In this case of a 206 the same response payload for 200 will work just fine. 207 responses are likely going to be the same shape as a 200, the only difference being the inclusion of some error messages in the payload. I've never seen anyone use 208 or 226.

Failing because of an unexpected status code is wrong according to HTTP and OpenAPI.

[dolauli]

@darrelmiller
Are there any special reasons that you can not list all 2xx responses in swagger for msgraph. I can change the logic to not return failures in unlisted 2xx. And this will cause issues.

1. What should I return for unlisted 2xx response? Currently we will define a C# class for each response based on schema defined in response. I can not assume what 206 returns follow the same schema with 200 even in most cases it could true. So the key point here is that I need schema of all the response.
2. Another case is for 204, if we found that 204 is in the response, we will add a parameter called -passthu for generated cmdlet. With this switch parameter turned on, we will return true to users instead of nothing when 204 is returned. Without 204 defined in swagger, how could we know whether we should add this parameter when generate the code.

[darrelmiller]

Are there any special reasons that you can not list all 2xx responses in swagger for msgraph.

Yes. Our APIs are described using CSDL. There is no way in CSDL to explicitly list which 2XX status codes will be returned. Exhaustively listing all the status codes that are returned today is bad practice as it encourages clients to couple to these values which prevents a service from returning additional status codes in the future. The reason AutoREST PowerShell is running into this problem is because it made the broken assumption that status codes are listed exhaustively.

This issue was discussed and finalized six years ago. OAI/OpenAPI-Specification#568

What should I return for unlisted 2xx response?

The meaning of 2XX status codes are well defined in the HTTP specification. Generally the behavior isn't specific to the particular resource.

So the key point here is that I need schema of all the response.

If there is a JSON response payload and it is a 2XX then the odds are pretty high that the payload is the shape described in the 200 status code. This is why in OpenAPI 3.0 we explicitly introduced the '2XX' status code option.

Another case is for 204, if we found that 204 is in the response, we will add a parameter called -passthu for generated cmdlet.

I don't really understand what the -passthru parameter does, but if the user of AutoREST wants the passthru parameter to appear then they can choose to explicitly list 204 in the OpenAPI description.

[dolauli]

@darrelmiller I can not infer that all 2XX response follow the same response schema with 200 unless it is explicitly defined in swagger. And following is a sample response I copied form msgraph users.yml (https://github.com/microsoftgraph/msgraph-sdk-powershell/blob/dev/openApiDocs/v1.0/Users.yml#L369). And according to openapi specification, if an unlisted response like 206 is returned, it will fall into default which is for the undeclared response, whose schema refers to an error. That's is why we return an error for 206 response.

      responses:
        '200':
          description: Retrieved entities
          content:
            application/json:
              schema:
                title: Collection of user
                type: object
                properties:
                  value:
                    type: array
                    items:
                      $ref: '#/components/schemas/microsoft.graph.user'
                  '@odata.nextLink':
                    type: string
                additionalProperties:
                  type: object
        default:
          $ref: '#/components/responses/error'

I partially agree with you that we do not need to list all response status in swagger e.g. 429, 202. But for response with a meaningful response payload to end users, I do think we should explicitly declared them with the schema.
Back to your case, if you do not want explicitly 206. Another choice is to use "2XX" for both 200 and 206, since these two responses share the same schema. But you will need to wait us to support range of response codes (2XX) in autorest.powershell.

[darrelmiller]

@dolauli Agreed. If we have default in our OpenAPI then you are doing the right thing. We should fix our OpenAPI. I'm assuming that even if we remove the default you would still throw an error.

We will find another way to resolve this in the meanwhile. We would really appreciate if you could add supporting 2XX to your backlog.