feat(maven): capture groupId namespace location in extractor output#176
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: fbe15c9 | Docs | Datadog PR Page | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d9cbdcbff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if fileposition.IsFilePositionExtractedSuccessfully(p.BlockLocation) { | ||
| pkg.Locations = make([]models.PackageLocations, 1) | ||
| pkg.Locations[0] = location.NewPackageLocations(p.BlockLocation, p.NameLocation, p.VersionLocation, p.LocationRole) | ||
| pkg.Locations[0] = location.NewPackageLocations(p.BlockLocation, p.NameLocation, p.VersionLocation, p.NamespaceLocation, p.LocationRole) |
There was a problem hiding this comment.
When scan results are converted to repo-relative paths, datadog_sbom_generator.go only normalizes Source, BlockLocation, NameLocation, and VersionLocation (lines 314-323), so passing the new p.NamespaceLocation here leaves Maven namespace filenames as absolute paths in the SBOM. This affects CLI scans of Maven POMs with a groupId location and leaks host-specific scan roots while the rest of the occurrence remains relative; the updated snapshots already show namespace.file_name containing <rootdir>/... next to relative block/name paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. NamespaceLocation path relativization is already applied at pkg/scanner/datadog_sbom_generator.go:325-326 — snapshots confirm relative paths (e.g. pom.xml, not absolute).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0d8b2bcae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| BlockLocation: blockLocation, | ||
| LocationRole: models.LocationRoleManifest, | ||
| NameLocation: &artifactPosition, | ||
| NamespaceLocation: &groupIDPosition, |
There was a problem hiding this comment.
For child POMs that inherit <groupId> from a parent and declare a dependency as <groupId>${project.groupId}</groupId>, ResolveGroupID returns the inherited parent's FilePosition, but resolvePropertiesValue labels project.* positions with lockfile.MainSourceFile unless the property is version; after mergeLockfiles, that is the child file. Passing that position through here emits a namespace location with the child filename and the parent's line/column numbers, so SBOM evidence points to the wrong text for common multi-module Maven projects. Track the source file for inherited project.groupId similarly to ProjectVersionSourceFile, or avoid exporting this namespace location when the source file is ambiguous.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Added a guard to skip NamespaceLocation when groupId is an inherited ${project.*} property reference — in that case the resolved position carries parent POM line/column mapped onto the child file, which would produce incorrect SBOM evidence. Commit c59c9e4.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c59c9e4fbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // resolved position carries the parent POM's line/column mapped onto the child file, | ||
| // which would point to the wrong text. | ||
| var namespaceLocation *models.FilePosition | ||
| if !strings.HasPrefix(lockPackage.GroupID.Value, "${project.") { |
There was a problem hiding this comment.
Fresh evidence beyond the earlier ${project.*} case: resolvePropertiesValue treats the deprecated but supported pom. prefix as a project. alias, so a child POM that inherits its groupId and declares a dependency as <groupId>${pom.groupId}</groupId> still hits the same bad-position path. Because this guard only skips ${project., NamespaceLocation is emitted with the inherited parent's line/column mapped onto the child filename after mergeLockfiles, causing SBOM evidence to point at the wrong text; handle ${pom. the same way or track the actual source file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Extended the guard to also cover ${pom.*} — the deprecated alias for ${project.*} that Maven treats identically.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c07e089a31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !strings.HasPrefix(lockPackage.GroupID.Value, "${project.") { | ||
| namespaceLocation = &groupIDPosition | ||
| } |
There was a problem hiding this comment.
When a child POM inherits its groupId and a dependency uses an intermediate property such as <dep.group>${project.groupId}</dep.group> followed by <groupId>${dep.group}</groupId>, resolvePropertiesValue recursively returns the inherited parent line/column mapped onto the child file, but this guard only checks the dependency's raw value for ${project.. Fresh evidence beyond the existing direct ${project.*}/${pom.*} comments is that an ordinary property alias still reaches the same bad resolved position, so the emitted namespace evidence points at unrelated text in the child POM; detect this resolved-project path or fall back to the dependency <groupId> element instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Indirect property aliases are an edge case that can be addressed in a follow-up — this PR already handles the common ${project.*} and ${pom.*} direct references.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a75371dff8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !strings.HasPrefix(lockPackage.GroupID.Value, "${project.") && | ||
| !strings.HasPrefix(lockPackage.GroupID.Value, "${pom.") { |
There was a problem hiding this comment.
When a POM defines its own <groupId> and a dependency uses <groupId>${project.groupId}</groupId> (or ${pom.groupId}), this guard drops the namespace even though the resolved position is valid in the current POM: resolvePropertiesValue maps project.groupId to lockfile.MainSourceFile when the child/project itself has the groupId. The bad inherited-parent case only applies when the project groupId came from a parent, so this loses SBOM groupId evidence for self-references in otherwise normal Maven projects; suppress only when the resolved project groupId is actually inherited or ambiguous.
Useful? React with 👍 / 👎.
- Add NamespaceLocation *models.FilePosition to PackageDetails struct - Add NamespaceLocation to cmpOptions ignore list in test helpers Rationale: The Maven extractor needs to capture groupId file positions for namespace location tracking. This adds the field to the shared type and updates the test infrastructure so existing tests that ignore locations continue to work. This commit made by [/dd:git:commit:atomic](https://github.com/DataDog/claude-marketplace/tree/main/dd/commands/git/commit/atomic.md)
- Add namespace *models.FilePosition parameter to NewPackageLocations signature - Populate PackageLocations.Namespace when a valid namespace position is provided - Add TestNewPackageLocations_RolePropagatedToNamespace test - Add TestNewPackageLocations_NilNamespaceRemainsNil test - Update all existing test calls to pass nil for the new namespace parameter Rationale: NewPackageLocations needs to accept a namespace file position so that extractors can thread namespace location data (e.g., Maven groupId position) through to the output model. The existing PackageLocations.Namespace field was already defined but never populated. This commit made by [/dd:git:commit:atomic](https://github.com/DataDog/claude-marketplace/tree/main/dd/commands/git/commit/atomic.md)
- Update the single call site in vulnerability_result.go to pass p.NamespaceLocation Rationale: With NewPackageLocations now accepting a namespace parameter, the caller must thread the NamespaceLocation field from PackageDetails through so that extractors that populate it (e.g., Maven) have their data reach the output model. This commit made by [/dd:git:commit:atomic](https://github.com/DataDog/claude-marketplace/tree/main/dd/commands/git/commit/atomic.md)
- Capture groupId position from ResolveGroupID() instead of discarding it - Fall back to lockPackage.GroupID.FilePosition when resolution returns empty position - Store groupId position in PackageDetails.NamespaceLocation field - Add NamespaceLocation assertions to all 50+ Maven extractor test cases Rationale: The Maven extractor was discarding the groupId position returned by ResolveGroupID(). This change captures and stores it as NamespaceLocation so downstream consumers can locate the namespace (groupId) in the source pom.xml, completing the location data pipeline for Maven dependencies. This commit made by [/dd:git:commit:atomic](https://github.com/DataDog/claude-marketplace/tree/main/dd/commands/git/commit/atomic.md)
…tion - Update Maven-related integration test snapshots to include namespace field - Snapshots now reflect groupId file position in location JSON for all Maven packages - Covers UTF-8, UTF-16, Windows-1252 encoding variants and multi-module projects Rationale: The integration test snapshots are auto-generated outputs that must be updated to reflect the new namespace location data now emitted by the Maven extractor. This commit made by [/dd:git:commit:atomic](https://github.com/DataDog/claude-marketplace/tree/main/dd/commands/git/commit/atomic.md)
…ests The NamespaceLocation.Filename was not being relativized in the scanner's path normalization loop, causing absolute paths to leak into output on Windows (and into snapshots on all platforms). Added the missing ToRelativePath call alongside the existing Block/Name/Version handlers. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Avoids emitting a NamespaceLocation whose line/column numbers point to the parent POM while the filename resolves to the child file after mergeLockfiles, which would produce incorrect SBOM evidence for multi-module Maven projects.
- extend the ${project.*} guard to cover ${pom.*}, the deprecated alias
that Maven resolves identically
a75371d to
fbe15c9
Compare
2 participants
🚀 Motivation
The Maven extractor in datadog-sbom-generator was discarding the groupId file position returned by
ResolveGroupID(). While the output model (PackageLocations.Namespace) already supports a namespace location field, no extractor populated it. This means downstream consumers (e.g., Code Intelligence, quality gates) could not pinpoint the exact<groupId>declaration inpom.xmlfiles, limiting the precision of location-based features for Maven dependencies.Documentation
📝 Summary
Core pipeline changes (3 layers):
pkg/extractor/types.go— AddedNamespaceLocation *models.FilePositionfield toPackageDetailsstruct, allowing any extractor to report a namespace file position.internal/utility/location/location.go— ExtendedNewPackageLocations()to accept anamespace *models.FilePositionparameter and populatePackageLocations.Namespacewhen provided.pkg/scanner/vulnerability_result.go— Updated the single call site ofNewPackageLocationsto passNamespaceLocationthrough fromPackageDetails.Maven extractor changes:
pkg/extractor/java/parse-maven-lock.go— Captured the groupId position fromResolveGroupID()(previously discarded with_), added fallback tolockPackage.GroupID.FilePositionwhen resolution returns empty, and stored it asNamespaceLocationin thePackageDetailsstruct.Reporter refactoring (cleanup alongside main change):
reporter.Reporterparameter threading withlog.Printfacross JavaScript (match-package-json.go,types.go), Python (parse-pyproject-toml.go,pyproject_package_collector.go), Ruby (match-gemfile.go,match-gemspec.go), scanner (datadog_sbom_generator.go,vulnerability_result.go), and reachability (codefile/java.go) packages.reporter.Effective()helper function frompkg/reporter/void_reporter.go.Tests:
NamespaceLocationassertions across all Maven extractor test cases (single packages, multi-module parents, interpolation, scopes, exclusions, dependency management, etc.).TestNewPackageLocations_RolePropagatedToNamespaceandTestNewPackageLocations_NilNamespaceRemainsNiltolocation_test.go.namespacefield in location JSON output.🧪 Testing
🚧 Staging validation
🆘 Recovery
Notes for on-call - select only one: