Skip to content

Validate fragments are allowed in path keys #4458

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

Closed

Conversation

duncanbeevers
Copy link
Contributor

@duncanbeevers duncanbeevers commented Mar 19, 2025

🗣️ Discussion

Related to Slack Discussion about valid vs lint-happy schemas.

This PR adds a test case asserting that paths entries including a URL fragment are valid, despite the fragment being functionally meaningless.

PRs which enforce rules forbidding such paths would not be compliant with the spec, so this test case makes explicit the answer to that question.

If this PR is merged, future PRs proposing lint rules forbidding fragments in paths keys will fail, and be forced to address the fact that their proposal represents opinion rather than conformance to the specification.

@duncanbeevers duncanbeevers requested review from a team as code owners March 19, 2025 01:17
@handrews
Copy link
Member

@duncanbeevers apologies for not thinking this example through in the slack discussion- I wasn't treating it as a PR-ready proposal.

This one is actually tricky. The specification for patterned fields here reads:

A relative path to an individual endpoint. The field name MUST begin with a forward slash (/). The path is appended (no relative URL resolution) to the expanded URL from the Server Object’s url field in order to construct the full URL. Path templating is allowed. When matching URLs, concrete (non-templated) paths would be matched before their templated counterparts. Templated paths with the same hierarchy but different templated names MUST NOT exist as they are identical. In case of ambiguous matching, it’s up to the tooling to decide which one to use.

This clearly requires the value to be a path as defined by RFC3986, modified by the path templating syntax, which reads in part:

The value for these path parameters MUST NOT contain any unescaped “generic syntax” characters described by [RFC3986] Section 3: forward slashes (/), question marks (?), or hashes (#).

Now, that path templating bit apples to the path parameter values and not the path template, but it demonstrates that this is all about paths (as including ? or # would end the path and cause the remaining characters to be interpreted differently.

I would argue that the refernce to paths is sufficiently clear, and the meaning of "path" sufficienly well-defined across a very, very large number of specifications, that entries in the Paths Object are in fact forbidden from including ? or #. So in this case, I would actually support adding such validation to the schema... except it's even more complex because of the lack of constraints on template variable names, so writing a regex that really captures this would be non-trivial.

This is another reason I dislike getting too precise with the schemas. At some point you really hit diminishing returns.

@handrews
Copy link
Member

I'm going to close this because I'm confident that this specific example is actually not allowed, and the general point is better suited to a discussion than a PR or issue.

@handrews handrews closed this Mar 19, 2025
@duncanbeevers
Copy link
Contributor Author

@handrews Thanks for the PR feedback.

I guess there are two discussion to be had…

  • Are path fragments allowed in paths keys? (@handrews Your answer seems pretty definitive here)
  • Should we introduce test cases asserting the validity of anti-patterns?

@duncanbeevers
Copy link
Contributor Author

duncanbeevers commented Mar 19, 2025

Kinda pushing back on this…

If this test case should fail then maybe it should be put into the fail corpus, (for the same reasons discussed above ☝️).

@handrews
Copy link
Member

I mostly closed the PR because it feels like a can of worms - someone is going to show up and disagree with me on this, and that level of arguing is better handled in an issue or discussion.

I don't think this PR is at the stage of agreement on whether there's a good addition here or how to handle it. There's also the question of changes to the schema. This needs further consideration before we decide what PRs, if any, to add. None of which should be taken as a rejection of the idea that something is likely worth doing here.

At least that's my take. If someone else wants to take on resolving this and wants to do it here, please feel free to re-open.

@handrews
Copy link
Member

Of course it's hard to tell what to open where given the historical state of the repo, but we have actually recently added guidance on this to CONTRIBUTING.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants