Skip to content
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

Annotated enums and extended validation #4111

Merged
merged 5 commits into from
Sep 26, 2024
Merged

Conversation

handrews
Copy link
Member

[This PR brought to you by insomnia... I know we said the releases are closed but I've had to explain these things a couple of times in the past week or two, and I realized that I've written the recommendations in issues and discussions many times to generally positive feedback. It seems worthwhile to include them, and we're waiting on Mike's wording changes anyway which have become a longer discussion.]

This tidies up the increasingly long Schema Object section and adds explanations of what I've been calling "extended validation", including validating readOnly and writeOnly, and also using a oneOf or anyOf with const plus annotations for enumerated types with additional information.

@handrews handrews added schema-object clarification requests to clarify, but not change, part of the spec labels Sep 24, 2024
@handrews handrews added this to the v3.1.1 milestone Sep 24, 2024
@handrews handrews requested a review from a team as a code owner September 24, 2024 13:57
@handrews handrews force-pushed the extended branch 2 times, most recently from ce8afae to f2b2585 Compare September 24, 2024 14:16
This tidies up the increasingly long Schema Object section and adds
explanations of what I've been calling "extended validation",
including validating `readOnly` and `writeOnly`, and also using
a `oneOf` or `anyOf` with `const` plus annotations for enumerated
types with additional information.

There are many OAS issues/discussions where we have recommended
these techniques, so it makes sense to include them in 3.1.1 where
draft 2020-12's formal collection of annotations enables tools
to build support for them.
Copy link
Contributor

@ralfhandl ralfhandl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, minor nits

versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
versions/3.1.1.md Outdated Show resolved Hide resolved
@ralfhandl
Copy link
Contributor

ralfhandl commented Sep 24, 2024

Please run

bash scripts/format-markdown.sh versions/3.1.1.md

Validation of these keywords MAY be done by checking the annotation, the read or write direction, and (if relevant) the current value of the field.
[JSON Schema Validation draft 2020-12 §9.4](https://datatracker.ietf.org/doc/html/draft-bhutton-json-schema-validation-00#section-9.4) defines the expectations of these keywords, including that resource (described as the "owning authority") MAY either ignore a `readOnly` field or treat it as an error.

An example of where ignoring a "written" `readOnly` field might be appropriate is a PUT request where the field is included but the value has not been changed, since the alternative of leaving out the field would result in the field's deletion per [[RFC7231]].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the example here is a bit off. A readOnly field is essentially "set by the server" -- and that would apply to both the initial creation of a resource as well as a replace of the resource. So I don't think it's accurate to say that omitting the field would result in its deletion.

I think a better argument for ignoring readOnly fields sent in a request body is to avoid requiring a client to remove these fields when updating a resource with a GET-followed-by-PUT sequence of operations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikekistler I'll update this tonight - I mostly agree with you but want to think on the right way to frame it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikekistler I've updated it and removed discussion of PUT semantics (even though I really, really, want to rant about PUT semantics... PUT is so misunderstood). I'm open to further tweaks but I think what I have shows a couple of closerly related use cases. Which feels a bit better than focusing on exactly one. I think it will encourage folks to think about the possibilities more. But I am definitely open to changing it further.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note on PUT: I never got around to consolidate my rants about PUT into a blog post on "How to use PUT without shooting yourself in the foot" that I can reference instead of writing a new rant - a blatant violation of the DRY principle 😎

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ralfhandl I, too, have a barely-started blog post on PUT that I should really finish...


The Schema Object's `enum` keyword does not allow associating descriptions or other information with individual values.

Implementations MAY support recognizing a `oneOf` or `anyOf` where each subschema in the keyword's array consists of a `const` keyword and annotations such as `title` or `description` as an enumerated type with additional information. The exact behavior of this pattern beyond what is required by JSON Schema is implementation-defined.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I like this! Great addition!

Co-authored-by: Ralf Handl <ralf.handl@sap.com>
@handrews
Copy link
Member Author

@ralfhandl running that script did not appear to do anything?

@handrews
Copy link
Member Author

We could backport the annotated enumeration pattern to 3.0 by replacing const with a one-element enum but I wasn't sure if it was worth the effort. Annotation collection is not a thing in the JSON Schema draft used by 3.0, so most of this is not relevant.

@ralfhandl
Copy link
Contributor

running that script did not appear to do anything?

It did reformat the JSON example a bit: 9e1e71d

@ralfhandl ralfhandl requested a review from a team as a code owner September 24, 2024 16:02
ralfhandl
ralfhandl previously approved these changes Sep 24, 2024
@ralfhandl ralfhandl self-requested a review September 24, 2024 16:10
@ralfhandl ralfhandl dismissed their stale review September 24, 2024 16:10

overlooked Mike's comment

@handrews
Copy link
Member Author

@ralfhandl

It did reformat the JSON example a bit

weird, git couldn't detect any changes after I ran it locally. I'll try to figure that out later- thanks for running it on my behalf!

@handrews
Copy link
Member Author

@ralfhandl that script still didn't do anything with my latest update, and I can't seem to get it to do anything even by introducing errors. Is it supposed to take parameters? It doesn't look like it.

@ralfhandl
Copy link
Contributor

ralfhandl commented Sep 25, 2024

Is it supposed to take parameters?

Yes, a list of markdown files to reformat, for example

scripts/format-markdown.sh versions/3.1.1.md

On my machine it for example changes dashed bullet lists into starred bullet lists and formats code blocks with Prettier.

Without parameters it silently loops over the empty set and does nothing.

@handrews
Copy link
Member Author

Without parameters it silently loops over the empty set and does nothing.

Oh somehow I read that as looping over the specs by default... (it was late and I'd been up since 4:30AM, my shell script reading skills were not at their best). Would it not make more sense to automatically apply to the specs?

Really, I want a make target so I don't have to keep remembering which random scripts are expected, but I find Makefiles incomprehensible most of the time so I've never set one up.

@ralfhandl
Copy link
Contributor

Would it not make more sense to automatically apply to the specs?

Only the work-in-progress releases should be automatically reformatted, and only before publishing, so this is something that I want to add to the new dev branch(es) as an npm script.

Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@miqui miqui merged commit 4e9c5d6 into OAI:v3.1.1-dev Sep 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification requests to clarify, but not change, part of the spec schema-object
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants