daveshanley / vacuum

vacuum is the worlds fastest OpenAPI 3, OpenAPI 2 / Swagger linter and quality analysis tool. Built in go, it tears through API specs faster than you can think. vacuum is compatible with Spectral rulesets and generates compatible reports.
https://quobix.com/vacuum
MIT License
617 stars 51 forks source link

Ambiguous paths that should not be 0.10.0 #504

Open Ryefalk opened 5 months ago

Ryefalk commented 5 months ago

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 commented 5 months ago

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

daveshanley commented 5 months ago

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

These two paths can both be

/a/z/b/c/d

Ryefalk commented 5 months ago

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/vacuum@0.9.16 vacuum lint openapi.yml -d

It does not give any errors for this one.

npm i -g @quobix/vacuum@0.10.0 vacuum lint openapi.yml -d

This version gives the errors.

daveshanley commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

LasneF commented 5 months ago

@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