-
Notifications
You must be signed in to change notification settings - Fork 66
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
Support nullable format
s for Openapi
#209
Conversation
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.
it makes sense!
Thinking about this some more, I'm wondering if the proposal should be the inverse of this. The schema's Consider a slightly more complex schema like schema = JSONSchemer.schema({ type: %w[string boolean null], format: 'email' })
schema.valid_schema? # true
schema.valid?('[email protected]') # true
schema.valid?('test') #false, with "value at root does not match format: email"
schema.valid?(true) # true, but should be false since input is not an email
We could instead make the format there more strict by returning EMAIL = proc do |instance, _format|
instance.is_a?(String) && instance.ascii_only? && valid_email?(instance)
end which now gives us schema = JSONSchemer.schema({ type: %w[string boolean null], format: 'email' })
schema.valid_schema? # true
schema.valid?('[email protected]') # true
schema.valid?('test') # false, with "value at root does not match format: email"
schema.valid?(true) # false, with "value at root does not match format: email" Again, not sure what the specification(s) say about this. A colleague of mine recalls reading something in the specification that suggests that keywords shouldn't relax a constraint from another keyword... although we haven't been able to find it again 😞. |
Hi @moberegger, this is great—thanks for opening it! I believe your initial thought is correct.
For the OpenAPI formats, I don't think checking
I think what we want is something like: 'int32' => proc { |instance, _format| !instance.is_a?(Numeric) || (instance.is_a?(Integer) && instance.bit_length <= 32) } Which gives:
There's an additional question of how strict we should be. Since
I think if we go down the more strict path, we'd need to copy the logic from
I'm leaning toward the more strict approach, but let me know what you think. I also noticed that the
|
Nice! That is reassuring. It means I don't have to fix all of my schemas 😆. Also appreciate the link to the relevant part of the specification!
Yeah, I only thought it was an issue specifically with |
According to https://json-schema.org/understanding-json-schema/reference/numeric#integer
It reads like your example of |
My apologies for the shotgun of comments... Would you like me to slip that change into this PR? Or would you prefer to handle it separately? |
Think there may be a similar problem with Openapi 3.0's 'byte' => proc { |instance, _value| ContentEncoding::BASE64.call(instance).first },
'binary' => proc { |instance, _value| instance.is_a?(String) && instance.encoding == Encoding::BINARY }, According to https://spec.openapis.org/oas/v3.0.4.html#data-type-format those should only apply to 'byte' => proc { |instance, _value| !instance.is_a?(String) || ContentEncoding::BASE64.call(instance).first },
'binary' => proc { |instance, _value| !instance.is_a?(String) || instance.encoding == Encoding::BINARY }, |
I think the issue is the float version won't go through the
That's pretty picky and it's unclear to me what the behavior should be exactly. Let me know what you think. The spec has a relevant note (though it didn't do much to clear things up for me):
No that's alright. I'll handle it separately once this is merged.
Good catch! Thanks for fixing that. |
Oooooh, OK. I see what you mean now. I wasn't cluing in that I agree; we should be strict here. As a consumer, I would expect that anything truthy for I've made a change to the Openapi 3.1 The Openapi 3.0
I've also extended the tests to include the following:
My hope is to capture the subtle differences between the two whilst also having some coverage over representative use cases. |
Haha that's my bad—should've been more clear about what I meant.
Excellent! Thanks so much for the help. And I really appreciate the thorough tests 👍
I might follow up by moving the |
My pleasure, honestly. Your feedback is always insightful and helpful, and really made this quite enjoyable for me. I learned a lot. I'm impressed with your attention to detail with the specs; I mean, I always presumed coding to these specs must have been a lot of work... but even just going through with this comparatively small change really opened my eyes to how much there is to think about. My goodness. |
Glad to hear it! And thank you for the kind words. I just released a new version of the gem with your changes. I ran into an issue changing the
|
I noticed some inconsistency in behaviour with how nullability works with the
format
keyword. Figured I'd put up a PR instead of just throwing it over the fence with an issue; you do enough hard work so thought I'd at least try to save you from some! 😆My assessment here may not be correct; I'm rather new-ish to Openapi and JSON Schema, so my understanding of the specification and expected behaviour might be wrong. Also not sure if my proposed solution here is the best path forward. My goal is more so to surface the problem I'm seeing - or at least what I think is a problem. I won't be offended if you don't accept it 😆.
Anyways!
Let's say I have a schema to validate that a value is either a
date-time
string ornull
. I can do so like this:Cool! But if I try the same thing with
int32
:That same behaviour happens for
int32
,int64
,double
, andfloat
(ie. theformat
s that Openapi add).I compared the implementation of the Openapi formatters to the ones in the JSON schema 2020 draft and noticed that the JSON schema ones would simply return
true
if the input wasn't aString
. This in effect meant that it would betrue
for anil
input. I thought that it might be appropriate to take a similar approach to the Openapi formatters and have them returntrue
when the input isnil
as well, which gets things working for the above example.Now, the tradeoff here is what happens when you don't provide a
type
and only provide aformat
.This does seem awkward, but it is actually consistent with how the JSON schema formatters work.
To be honest, I don't know what the expected behaviour should be here. I promise I did my best to look through both Openapi and JSON schema specifications to get a better understanding of how nullable
format
s should work, but I wasn't able to find anything that cleared that up for me.I admit that I may be overstepping my bounds here by making the formatters more lenient with
nil
. It doesn't feel right, but since I observed that the same behaviour was happening with the JSON Schema formatters, I thought that maybe this was an appropriate solution.It's worth noting that nullable formats can be achieved by using
oneOf
so perhaps this is the correct way to specify a nullable format?