Conversation
Only supply patch_software_title_id when the policy type is PolicyTypePatch. Previously fmaTitleID was passed for all policy inserts, which could cause unique index collisions (idx_team_id_patch_software_title_id) when dynamic policies (e.g. install_software) carried fleet_maintained_app_slug. Introduce patchSoftwareTitleIDArg set to the title ID for patch policies and nil otherwise, and use it in the tx.ExecContext call.
allenhouchins
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44179 +/- ##
==========================================
+ Coverage 66.76% 66.78% +0.01%
==========================================
Files 2629 2627 -2
Lines 211215 211151 -64
Branches 9539 9539
==========================================
- Hits 141027 141017 -10
+ Misses 57361 57315 -46
+ Partials 12827 12819 -8
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:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
Updates ApplyPolicySpecs to avoid patch_software_title_id uniqueness collisions by only setting that column for patch policies, even when other (dynamic) policy types carry fleet_maintained_app_slug.
Changes:
- Introduce a separate SQL argument (
patchSoftwareTitleIDArg) that is set only whenspec.Type == fleet.PolicyTypePatch. - Pass
NULLforpatch_software_title_idon non-patch policy inserts/updates to avoid triggeringidx_team_id_patch_software_title_idcollisions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughApplyPolicySpecs in the MySQL datastore now supplies the Possibly related PRs
🚥 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 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.
🧹 Nitpick comments (1)
server/datastore/mysql/policies.go (1)
1506-1519: Fix looks correct — gatingpatch_software_title_idbehindtype == patchis the right call.The deref
*fmaTitleIDon line 1511 is safe given the nil-guard at lines 1481-1485, and a zero-valueanytranslates to SQLNULLfor the parameter. Per the schema (context snippet 1),patch_software_title_idis nullable and the unique indexidx_team_id_patch_software_title_idtreats multiple NULLs as distinct, so dynamic policies that still carryfleet_maintained_app_slugno longer collide. Upserts of previously-buggy rows will also self-heal viapatch_software_title_id = VALUES(patch_software_title_id)in theON DUPLICATE KEY UPDATEclause.A couple of follow-ups worth considering (non-blocking):
- Add a regression test for
ApplyPolicySpecscovering the failure mode this fixes — e.g., applying two specs on the same team where one istype: patchand another istype: dynamic(install_software) referencing the samefleet_maintained_app_slug. Without a test, it's easy for a future refactor to reintroduce the collision. The PR description notes no tests were added.- Pre-existing rows: any dynamic policies that were previously inserted with a non-NULL
patch_software_title_iddue to this bug will only be corrected the next time the spec is re-applied (the upsert path will reset it to NULL). If you suspect production already has such rows, a one-off backfill (UPDATE policies SET patch_software_title_id = NULL WHERE type = 'dynamic') may be worth considering as a separate change.Want me to draft the regression test against
server/datastore/mysql/policies_test.go?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/policies.go` around lines 1506 - 1519, The current fix correctly gates patch_software_title_id to only set a value when spec.Type == fleet.PolicyTypePatch; add a regression test to ensure this doesn't regress by creating two specs for the same team — one with Type == fleet.PolicyTypePatch and one with Type == fleet.PolicyTypeDynamic (e.g., install_software) that reference the same fleet_maintained_app_slug — and assert ApplyPolicySpecs (call the function ApplyPolicySpecs in the code under test) results in one row with a non-NULL patch_software_title_id for the patch policy and a NULL patch_software_title_id for the dynamic policy; add this test to server/datastore/mysql/policies_test.go and, optionally, consider scripting a one-off DB migration/backfill to NULL out patch_software_title_id for rows where type != 'patch' if production might contain bad data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/datastore/mysql/policies.go`:
- Around line 1506-1519: The current fix correctly gates patch_software_title_id
to only set a value when spec.Type == fleet.PolicyTypePatch; add a regression
test to ensure this doesn't regress by creating two specs for the same team —
one with Type == fleet.PolicyTypePatch and one with Type ==
fleet.PolicyTypeDynamic (e.g., install_software) that reference the same
fleet_maintained_app_slug — and assert ApplyPolicySpecs (call the function
ApplyPolicySpecs in the code under test) results in one row with a non-NULL
patch_software_title_id for the patch policy and a NULL patch_software_title_id
for the dynamic policy; add this test to server/datastore/mysql/policies_test.go
and, optionally, consider scripting a one-off DB migration/backfill to NULL out
patch_software_title_id for rows where type != 'patch' if production might
contain bad data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4889e7a-0d87-4737-bd50-164b73bc29f2
📒 Files selected for processing (1)
server/datastore/mysql/policies.go
Verifies ApplyPolicySpecs accepts a dynamic install_software policy and a patch policy that reference the same fleet_maintained_app_slug on the same team, and asserts that only the patch policy carries patch_software_title_id in the DB.
allenhouchins
had a problem deploying
to
Docker Hub
GitHub Actions
Error
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
allenhouchins
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Resolves: #44183
Only supply patch_software_title_id when the policy type is PolicyTypePatch. Previously fmaTitleID was passed for all policy inserts, which could cause unique index collisions (idx_team_id_patch_software_title_id) when dynamic policies (e.g. install_software) carried fleet_maintained_app_slug. Introduce patchSoftwareTitleIDArg set to the title ID for patch policies and nil otherwise, and use it in the tx.ExecContext call.
Related issue: Resolves #
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
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.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsfleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit