-
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
Adjust uri to uri-reference for security schema fixed fields #4328
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 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.
This one is the twin to JSON Schema's
$schemakeyword, and therefore cannot be a uri-reference.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.
@karenetheridge philosophically I agree with you, but the way the OAS field is written unfortunately allows a relative URI-reference. Of course you'd have to resolve that before you could use it as a dialect URI, and the only way that might work is with a custom dialect, but there's no reason it couldn't work that way.
I do agree that it should have been written as requiring a non-relative URI.
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.
Happy to make the change if needed. Waiting for maintainer to come to an agreement.
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.
@handrews what do you mean "the way the OAS field is written unfortunately allows a relative URI-reference"? What are you referencing here?
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.
@karenetheridge It's the combination of two things:
§4.6.7 states:
while the fixed fields description for
jsonSchemaDialectsays:Strictly speaking, this text allows a URI reference. The value used with
$schemaof course must be a full URI, but that can be produced by resolving the relative URI-reference normally, just as with any other field that the OAS says "MUST be in the form of a URI". What is used with$schemais the resolved value, not the literal text.At least that is my interpretation. We can always kick it up to @OAI/tsc for a ruling if you disagree. Again, I prefer your reading (and really dislike that we don't just explicitly say "URI-reference" and instead say "URI means URI-reference unless otherwise stated") but that's what we have to work with.
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.
At the very least we need to tighten up the language that leaves this vague. It is not reasonable to say in one place that a value is a URI, and then at the top say "actually that means URI-reference" because now there's no way to say that no, really, this actually has to be a fully-resolved URI.
Also, in any place where we do allow a URI-reference, we should be explicit as to how that value will be resolved -- whether it's against the URI used to identify the entry document, the current document, or the Host header on the current HTTP request under consideration (when relevant) -- I can see places where each of these might be the appropriate choice for URI resolution.
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.
@karenetheridge take a look at the XML Object's
namespacefield for how we say a field is a fully-resolve URI (again, I agree we should use the correct terminology per RFC3986 and not what we're doing now, but we do have ways to say we don't want a non-relative reference). We are very explicit about how OAD URI-referencess and API URL-references are resolved. Each of those topics gets a full section in the 3.1 specification. In any event, this is drifting outside of the scope of this PR and should continue in a discussion or issue.