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

False positives in unused components rule #531

Open
lobocv opened this issue Aug 16, 2024 · 3 comments
Open

False positives in unused components rule #531

lobocv opened this issue Aug 16, 2024 · 3 comments

Comments

@lobocv
Copy link

lobocv commented Aug 16, 2024

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
Copy link
Author

lobocv commented Aug 16, 2024

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
Copy link
Author

lobocv commented Aug 16, 2024

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
Copy link
Author

lobocv commented Aug 16, 2024

Here is the fix for this ticket. It relies on the PR in libopenapi above
#532

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

No branches or pull requests

1 participant