Invalid schemas generated for tuples and (some) `Map`s

brprice commented 3 years ago

Description

It seems we generate invalid schemas for all tuples and Maps with "complex" keys (those that aeson's toJSONKey is ToJSONKeyValue and thus the map is encoded as an array of key-value pairs, rather than as a json object). This is because Map k v is encoded the same as [(k,v)] for these keys, and (k,v) generates a schema similar to

{
  "minItems": 2,
  "items": [
      {
          "type": "string"
      },
      {
          "type": "boolean"
      }
  ],
  "maxItems": 2,
  "type": "array"
}

According to the OpenAPI3 spec, the items field must not be an array.

items - Value MUST be an object and not an array

More details

Note that even homogenous tuples (e.g. (String,String)) have the same problem, since there is no special handling. In fact, due to this restriction I don't see how to give a schema for any non-record product type, unless all its fields have the same type.

A full example, generating an invalid spec is

{-# LANGUAGE OverloadedLists #-}
{-# LANGUAGE OverloadedStrings #-}

import Control.Lens
import Data.Aeson.Encode.Pretty (encodePretty)
import Data.ByteString.Lazy.Char8 as BSL
import Data.Map (Map)
import Data.Proxy
import Data.OpenApi

main :: IO ()
main = BSL.putStrLn $ encodePretty $ (mempty :: OpenApi) &
  components . schemas .~ [ ("Buggy", toSchema (Proxy :: Proxy (Map [Int] Bool)))]

(See also this gist for the generated schema, and a few other examples) The output of this is rejected as invalid by https://validator.swagger.io/validator and https://editor.swagger.io/ and https://openapi-generator.tech/ (both of which give more useful error messages)

The code that constructs these specs

https://hackage.haskell.org/package/openapi3-3.1.0/docs/src/Data.OpenApi.Internal.Schema.html#line-695 says ToSchema (Map k v) = ToSchema [(k,v)] for "complex" keys
https://hackage.haskell.org/package/openapi3-3.1.0/docs/src/Data.OpenApi.Internal.Schema.html#line-583 says ToSchema [a] is an openapi array of ToSchema a
https://hackage.haskell.org/package/openapi3-3.1.0/docs/src/Data.OpenApi.Internal.Schema.html#line-630 says ToSchema (a,b) is implemented with genericDeclareNamedSchema, which is where I stopped digging.

I expect this code was ported from / inspired by swagger2. The swagger (i.e. openapi2) spec is difficult to read, but I think it does support items being an array. However, openapi3 explicitly does not.

(re swagger/openapi2:

https://github.com/OAI/OpenAPI-Specification/blob/main/versions/2.0.md#schemaObject says that type and items are as in the JSON Schema definition (with the minor change that when the JSON Schema spec says something must be "a JSON Schema", we should read "an openapi2 schema object" instead) The JSON Schema (validation) spec is linked as https://datatracker.ietf.org/doc/html/draft-fge-json-schema-validation-00#page-9, which says

The value of "items" MUST be either an object or an array
However it contradictarily seems not accepted at https://github.com/OAI/OpenAPI-Specification/blob/main/versions/2.0.md#itemsObject, which says that items must be an Items Object
see also https://github.com/GetShopTV/swagger2/issues/107 which is also confused on this point

)

The future

I wonder if it is worth adding a custom type error to explain why hetrogenous tuples / "complex" maps cannot be given a spec? Longer term, it seems that openapi-3.1.0 is out which (if I read (the linked JSON Schema docs) correctly) brings back the ability for items to be an array, under the name prefixItems. However, this does not seem well supported yet, for instance none of the validators above support it.

maksbotan commented 2 years ago

Hi! Thanks for such a detailed report.

Unfortunately, OAS 3.0 dropped support for tuple-like arrays, leaving only heterogeneous ones:

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.

I've however decided to leave the relevant code in openapi3 for two reasons: 1. such types are really useful and it would OK to have them for human readers of the spec, if not for automated tools, and 2. I was being lazy :)

Thanks for the link to OAS 3.1, I will look at it. However, as far as I can tell, 3.0 remains the official version for now, so I'm not sure if it makes sense to migrate to 3.1. Will swagger-ui support it, for example?

brprice commented 2 years ago

Thanks for the link to OAS 3.1, I will look at it. However, as far as I can tell, 3.0 remains the official version for now

I think 3.1 is officially released, according to https://github.com/OAI/OpenAPI-Specification#current-version---310 and https://www.openapis.org/ (which has a big image saying "3.1.0 released").

so I'm not sure if it makes sense to migrate to 3.1. Will swagger-ui support it, for example?

I don't know much on this front, I'm afraid. My impression is that tool support seems to be lacking at the moment, so I suspect it may not be worth the effort yet (I hope this will change in the future!). Swagger-ui in particular does not support 3.1 yet; there is an open issue https://github.com/swagger-api/swagger-ui/issues/5891, though there doesn't seem to be much activity on this. I have seen that 3.1 support is in the "short-term roadmap" for https://github.com/swagger-api/swagger-parser/issues/1535#issuecomment-936707841, but I don't know an ETA.

JohanWinther commented 2 years ago

I'm reviving this issue, because I think I found a solution for the tuple case.

This tuple representation does not validate:

{
  "minItems": 2,
  "items": [
      {
          "type": "string"
      },
      {
          "$ref": "#/components/schemas/Dog"
      }
  ],
  "maxItems": 2,
  "type": "array"
}

however this does:

{
  "minItems": 2,
  "items": {
    "oneOf": [
      { "type": "string" },
      {
        "$ref": "#/components/schemas/Dog"
      }
    ]
  },
  "maxItems": 2,
  "type": "array"
}

I'm not sure if this means that the schemas are applied to each array item in order though.

maksbotan commented 1 year ago

{
  "minItems": 2,
  "items": {
    "oneOf": [
      { "type": "string" },
      {
        "$ref": "#/components/schemas/Dog"
      }
    ]
  },
  "maxItems": 2,
  "type": "array"
}

@JohanWinther sadly, this one defines an array of any length, each item of which can be either string or Dog — not a tuple of two elements, first of which is string, second is Dog.

There is really no way to specify tuples in the OpenAPI 3.0 spec.

dminuoso commented 1 year ago

openapi-generator chokes on these as it abides by OAS 3.0.

Errors: 
    -attribute components.schemas.PoolReportAll.items is missing
    -attribute components.schemas.PoolReportAll.items is not of type `object`
Warnings: 
    -attribute components.schemas.PoolReportAll.items is missing
    -attribute components.schemas.PoolReportAll.items is not of type `object`

Would be nice if we could merge https://github.com/biocad/openapi3/pull/69 and resolve this

avandecreme commented 1 year ago

If anyone is stuck using openapi-generator because of this, I implemented the above suggested workaround in https://github.com/novadiscovery/openapi3/pull/2

Note that I used anyOf instead of oneOf because for example if you have [1, 2.1] that you want to load into (Int, Double), 1 matches both Int and Double schemas.

I also made sure to add null in the anyOf list whenever we have a Maybe a.

Finally, I also re-implemented https://github.com/biocad/openapi3/pull/46 with some tests added. While I don't expect any interest to merge the first commit, let me know if you would like me to open a PR for the second one.

georgefst commented 1 year ago

@brprice FWIW, the problem here is actually slightly more general than the current thread title suggests, as it also applies to any sum type where a constructor has multiple fields, as Aeson also represents these as a two-element array, just as for tuples.

Also, to save others misreading the previous few comments as I did, the "above suggested workaround" in https://github.com/biocad/openapi3/issues/31#issuecomment-1415875798 refers to https://github.com/biocad/openapi3/issues/31#issuecomment-1282202386, which as pointed out in https://github.com/biocad/openapi3/issues/31#issuecomment-1320836239, is far from a universally-applicable workaround. It just uses a much looser type: List (A | B) instead of Pair A B. This might not be too bad for a client which only creates data of the type in question, but it's very awkward when consuming data of that type, as manual unsafe coercions would be required. The best solution I've thought of so far is to implement a tuple-as-record (e.g. data RecordPair a b = RecordPair {fst :: a, snd :: b}), and use this in place of ordinary Haskell tuples. It's potentially a bit cumbersome, but at least the type is precise, and it's reusable.

biocad / openapi3