-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
removes the use of integer type from registries format #4015
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.
integer
is not a type in the JSON data model. This is explicitly addressed in JSON Schema, and is why integer
is never supposed to be present in base_type
.
Both keyword and format
applicability work with JSON types, not the type
keyword values (type: integer
is more like a format, except that it's actually reliable).
@handrews I did come across this section of the specification, this other seems to contradict it though. (Link in my original post)
Not sure which one is correct, but it seems at least some descriptions in the wild are using this type. Also sf-integer in the registry here mentions this type. |
@baywet The sentence you quoted does not contradict anything- it expliclty states six primitive types, then lists the six, and then says "or The JSON RFC does not define integers as distinct from numbers, so JSON parsers do not necessarily make any distinction (in part because JavaScript does not either). This was a significant point of discussion at one point in JSON Schema's history and I am very certain of what it means. |
Before approving this PR I wondered whether
and
So @baywet's changes to |
I could revert the changes to uint etc to only include int32 and int64 to stop the bleeding here. But I guess if we did so, I'd have to leave a note. Plus what should we do with sf-integer then? |
No, We could deprecate |
In that scenario, should we add a note in the registry for all the formats that mention the integer type? |
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.
Looks good. 👍
I think it is pretty clear that integer
is allowed/supported in OpenAPI, so these changes are appropriate. Further, I think we'd need to have compelling reasons to remove it in any future version.
This is not about OAS, this is about JSON Schema. JSON Schema's |
To try and make this a bit more clear, JSON Schema keywords and As I noted in a previous comment, the JSON Schema data model does not include integers. The If we were to treat integer as a data model type (which it is not), and we were to say that Consider |
Thank you for the super detailed description of how things are supposed to work. I had no idea about the data model aspects and its subtilities. I find it odd that JSON schema went with this opinionated way of dealing with numbers, which is different from JSON as far as I understand and loses information. When you have In that context, it would make sense to me that an API producer would want to say this API only produces information with this or that level of precision. And consequently that somebody would want to validate the data it's receiving when compared with the spec. But I understand this is not how JSON schema was designed to work and trying to make it work this way would be difficult because of that. Now, to Ralph's earlier comment. It would make sense that we clean this up moving forward. |
I don't think that's a good idea. Integer is a type in OAS, and that might have some peculiarities, but there's lots of stuff in the spec that is peculiar. |
I'm not quite sure what we are arguing about here. My take on the current specification and format registry entries:
Since An OpenAPI-aware payload validator that supports Do we need to explicitly list
So the argument seems to be:
|
Which was my original thought process as well. To which I'm thinking "no it doesn't harm". But I'm ok if we say we need to clean it up instead. I just want us to be consistent. |
@ralfhandl @baywet It's objectively wrong according to the JSON Schema specification to list "integer" as a data model type for the purpose of keyword or format applicability. Applicability of keywords and formats is only determined by the six data model types: number, string, boolean, null, object, and array. Anything else is not in compliance with JSON Schema. |
@handrews You are absolutely right from a pure JSON Schema perspective newer than Draft 04. OAS 3.0 and 3.1 (probably due to the removal of I've opened #4038 to discuss and fix this. |
@handrews I am unclear what the issue is here. Is it:
Or is it something else entirely? |
@ralfhandl @mikekistler I have tried to explain again in #4038. I don't know how to be more clear about this. The set of types that are data model types and the set of values for the We cannot change that. It is not part of our spec. It is part of JSON Schema. It doesn't matter what we say, it's not going to change the meaning of JSON Schema. |
Why are they different? Is integer the only difference here? In which case, why was it introduced? |
@baywet it's literally in the text you quoted elsewhere:
This explicitly states that it is not part of the data model, it's just a convenience for a specific keyword. |
@handrews I am not familiar with the concept of "data model types" in JSON Schema. Can you point to the part of the JSON Schema spec that describes this, so I can educate myself? |
Allow me to rephrase my question. You probably have the most historical context among us here over why JSON schema has this integer type. |
@mikekistler this is what is being referred to here I believe. https://json-schema.org/draft/2020-12/json-schema-core#name-instance-data-model |
Adjusted the other markdown file. |
LGTM! (ralf's latest changes) |
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.
I think we need a short explanation of the purpose of the Type column, and I think "Instance Type" is a slightly better term.
Co-authored-by: Mike Kistler <[email protected]>
@mikekistler Then why not use "Instance Type" also in #4045? |
As noted in #4045, I don't think most people know what an "instance" is, so "JSON Data Type" would be better. |
Signed-off-by: Vincent Biret <[email protected]>
alright, I reverted back to JSON data type everyone. Final review and merge? |
@baywet Almost there 😎 |
@mikekistler Please check, and if you approve, please merge. Thanks! |
Thanks everyone! now we just need @mikekistler to approve. |
@mikekistler Could you please re-review? The wording is now in line with #4045 |
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.
Looks good! 👍
Thanks! We're ready to merge. Who has the permissions to do so? |
Me, already merged. Thanks for all the work you put into this! |
context microsoft/kiota#5147
https://json-schema.org/draft/2020-12/json-schema-validation#name-type
https://spec.openapis.org/oas/latest.html#data-types
This adds the missing integer alternative type for integer formats.