fix(sqlite): select latest search result semantically#6643
fix(sqlite): select latest search result semantically#6643niheaven merged 5 commits intoScoopInstaller:developfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughMoves latest-version selection from SQL ordering to in-memory semantic comparison: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (2)
test/Scoop-Database.Tests.ps1 (1)
1-53: LGTM — covers the two regression cases from#6419.Setup dot-sources
versions.ps1beforedatabase.ps1soCompare-Versionis available, and the threeItblocks directly exercise the regressions (1.0.31 > 1.0.7,7.0.20 > 7.0.9) for both grouped and ungrouped paths.Optional: consider adding coverage for an empty
DataTable(exercises therows.Count -eq 0short-circuit inSelect-LatestScoopDBRows), equal versions (tie‑breaking behavior), and a pre-release like1.0.0vs1.0.0-rc1to lock inCompare-Version's semver semantics. Not required for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Scoop-Database.Tests.ps1` around lines 1 - 53, Ensure versions.ps1 is dot-sourced before database.ps1 so Compare-Version is available to the database helpers: confirm the BeforeAll block sources "versions.ps1" prior to "database.ps1" (affecting Compare-Version used by Select-LatestScoopDBRows and Get-LatestScoopDBRow) and keep the three It blocks as-is to validate the regression cases; optionally add tests for an empty DataTable, equal-version ties, and a pre-release comparison to exercise edge cases.lib/database.ps1 (1)
3-48: Helpers look correct; consider singular noun per PowerShell convention.
Get-LatestScoopDBRowusesCompare-Versioncorrectly (the> 0comparison updates$latestwhen$row.version > $latest.version), andSelect-LatestScoopDBRowscorrectly clones schema via$Table.Clone(), handles the zero-rows case, and imports one row per group. The lazy dot-source guard on lines 3–5 is clean.One optional improvement: PSScriptAnalyzer flags
PSUseSingularNounsonSelect-LatestScoopDBRows. PowerShell convention prefers singular nouns (e.g.Get-Process) even when emitting multiple results. Renaming toSelect-LatestScoopDBRowwould align withGet-LatestScoopDBRow,Select-ScoopDBItem, andGet-ScoopDBItemin this file. The function is already used in tests (test/Scoop-Database.Tests.ps1, lines 31 and 48) and internally (lib/database.ps1, lines 315 and 379), so the rename would require updating those four call sites—still straightforward, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/database.ps1` around lines 3 - 48, Rename the plural-named function Select-LatestScoopDBRows to the singular Select-LatestScoopDBRow to follow PowerShell PSUseSingularNouns convention; update the function declaration and all call sites that reference Select-LatestScoopDBRows (including tests test/Scoop-Database.Tests.ps1 and internal uses in lib/database.ps1) so they call Select-LatestScoopDBRow, and ensure the exported/manifest names (if any) are updated accordingly so behavior and tests remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/database.ps1`:
- Around line 3-48: Rename the plural-named function Select-LatestScoopDBRows to
the singular Select-LatestScoopDBRow to follow PowerShell PSUseSingularNouns
convention; update the function declaration and all call sites that reference
Select-LatestScoopDBRows (including tests test/Scoop-Database.Tests.ps1 and
internal uses in lib/database.ps1) so they call Select-LatestScoopDBRow, and
ensure the exported/manifest names (if any) are updated accordingly so behavior
and tests remain identical.
In `@test/Scoop-Database.Tests.ps1`:
- Around line 1-53: Ensure versions.ps1 is dot-sourced before database.ps1 so
Compare-Version is available to the database helpers: confirm the BeforeAll
block sources "versions.ps1" prior to "database.ps1" (affecting Compare-Version
used by Select-LatestScoopDBRows and Get-LatestScoopDBRow) and keep the three It
blocks as-is to validate the regression cases; optionally add tests for an empty
DataTable, equal-version ties, and a pre-release comparison to exercise edge
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd0b2840-2976-4763-b759-20a9889248c7
📒 Files selected for processing (2)
lib/database.ps1test/Scoop-Database.Tests.ps1
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/database.ps1 (1)
7-22: Sign convention is correct; consider a defensive guard for empty input.The logic correctly handles version comparison:
Compare-Versionreturns1whenDifferenceVersionis newer thanReferenceVersion, so promoting$rowwhen the return value is> 0correctly selects the latest version.However, passing an empty
$Rowsarray would silently return$nullsince$Rows[0]evaluates to$null. Currently,Select-LatestScoopDBRowguards$rows.Count -eq 0before calling this function, so there's no live bug—but this is fragile if another caller appears.🛡️ Optional defensive guard
function Get-LatestScoopDBRow { param( [Parameter(Mandatory)] [object[]] $Rows ) + if ($null -eq $Rows -or $Rows.Count -eq 0) { + return $null + } + $latest = $Rows[0] foreach ($row in ($Rows | Select-Object -Skip 1)) { if ((Compare-Version -ReferenceVersion $latest.version -DifferenceVersion $row.version) -gt 0) { $latest = $row } } return $latest }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/database.ps1` around lines 7 - 22, Get-LatestScoopDBRow currently assumes $Rows[0] exists; add a defensive guard at the start of the function to handle null or empty input (e.g., if (-not $Rows -or $Rows.Count -eq 0) then either throw a clear ArgumentException or return $null) so callers like Select-LatestScoopDBRow and any future callers won't rely on implicit $null behavior; place the check at the top of Get-LatestScoopDBRow and document the chosen behavior in the function comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/database.ps1`:
- Around line 7-22: Get-LatestScoopDBRow currently assumes $Rows[0] exists; add
a defensive guard at the start of the function to handle null or empty input
(e.g., if (-not $Rows -or $Rows.Count -eq 0) then either throw a clear
ArgumentException or return $null) so callers like Select-LatestScoopDBRow and
any future callers won't rely on implicit $null behavior; place the check at the
top of Get-LatestScoopDBRow and document the chosen behavior in the function
comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dafe0f34-6665-45c9-b0f2-b40ad00caccf
📒 Files selected for processing (2)
lib/database.ps1test/Scoop-Database.Tests.ps1
✅ Files skipped from review due to trivial changes (1)
- test/Scoop-Database.Tests.ps1
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/database.ps1 (2)
386-390: Branching on$Versionis correct, but a clarifying comment would help.When
$Versionis supplied, the(name, bucket, version)PRIMARY KEY guarantees at most one row, so bypassingSelect-LatestScoopDBRowis fine. When it isn't supplied, delegating toSelect-LatestScoopDBRow -Table $result(no-GroupBy, since rows are already constrained to a single name+bucket) returns a single-row DataTable matching the previousLIMIT 1shape. A one-line comment explaining why-GroupByis intentionally omitted here (as opposed toSelect-ScoopDBItem) would make the asymmetry obvious to future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/database.ps1` around lines 386 - 390, Add a one-line clarifying comment above the conditional return explaining why we bypass Select-LatestScoopDBRow when $Version is provided and why Select-LatestScoopDBRow is called without -GroupBy when $Version is null; reference the PRIMARY KEY (name, bucket, version) guaranteeing at most one row when $Version is set and note that omitting -GroupBy here differs from Select-ScoopDBItem because the result is already constrained to a single name+bucket pair.
35-59: Add comment-based help and a small iteration tidy-up.
Get-LatestScoopDBRowhas a<# .SYNOPSIS ... #>block butSelect-LatestScoopDBRowdoes not — the rest of this file consistently documents public helpers (seeGet-SQLite,Open-ScoopDB,Select-ScoopDBItem,Get-ScoopDBItem). Worth adding a brief help block describing-Table,-GroupBy, and the cloned-DataTable return contract so future callers understand that group keys come from the DataRow column names.Separately, the implementation itself is correct (clone preserves schema,
Group-Object -Property @('name','bucket')keys by the composite tuple, and empty-table short-circuit returns the cloned empty table). Minor: inGet-LatestScoopDBRowat Line 26 you could drop| Select-Object -Skip 1in favor of an indexed loop to avoid the pipeline overhead, but it's not behavior-affecting.📝 Example help block
+<# +.SYNOPSIS + Return a DataTable containing only the semantically latest row(s). +.DESCRIPTION + Clones the schema of $Table and imports the latest row per group (when -GroupBy + is supplied, grouped by the given DataRow column names) or the single latest + row across the whole table. Returns an empty cloned table when $Table has no rows. +.PARAMETER Table + The source System.Data.DataTable produced by the Scoop DB query. +.PARAMETER GroupBy + Optional column names used to group rows before latest-row selection + (e.g. @('name','bucket')). +#> function Select-LatestScoopDBRow {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/database.ps1` around lines 35 - 59, Add a comment-based help block for Select-LatestScoopDBRow describing the -Table (System.Data.DataTable), -GroupBy (string[]), and that it returns a cloned DataTable preserving schema with group keys matching DataRow column names; include SYNOPSIS, PARAMETER entries for -Table and -GroupBy, and a brief RETURNS description. Also tidy Get-LatestScoopDBRow by removing the pipeline "| Select-Object -Skip 1" and replace it with an indexed loop over the rows collection (use numeric indexing or a for loop) to skip the first item without pipeline overhead; keep function names Select-LatestScoopDBRow and Get-LatestScoopDBRow as the targets of these edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/database.ps1`:
- Around line 14-33: The parameter binding prevents empty arrays from being
passed into Get-LatestScoopDBRow because [Parameter(Mandatory)] on [object[]]
rejects empty collections; add the [AllowEmptyCollection()] attribute to the
Rows parameter declaration so callers (e.g., tests calling Get-LatestScoopDBRow
-Rows @()) can pass an empty array and the function's existing empty-input guard
(return $null) will execute; alternatively, if callers are guaranteed never to
pass empty arrays, remove the unreachable empty-input guard instead—update the
Rows parameter in Get-LatestScoopDBRow or delete the guard accordingly to make
behavior consistent with Select-LatestScoopDBRow.
---
Nitpick comments:
In `@lib/database.ps1`:
- Around line 386-390: Add a one-line clarifying comment above the conditional
return explaining why we bypass Select-LatestScoopDBRow when $Version is
provided and why Select-LatestScoopDBRow is called without -GroupBy when
$Version is null; reference the PRIMARY KEY (name, bucket, version) guaranteeing
at most one row when $Version is set and note that omitting -GroupBy here
differs from Select-ScoopDBItem because the result is already constrained to a
single name+bucket pair.
- Around line 35-59: Add a comment-based help block for Select-LatestScoopDBRow
describing the -Table (System.Data.DataTable), -GroupBy (string[]), and that it
returns a cloned DataTable preserving schema with group keys matching DataRow
column names; include SYNOPSIS, PARAMETER entries for -Table and -GroupBy, and a
brief RETURNS description. Also tidy Get-LatestScoopDBRow by removing the
pipeline "| Select-Object -Skip 1" and replace it with an indexed loop over the
rows collection (use numeric indexing or a for loop) to skip the first item
without pipeline overhead; keep function names Select-LatestScoopDBRow and
Get-LatestScoopDBRow as the targets of these edits.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65a50e07-f567-4353-a79a-328bcd1ab5a9
📒 Files selected for processing (2)
lib/database.ps1test/Scoop-Database.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Scoop-Database.Tests.ps1
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
|
Thanks for the jobs! Please rebase the PR to develop branch. |
niheaven
force-pushed
the
fix/sqlite-search-version-order
branch
from
928bce2 to
efc3b7f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/database.ps1 (1)
52-76: Ordering may be non-deterministic; consider whether stable results are needed.The
Select-LatestScoopDBRowfunction correctly clones the table schema and imports rows viaImportRow. However,Find-ScoopDBItem(called bylibexec/scoop-search.ps1:161) returns results from this function withoutORDER BY, meaning rows are emitted in the orderGroup-Objectfirst encounters each group. Sincelibexec/scoop-search.ps1displays these results without subsequent sorting, users may see unstable or unexpected ordering across searches. If consistent, user-facing ordering is needed, consider sorting by a stable key (e.g., name) before returning from eitherSelect-LatestScoopDBRowor from the downstream search command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/database.ps1` around lines 52 - 76, Select-LatestScoopDBRow currently returns rows in the order Group-Object yields groups, causing non-deterministic user-visible ordering downstream (seen by Find-ScoopDBItem / libexec/scoop-search.ps1); fix by applying a deterministic sort (e.g., by the stable key "Name" or another chosen column) before returning: after building $latestRows (or before ImportRow in the grouped branch) perform a stable Sort-Object on the relevant column(s) or set $latestRows.DefaultView.Sort and return the sorted table so search/display consumers always receive a consistent order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/database.ps1`:
- Around line 52-76: Select-LatestScoopDBRow currently returns rows in the order
Group-Object yields groups, causing non-deterministic user-visible ordering
downstream (seen by Find-ScoopDBItem / libexec/scoop-search.ps1); fix by
applying a deterministic sort (e.g., by the stable key "Name" or another chosen
column) before returning: after building $latestRows (or before ImportRow in the
grouped branch) perform a stable Sort-Object on the relevant column(s) or set
$latestRows.DefaultView.Sort and return the sorted table so search/display
consumers always receive a consistent order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e1ffd48-77a1-4624-92f5-c37ec2d1b6f6
📒 Files selected for processing (2)
lib/database.ps1test/Scoop-Database.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Scoop-Database.Tests.ps1
2 participants
Summary
Compare-VersionWhy
SQLite stores cached manifest versions as plain text, so
ORDER BY version DESCcan prefer1.0.7over1.0.31and7.0.9over7.0.20whenuse_sqlite_cacheis enabled.This keeps the cache fast for filtering, while moving the final "which version is latest" decision back to Scoop's existing semantic version logic.
Closes #6419.
Addresses #6639.
Verification
Select-LatestScoopDBRowsreturns1.0.31over1.0.77.0.20over7.0.9test/Scoop-Database.Tests.ps1Summary by CodeRabbit
Bug Fixes
Tests