Skip to content

feat: Add asynchronous SBOM support - #4543

Open
tunztunztunz wants to merge 3 commits into
google:masterfrom
tunztunztunz:feat/async-sbom
Open

feat: Add asynchronous SBOM support#4543
tunztunztunz wants to merge 3 commits into
google:masterfrom
tunztunztunz:feat/async-sbom

Conversation

@tunztunztunz

Copy link
Copy Markdown

Adds GenerateSBOM and FetchSBOM to generate and retrieve SBOMs asynchronously.

  • Rate limits: Fetching uses the core allowance, so exhausting the SBOM allowance does not block polling. Confirmed through x-ratelimit-resource on live requests.
  • Response shape: Downloads contain the SPDX document without the legacy endpoint’s sbom wrapper. I compared both responses from one repository and found the same fields, so FetchSBOM wraps the download to return the existing SBOM type.
  • Authentication: The download client must omit authentication because signed URLs reject tokens. GitHub checks repository access before redirecting.

Testing

Tests cover generation, polling, redirects, downloads, errors, and rate-limit selection. Manually tested generation and download with a scratch program against large public repositories and private repositories.

Questions

  • Should download errors redact signed URLs? Transport errors can include the full URL, potentially exposing access to the report until the link expires.
  • Is SBOMGeneration.UUID() worth keeping? Generation and fetching are usually called together, so I figured this would save callers the trouble of extracting the UUID themselves.
  • Should FetchSBOM return the existing SBOM type for consistency with GetSBOM, or return SBOMInfo directly to match the unwrapped download?

AI disclosure: assisted with implementation, tests, and this PR body.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Sep 11, 2026
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.54%. Comparing base (2433615) to head (d401671).

Files with missing lines Patch % Lines
github/dependency_graph.go 97.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4543      +/-   ##
==========================================
- Coverage   98.54%   98.54%   -0.01%     
==========================================
  Files         196      196              
  Lines       17938    17981      +43     
==========================================
+ Hits        17677    17719      +42     
- Misses        261      262       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gmlewis

gmlewis commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Questions

  • Should download errors redact signed URLs? Transport errors can include the full URL, potentially exposing access to the report until the link expires.

No, in general we do not attempt to anticipate what users of these methods wish to do with the responses.
So I recommended that we pass back the response and let the user decide what to do after that.

  • Is SBOMGeneration.UUID() worth keeping? Generation and fetching are usually called together, so I figured this would save callers the trouble of extracting the UUID themselves.

No, for the same reason stated above.

  • Should FetchSBOM return the existing SBOM type for consistency with GetSBOM, or return SBOMInfo directly to match the unwrapped download?

I find having a // GetSBOM fetches the software bill of materials for a repository. AND a FetchSBOM to be very confusing from a user's standpoint. "Should I GetSBOM which fetches, or should I FetchSBOM instead?"

I'm thinking that either we don't need both or we need better names for both and maybe much clearer documentation as to WHEN to use one method over another.

@tunztunztunz

Copy link
Copy Markdown
Author

I find having a // GetSBOM fetches the software bill of materials for a repository. AND a FetchSBOM to be very confusing from a user's standpoint. "Should I GetSBOM which fetches, or should I FetchSBOM instead?"

I agree. I was a little worried about submitting a PR that removes existing functionality 😅

On November 13 GitHub plans to replace the synchronous workflow used by GetSBOM with the asynchronous workflow this PR is adding. Is it reasonable to remove it altogether in this PR or would you prefer deprecating it now with a comment and looping back on the November 13 to fully remove it after the endpoint is retired?

I'm now wondering if it would be clearer to split FetchSBOM into two explicitly named methods, like DownloadSBOM which downloads and returns the report, andGetSBOMDownloadURL, which only return the temporary download URL. The current implementation follows the pattern established by RepositoriesService.DownloadReleaseAsset, where supplying a client controls whether the redirect is followed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NeedsReview PR is awaiting a review before merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants