Conversation
… match the corresponding title name as well
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #38103 +/- ##
==========================================
+ Coverage 65.86% 65.89% +0.02%
==========================================
Files 2392 2393 +1
Lines 190711 190950 +239
Branches 8469 8469
==========================================
+ Hits 125611 125822 +211
- Misses 53684 53695 +11
- Partials 11416 11433 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughExpands software search functionality to include matching by software title name in addition to software name and version. When a search query is provided, a LEFT JOIN to software_titles is added and title name is included in filter conditions, enabling discovery of all software versions grouped under a matching title. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @changes/35028-software-versions-search:
- Line 1: Update the user-facing description that currently says "Searching
software versions by name will now return all versions that match the
corresponding title name as well." to clarify what "title name" means; replace
it with either a more explicit phrase such as "Searching software versions by
name will now return all versions that match the software's title (the display
name shown in Fleet) in addition to its internal name" or a shorter alternative
like "Searching software versions by name will also match the software's display
title." Modify the description text in the
changes/35028-software-versions-search entry so users clearly understand that
"title" refers to the display name shown in Fleet, not an internal identifier.
In @server/datastore/mysql/software_test.go:
- Around line 10346-10416: The local variable titleID is assigned but never used
causing a Go compile error; either remove the unused GetContext block or use the
retrieved ID to assert grouping. Fix by updating
testListSoftwareVersionsSearchByTitleName: after creating initialSoftware,
either delete the titleID query entirely, or change it to fetch a value you
assert (e.g. fetch the title name or re-query software rows to assert their
title_id equals the retrieved id) using ds.writer(ctx).GetContext and then use
that variable in an assertion so it is referenced (symbols: titleID, ds.writer,
testListSoftwareVersionsSearchByTitleName, ListSoftware).
🧹 Nitpick comments (1)
server/datastore/mysql/software.go (1)
2050-2080: Verify join order dependencyThe
needsTitleJoinflag is set based onMatchQuery != "", which creates a dependency onneedsSoftwareJoinalso being true (line 2050). The code correctly ensures the software table (s) is joined first (line 2072) before the software_titles table (st) referencess.title_id(line 2079).This dependency is implicit and could be fragile if the conditions on lines 2050-2051 ever diverge. Consider adding an assertion or comment to make this dependency explicit:
needsSoftwareJoin := opts.ListOptions.MatchQuery != "" needsTitleJoin := opts.ListOptions.MatchQuery != "" // Requires needsSoftwareJoin to be true for s.title_id reference
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changes/35028-software-versions-searchserver/datastore/mysql/software.goserver/datastore/mysql/software_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/datastore/mysql/software.goserver/datastore/mysql/software_test.go
🧬 Code graph analysis (2)
server/datastore/mysql/software.go (2)
server/platform/mysql/list_options.go (1)
ListOptions(14-23)server/fleet/app.go (1)
ListOptions(1253-1277)
server/datastore/mysql/software_test.go (1)
server/fleet/software.go (3)
Software(83-149)Software(151-153)SoftwareListOptions(634-652)
🔇 Additional comments (3)
server/datastore/mysql/software_test.go (1)
106-107: Good test wiring; ensures the regression runs as part ofTestSoftware.
No concerns with adding the new case entry.server/datastore/mysql/software.go (2)
1875-1884: LGTM - Well-implemented search enhancementThe LEFT JOIN to
software_titleswhen searching is correctly implemented:
- The join is only added when
MatchQueryis provided, avoiding unnecessary overhead- LEFT JOIN is appropriate since not all software entries may have a
title_id- The join condition
s.title_id = st.idis correct- The same enhancement is consistently applied to both the list query here and the count query (lines 2076-2080)
The implementation correctly handles NULL values from the LEFT JOIN - when
st.nameis NULL, the ILIKE condition on line 1936 won't match, which is the expected behavior.
2122-2128: Join dependency is correctly implemented and testedThe search clause references
scv.cve(line 2126), which depends on thesoftware_cvetable being joined via the LEFT JOIN added whenneedsCVEJoinis true on line 2052. This is correctly guaranteed wheneverMatchQuery != "".The OR-based WHERE clause handles the LEFT JOIN correctly: rows matching any condition (software name, version, CVE, or title name) are included, so NULL values in
scv.cvefrom the LEFT JOIN don't incorrectly filter out matches on other columns.The existing test
testListSoftwareVersionsSearchByTitleNamevalidates this works as intended—searching for a title name returns all software entries under that title, even when their individual names differ. The implicit dependency betweenneedsCVEJoinand the search clause creates fragility to future modifications, but the current implementation is correct.
ksykulev
left a comment
There was a problem hiding this comment.
Looks good to me. Just had a quick non-blocking comment.
Related issue: Resolves #35028
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.