Azure / autorest

OpenAPI (f.k.a Swagger) Specification code generator. Supports C#, PowerShell, Go, Java, Node.js, TypeScript, Python
MIT License
4.62k stars 739 forks source link

Null Reference Exception generating client #1391

Closed Osipion closed 7 years ago

Osipion commented 8 years ago

AutoRest fails to generate a client for the following swagger (refs point to documents on an internal server):

{
  "swagger": "2.0",
  "info": {
    "title": "Bug Example",
    "description": "A summary of a a bug",
    "version": "01.00.00"
  },
  "schemes": [
    "https"
  ],
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {
    "/a/path": {
      "get": {
        "operationId": "getAPath",
        "description": "a path",
        "responses": {
          "200": {
            "description": "A positive response",
            "schema": {
              "$ref": "#/definitions/aModel"
            }
          },
          "401": {
            "$ref": "http://internalserver/api-governance/shared/responses/401.json"
          },
          "404": {
            "$ref": "http://internalserver/api-governance/shared/responses/404.json"
          }
        }
      }
    }
  },
  "definitions": {
    "aModel": {
      "type": "object",
      "title": "someModel",
      "description": "Representation for a model",
      "properties": {
        "data": {
          "type": "object",
          "properties": {
            "aModel": {
              "type": "array",
              "items": {
                "type": "object",
                "allOf": [
                  {
                    "$ref": "#/definitions/aModelAttributes"
                  }
                ]
              }
            }
          }
        },
        "meta": {
          "type": "object",
          "description": "The metadata for the resource",
          "allOf": [
            {
              "$ref": "http://internalserver/api-governance/shared/definitions/pagination-meta.json"
            }
          ]
        }
      }
    },
    "aModelAttributes": {
      "properties": {
        "prop1": {
          "type": "string"
        }
      }
    }
  }
}

The stack trace is:

Code generation failed.
AutoRest.exe -namespace MyNamespace -CodeGenerator CSharp -Input .\example.json -verbose
error: [FATAL] Error generating client model: Object reference not set to an instance of an object.
System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.Rest.Modeler.Swagger.JsonConverters.ResponseRefConverter.ReadJson(JsonReader reader, Type objectType, Object existingValue, JsonSerializer serializer)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.DeserializeConvertable(JsonConverter converter, JsonReader reader, Type objectType, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateDictionary(IDictionary dictionary, JsonReader reader, JsonDictionaryContract contract, JsonProperty containerPrope
rty, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract contain
erContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract
containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.SetPropertyValue(JsonProperty property, JsonConverter propertyConverter, JsonContainerContract containerContract, JsonProp
erty containerProperty, JsonReader reader, Object target)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract contain
erContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract
containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateDictionary(IDictionary dictionary, JsonReader reader, JsonDictionaryContract contract, JsonProperty containerPrope
rty, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract contain
erContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract
containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
   at Newtonsoft.Json.JsonConvert.DeserializeObject[T](String value, JsonSerializerSettings settings)
   at Microsoft.Rest.Modeler.Swagger.JsonConverters.PathLevelParameterConverter.ReadJson(JsonReader reader, Type objectType, Object existingValue, JsonSerializer serializer)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.DeserializeConvertable(JsonConverter converter, JsonReader reader, Type objectType, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
   at Newtonsoft.Json.JsonConvert.DeserializeObject[T](String value, JsonSerializerSettings settings)
   at Microsoft.Rest.Modeler.Swagger.JsonConverters.PathItemRefConverter.ReadJson(JsonReader reader, Type objectType, Object existingValue, JsonSerializer serializer)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.DeserializeConvertable(JsonConverter converter, JsonReader reader, Type objectType, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateDictionary(IDictionary dictionary, JsonReader reader, JsonDictionaryContract contract, JsonProperty containerPrope
rty, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract contain
erContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract
containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.SetPropertyValue(JsonProperty property, JsonConverter propertyConverter, JsonContainerContract containerContract, JsonProp
erty containerProperty, JsonReader reader, Object target)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract contain
erContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract
containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
   at Newtonsoft.Json.JsonConvert.DeserializeObject[T](String value, JsonSerializerSettings settings)
   at Microsoft.Rest.Modeler.Swagger.SwaggerParser.Parse(String swaggerDocument)
   at Microsoft.Rest.Modeler.Swagger.SwaggerParser.Load(String path, IFileSystem fileSystem)
   at Microsoft.Rest.Modeler.Swagger.SwaggerModeler.Build()
   at Microsoft.Rest.Generator.AutoRest.Generate(Settings settings)
INFO: AutoRest Core 0.16.0.0
INFO: Initializing modeler.
INFO: Initializing modeler.
INFO: Parsing swagger json file.

If the error response definitions are removed (like so):

{
  "swagger": "2.0",
  "info": {
    "title": "Bug Example",
    "description": "A summary of a a bug",
    "version": "01.00.00"
  },
  "schemes": [
    "https"
  ],
  "consumes": [
    "application/json"
  ],
  "produces": [
    "application/json"
  ],
  "paths": {
    "/a/path": {
      "get": {
        "operationId": "getAPath",
        "description": "a path",
        "responses": {
          "200": {
            "description": "A positive response",
            "schema": {
              "$ref": "#/definitions/aModel"
            }
          }
        }
      }
    }
  },
  "definitions": {
    "aModel": {
      "type": "object",
      "title": "someModel",
      "description": "Representation for a model",
      "properties": {
        "data": {
          "type": "object",
          "properties": {
            "aModel": {
              "type": "array",
              "items": {
                "type": "object",
                "allOf": [
                  {
                    "$ref": "#/definitions/aModelAttributes"
                  }
                ]
              }
            }
          }
        },
        "meta": {
          "type": "object",
          "description": "The metadata for the resource",
          "allOf": [
            {
              "$ref": "http://internalserver/api-governance/shared/definitions/pagination-meta.json"
            }
          ]
        }
      }
    },
    "aModelAttributes": {
      "properties": {
        "prop1": {
          "type": "string"
        }
      }
    }
  }
}

Then I get:

Code generation failed.
AutoRest.exe -namespace MyNamespace -CodeGenerator CSharp -Input .\example.json -verbose
error: [FATAL] Error generating client model: The given key was not present in the dictionary.
System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Microsoft.Rest.Modeler.Swagger.SwaggerModeler.Build()
   at Microsoft.Rest.Generator.AutoRest.Generate(Settings settings)
INFO: AutoRest Core 0.16.0.0
INFO: Initializing modeler.
INFO: Initializing modeler.
INFO: Parsing swagger json file.
INFO: Generating client model from swagger model.

If I remove any refs that point to external documents, then the client object model is generated successfully.

amarzavery commented 8 years ago

One thing that caught my attention. "Every response should have a schema as per the description in the swagger spec over here". In the example where you are referencing a model for 401 to an internal server, you are directly referencing it. I think it should be inside a schema, the way 200 is defined.

"401": {
    "schema": {
                  "$ref": "http://internalserver/api-governance/shared/responses/401.json"
               }
         }
amarzavery commented 8 years ago

I would also recommend, reading this section of the documentation that talks about default responses and processing negative responses.

In essence, the generated would not throw an exception for 401 and 404 but would deserialize as per the schema defined for that response. However, the return type of the method would be the common ancestor of all the deserializable responses (200, 401, 404) which in most cases would turn out to be an Object. (just wanted to let you know).

Osipion commented 8 years ago

@amarzavery thanks for the response. I will try moving the refs inside a schema node when I'm back on site tomorrow. The behaviour described in your second comment is desired (strongly typed would be nice, but we can make do :-)) - we want to use AutoRest to generate test clients. Thanks for the tip though.

amarzavery commented 8 years ago

For example for an operation Members_Get, in 200 you return a model named "Member" and you described 401 with a schema that returns a model named "FooError". So in your definitions section you will have these model definitions. If they share a common parent then that would be the return type of the method. However, in most cases I don't see that these model types (Member and FooError) would have a common parent. However in languages like C#, Java, javascript all objects inherit from "Object". Hence the return type in most cases would end up being an Object. I am not sure if you would want that as your method signature in strongly typed languages.

Osipion commented 8 years ago

Ah OK - apologies for deleting the question, I realised it was answered in the document you linked to. Defining a default schema in the swagger would allow for a common base type, and that base type would be the return type of the api methods? Otherwise they'd probably inherit from System.Object.

Osipion commented 8 years ago

@amarzavery I've taken a look at the response references in more detail. The OpenAPI spec says that if the schema node "does not exist [in the ResponseObject], it means no content is returned as part of the response." In actual fact, the $ref doesn't point to a schema for the response, but to the ResponseObject definition itself:

{
  "description": "A response"
}

This appears to be valid according to the json reference spec, and the swagger editor is OK with it.

amarzavery commented 8 years ago

In actual fact, the $ref doesn't point to a schema for the response, but to the ResponseObject definition itself

In that case, remove the $ref as it makes no sense. Just provide the description.

tbombach commented 8 years ago

Hi @Osipion. Can this issue be closed?

Osipion commented 8 years ago

Hi @tbombach - we'll, I've worked around the issue by first processing the spec with the npm package json-refs, which resolves the remote json fragments. We use these fragments because we support a different API for each product domain, but share error definitions across them. It's up to the contributors as to whether this behaviour is a bug or by design, but a better exception message would be nice :-).

olydis commented 7 years ago

Note that there is a difference between the (very liberal) JSON refs specification and Swagger, which has stricter rules for where refs are allowed and what they may point at. As far as I can derive from the discussion above, the issue encountered was due to this discrepancy.

I will close this issue, but feel free to post a (minimal) example of both main and referenced Swagger file that adheres to the Swagger spec but crashes AutoRest and I will gladly reopen. :)

Osipion commented 7 years ago

@olydis Looks like $refs can be used other than to reference schemas although I don't know if that includes my example. The spec seems a little confusing on where $ref may be used - from what I can work out:

  1. To point to a definition schema
  2. Within a definition schema to point to a sub schema
  3. To point to a Path Object definition
  4. "To reference parameters and responses that are defined at the top level" - not sure what "top level" means here, but sounds like I can use $ref to point to a response definition, which was my problem.

The spec also says:

parts of the definitions can be split into separate files, at the discretion of the user. This is applicable for $ref fields in the specification as follows from the JSON Schema definitions

Which suggests a relatively arbitrary use of JSON refs is supported, and the official swagger editor allows it.

I guess the problem would be that it's difficult to work out what the error is when AutoRest rejects it since other swagger tooling doesn't highlight it. Would a better exception message be possible?

olydis commented 7 years ago

Absolutely correct. Looking at your examples again, it seems I was drawing the wrong conclusions from the discussion... your Swagger is perfectly fine 👍 but using the current AutoRest, both of your examples work for me (after pointing the external URLs to some files/URLs that are accessible)!

Do you have a counterexample (with URLs that work for me - or post the referenced swagger)? :)