Open
Conversation
isaac-uptrust
force-pushed
the
ie/fix-maybe-schemas
branch
3 times, most recently
from
77fbf18 to
f5075ee
Compare
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`.
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.
isaac-uptrust
force-pushed
the
ie/fix-maybe-schemas
branch
from
f5075ee to
6111955
Compare
|
Clever use of overlapping instances. Love it 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
Generic-derived schema for polymorphic datatypes is incorrect. The schema for a datatype likedata X a = X{ field :: a }hasfieldas a required property. This means that the schema forX (Maybe something)also hasfieldmarked as a required field. The optionality of theMaybeis forgotten. A service relying on the erroneous schema would expect a non-nullable value, but a service sendingX (Maybe something)values can sendnullin thefieldpropety, but a service relying on the erroneous schema will expect nonullinfield, leading to a runtime error.Given
data X a = X{ field :: a }, theGeneric-derived schema must markfieldas required, because it needs to work for alla. Therefore,instance ToSchema a => ToSchema (Maybe a)has to generate anullableschema to be correct. Then the schema forX (Maybe something)will have required propertyfieldwith anullableschema forsomething.This change changes the
ToSchemainstance forMaybe a. The schema forMaybe ais prefixed withMaybe_to distinguish it from the schema fora. Without this,declareSchemaRefwill generate the same reference for anaor aMaybe a. When an API uses anaand aMaybe a, only one of the two schemas will make it into thecomponents.schemassection (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
declareSchemaRefto strip thenullableproperty from the declared schema and return the nullability value alongside theReferenced Schema. This would increase sharing (you wouldn't have near duplicate schemasXandMaybe_X), and I think it would be cleaner, but at the cost of forcing downstream libraries (such asservant-openapi3) to explicitly handle that nullability information. I chose the solution that's compatible with current downstream code.I think this supercedes the previous attempts at fixing
Maybe:Maybe aschemas #103