From 5d9d67fcb60280e7deb455e97a6ef05b22984246 Mon Sep 17 00:00:00 2001 From: Isaac Elliott Date: Wed, 20 Aug 2025 15:53:56 +1000 Subject: [PATCH 1/3] Add failing test The `Generic`-derived schema for polymorphic datatypes is incorrect. The schema for a datatype like `data X a = X{ field :: a }` has `field` as a required property. This means that the schema for `X (Maybe something)` also has `field` marked as a required field. The optionality of the `Maybe` is forgotten. A service relying on the erroneous schema would expect a non-nullable value, but The service sending `X (Maybe something)` values can send `null` in the `field` propety, but the service relying on the erroneous schema will expect no `null` in `field`, leading to a runtime error. Given `data X a = X{ field :: a }`, the `Generic`-derived schema *has* to mark `field` as required, because it must work for all `a`. Therefore, `instance ToSchema a => ToSchema (Maybe a)` has to generate a `nullable` schema to be correct. Then the schema for `X (Maybe something)` will have required property `field` with a `nullable` schema for `something`. --- test/Data/OpenApiSpec.hs | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/test/Data/OpenApiSpec.hs b/test/Data/OpenApiSpec.hs index cb860747..7fb3bde1 100644 --- a/test/Data/OpenApiSpec.hs +++ b/test/Data/OpenApiSpec.hs @@ -2,6 +2,7 @@ {-# LANGUAGE OverloadedStrings #-} {-# LANGUAGE ScopedTypeVariables #-} {-# LANGUAGE QuasiQuotes #-} +{-# LANGUAGE DeriveGeneric #-} module Data.OpenApiSpec where import Prelude () @@ -13,11 +14,13 @@ import Data.Aeson import Data.Aeson.QQ.Simple import Data.HashMap.Strict (HashMap) import qualified Data.HashSet.InsOrd as InsOrdHS +import Data.Proxy (Proxy(..)) import Data.Text (Text) import Data.OpenApi import SpecCommon import Test.Hspec hiding (example) +import GHC.Generics (Generic) spec :: Spec spec = do @@ -53,6 +56,8 @@ spec = do it "merged correctly" $ do let merged = oAuth2SecurityDefinitionsReadOpenApi <> oAuth2SecurityDefinitionsWriteOpenApi <> oAuth2SecurityDefinitionsEmptyOpenApi merged `shouldBe` oAuth2SecurityDefinitionsOpenApi + it "Type Parameters Example" $ do + toJSON typeParametersExample `shouldBe` typeParametersExampleJSON main :: IO () main = hspec spec @@ -1003,3 +1008,33 @@ compositionSchemaExampleJSON = [aesonQQ| ] } |] + +data TypeParameters a + = TypeParameters + { typeParametersField1 :: Maybe Double + , typeParametersField2 :: a + } deriving (Generic) + +instance ToSchema a => ToSchema (TypeParameters a) + +typeParametersExample :: Schema +typeParametersExample = toSchema (Proxy :: Proxy (TypeParameters (Maybe Double))) + +typeParametersExampleJSON :: Value +typeParametersExampleJSON = [aesonQQ| +{ + "properties": { + "typeParametersField1": { + "format": "double", + "type": "number" + }, + "typeParametersField2": { + "format": "double", + "type": "number", + "nullable": true + } + }, + "required": ["typeParametersField2"], + "type": "object" +} +|] From 4573caffd09d2d3575b9094181904ad4e8379d8c Mon Sep 17 00:00:00 2001 From: Isaac Elliott Date: Wed, 20 Aug 2025 16:37:14 +1000 Subject: [PATCH 2/3] Fix schemas involving `Maybe` See the previous commit for an explanation of the problem. This change also rules out `ToSchema (Maybe (Maybe _))` for correctness. --- The schema for `Maybe a` is prefixed with `Maybe_` to distinguish it from the schema for `a`. Without this, `declareSchemaRef` will generate the same reference for an `a` or a `Maybe a`. When an API uses an `a` and a `Maybe a`, only one of the two schemas will make it into the `components.schemas` section (because of the name conflict). This can lead to a schema that says it produces non-nullable values, but may actually produce nulls. This problem could also be solved by changing `declareSchemaRef` to strip the `nullable` property from the declared schema and return the nullability value alongside the `Referenced Schema`. This would increase sharing (you wouldn't have near duplicate schemas `X` and `Maybe_X`), with the cost of forcing downstream libraries (such as `servant-openapi3`) to explicitly handle that nullability information. I chose the solution that's compatible with current downstream code. --- src/Data/OpenApi/Internal/Schema.hs | 19 +++++++++++++++++-- test/Data/OpenApi/SchemaSpec.hs | 2 +- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/Data/OpenApi/Internal/Schema.hs b/src/Data/OpenApi/Internal/Schema.hs index da56acf0..525853ba 100644 --- a/src/Data/OpenApi/Internal/Schema.hs +++ b/src/Data/OpenApi/Internal/Schema.hs @@ -623,7 +623,22 @@ instance ToSchema Float where declareNamedSchema = plain . paramSchemaToSc instance (Typeable (Fixed a), HasResolution a) => ToSchema (Fixed a) where declareNamedSchema = plain . paramSchemaToSchema instance ToSchema a => ToSchema (Maybe a) where - declareNamedSchema _ = declareNamedSchema (Proxy :: Proxy a) + declareNamedSchema _ = do + namedSchema <- declareNamedSchema (Proxy :: Proxy a) + pure + namedSchema + { _namedSchemaName = fmap ("Maybe_" <>) (_namedSchemaName namedSchema) + , _namedSchemaSchema = + _namedSchemaSchema namedSchema + & nullable ?~ True + } + +type NestedMaybeError = + Text "Nested " :<>: ShowType Maybe :<>: Text "s are not supported." :$$: + Text "OpenAPI 3 only supports a single level of nullability, so it can't distinguish between Nothing and Just Nothing." + +instance {-# OVERLAPPING #-} (TypeError NestedMaybeError, Typeable a) => ToSchema (Maybe (Maybe a)) where + declareNamedSchema _ = undefined instance (ToSchema a, ToSchema b) => ToSchema (Either a b) where -- To match Aeson instance @@ -1010,7 +1025,7 @@ withFieldSchema opts _ isRequiredField schema = do -- | Optional record fields. instance {-# OVERLAPPING #-} (Selector s, ToSchema c) => GToSchema (S1 s (K1 i (Maybe c))) where - gdeclareNamedSchema opts _ = fmap unnamed . withFieldSchema opts (Proxy2 :: Proxy2 s (K1 i (Maybe c))) False + gdeclareNamedSchema opts _ = fmap unnamed . withFieldSchema opts (Proxy2 :: Proxy2 s (K1 i c)) False -- | Record fields. instance {-# OVERLAPPABLE #-} (Selector s, GToSchema f) => GToSchema (S1 s f) where diff --git a/test/Data/OpenApi/SchemaSpec.hs b/test/Data/OpenApi/SchemaSpec.hs index 0a3f96e7..0343e25b 100644 --- a/test/Data/OpenApi/SchemaSpec.hs +++ b/test/Data/OpenApi/SchemaSpec.hs @@ -102,7 +102,7 @@ spec = do context "Character" $ checkDefs (Proxy :: Proxy Character) ["Player", "Point"] context "MyRoseTree" $ checkDefs (Proxy :: Proxy MyRoseTree) ["RoseTree"] context "MyRoseTree'" $ checkDefs (Proxy :: Proxy MyRoseTree') ["myrosetree'"] - context "[Set (Unit, Maybe Color)]" $ checkDefs (Proxy :: Proxy [Set (Unit, Maybe Color)]) ["Unit", "Color"] + context "[Set (Unit, Maybe Color)]" $ checkDefs (Proxy :: Proxy [Set (Unit, Maybe Color)]) ["Unit", "Maybe_Color"] context "ResourceId" $ checkDefs (Proxy :: Proxy ResourceId) [] describe "Inlining Schemas" $ do context "Paint" $ checkInlinedSchema (Proxy :: Proxy Paint) paintInlinedSchemaJSON From 6111955c8a6464f611a53efe9778c9e71d24ee5b Mon Sep 17 00:00:00 2001 From: Isaac Elliott Date: Thu, 21 Aug 2025 11:50:37 +1000 Subject: [PATCH 3/3] Update changelog and bump library version --- CHANGELOG.md | 2 ++ openapi3.cabal | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ae62105..696222fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ Unreleased ---------- +- (breaking) Use `nullable` for `Maybe` schemas [#113](https://github.com/biocad/openapi3/pull/113) + 3.2.4 ----- - Give `title` to sub schemas of sum types [#88](https://github.com/biocad/openapi3/pull/88). diff --git a/openapi3.cabal b/openapi3.cabal index 1185d038..4cda8fd4 100644 --- a/openapi3.cabal +++ b/openapi3.cabal @@ -1,6 +1,6 @@ cabal-version: >=1.10 name: openapi3 -version: 3.2.4 +version: 3.3.0 synopsis: OpenAPI 3.0 data model category: Web, Swagger, OpenApi