Refactor to support related purl#51
Merged
Merged
Conversation
…results. Improved VERSION_NOT_FOUND detection
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralize component version resolution into a new resolver, remove local semver/URL-selection helpers, and refactor multiple use cases to consume resolved metadata with optional source-purl fallback and shared filtering helpers. ChangesComponent Resolution Centralization and Use Case Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/usecase/cryptography_search.go (1)
45-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the resolved purl type before selecting URLs.
This path now matches KB rows by
ComponentNameand then narrows only by exact version. BecauseComponentCryptoMetadatano longer carries the resolvedPurlType, same-name packages from different ecosystems can share URLs and return crypto hits for the wrong component. Please keep the resolved type in metadata and filter the selected URLs by both type and version before buildingurlHashes.Also applies to: 181-203, 224-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/usecase/cryptography_search.go` around lines 45 - 54, ComponentCryptoMetadata currently loses the resolved purl type which lets same-name packages across ecosystems share SelectedURLS and produce incorrect crypto hits; add a PurlType field to ComponentCryptoMetadata to preserve the resolved type, populate it wherever metadata is created, and update the URL selection logic that builds urlHashes (the code that filters SelectedURLS/models.AllURL) to filter by both PurlType and exact Version before constructing urlHashes; also apply the same change in the other two places noted (the other URL-selection blocks that use SelectedURLS and build urlHashes).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/usecase/component_resolver_test.go`:
- Around line 26-83: Add a test case to TestSourcePurlForFallback that covers a
SourcePurl with Status.Success and a non-empty PurlInfo.Name but an empty
PurlInfo.PurlType (e.g., PurlInfo{Name: "jonschlinkert/word-wrap"}), and assert
that sourcePurlForFallback(component) returns ok == false; reference the structs
SourcePurl, PurlInfo, componenthelper.Component and the function
sourcePurlForFallback when adding this case to the existing tests.
In `@pkg/usecase/component_resolver.go`:
- Around line 55-59: The function sourcePurlForFallback currently returns
ok=true if c.SourcePurl.Name is non-empty but ignores c.SourcePurl.PurlType;
update the guard in sourcePurlForFallback to also require c.SourcePurl.PurlType
!= "" (and that Status.StatusCode == status.Success) before returning (name,
purlType, true) so callers never get an empty purlType; reference the
componenthelper.Component's SourcePurl, SourcePurl.Status.StatusCode,
SourcePurl.Name and SourcePurl.PurlType fields when making this change.
- Around line 93-97: The loop building ordered results assumes every key exists
in the byKey map and will silently produce zero-value componenthelper.Component
entries if a helper failed; update the assembly in component_resolver.go (the
ordered := … loop that iterates over keys and reads byKey[k]) to check map
presence (e.g., v, ok := byKey[k]) for each k, collect any missing keys, and
handle them explicitly—either log the missing keys via the resolver's logger and
return a descriptive error from the resolver method (or alternatively skip/omit
missing entries if intended), ensuring you reference componenthelper.Component,
keys, byKey and the function that builds/returns ordered to locate the change.
- Around line 123-125: The purl-type filter currently only skips when both
purlType and u.PurlType are non-empty and unequal, allowing entries with empty
u.PurlType to pass when a purlType is specified; change the logic so any time
the incoming purlType parameter is non-empty and u.PurlType does not equal it
(including when u.PurlType is empty) the entry is skipped—update the conditional
that references purlType and u.PurlType in component_resolver.go (the place
where u.PurlType is compared to purlType) to use a single check: if purlType !=
"" && u.PurlType != purlType { continue } so empty u.PurlType values are
excluded when filtering by a specific purlType.
- Line 114: In filterUrlsInRange: broaden the wildcard check and tighten
purlType filtering — replace the requirement check that uses
strings.HasPrefix(requirement, "v*") with a test that treats any presence of a
v-style wildcard or an operator-prefixed wildcard as a skip (e.g.,
strings.Contains(requirement, "v*") and also trim and check for operator+v*
patterns) so semver expressions like ">=v*" or ">v*" are rejected too; and
change the purlType filter from "if purlType != "" && u.PurlType != "" &&
u.PurlType != purlType" to require a non-empty match (e.g., when purlType != ""
only keep URLs where u.PurlType != "" && u.PurlType == purlType) so entries with
empty u.PurlType are excluded when a purlType filter is supplied.
In `@pkg/usecase/cryptography_major.go`:
- Around line 97-101: The DB/query error handling in the block calling
d.allUrls.GetUrlsByPurlList (and the similar block around lines 125-131)
currently logs the error then converts it into a domain "not found/no info"
status by setting c.Status to status.ComponentNotFound/NoInfo; instead, preserve
and propagate the underlying storage error (or translate it to a distinct
backend-failure status) rather than treating it as business data. Update the
error handling in the GetUrlsByPurlList call (and the analogous query at
125-131) to return the error up the stack (or set c.Status to a dedicated
backend-failure status) after logging via s.Debugf, and only set
ComponentNotFound/NoInfo when you have confirmed the data truly does not exist
(e.g., zero results with no query error).
In `@pkg/usecase/cryptography_search.go`:
- Line 125: The function declaration for applySourceFallback exceeds the
line-length linter; break the long signature into multiple lines to satisfy lll
by wrapping parameters, e.g., place the receiver and function name on one line
and put parameters (ctx context.Context, s *zap.SugaredLogger, output
*domain.CryptoOutput, metadata []ComponentCryptoMetadata, components
[]dtos.ComponentDTO) each or grouped across subsequent lines; update the
signature in the applySourceFallback method accordingly so the line length is
under the linter limit.
In `@pkg/usecase/cryptography_versions_using.go`:
- Around line 95-98: The code in GetVersionsInRangeUsingCrypto currently
swallows backend/model/DB errors by setting item.Status to
status.ComponentNotFound or status.ComponentWithoutInfo (via assignments like
item.Status = status.ComponentStatus{StatusCode: status.ComponentNotFound, ...})
and returning; instead, either return the original error from
GetVersionsInRangeUsingCrypto (bubble it up) or set a dedicated internal-failure
status (e.g., status.ComponentInternalError) so real failures are
distinguishable from business "not found" results—replace the current
error-to-ComponentNotFound/ComponentWithoutInfo mappings around the error
handling blocks (where s.Debugf logs the error and item.Status is set) with code
that either returns the error to the caller or assigns a specific internal
failure ComponentStatus while preserving the original error message for logging.
- Around line 94-112: The code currently fetches URLs via
d.allUrls.GetUrlsByPurlList using purlName only and then treats an empty
post-filter result as VersionNotFound; change this so you distinguish "component
not found for this ecosystem" from "no version matches": either query
GetUrlsByPurlList with both purlName and purlType (if supported) or immediately
filter the returned urls by purl.Type == purlType (using the variable purlType)
before calling filterUrlsInRange; after this type-filter, if
len(typeFilteredUrls) == 0 set item.Status to status.ComponentNotFound (using
status.ComponentNotFound and the same message format), otherwise call
filterUrlsInRange(typeFilteredUrls, purlType, item.Requirement) and keep the
existing handling that sets status.VersionNotFound when the result is empty.
In `@pkg/usecase/library_detections.go`:
- Line 251: GetUrlsByPurlList + filterUrlsInRange can return mixed-type rows
because utils.PurlReq has no type and filterUrlsInRange only rejects mismatches
when both sides are non-empty; update the logic so callers can avoid cross-type
collisions: either extend utils.PurlReq to include a PurlType and have
allUrls.GetUrlsByPurlList filter by m.purl_type when PurlType is provided, or
(simpler) change filterUrlsInRange to also reject rows where a requested
purlType is non-empty but the returned u.PurlType is empty (i.e., discard rows
with empty u.PurlType when purlType != ""), referencing utils.PurlReq,
allUrls.GetUrlsByPurlList and filterUrlsInRange to locate where to implement the
change.
- Around line 53-66: The loop indexing is fine, but you must normalize any
"file:" prefixed requirement before passing it to range filtering to avoid
semver.NewConstraint treating it as invalid; in GetDetectionsInRange/where you
call processSinglePurl and ultimately filterUrlsInRange, strip the "file:"
prefix (or use the resolved item's c.OriginalRequirement/normalized requirement)
so filterUrlsInRange receives a plain semver requirement (or empty string to
mean "latest"), ensuring semver.NewConstraint won't return status.InvalidSemver;
update calls that use components[i].Requirement to use the normalized
requirement from resolveComponentVersions (or c.OriginalRequirement) instead.
---
Outside diff comments:
In `@pkg/usecase/cryptography_search.go`:
- Around line 45-54: ComponentCryptoMetadata currently loses the resolved purl
type which lets same-name packages across ecosystems share SelectedURLS and
produce incorrect crypto hits; add a PurlType field to ComponentCryptoMetadata
to preserve the resolved type, populate it wherever metadata is created, and
update the URL selection logic that builds urlHashes (the code that filters
SelectedURLS/models.AllURL) to filter by both PurlType and exact Version before
constructing urlHashes; also apply the same change in the other two places noted
(the other URL-selection blocks that use SelectedURLS and build urlHashes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4d3f225-060e-47ea-ae07-18b6ceaf02a0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
go.modpkg/handlers/cryptography_support.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/common.gopkg/models/doc.gopkg/models/tests/licenses.sqlpkg/responsebuilder/test.jsonpkg/usecase/component_resolver.gopkg/usecase/component_resolver_test.gopkg/usecase/component_validator.gopkg/usecase/cryptography_major.gopkg/usecase/cryptography_search.gopkg/usecase/cryptography_versions_using.gopkg/usecase/docs.gopkg/usecase/library_detections.gopkg/utils/doc.gopkg/utils/semver.gopkg/utils/semver_test.go
💤 Files with no reviewable changes (8)
- pkg/utils/semver_test.go
- pkg/utils/semver.go
- pkg/usecase/component_validator.go
- pkg/handlers/cryptography_support.go
- pkg/models/doc.go
- pkg/models/all_urls.go
- pkg/utils/doc.go
- pkg/models/all_urls_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores