Ensure that query param types are not inlined in OpenAPI spec #861
Conversation
georgefst
force-pushed
the
georgefst/openapi-level
branch
from
8fbea88 to
92a5b2e
Compare
As can be seen from the changes to `openapi.json`, this removes some duplication in the spec. One particular effect is that Orval-generated TypeScript bindings now use a common concise name for all occurrences of `Level`, `InputAction` and `NoInputAction`.
georgefst
force-pushed
the
georgefst/openapi-level
branch
from
92a5b2e to
89fc6ae
Compare
georgefst
changed the title
primer-service/src/Primer/Server.hs
Outdated
| {- This is a workaround for an upstream issue: https://github.com/biocad/servant-openapi3/issues/37. | ||
| Given a type, and some query parameters of that type, | ||
| this ensures that the specification of each parameter references a common schema, | ||
| instead of inlining it. | ||
| -} |
There was a problem hiding this comment.
This sounds sensible. It does mean that we have to manually enumerate them (and thus potentially miss some). Another approach would be to replace any inlined thing of the correct shape with a reference (less flexible -- perhaps we want some of them inlined / there are two different types that happen to serialise the same).
primer-service/src/Primer/Server.hs
Outdated
| refParamSchemas :: forall a. ToSchema a => [(FilePath, Text)] -> OpenApi -> OpenApi | ||
| refParamSchemas params api = | ||
| api | ||
| & #components % #schemas %~ IOHM.insert name (toSchema $ Proxy @a) | ||
| & #paths %~ foldr ((.) . uncurry (flip adjustParam)) identity params | ||
| where | ||
| adjustParam paramName = | ||
| IOHM.adjust $ | ||
| #post % mapped % #parameters % mapped %~ \case | ||
| Inline x | x ^. #name == paramName -> Inline $ x & #schema ?~ Ref (Reference name) | ||
| p -> p | ||
| name = show $ typeRep @a |
There was a problem hiding this comment.
Small worries (that happen not to be problematic given how we are using this):
- We only look at
PUTs - What happens if there is already a referenced schema of that name -- is it guaranteed that it will be the same? (should be (modulo the next point), since we reference by
show $ typeRep) - The usual issues around typereps and openapi: see Invalid schema with types with primes (
') biocad/openapi3#57 and Generically derived schema reference names can clash biocad/openapi3#56, and also https://github.com/biocad/openapi3/pull/19/files#diff-332df4b0191a0ce0d1f773c84d99030b1376745df15e86c66391f48185d0b557R866-R886 where upstream replaces spaces in the output ofshow $ typeRep @a
There was a problem hiding this comment.
These are all good points, as is #861 (comment).
I've got my fingers crossed that this gets fixed upstream (even if that means fixing it ourselves) before we have to make this workaround any more general.
There was a problem hiding this comment.
I'm happy with this as-is, but let's add some comments documenting the lack of generality (i.e. manually enumerate, only look at puts)
primer-service/src/Primer/Server.hs
Outdated
| refParamSchemas params api = | ||
| api | ||
| & #components % #schemas %~ IOHM.insert name (toSchema $ Proxy @a) | ||
| & #paths %~ foldr ((.) . uncurry (flip adjustParam)) identity params |
There was a problem hiding this comment.
To me, this is rather inscrutable. Perhaps worth pulling out a helper for "compose this list of functions" compose = appEndo . foldMap' Endo?
primer-service/src/Primer/Server.hs
Outdated
| @@ -127,6 +127,10 @@ openAPIInfo = | |||
| Given a type, and some query parameters of that type, | |||
| this ensures that the specification of each parameter references a common schema, | |||
| instead of inlining it. | |||
| This could be made more general, (visit non-POST endpoints, | |||
| modify `show . typeRep` output when it contains `'` to match `openapi3`, | |||
There was a problem hiding this comment.
You mean (a literal space) here (openapi3 does not do anything special with ' and generates broken schemas because of it, but it does replace spaces with underscores)
There was a problem hiding this comment.
Ah, thanks, maybe I should have read that issue properly. Fixed.
georgefst
force-pushed
the
georgefst/openapi-level
branch
from
88c7b8a to
491ea6d
Compare
A workaround for biocad/servant-openapi3#37.
As can be seen from the changes to
openapi.json, this removes some duplication in the spec. One particular effect is that Orval-generated TypeScript bindings now use a common concise name for all occurrences ofLevel,InputActionandNoInputAction.