mattpolzin / OpenAPIKit

Codable Swift OpenAPI implementation.
MIT License
281 stars 35 forks source link

Make `style` parsing more lenient to allow e.g. `FORM` to be parsed for the `form` style. #307

Closed nexon closed 11 months ago

nexon commented 11 months ago

I'm trying to load an OpenAPI specification, The piece that is giving me some issues is the following:

....
"/api/v3/cars/{carId}" : {
      "get" : {
        "tags" : [ "/api/v3" ],
        "operationId" : "getCarDetails",
        "parameters" : [ {
          "name" : "brand",
          "in" : "query",
          "style" : "FORM",
          "explode" : true,
          "schema" : {
            "type" : "array",
            "items" : {
              "type" : "integer",
              "format" : "int32"
            }
          }
        },
....

And OpenAPIKit gives me the following error:

Found neither a $ref nor a Parameter in .parameters[0] for the GET endpoint under /api/v3/cars/{carId}. Parameter could not be decoded because: Could not parse style..

mattpolzin commented 11 months ago

Interesting. I've never seen anyone capitalize the style value before. The specification (https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#style-values) doesn't say "these values must be case-sensitive," but it does use camel-casing for some of the values so all-caps seems like an odd choice.

nevertheless, I think it is worth being more accommodating around the style property so I'll implement some fallback support for different character cases.

nexon commented 11 months ago

Thanks!, I forgot to mention that this came from a conversion (swg -> oapi) using swagger-parser. (From collectionFormat to style) as well.

mattpolzin commented 11 months ago

I do plan to make OpenAPIKit more lenient with this style property, but you may be able to get yourself unblocked even more quickly than that if you play around with the version of the swagger-parser you are using. By my read, the all-caps serialization of FORM is itself a bug because I see the proper-cased strings associated with the StyleEnum in swagger-core's code: https://github.com/swagger-api/swagger-core/blob/cbb955bfe9968630f2195dd9f65816ceda59e267/modules/swagger-models/src/main/java/io/swagger/v3/oas/models/parameters/Parameter.java#L31..L50

nexon commented 11 months ago

I do plan to make OpenAPIKit more lenient with this style property, but you may be able to get yourself unblocked even more quickly than that if you play around with the version of the swagger-parser you are using. By my read, the all-caps serialization of FORM is itself a bug because I see the proper-cased strings associated with the StyleEnum in swagger-core's code: https://github.com/swagger-api/swagger-core/blob/cbb955bfe9968630f2195dd9f65816ceda59e267/modules/swagger-models/src/main/java/io/swagger/v3/oas/models/parameters/Parameter.java#L31..L50

Yeep actually I believe that adding a MixIn for the StyleEnum (or Parameter) will solve the issue for the moment.

nexon commented 11 months ago

Hey Matt I was thinking that this could be a good starter that I could collaborate with in the library. If you like I can try to tackle it and submit a PR.

If so, I was thinking on adding the init(decoder:) in the ParameterSchemaContext, so I can edit the enum. This allow me to lowercase the incoming string value

Let me know.

mattpolzin commented 11 months ago

Hey Matt I was thinking that this could be a good starter that I could collaborate with in the library

That's a lovely offer and I would take you up on it in a heartbeat, except I am no longer sure I want to implement this feature after looking into it more.

If it were just a matter of parsing e.g. "FORM" as "form" then it could be done locally in the init(decoder:) like you were suggesting, but if OpenAPIKit silently does so then I actually lose some of the strictness that I have always aimed for as a desired feature of OpenAPIKit; that is, I really want OpenAPIKit to fail to parse if a document is invalid rather than letting the author of the OpenAPI document think everything is ok until they go to plug the same document into a different tool.

I took the following example of a document that is fine except it uses FORM instead of form and I plugged it into two different online validators, one of them hosted at swagger.io (https://editor.swagger.io), and in both cases FORM failed validation:

info:
  title: test
  version: '1.0'
openapi: '3.0.3'
paths:
  '/test':
    get:
      responses: 
        '200': 
          description: success
      parameters:
        - name: p1
          in: query
          schema:
            type: string
          style: FORM

The error given was

Structural error at paths./test.get.parameters.0.style
should be equal to one of the allowed values allowedValues: form, spaceDelimited, pipeDelimited, deepObject

That's why I don't want to make OpenAPIKit silently accept a schema containing FORM. The alternative I have adopted in some cases is to perform lenient parsing but store warnings when encountering problems like FORM so that I can make parsing succeed but document validation fail. I started to look into this approach on a branch here but it started to become a pretty large amount of changes so I am hesitant to finish that work unless this type of malformed document with capitalized style properties becomes common.

nexon commented 11 months ago

I'm totally agree with you. I prefer to have OpenAPIKit fail on this that have it parse it because it is less strict only because the conversion that a official library does is not strict/correct.

This sadly means that I would need to put my hands on Java to modify this thing. But's that's ok.

Anyway thanks for the response. I will look something in the issues that I can collaborate with, or if you have something in mind let me know.

mattpolzin commented 11 months ago

Take a look at the "good first issue" labels in the issues and if either of them sounds interesting then leave a comment to let me and others know you are working on something. I'm happy to help or answer questions about the task at any time.

There's also an open bug related to dereferencing for which the fix is likely to be similar to this PR.

I've been making a push for a release candidate for v3 so some of the good first issues were tackled by me in the past week and the remaining ones aren't guaranteed to stick around forever either!

nexon commented 11 months ago

@mattpolzin I think that we can close this issue. Since like you pointing out, the idea is that OpenAPIKit fails if it try to parse a spec that is not compliant with OpenAPI3.0.

Just for completion, and if someone in the future is looking an answer to this because swagger-parse still doesn't fix the upper case when converting to StyleEnum. You can create a Serializer (JsonSerializer<Parameter.StyleEnum>) that return a modified version of the StyleEnum (in this case return a lowercase string of the StyleEnum) and then inject that Serializer into a mixIn (@JsonSerialize(using = SUIStyleEnumSerializer.class)). With that you modified the Serializer to return a valid version of the enum.

mattpolzin commented 11 months ago

I agree. Thanks for reporting and working through this with me!