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

Pkglock nested deps #779

Merged
merged 6 commits into from
Jan 25, 2022
Merged

Pkglock nested deps #779

merged 6 commits into from
Jan 25, 2022

Conversation

skilly-lily
Copy link
Contributor

Overview

When a package-lock.json has the following structure (simplified for clarity), we were not reporting the edge between js-yaml and argparse 2.0.1:

{
  "dependencies": {
    "argparse": {
      "version": "1.0.10",
      "requires": {
        "sprintf-js": "~1.0.2"
      }
    },
    "js-yaml": {
      "version": "4.1.0",
      "requires": {
        "argparse": "^2.0.1"
      },
      "dependencies": {
        "argparse": {
          "version": "2.0.1",
        }
      }
    },
    "sprintf-js": {
      "version": "1.0.3",
    }
  }
}

This was causing argparse 2.0.1 to not be reported, since it was then pruned in the SourceUnit conversion (it was an orphan node in the graph without that edge).

To resolve this, we carry the parent into the recursive dep analysis, and add an edge when analyzing the child.
When implementing, another oversight was revealed: When adding edges from packages under the requires key, we try to detect the package's version from the top-level deps. Now, we look at the local dependecies block first (at the same level of the requires that we're currently working on), before falling back to the top-level dependencies package list.

Acceptance criteria

  • Tests cover the failing case described in the linked issue
    • Specifically, we test that an edge is created between js-yaml and argparse 2.0.1 in the example above. This causes pruneUnreachable to not delete argparse 2.0.1, which would otherwise be an orphan node.
  • Tests pass

Testing plan

Unit testing should be sufficient

References

Fixes fossas/team-analysis#874

Checklist

  • I added tests for this PR's change (or confirmed tests are not viable).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • I updated Changelog.md if this change is externally facing. If this PR did not mark a release, I added my changes into an # Unreleased section at the top.
  • I updated *schema.json if I have made changes for .fossa.yml, fossa-deps.{json, yaml, yml}. You may also need to update these if you have added/removed new dependency (e.g. pip) or analysis target type (e.g. poetry).
  • I linked this PR to any referenced GitHub issues, if they exist.

* Add edges for recursing parents
* Try to resolve package versions from package-local
dep lists, then fallback to global.
@skilly-lily skilly-lily requested review from a team and cnr and removed request for a team January 21, 2022 20:53
Copy link
Contributor

@cnr cnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@skilly-lily skilly-lily merged commit d208da6 into master Jan 25, 2022
@skilly-lily skilly-lily deleted the pkglock-nested-deps branch January 25, 2022 19:07
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

Successfully merging this pull request may close these issues.

None yet

2 participants