Sort host device_mapping emails deterministically#48602
Conversation
The device_mapping GROUP_CONCAT in the host list and label host list queries had no ORDER BY, so emails came back in an unspecified order that varied between executions. This caused flaky test failures and inconsistent API output. Sort by email, source to match the single-host device mapping query. Claude-Session: https://claude.ai/code/session_01J77VQSSuPptxpoZDrFQxdj
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48602 +/- ##
==========================================
- Coverage 68.01% 67.97% -0.04%
==========================================
Files 3678 3678
Lines 233760 233760
Branches 12267 12267
==========================================
- Hits 158986 158908 -78
- Misses 60475 60535 +60
- Partials 14299 14317 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThis PR makes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)server/datastore/mysql/hosts.go[{"text":"deviceAuthTokenQuery = ... [truncated 1339 characters] ... olumn":7},"end":{"line":2997,"column":161}},"style":"primary"},{"text":"deviceAuthTokenQuery","range":{"byteOffset":{"start":109813,"end":109833},"start":{"line":2997,"column":7},"end":{"line":2997,"column":27}},"style":"secondary"},{"text":" server/datastore/mysql/labels.go[{"text":"deviceAuthTokenQuery = ... [truncated 1339 characters] ... olumn":7},"end":{"line":2997,"column":161}},"style":"primary"},{"text":"deviceAuthTokenQuery","range":{"byteOffset":{"start":109813,"end":109833},"start":{"line":2997,"column":7},"end":{"line":2997,"column":27}},"style":"secondary"},{"text":" server/service/integration_core_test.go[{"text":"deviceAuthTokenQuery = ... [truncated 1339 characters] ... olumn":7},"end":{"line":2997,"column":161}},"style":"primary"},{"text":"deviceAuthTokenQuery","range":{"byteOffset":{"start":109813,"end":109833},"start":{"line":2997,"column":7},"end":{"line":2997,"column":27}},"style":"secondary"},{"text":" 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.
🧹 Nitpick comments (1)
server/datastore/mysql/hosts.go (1)
1170-1170: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSort key uses raw
source, but the emitted value is translated.
ORDER BY he.email, he.sourcesorts on the rawsourcecolumn, whileJSON_OBJECTemits the translated value fromdeviceMappingTranslateSourceColumn("he"). If a single host has multiple emails with the same address but different sources, the tie-break order could diverge from the displayed (translated) source values. This is a narrow edge case (same email, multiple sources) but worth confirming is acceptable.🤖 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 `@server/datastore/mysql/hosts.go` at line 1170, The host JSON aggregation query sorts by the raw he.source value while emitting the translated source from deviceMappingTranslateSourceColumn("he"), so the ordering can differ from what is shown. Update the GROUP_CONCAT ORDER BY in the hosts query to use the same translated source expression used in JSON_OBJECT, or otherwise make the sort key explicitly match the emitted value in the host aggregation logic so the result ordering stays consistent.
🤖 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.
Nitpick comments:
In `@server/datastore/mysql/hosts.go`:
- Line 1170: The host JSON aggregation query sorts by the raw he.source value
while emitting the translated source from
deviceMappingTranslateSourceColumn("he"), so the ordering can differ from what
is shown. Update the GROUP_CONCAT ORDER BY in the hosts query to use the same
translated source expression used in JSON_OBJECT, or otherwise make the sort key
explicitly match the emitted value in the host aggregation logic so the result
ordering stays consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fa609a1-5cd9-4889-8bf1-6d8c335478c0
📒 Files selected for processing (4)
changes/device-mapping-orderserver/datastore/mysql/hosts.goserver/datastore/mysql/labels.goserver/service/integration_core_test.go
Related issue: NA
The
device_mappingGROUP_CONCATin the host list (hosts.go) and label host list (labels.go) queries had noORDER BY, so a host's emails came back in an unspecified order that varied between query executions. This produced inconsistent API output and flaky integration test failures (TestHostsReportDownload,TestListHostsByLabel). Emails are now sorted byemail, source, matching the single-hostListHostDeviceMappingquery.Checklist for submitter
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Summary by CodeRabbit