vacuum icon indicating copy to clipboard operation
vacuum copied to clipboard

Ambiguous paths that should not be 0.10.0

Open Ryefalk opened this issue 1 year ago • 9 comments

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

Ryefalk avatar May 31 '24 11:05 Ryefalk

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

daveshanley avatar May 31 '24 12:05 daveshanley

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 avatar May 31 '24 14:05 daveshanley

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.

Ryefalk avatar May 31 '24 14:05 Ryefalk

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 avatar May 31 '24 15:05 daveshanley

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 avatar May 31 '24 15:05 daveshanley

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

daveshanley avatar May 31 '24 15:05 daveshanley

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?

Ryefalk avatar May 31 '24 15:05 Ryefalk

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

daveshanley avatar May 31 '24 17:05 daveshanley

@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 https://github.com/OAI/OpenAPI-Specification/issues/3256

LasneF avatar Jun 14 '24 12:06 LasneF

The rule has been upgraded and handles more use cases and will also look at parameter types.

Available in v0.17.11

daveshanley avatar Aug 30 '25 04:08 daveshanley