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
560 stars 45 forks source link

False positives in unused components rule #531

Open lobocv opened 3 weeks ago

lobocv commented 3 weeks ago

It seems that if the spec is organized into multiple .yaml files (say one for each path item), and the sub-yaml files refer to components (defined in the root yaml), the check for unused components does not work.

Now I have been investigating this with a debugger and I found the issue. It's comprised of two parts:

  1. Where the rule gathers "all" references, it does not do so recursively on the sub-yaml files.

Changing the line to this solves it:

var allRefs = make(map[string]*index.Reference)
for _, idx := range context.Document.GetRolodex().GetIndexes() {
refs := idx.GetAllReferences()
maps.Copy(allRefs, refs)
}

We may need to do the same change for mappedRefs here too.

  1. After the code changes I mention in part 1 are implemented, I have the full list of references and I see the component in the allRefs map. However, the key it uses is slightly different. The code checks two keys: key and altKey. key seems to be a relative path while altKey is the absolute path. The maps only contain absolute paths so i'm only focused on the value of altKey now.

The keys in the maps refer to the actual root filename (openapi.yaml) while the key being checked uses "root.yaml" instead. In other words:

It looks for: /Users/calvin.lobo/hs/billing-plans-idl/schema/provider/root.yaml#/components/schemas/PlanType but the map contains: /Users/calvin.lobo/hs/billing-plans-idl/schema/provider/openapi.yaml#/components/schemas/PlanType

Digging deeper into where this "root.yaml" comes from, I see that it is set here in libopenapi. According to this comment, it seems like "root.yaml" was chosen as a theoretical root file

// if there is a base path, then we need to set the root spec config to point to a theoretical root.yaml
// which does not exist, but is used to formulate the absolute path to root references correctly.

image image

From my perspective, it can be solved two ways:

  1. Instead of using "root.yaml", use the real root file name. This should be possible since it's passed as an argument to the lint command.
  2. In the indexer, build all references using the theoretical root.

I'm not sure which solution you would prefer @daveshanley.

I've attached a tar ball of an example that recreates the issue. unused-components-bug.tar.gz

lobocv commented 3 weeks ago

So I can "solve" issue 2 by changing the theoretical root file to be "openapi.yaml". This doesn't really solve it, but I think it's a better default than "root.yaml". Most people name their root spec file openapi.yaml.

So one option is to change "root.yaml" ---> "openapi.yaml". The other option is to detect and pass down the real root file. I'm looking into this right now. Unfortunately it is not readily available so I need to pass it down all the way from the lint CLI command.

lobocv commented 3 weeks ago

I have a PR in libopenapi to fix the theoretical root file and use the real filename instead. Once this merges I can make the PR in vacuum with the recursive reference checking

lobocv commented 3 weeks ago

Here is the fix for this ticket. It relies on the PR in libopenapi above https://github.com/daveshanley/vacuum/pull/532