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.
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent Apple DDM declarations/profiles from being recreated (and resent) when only the profile display name casing changes by making the “name match” logic case-insensitive.
Changes:
- Normalize incoming/existing declaration names to lowercase for in-memory matching during batch set.
- Preserve existing
declaration_uuidacross case-only name changes and update host declaration name records after updates. - Add an automated regression test covering case-only name changes and ensuring host install states are not disrupted.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
server/datastore/mysql/apple_mdm.go |
Lowercases names for matching; preserves declaration UUID on case-only changes; updates host_mdm_apple_declarations.declaration_name after declaration updates. |
server/datastore/mysql/apple_mdm_test.go |
Adds a regression test verifying case-only changes don’t create duplicates or disrupt install/pending states. |
changes/40332-make-ddm-name-check-case-insensitive |
Adds a changelog entry describing the user-visible fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43347 +/- ##
==========================================
+ Coverage 66.86% 66.88% +0.01%
==========================================
Files 2586 2589 +3
Lines 207474 207660 +186
Branches 9276 9276
==========================================
+ Hits 138736 138898 +162
- Misses 56109 56121 +12
- Partials 12629 12641 +12
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:
|
|
Failing test is unrelated |
WalkthroughThe changes address an issue where updating a DDM configuration profile's display name with different casing triggered profile recreation and stuck remove operations. The fix implements case-insensitive name matching for DDM declarations while preserving the updated casing in the profile name. Changes include normalization of incoming declaration names to lowercase for lookups, synchronization of declaration names across data structures, and tests verifying that case-only name changes do not create duplicate declarations or alter host profile operation statuses. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 5111-5113: When building incomingDeclarationsMap using normalized
keys (strings.ToLower(p.Name)), detect if a normalized name already exists and
reject the request instead of overwriting: inside the loop where
incomingNames[i] and incomingDeclarationsMap[name] are assigned, check if
incomingDeclarationsMap already has an entry for that normalized name and return
an error (including both conflicting original names/p.Name values) so duplicate
declarations like "Foo" and "foo" are rejected; update the function that
contains incomingNames/incomingDeclarationsMap to perform this validation before
continuing with upserts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f9cec3b2-a5c4-4c10-9b55-59a43ac76e73
📒 Files selected for processing (3)
changes/40322-make-ddm-name-check-case-insensitiveserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.go
| name := strings.ToLower(p.Name) | ||
| incomingNames[i] = name | ||
| incomingDeclarationsMap[name] = p |
There was a problem hiding this comment.
Reject duplicate declaration names after normalization.
Lowercasing the key here makes two incoming declarations like Foo and foo collapse into one incomingDeclarationsMap entry. The rest of the flow still iterates both original declarations, so one upsert can overwrite the other while only one declaration's labels/UUID bookkeeping survives. Please validate and return an error when two incoming names normalize to the same value instead of letting the last one win.
Proposed fix
incomingNames := make([]string, len(incomingDeclarations))
incomingDeclarationsMap := make(map[string]*fleet.MDMAppleDeclaration, len(incomingDeclarations))
for i, p := range incomingDeclarations {
- name := strings.ToLower(p.Name)
- incomingNames[i] = name
- incomingDeclarationsMap[name] = p
+ name := strings.ToLower(p.Name)
+ if existing, ok := incomingDeclarationsMap[name]; ok {
+ return false, &fleet.BadRequestError{
+ Message: fmt.Sprintf(
+ "declaration names %q and %q differ only by letter case; declaration names must be unique",
+ existing.Name, p.Name,
+ ),
+ }
+ }
+ incomingNames[i] = name
+ incomingDeclarationsMap[name] = p
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name := strings.ToLower(p.Name) | |
| incomingNames[i] = name | |
| incomingDeclarationsMap[name] = p | |
| name := strings.ToLower(p.Name) | |
| if existing, ok := incomingDeclarationsMap[name]; ok { | |
| return false, &fleet.BadRequestError{ | |
| Message: fmt.Sprintf( | |
| "declaration names %q and %q differ only by letter case; declaration names must be unique", | |
| existing.Name, p.Name, | |
| ), | |
| } | |
| } | |
| incomingNames[i] = name | |
| incomingDeclarationsMap[name] = p |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/datastore/mysql/apple_mdm.go` around lines 5111 - 5113, When building
incomingDeclarationsMap using normalized keys (strings.ToLower(p.Name)), detect
if a normalized name already exists and reject the request instead of
overwriting: inside the loop where incomingNames[i] and
incomingDeclarationsMap[name] are assigned, check if incomingDeclarationsMap
already has an entry for that normalized name and return an error (including
both conflicting original names/p.Name values) so duplicate declarations like
"Foo" and "foo" are rejected; update the function that contains
incomingNames/incomingDeclarationsMap to perform this validation before
continuing with upserts.
Related issue: Resolves #40322
The issue was due to case insensitivity in go maps, so when we checked if we should keep the DDM profile or not, it failed. So we now default to lowercasing all profile names to make it case insensitive.
While that fixes the cause, for the issue, I will follow up with another PR for the all profiles stuck in pending, since it's a scaling issue due to batching and always taking installs before removes, so with 11k hosts it would never have both install and remove in the same run, failing to clear out the stuck pending. It can be manually remediated, but we want to have a better fix for this that actually cleans it up, if this is met as it can be perfectly valid.
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
Summary by CodeRabbit
Bug Fixes
Tests