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

Ambiguous paths that should not be 0.10.0 #504

Open
Ryefalk opened this issue May 31, 2024 · 9 comments
Open

Ambiguous paths that should not be 0.10.0 #504

Ryefalk opened this issue May 31, 2024 · 9 comments
Labels
invalid This doesn't seem right

Comments

@Ryefalk
Copy link

Ryefalk commented May 31, 2024

When running vacuum 0.10.0 Two paths that should not be ambiguous are flagged as such. But the simpler example it can manage. This problem did not exist in the previous version we used 0.9.10 so somewhere between these two versions this problem appeared.
This is a minimal working example of the issue.

openapi: 3.0.2
info:
  title: asdf
  version: 0.0.0
  description: |
    asdf REST API.

paths:
  # These two work.
  /a/{Id1}:
    parameters:
      - $ref: "#/components/parameters/Id1"
      
  /a/b:
    description: /a/b

  # These two are ambiguous even though they should not be.
  /a/{Id1}/b/c/{Id3}:
    parameters:
      - $ref: "#/components/parameters/Id1"
      - $ref: "#/components/parameters/Id3"

  /a/{Id1}/b/{Id2}/d:
    parameters:
      - $ref: "#/components/parameters/Id1"
      - $ref: "#/components/parameters/Id2"

components:
  parameters:
    Id1:
      in: path
      name: Id1
      description: Id for a
      required: true
      schema:
        $ref: "#/components/schemas/id"
    Id2:
      in: path
      name: Id2
      description: Id for b
      required: true
      schema:
        $ref: "#/components/schemas/id"
    Id3:
      in: path
      name: Id3
      description: Id for c
      required: true
      schema:
        $ref: "#/components/schemas/id"

  schemas:
    id:
      description: Numeric Id
      type: integer
      format: int32
      readOnly: true
@daveshanley
Copy link
Owner

hmmm... I see this also, will investigate, I don't think this rule changed at all.

@daveshanley
Copy link
Owner

The code has not changed at all in this area to suddenly start changing the behavior of this rule.

Looking at the logic and looking at your spec.. it's behaving correctly.

 /a/{Id1}/b/c/{Id3}: 

 /a/{Id1}/b/{Id2}/d: 

Are actually ambiguous.

/a/ <-- matches

  • /{Id1}/ <-- matches
    • /b/ <-- matches
      • /c && {Id2} <-- matches (there is no way to know which is which)
        • /d && {id3} <-- matches (there is no way to know which is which)

These two paths can both be

/a/z/b/c/d

@daveshanley daveshanley added invalid This doesn't seem right and removed needs investigation labels May 31, 2024
@Ryefalk
Copy link
Author

Ryefalk commented May 31, 2024

But this should be allowed in the openapi specification as far as I can read. Id2 is required to be an integer and c is not one. Why would this then still be ambiguous? I see the point if Id2 could be anything but it can't. And the behavior changed this I know. Though i don't know why if there has been no change to it. And why then does the first two work and not the next two, should it not be the same case for those?

# These two work.
  /a/{Id1}:
    parameters:
      - $ref: "#/components/parameters/Id1"
      
  /a/b:
    description: /a/b

npm i -g @quobix/[email protected]
vacuum lint openapi.yml -d

It does not give any errors for this one.

npm i -g @quobix/[email protected]
vacuum lint openapi.yml -d

This version gives the errors.

@daveshanley
Copy link
Owner

The rule is not checking the types of the parameters, it is just looking at the paths themselves and the parameter positions on the path. So the logic is accurate - what is missing is a parameter type check also. That would be an upgrade.

I will recompile 0.9.16 from source and re-run this test. Because I am confused as to how the version change altered behavior, this code has not been touched (the logic in the rule anyway).

@daveshanley
Copy link
Owner

daveshanley commented May 31, 2024

OK the reason why 0.9.16 does not show this error, is because the rule was never running in the first place. This was the goal of v0.10.0 to clear up all loose ends with functions and rule names. There were a few typos and mixups that mean that certain functions were not running and rules were passing inaccurately.

In this case noAmbiguousPaths was not running (the function) (https://github.com/daveshanley/vacuum/blob/v0.9.16/functions/functions.go#L95)

Because the built in rule was calling the wrong thing. (ambiguousPaths) https://github.com/daveshanley/vacuum/blob/v0.9.16/rulesets/ruleset_functions.go#L1180

There were a few of these issues that were cleared up in v0.10.0. Which means results will have changed, and in all likely hood, reduced scores across the board, but those scores are now more accurate.

@daveshanley
Copy link
Owner

This logic was also lifted from the spectral rule of the same name. It's the same logic, just implemented in a different language.

@Ryefalk
Copy link
Author

Ryefalk commented May 31, 2024

So untill the rule is updated with support for types I assume the only thing we can do is to disable the rule? Or is there some other thing I can do?

@daveshanley
Copy link
Owner

Yes, it's due for an upgrade soon anyway - but there is no timeline available. The only option is to disable the rule.

@LasneF
Copy link

LasneF commented Jun 14, 2024

@Ryefalk , @daveshanley notice that there are long running ticket on path templating model in OAS , so it s no 100 % clear (at least to me) if those path should be considered as ambigous or not
OAI/OpenAPI-Specification#3256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants