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
[Draft] oneOf
support for query strings
#1018
base: master
Are you sure you want to change the base?
[Draft] oneOf
support for query strings
#1018
Conversation
In particular what is going to need some design-elbow-grease is the question if we want to do this fallback strategy. Right now there's nothing stopping you from doing something like this:
Note that in terms of structural subtyping (depending on the particular encoding you choose for the case classes) we have |
Codecov Report
@@ Coverage Diff @@
## master #1018 +/- ##
==========================================
- Coverage 75.71% 74.92% -0.80%
==========================================
Files 144 145 +1
Lines 4756 4825 +69
Branches 56 50 -6
==========================================
+ Hits 3601 3615 +14
- Misses 1155 1210 +55
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @GrafBlutwurst.
May I ask you a bit more details about your use cases? It might be easier to support a special case of oneOf
: the case of enumerations, where the type of all the alternatives is the same, but the set of valid values is a fixed subset of the possible values for that type.
Regarding the tests, the current approach consists in adding them here for server interpreters and here for client interpreters.
@@ -83,6 +83,32 @@ trait Urls extends algebra.Urls { | |||
) | |||
type QueryStringParam[A] = DocumentedQueryStringParam[A] | |||
|
|||
//TODO: This is kinda ugly. But discriminated schemas make little sense on query strings? | |||
//Stil maybe this should be done better, but I haven't yet fully understood the openapi schema to figure out how to deal with nesting here | |||
private def unpackSchema[A](qsp:QueryStringParam[A]):List[Schema] = qsp.schema match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this is a signal that tells us that something is wrong with our approach. Most likely we need an intermediate structure somewhere to avoid this.
example = None, //TODO: What to put here? | ||
title = None,//TODO: What to put here? | ||
), | ||
isRequired = qspa.isRequired || qspb.isRequired, //TODO: This feels wrong? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, this is a signal that the signature of oneOfQueryStringParam
is wrong, or that the type of QueryStringParam
is wrong in the openapi interpreter (maybe it should be something else than the current DocumentedQueryStringParam
).
In most cases the usecase is really just straight up enums. So in particular we have a search endpoint where we'd like to have the search parameters in the URL so that this search can easily be bookmarked or shared by users. Most of the parameters are e.g. The other much harder case is e.g. Right now we just work with a stringly types query string that we |
For that use case, an approach similar to
Here, I am not even sure there is a way to express that in JSON Schema. It seems that “ranges” are supported for numeric values only. I think the only solution is to indicate in the description of the query parameter what are the validation constraints. |
I think you're right. I'll probably start from scratch again and restrict it to "true" enums. Maybe it would be interesting to explore to "derive" documentation for query params that actually carry data (so like the So I think I'll restrict this MR purely to enums and then internally develop the documentation block derivation, if it pans out I'll upstream it. |
As mentioned in gitter this tackles the oneOf support for query string. But I'll need some help with a couple bits.
Formatting and cleaning is not done yet as it's still a draft.
I also added a possible encoding of
Alternative
and anolog toTupler
that could help people have less issues with heavily nested eithers. If we decide to retain this I'll probably use it for a bit more sizable test for query string enums.