Add migration to prevent deletion of labels referenced by MDM profiles - #46436
Merged
Conversation
![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.
Contributor
WalkthroughThis PR implements a database migration to enforce referential integrity on label associations. The migration changes the foreign key constraint on Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/datastore/mysql/migrations/tables/20260526192801_RestrictLabelFKOnMDMConfigurationProfileLabels_test.go (1)
23-27: ⚡ Quick winSplit this into per-table assertions.
With both join tables still pointing at the same label on Line 26, this only proves that one FK became
RESTRICT. The test would still pass if eithermdm_configuration_profile_labelsormdm_declaration_labelswere accidentally left onSET NULL. Use separate subtests, or remove one reference before each delete, so each constraint is verified independently.🤖 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/migrations/tables/20260526192801_RestrictLabelFKOnMDMConfigurationProfileLabels_test.go` around lines 23 - 27, The test currently deletes the label once and assumes both FKs are RESTRICT; instead assert each FK independently: create two subtests (or do two delete attempts) that isolate each join table—e.g., in one subtest delete the row in mdm_declaration_labels (or remove its reference) and then attempt DELETE FROM labels WHERE id = labelID to assert an error caused by the mdm_configuration_profile_labels FK, and in the other subtest remove the mdm_configuration_profile_labels reference and then assert the DELETE is blocked by the mdm_declaration_labels FK; keep references to applyNext(t, db), labelID, db.Exec(...) and the table names mdm_configuration_profile_labels and mdm_declaration_labels so the assertions verify each constraint independently.
🤖 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/migrations/tables/20260526192801_RestrictLabelFKOnMDMConfigurationProfileLabels_test.go`:
- Around line 23-27: The test currently deletes the label once and assumes both
FKs are RESTRICT; instead assert each FK independently: create two subtests (or
do two delete attempts) that isolate each join table—e.g., in one subtest delete
the row in mdm_declaration_labels (or remove its reference) and then attempt
DELETE FROM labels WHERE id = labelID to assert an error caused by the
mdm_configuration_profile_labels FK, and in the other subtest remove the
mdm_configuration_profile_labels reference and then assert the DELETE is blocked
by the mdm_declaration_labels FK; keep references to applyNext(t, db), labelID,
db.Exec(...) and the table names mdm_configuration_profile_labels and
mdm_declaration_labels so the assertions verify each constraint independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7081e1f9-ce6e-4f1a-b9ad-db102e857b53
📒 Files selected for processing (4)
server/datastore/mysql/mdm_test.goserver/datastore/mysql/migrations/tables/20260526192801_RestrictLabelFKOnMDMConfigurationProfileLabels.goserver/datastore/mysql/migrations/tables/20260526192801_RestrictLabelFKOnMDMConfigurationProfileLabels_test.goserver/datastore/mysql/schema.sql
andymFleet
marked this pull request as draft
May 29, 2026 10:42
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46436 +/- ##
==========================================
- Coverage 66.89% 66.89% -0.01%
==========================================
Files 2822 2823 +1
Lines 224712 224744 +32
Branches 11517 11517
==========================================
+ Hits 150324 150334 +10
- Misses 60748 60760 +12
- Partials 13640 13650 +10
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:
|
andymFleet
marked this pull request as draft
May 29, 2026 19:02
andymFleet
marked this pull request as ready for review
May 29, 2026 19:47
MagnusHJensen
previously approved these changes
Jun 1, 2026
…aint independently
…instead of DeleteLabel
andymFleet
force-pushed
the
45182-cpie-migration-broken-label-prevention
branch
from
June 3, 2026 10:04
fb0d0bb to
6ad80c3
Compare
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
MagnusHJensen
approved these changes
Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #45182
What this does
Changes the foreign key constraints on
mdm_configuration_profile_labelsandmdm_declaration_labelsfromON DELETE CASCADE(or no restriction) toON DELETE RESTRICT. This prevents a label from being deleted while it is still referenced by an MDM configuration profile or declaration.Previously, deleting a label that was targeted by a profile would silently remove the label reference, leaving the profile in a "broken" state in the UI (showing a "Label deleted" warning with no way to recover without re-uploading the profile). With this change, Fleet returns an error when attempting to delete a label that is in use by a profile, prompting the user to remove the profile's label targeting first.
Why
This is part of a broader set of changes (CPIE include/exclude label targeting) that introduces combined include+exclude label targeting on profiles. Allowing silent label deletion would cause ambiguous broken states when both include and exclude labels are in use on a single profile.
Testing
_test.gofile, which covers:ALTER TABLEDDL change does not triggerON UPDATE CURRENT_TIMESTAMPonmdm_configuration_profile_labels.updated_at(DDL does not fire row-level triggers)Checklist for submitter
Testing
Database migrations
COLLATE utf8mb4_unicode_ci).