Add support for `relationship` attribute in `DependencySubmission` payload #12768

phillmv · 2025-08-01T20:30:59Z

What are you trying to accomplish?

Per https://github.com/github/dependency-graph/issues/7121,

[for those of you who do not work at GitHub, the above link describes an effort we're undergoing to refactor technical debt and start using dependabot-core to parse dependencies for our dependency graph service.]

This PR is a follow up on #12734 . Now that we can generate payloads, the next step is to fill out the remaining attributes that the Dependency Submission API expects.

This PR then:

Adds Dependency#direct? as a bit of state we can track when parsing dependencies
Ensures this gets submitted in the payload

Anything you want to highlight for special attention from reviewers?

In order to get here there was a meandering path. The first step involved realizing that the DependencySet class squashes all of the Dependencies it has parsed and combines them into a flat list; this means that we could not produce an accurate list of the lockfile's dependencies because the overlapping ones would be assigned to the gemfile.

In order to get around this now associate dependencies directly with their DependencyFile via a new association DependencyFile#dependencies.

In order to start tracking whether a dependency is direct or transitive, we need to keep track of this within the Dependency class. At first we tried adding requirements objects to direct dependencies parsed from the lockfile but this caused weird test failures deep within Dependabot, so we skipped that. We eventually settled on adding a new Dependency#direct? method that either mimics top_level? or can be set directly via ivar on initialize.

How will you know you've accomplished your goal?

All tests pass & we deploy it and see it send requests to our API.

Checklist

I have run the complete test suite to ensure all tests and linters pass.
- YES but some of the e2e tests are failing. I see they're failing on other unrelated recently merged PRs so it feels safe for now – it doesn't make sense why these changes would impact npm parsing.
I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
I have written clear and descriptive commit messages.
I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
I have ensured that the code is well-documented and easy to understand.

…? Could we use it to track transitive dependencies?

This commit: - backs out previous WIP that added `dependency_file` to `Dependabot::Dependency` - introduces `Dependabot::DependencyFile#dependencies` list Now, for the bundler ecosystem at least, DependencyFiles included in DependencySnapshots will be aware of their Dependabot::Dependency objects – and DependencySubmission payloads can accurately reflect the contents of the specified manifests.

In order to create DependencySubmission payloads, we must keep track of dependencies on a per-DependencyFile level, and those dependency objects must know whether they are direct dependencies (`#top_level?`) and whether they are usined in `production?`. First we tried faking `Dependency#requirements` objects but this broke a number of tests; somewhere deep within Dependabot there is an assumption that lockfiles do not produce dependencies with requirements. Instead of trying to fix the tests, here is a different approach: - Instead of faking `requirements` objects, we instead directly supply the `top_level?` boolean at instantiation, i.e. before, top_level was a synonym for `requirements.any?` and now it is not. Hopefully this turns out to be a simpler modification going forward.

brrygrdn

👍🏻 On this approach, I'm nervous about changing the contract of top_level? but I also don't love having two methods meaning slightly different things in different contexts.

common/lib/dependabot/dependency_file.rb

common/lib/dependabot/dependency.rb

Instead of overriding `top_level?`, which on inspection is a leaky abstraction – it's often used elsewhere as a synonym for "has requirements" – here we introduce our own method that only means "this dependency has a direct relationship".

Co-authored-by: Barry Gordon <[email protected]>

…h-dep-files

common/lib/dependabot/dependency_file.rb

…h-dep-files

brrygrdn

I lack the real power of approval, but this LGTM

brrygrdn · 2025-08-07T15:53:50Z

updater/spec/github_api/dependency_submission_spec.rb

+      gemfile = payload[:manifests].fetch("/Gemfile")
+      expect(gemfile[:name]).to eq("/Gemfile")


I was going to ask why we were adding a / here, but on checking the API docs, this is just an arbitrary name. Comparing with a real world payload, I note people doing esoteric things like:

project-a/pom.xml > project-a

brrygrdn · 2025-08-07T15:54:14Z

updater/spec/github_api/dependency_submission_spec.rb

-      expect(capybara[:scope]).to eq("development")
+      # the lockfile should be reporting 4 direct dependencies and 24 indirect ones
+      expect(lockfile[:resolved].values.count { |dep| dep[:relationship] == "direct" }).to eq(4)
+      expect(lockfile[:resolved].values.count { |dep| dep[:relationship] == "indirect" }).to eq(24)


brrygrdn · 2025-08-07T15:55:30Z

Huh, I do not lack the real power of approval 😅 - I'll set it to neutral until a -er has a chance to review

brrygrdn

I approve in a non-binding way

jakecoffman

LGTM

We've been kind of... working around DependencySet for a while. If you think introducing a new data structure that doesn't smoosh dependencies into a set would be easier, feel free and we can work that back into Dependabot's update code at some point.

brrygrdn · 2025-08-08T13:16:01Z

We've been kind of... working around DependencySet for a while. If you think introducing a new data structure that doesn't smoosh dependencies into a set would be easier, feel free and we can work that back into Dependabot's update code at some point.

IIRC @phillmv and I talked about this lightly where we'd ideally replace DependencySet with a method that smooshed the right subset of dependencies from the DependencyFiles into a big list - but building both in parallel allows us to maybe do that as a feature flag on the long tail of this work?

…h-dep-files

github-actions bot added the L: ruby:bundler RubyGems via bundler label Aug 1, 2025

brrygrdn force-pushed the brrygrdn/experiment-dependency-submission branch from aa28e82 to 8782ea9 Compare August 4, 2025 11:31

brrygrdn mentioned this pull request Aug 4, 2025

[Experiment] Transform the dependency list into a dependency submission payload #12734

Merged

5 tasks

Base automatically changed from brrygrdn/experiment-dependency-submission to main August 4, 2025 15:40

phillmv added 3 commits August 4, 2025 18:15

WIP: what if Dependabot::Dependency kept track of its dependency_file…

c44475b

…? Could we use it to track transitive dependencies?

Removed superfluous commas, file

f02ba8a

brrygrdn force-pushed the phillmv/experiment-dependency-submission-with-dep-files branch from 2e98f11 to f02ba8a Compare August 4, 2025 17:15

brrygrdn self-assigned this Aug 4, 2025

brrygrdn added 2 commits August 5, 2025 12:53

Align depencency submission specs, Fix lint

e104b2b

Source should be an empty hash

4f72b98

brrygrdn force-pushed the phillmv/experiment-dependency-submission-with-dep-files branch from 36eed25 to 5c56412 Compare August 5, 2025 13:42

Avoid adding requirements outside our usecase

ea7d193

brrygrdn force-pushed the phillmv/experiment-dependency-submission-with-dep-files branch from 5c56412 to ea7d193 Compare August 5, 2025 14:00

brrygrdn reviewed Aug 6, 2025

View reviewed changes

common/lib/dependabot/dependency_file.rb Outdated Show resolved Hide resolved

common/lib/dependabot/dependency_file.rb Show resolved Hide resolved

common/lib/dependabot/dependency.rb Outdated Show resolved Hide resolved

common/lib/dependabot/dependency.rb Outdated Show resolved Hide resolved

phillmv and others added 8 commits August 6, 2025 15:57

Introduce Dependency#direct?

4e9f346

Instead of overriding `top_level?`, which on inspection is a leaky abstraction – it's often used elsewhere as a synonym for "has requirements" – here we introduce our own method that only means "this dependency has a direct relationship".

revert comment in bundler/file_parser.rb

730d8ae

Update common/lib/dependabot/dependency_file.rb

e6d1acc

Co-authored-by: Barry Gordon <[email protected]>

pointless busy work for sorbet

40de196

pointless busy work for sorbet

014c867

satisfy linter

202baf3

Merge branch 'main' into phillmv/experiment-dependency-submission-wit…

e8198aa

…h-dep-files

linting

ee7863b

phillmv commented Aug 7, 2025

View reviewed changes

common/lib/dependabot/dependency_file.rb Outdated Show resolved Hide resolved

Undo sig change linter weirdly flagged

dc9d7b9

phillmv changed the title ~~Adds support for relationship attribute in DependencySubmission payload~~ Add support for relationship attribute in DependencySubmission payload Aug 7, 2025

phillmv marked this pull request as ready for review August 7, 2025 15:40

phillmv requested a review from a team as a code owner August 7, 2025 15:40

Merge branch 'main' into phillmv/experiment-dependency-submission-wit…

8048644

…h-dep-files

brrygrdn approved these changes Aug 7, 2025

View reviewed changes

brrygrdn reviewed Aug 7, 2025

View reviewed changes

brrygrdn self-requested a review August 7, 2025 15:56

jakecoffman approved these changes Aug 7, 2025

View reviewed changes

Merge branch 'main' into phillmv/experiment-dependency-submission-wit…

856be51

…h-dep-files

phillmv merged commit 1980e12 into main Aug 8, 2025
178 of 180 checks passed

phillmv deleted the phillmv/experiment-dependency-submission-with-dep-files branch August 8, 2025 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for `relationship` attribute in `DependencySubmission` payload #12768

Add support for `relationship` attribute in `DependencySubmission` payload #12768

Uh oh!

phillmv commented Aug 1, 2025 •

edited

Loading

Uh oh!

brrygrdn left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brrygrdn left a comment

Uh oh!

brrygrdn Aug 7, 2025 •

edited

Loading

Uh oh!

brrygrdn Aug 7, 2025

Uh oh!

brrygrdn commented Aug 7, 2025

Uh oh!

brrygrdn left a comment

Uh oh!

jakecoffman left a comment

Uh oh!

brrygrdn commented Aug 8, 2025

Uh oh!

Uh oh!

Uh oh!

		gemfile = payload[:manifests].fetch("/Gemfile")
		expect(gemfile[:name]).to eq("/Gemfile")

Add support for relationship attribute in DependencySubmission payload #12768

Add support for relationship attribute in DependencySubmission payload #12768

Uh oh!

Conversation

phillmv commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are you trying to accomplish?

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

Checklist

Uh oh!

brrygrdn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brrygrdn left a comment

Choose a reason for hiding this comment

Uh oh!

brrygrdn Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brrygrdn Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

brrygrdn commented Aug 7, 2025

Uh oh!

brrygrdn left a comment

Choose a reason for hiding this comment

Uh oh!

jakecoffman left a comment

Choose a reason for hiding this comment

Uh oh!

brrygrdn commented Aug 8, 2025

Uh oh!

Uh oh!

Uh oh!

Add support for `relationship` attribute in `DependencySubmission` payload #12768

Add support for `relationship` attribute in `DependencySubmission` payload #12768

phillmv commented Aug 1, 2025 •

edited

Loading

brrygrdn left a comment •

edited

Loading

brrygrdn Aug 7, 2025 •

edited

Loading