fix(all_urls):SP-4237 add is_mined and package_hash filter condition…#12
fix(all_urls):SP-4237 add is_mined and package_hash filter condition…#12agustingroh wants to merge 1 commit intomainfrom
Conversation
…s to all_urls queries
📝 WalkthroughWalkthroughThis PR adds filtering constraints to exclude unmined records and empty package hashes across multiple data retrieval methods in the all_urls model. It includes corresponding changelog documentation and a debug logging statement in the component service. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 12: Update the changelog entry to also list the CheckPurlByNameType query
as being modified: editing the line that currently mentions
GetURLsByPurlNameType, GetURLsByPurlNameTypeVersion, and
GetVersionsByPurlNameType to add CheckPurlByNameType (the function in
pkg/models/all_urls.go around the CheckPurlByNameType implementation) so the
release notes reflect that CheckPurlByNameType now also includes the `is_mined
!= false AND package_hash != ''` filter.
In `@pkg/models/all_urls.go`:
- Around line 74-76: The new WHERE predicates referencing all_urls.is_mined and
all_urls.package_hash in pkg/models/all_urls.go break older DBs; add a
compatibility check at startup (e.g., query PRAGMA table_info('all_urls') or
information_schema for your DB) to detect presence of columns is_mined and
package_hash, expose two booleans (hasIsMined, hasPackageHash), and change the
SQL builder(s) in pkg/models/all_urls.go that currently concatenate "AND
u.is_mined != false" / "AND u.package_hash != ''" (the three query strings
shown) to conditionally append those predicates only when the corresponding
boolean is true, or provide a migration path that adds the columns and fixtures
before using the new queries.
🪄 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: d1a855e4-6c20-43fc-a06d-87528e744bbe
📒 Files selected for processing (3)
CHANGELOG.mdpkg/models/all_urls.gopkg/services/component.go
|
|
||
| ## [0.10.0] - 2026-04-06 | ||
| ### Fixed | ||
| - Added `is_mined != false AND package_hash != ''` filter conditions to `GetURLsByPurlNameType`, `GetURLsByPurlNameTypeVersion`, and `GetVersionsByPurlNameType` queries to exclude unmined and empty-hash records |
There was a problem hiding this comment.
Changelog entry is missing one updated query method.
Line [12] documents three methods, but this PR also changes CheckPurlByNameType in pkg/models/all_urls.go (Lines [178-179]). Please include it to keep release notes accurate.
Suggested changelog tweak
-- Added `is_mined != false AND package_hash != ''` filter conditions to `GetURLsByPurlNameType`, `GetURLsByPurlNameTypeVersion`, and `GetVersionsByPurlNameType` queries to exclude unmined and empty-hash records
+- Added `is_mined != false AND package_hash != ''` filter conditions to `GetURLsByPurlNameType`, `GetURLsByPurlNameTypeVersion`, `GetVersionsByPurlNameType`, and `CheckPurlByNameType` queries to exclude unmined and empty-hash records📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Added `is_mined != false AND package_hash != ''` filter conditions to `GetURLsByPurlNameType`, `GetURLsByPurlNameTypeVersion`, and `GetVersionsByPurlNameType` queries to exclude unmined and empty-hash records | |
| - Added `is_mined != false AND package_hash != ''` filter conditions to `GetURLsByPurlNameType`, `GetURLsByPurlNameTypeVersion`, `GetVersionsByPurlNameType`, and `CheckPurlByNameType` queries to exclude unmined and empty-hash records |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 12, Update the changelog entry to also list the
CheckPurlByNameType query as being modified: editing the line that currently
mentions GetURLsByPurlNameType, GetURLsByPurlNameTypeVersion, and
GetVersionsByPurlNameType to add CheckPurlByNameType (the function in
pkg/models/all_urls.go around the CheckPurlByNameType implementation) so the
release notes reflect that CheckPurlByNameType now also includes the `is_mined
!= false AND package_hash != ''` filter.
| " WHERE m.purl_type = $1 AND u.purl_name = $2" + | ||
| " AND u.is_mined != false AND u.package_hash != ''" + | ||
| " ORDER BY date DESC" |
There was a problem hiding this comment.
Blocking: new filters break queries on current DB schema.
The added predicates on Lines [75], [115], [148], and [179] are causing runtime SQL failures (no such column: ...is_mined) per pipeline logs. This currently breaks all affected lookup paths.
Please ship this with matching schema support (migration + fixtures) or add a compatibility path for DBs that don’t yet have all_urls.is_mined / all_urls.package_hash.
Also applies to: 114-116, 147-149, 178-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/models/all_urls.go` around lines 74 - 76, The new WHERE predicates
referencing all_urls.is_mined and all_urls.package_hash in
pkg/models/all_urls.go break older DBs; add a compatibility check at startup
(e.g., query PRAGMA table_info('all_urls') or information_schema for your DB) to
detect presence of columns is_mined and package_hash, expose two booleans
(hasIsMined, hasPackageHash), and change the SQL builder(s) in
pkg/models/all_urls.go that currently concatenate "AND u.is_mined != false" /
"AND u.package_hash != ''" (the three query strings shown) to conditionally
append those predicates only when the corresponding boolean is true, or provide
a migration path that adds the columns and fixtures before using the new
queries.
scanoss-qg
left a comment
scanoss-qg
left a comment
There was a problem hiding this comment.
Adding a parameter could be interesting for those cases where mined data is not needed
| " LEFT JOIN versions v ON u.version_id = v.id" + | ||
| " WHERE m.purl_type = $1 AND u.purl_name = $2 ORDER BY date DESC" | ||
| " WHERE m.purl_type = $1 AND u.purl_name = $2" + | ||
| " AND u.is_mined != false AND u.package_hash != ''" + |
There was a problem hiding this comment.
Could is_mined flag be configurable throw a parameter?
agustingroh
deleted the
fix/SP-4237-add-filter-conditions-for-not-mined-urls
branch
2 participants
…s to all_urls queries
Summary by CodeRabbit
Bug Fixes
Chores