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

What part of Swagger spec describes SwaggerItemsArray? #107

Open
qrilka opened this issue May 31, 2017 · 6 comments
Open

What part of Swagger spec describes SwaggerItemsArray? #107

qrilka opened this issue May 31, 2017 · 6 comments

Comments

@qrilka
Copy link
Contributor

qrilka commented May 31, 2017

In our code I've tried using SwaggerItemsArray with the following ToSchema instance:

instance ToSchema QuartileCounts where
  declareNamedSchema _  = do
    iSchema <- declareSchemaRef (Proxy @ (Sum Integer))
    return . named "QuartileCounts" $ mempty
      & type_ .~ SwaggerArray
      & items ?~ SwaggerItemsArray [iSchema, iSchema, iSchema, iSchema]

which should cover as I understand values like e.g. [21, 6, 4, 1].
But with a swagger.json for that Python client (which uses bravado to access Swagger endpoints) on unmarshaling line:

obj_type = schema_object_spec.get('type')

And it looks like according to the spec (as shown at http://swagger.io/specification/#itemsObject or https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#itemsObject ) items property should be a "Items Object" and not an array.
For our case I'll go with SwaggerItemsObject as this "tuple" is homogeneous but what about other tuples - at the moment it looks to me that swagger file generated with swagger2 will be incorrect.

@qrilka
Copy link
Contributor Author

qrilka commented May 31, 2017

It looks like jso-schema spec allows arrays though - https://tools.ietf.org/html/draft-fge-json-schema-validation-00#page-9

@qrilka
Copy link
Contributor Author

qrilka commented May 31, 2017

And in the next spec https://github.com/OAI/OpenAPI-Specification/blob/OpenAPI.next/versions/3.0.md#properties it is even said explicitly

items - Value MUST be an object and not an array. Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema. items MUST be present if the type is array.

@fizruk
Copy link
Member

fizruk commented May 31, 2017

@qrilka array stands for both homogeneous lists (like haskell lists) and tuples (heterogeneous lists of fixed size).

For homogeneous lists items property is just one Schema that describes type for all items.
For tuples items property is a list of Schemas, one Schema per value in a tuple.

Note though that you can limit the size for homogeneous lists using maxItems and minItems.
So in your example it's best to use a homogeneous list:

instance ToSchema QuartileCounts where
  declareNamedSchema _  = declareNamedSchema (Proxy @ [Sum Integer])
    & name ?~ "QuartileCounts"
    & schema.minItems ?~ 4
    & schema.maxItems ?~ 4

It is possible that I misread the specification (which was and is incredibly hard to read, TBH).

It is also possible that at the time when I introduced SwaggerItemsArray this option was not excluded from the spec (it certainly looked differently a year ago). I would not be surprised if it was a bug in the spec that I brought into swagger2 and that they've patched it later silently without any version bump.

Note that without SwaggerItemsArray it becomes impossible to create Schema for non-record product types. If we remove it, we have to add a TypeError for GToSchema and GToParamSchema explaining this to users.

@qrilka
Copy link
Contributor Author

qrilka commented May 31, 2017

@fizruk actually I've written almost the same code as you've pasted above :)

@fizruk
Copy link
Member

fizruk commented May 31, 2017

To be clear, I'm happy with the way things are right now. I don't think any of my APIs rely on SwaggerItemsArray, but it makes a nice representation for tuples.

Nonetheless, someone should read Swagger specification thoroughly and decide whether SwaggerItemsArray has to go. If yes, then someone (else) should send a PR fixing it for everyone.

@paulyoung
Copy link
Contributor

I pasted my swagger JSON into https://editor.swagger.io/ and saw this:

Screen Shot 2020-04-14 at 2 35 57 PM

Screen Shot 2020-04-14 at 2 35 44 PM

All of my instances are derived.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants