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

Nullary schema should render valid swagger code #167

Closed
michalrus opened this issue Sep 25, 2018 · 14 comments
Closed

Nullary schema should render valid swagger code #167

michalrus opened this issue Sep 25, 2018 · 14 comments

Comments

@michalrus
Copy link
Contributor

michalrus commented Sep 25, 2018

What we’re doing currently returns quite a few errors from the validator (and crashes swagger-codegen etc.):

nullarySchema :: Schema
nullarySchema = mempty
& type_ .~ SwaggerArray
& items ?~ SwaggerItemsArray []

{
  "items": [],
  "type": "array"
}

E.g. notice that "items" must be an object, and they’re an array above.

But this does not, and closely matches Aeson’s [] representation of () and mixed nullary/non-nullary constructors of a datatype:

{
  "type": "array",
  "items": {},
  "maxItems": 0,
  "example": []
}
@michalrus
Copy link
Contributor Author

This hack-ish patch fixes the issue (minus tests) — mixed nullary/non-nullary constructor schemas are now valid according to https://online.swagger.io/validator/debug?url=:url::

diff --git a/src/Data/Swagger/Internal.hs b/src/Data/Swagger/Internal.hs
index 54dc7ee..426273a 100644
--- a/src/Data/Swagger/Internal.hs
+++ b/src/Data/Swagger/Internal.hs
@@ -368,10 +368,13 @@ data ParamOtherSchema = ParamOtherSchema
 -- @'SwaggerItemsObject'@ should be used to specify homogenous array @'Schema'@s.
 --
 -- @'SwaggerItemsArray'@ should be used to specify tuple @'Schema'@s.
+--
+-- @'SwaggerItemsNullary'@ should be used for nullary constructors and '()'.
 data SwaggerItems t where
   SwaggerItemsPrimitive :: Maybe (CollectionFormat k) -> ParamSchema k-> SwaggerItems k
   SwaggerItemsObject    :: Referenced Schema   -> SwaggerItems 'SwaggerKindSchema
   SwaggerItemsArray     :: [Referenced Schema] -> SwaggerItems 'SwaggerKindSchema
+  SwaggerItemsNullary   :: SwaggerItems 'SwaggerKindSchema
   deriving (Typeable)
 
 deriving instance Eq (SwaggerItems t)
@@ -1051,6 +1054,10 @@ instance ToJSON (ParamSchema t) => ToJSON (SwaggerItems t) where
     , "items"            .= schema ]
   toJSON (SwaggerItemsObject x) = object [ "items" .= x ]
   toJSON (SwaggerItemsArray  x) = object [ "items" .= x ]
+  toJSON SwaggerItemsNullary = object
+    [ "items" .= object []
+    , "maxItems" .= (0 :: Int)
+    , "example" .= ([] :: [()]) ]
 
 instance ToJSON Host where
   toJSON (Host host mport) = toJSON $
diff --git a/src/Data/Swagger/Internal/Schema.hs b/src/Data/Swagger/Internal/Schema.hs
index 927e9e6..3609725 100644
--- a/src/Data/Swagger/Internal/Schema.hs
+++ b/src/Data/Swagger/Internal/Schema.hs
@@ -711,7 +711,7 @@ paramSchemaToSchema _ = mempty & paramSchema .~ toParamSchema (Proxy :: Proxy a)
 nullarySchema :: Schema
 nullarySchema = mempty
   & type_ .~ SwaggerArray
-  & items ?~ SwaggerItemsArray []
+  & items ?~ SwaggerItemsNullary
 
 gtoNamedSchema :: GToSchema f => SchemaOptions -> proxy f -> NamedSchema
 gtoNamedSchema opts proxy = undeclare $ gdeclareNamedSchema opts proxy mempty

But what’s the correct solution?

@phadej
Copy link
Collaborator

phadej commented Sep 26, 2018

@michalrus Is

-- >>> encode $ sketchSchema (1, 2, 3)
-- "{\"example\":[1,2,3],\"items\":{\"type\":\"number\"},\"type\":\"array\"}"

correct schema though? I.e. is the empty array the only wrong thing to do?

@phadej
Copy link
Collaborator

phadej commented Sep 26, 2018

oh sorry, wrong example

correct one:

-- >>> encode $ sketchSchema ("Jack", 25)
-- "{\"example\":[\"Jack\",25],\"items\":[{\"type\":\"string\"},{\"type\":\"number\"}],\"type\":\"array\"}"

@fizruk
Copy link
Member

fizruk commented Sep 26, 2018

E.g. notice that "items" must be an object, and they’re an array above.

I think in Swagger 2.0 arrays as items were used to represent tuples and I thought empty array would do. Unfortunately I can't find a confirmation of that now :(

OpenAPI 3.0 allows items to only be a schema and deals with tuples using oneOf (which is not as strong since it does not enforce order and multiplicity). See https://swagger.io/docs/specification/data-models/data-types/ for details.

So yes, apparently, we should use some schema for items (I don't think {} is a correct schema in Swagger 2.0, see #164) and enum to specify that only [] is a valid value. E.g. something like

{
  "type": "array",
  "items": {"type":"null"},
  "maxItems": 0,
  "example": []
}

@fizruk
Copy link
Member

fizruk commented Sep 26, 2018

I.e. is the empty array the only wrong thing to do?

I'm not sure anymore if Swagger 2.0 allows tuples. I thought it did, but since I introduced SwaggerItemsArray a few years back they've changed the spec a couple times, so I don't know anymore.

@michalrus
Copy link
Contributor Author

@phadej your example:

{"example":["Jack",25],"items":[{"type":"string"},{"type":"number"}],"type":"array"}

… is not a schema error, but a semantic error, returning:

Semantic error at definitions.X.items
`items` must be an object

Semantic errors don’t make the document invalid, for some reason. Also it works well in Swagger-UI.


This one, on the other hand (more/less our current one):

{"example":[],"items":[],"type":"array"}

returns:

Schema error at definitions['X'].items
should be object

Schema error at definitions['X'].items
should NOT have less than 1 items
limit: 1

Schema error at definitions['X'].items
should match some schema in anyOf

and is not valid.


@fizruk this one is valid as well (according to the validator, I don’t know how correct it is w.r.t. spec):

{
  "type": "array",
  "items": {"type":"null"},
  "maxItems": 0,
  "example": []
}

@phadej
Copy link
Collaborator

phadej commented Sep 26, 2018

ok, so if we return that object for an empty SwaggerItemsArray things should pass validation?

@michalrus
Copy link
Contributor Author

I think so. =)

@phadej
Copy link
Collaborator

phadej commented Sep 26, 2018

@michalrus could you make a PR?

@fizruk
Copy link
Member

fizruk commented Sep 26, 2018

Schema error at definitions['X'].items
should NOT have less than 1 items
limit: 1

This strongly suggests that I am not delusional and tuples were indeed supported at some point! 😅

@michalrus let us know if you're willing to make that PR 😊

@michalrus
Copy link
Contributor Author

michalrus commented Sep 27, 2018

Sure, I can. =)

So, for an empty SwaggerItemsArray, is it this one:

{
  "type": "array",
  "items": {"type":"null"},
  "maxItems": 0,
  "example": []
}

… or this one:

{
  "type": "array",
  "items": {},
  "maxItems": 0,
  "example": []
}

…?

Neither of these returns a schema or a semantic error, so they’re both fine.

I like untyped a bit more, since it doesn’t display the null in Swagger-UI (in the models section and tabs), but just an empty object instead.

@phadej
Copy link
Collaborator

phadej commented Sep 27, 2018 via email

@fizruk
Copy link
Member

fizruk commented Sep 27, 2018

@michalrus I'm okay with any one as long as you clearly document the choice. AFAIK the untyped one is not a valid Swagger 2.0 Schema, but it does not matter when there are zero values of that type I guess (lazy typing?) 😅

michalrus added a commit to michalrus/swagger2 that referenced this issue Sep 27, 2018
@michalrus
Copy link
Contributor Author

Okay. =)

Please, take a look at the horrendous draft… … …

michalrus added a commit to michalrus/swagger2 that referenced this issue Nov 22, 2018
michalrus added a commit to michalrus/swagger2 that referenced this issue Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants