stripe / openapi

An OpenAPI specification for the Stripe API.
MIT License
394 stars 123 forks source link

v2 spec is invalid #33

Closed mutsys closed 1 year ago

mutsys commented 6 years ago

I set out to create a custom strip client using swagger-codegen and was rewarded by a HUGE list of errors and warnings. I settled in to dig through spec2.yaml to see if I could figure out what was broken and what needed to be done to fix it. In the end I was able to clean it up and successfully generate my client code. Being a rather large document, it seemed that trying to fix it manually was a losing proposition so I ended up writing some code to fix up the document by overriding the preprocessSwagger method in the base DefaultCodegen class. Fortunately, provides me with the list of issues I discovered as well as the fix for each, which I can share with you here. The issues I discovered fall into one of three categories:

  1. violations of OpenAPI v2 spec
  2. violations of JSON Schema Validation spec
  3. things that cause swagger-codegen to fail or produce poor code

This is just a quick summary of the issues I found and fixed. If you like I can provide much more detail, including the locations within the document where each of these issues can be found.

Issues

Invalid property declarations

Untyped properties

There are many property declarations that do not have a type attribute. This is valid JSON Schema, corresponding to the "any" type, but this is not valid Swagger. swagger-codegen detects the missing type attribute of properties, emits a warning message and sets it to ERROR_UNKNOWN, resulting in generated code that fails to compile. There are a total of 181 such errors.

Arrays without items

There are many array properties that are missing an items attribute. This is valid JSON Schema, which defaults to items to the empty schema ({}), which corresponds to an object with no properties. This is not valid Swagger, which states that items is a required attribute of an array property definition. There are a total of 101 such errors. 7 occur within schema definitions, 94 occur within definitions of the expand query parameter.

Arrays without type

There are a number of properties defined with have an items attribute without any type attribute. swagger-codegen sets the missing type attribute to ERROR_UNKNOWN, resulting in generated code that fails to compile. There are 6 such definitions.

Empty schemas

There are a number of schemas and properties whose type is defined as the empty schema ({}). Valid according to JSON Schema but not according to Swagger. There are 21 such definitions.

Non-unique operationId

The Swagger spec states that each value of operationId must be globally unique within each document. There are 29 instances of an operationId that duplicate an existing operationId.

Non-unique Body Parameter names

This one isn't a violation of either JSON Schema or Swagger but does present challenges for swagger-codegen. When swagger-codegen does its work, it uses the names of schemas to derive the name of the interface/class it will create. When it discovers a new schema that has the same name as an existing schema, it increments a counter and creates a new unique name by appending the counter value to the duplicate name. This results in the generation of client/server code that although valid is very confusing and difficult to use. For example, the schema for every body parameter definition is named payload and there are 90 such definitions.

Schema naming

Anonymous schemas

Once again not a violation of JSON Schema or Swagger but results in low quality generated code. When swagger-codegen encounters a schema definition without a name (ie - any schema not defined in definitions) it uses a rule to generate a unique name based on where the anonymous schema is found in the definition to use when creating its interface/class code, resulting in generated client/server code that is again confusing and difficult to use. This seems to be most prevalent in the definition of response schemas. There are 20 response schemas defined in-line which results in interface/class names such as InlineResponse2001.

Non-unique schema names

Another issue that results in low quality generated code. There a number of schemas sharing the same name. In each case, it appears to be a redefinition of the same schema. There are 20 such repeated schema definitions.

brandur-stripe commented 6 years ago

I just wanted to say for now: thank you for the detailed report here.

Unfortunately, I'm not sure when I can get around to fixing most of this, but it's good to have this level of detail of information available.

mutsys commented 6 years ago

No doubt that this would be a large effort if you were to try and and attack this by fixing code and Swagger annotations, but it isn't such a big deal if you clean it up in a post-processor like I did. I would be happy to share my preprocessSwagger method in the swagger-codegen plugin I created. It cleans everything up enough so that it passes validation and can be used with swagger-codegen. It is written in Java, of course, but it would be rather easy to translate it to the Ruby equivalent. Just tack it on to the end of your build process and you should be good to go.

brandur-stripe commented 6 years ago

@mutsys Thanks a lot for the offer!

I think you're right in that applying a post processor would be the easiest fix for our problems, but I kind of feel like we'd just be papering over real problems and adding another layer of complexity overhead to our stack. Pretty much everything here is representative of an actual issue (e.g., non-unique operation IDs), and by far the cleanest fix is one that happens right at the root where we generate the original spec.

sanderPostma commented 6 years ago

How about this one: all input parameters are defined as body parameters while Stripe wants x-www-form-urlencoded which is defined like this: ` parameters:

    - in: formData

      name: name

      type: string

      description: A person's name.

    - in: formData

      name: fav_number

      type: number

      description: A person's favorite number.

`

brandur-stripe commented 6 years ago

@sanderPostma I'm sorry to say that one is certainly not going to be changed.

The formData specification is simply not adequately expressive enough for our purposes, which often include more complex data structures. OpenAPI 3.0 does a better job of addressing this, so I'd recommend moving to that for a system that's a little more adherent to recommended practices.

I'm also not even sure this is necessarily invalid. Reading describing request body, it's suggested that formData is usually used, but the language doesn't imply that that use is necessarily exclusive.

pakrym-stripe commented 1 year ago

I apologize for the extremely delayed response.

We've since moved to OpenAPI v3 and improved the quality of API descriptions. If you encounter problems when using the new OpenAPI 3 spec please file a new issue.