-
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
Clarify how Schema Objects require full-document parsing (3.1.1) #3758
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 may be missing some distinction, but this seems like the same behavior that's described as "implementation defined" in https://github.com/OAI/OpenAPI-Specification/pull/3732/files#diff-b92507e7acda65ae00a02236c555cefc68b6fca4661077b84c2fb9ab150e5e17R151
This section particularly addresses schema objects but the other PR seems to encompass schema objects too.
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.
@notEthan eh... (wobbles hand back and forth in a wishy-washy way). There's an overlap, but it's not quite the same. This is where we get into the "it's too hard to even explain" that I mentioned in the undefined/implementation-defined PR, but this ones not one of the worst so here's an explanation:
If you have a document that consists of an empty JSON object,
{}
, that document is syntactically valid as several different Objects. Parsing that document from different reference contexts is still full-document parsing, so it does not fall into this undefined behavior. But it is parsing using multiple conflicting contexts, so it falls into the implementation-defined behavior of the other PR.It's a pretty fine hair to split, but it is deterministic. The whole-document case is implementation-defined because at least one implementor I've spoken with did not think it required a fix in the spec. I'm trying to invalidate as few existing implementations as possible, which is tricky to balance with the scenarios that result in outright wrong behavior.
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.
Neither of these changes get into the deeper question of whether documents are parsed as JSON/YAML once or each time, or whether, once parsed as JSON/YAML, the resulting structures are further parsed as OAS Objects once or each time. That would start getting into whether various parsing steps are cached, and what is done when a cached Object doesn't agree with a new parsing context. There are many ways this might or might not be detected, and many strategies one could pick to handle it - new each time, first wins, last wins, any conflict is an error, etc.
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'm happy to accept that, and appreciate the explanation.
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 figured it might help folks to see what "complicated to explain" looks like :-)