SAAD: DDM Asset table migration - #48866
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 for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughThis PR adds a new MySQL migration that creates two tables, Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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/migrations/tables/20260707140752_AddDDMAssetsTable_test.go (1)
31-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting the generated
tokenvalue.FK/cascade/restrict coverage is thorough, but the test never reads back
tokento confirm it's actually derived fromraw_json/secrets_updated_atas intended. Since the PR objectives state asset changes must propagate via the token so referenced configs re-sync hosts, a quick assertion (e.g.,SELECT HEX(token) ...compared to an expected MD5 hash, or verifying token changes after anUPDATE raw_json) would directly validate the core sync mechanism this migration exists to support.🤖 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/20260707140752_AddDDMAssetsTable_test.go` around lines 31 - 57, The migration test for AddDDMAssetsTable currently verifies foreign keys and cascade behavior but does not validate the token column used for sync propagation. In the test function in 20260707140752_AddDDMAssetsTable_test.go, add a readback/assertion for the generated token on mdm_apple_declaration_assets, either by querying it after insert and comparing it to the expected value derived from raw_json and secrets_updated_at or by updating raw_json and confirming the token changes accordingly. Use the existing db.ExecContext and db.QueryRowContext flow in the test to keep the assertion alongside the current FK coverage.
🤖 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/20260707140752_AddDDMAssetsTable_test.go`:
- Around line 31-57: The migration test for AddDDMAssetsTable currently verifies
foreign keys and cascade behavior but does not validate the token column used
for sync propagation. In the test function in
20260707140752_AddDDMAssetsTable_test.go, add a readback/assertion for the
generated token on mdm_apple_declaration_assets, either by querying it after
insert and comparing it to the expected value derived from raw_json and
secrets_updated_at or by updating raw_json and confirming the token changes
accordingly. Use the existing db.ExecContext and db.QueryRowContext flow in the
test to keep the assertion alongside the current FK coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 844cda6b-7909-4c92-be79-b482c880b123
📒 Files selected for processing (3)
server/datastore/mysql/migrations/tables/20260707140752_AddDDMAssetsTable.goserver/datastore/mysql/migrations/tables/20260707140752_AddDDMAssetsTable_test.goserver/datastore/mysql/schema.sql
There was a problem hiding this comment.
Pull request overview
Adds the initial MySQL schema support for SAAD DDM “asset” declarations by introducing new asset tables and wiring asset change propagation into host-level DDM tokens via a new timestamp column.
Changes:
- Added
mdm_apple_declaration_assetsandmdm_apple_declaration_asset_referencestables with generated token + FK constraints. - Added
host_mdm_apple_declarations.assets_updated_atcolumn. - Updated
server/datastore/mysql/schema.sqlto reflect the new schema and migration list.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/datastore/mysql/schema.sql | Adds the new DDM asset tables and assets_updated_at column to the canonical schema dump. |
| server/datastore/mysql/migrations/tables/20260707140752_AddDDMAssetsTable.go | Introduces the migration creating the asset tables and adding assets_updated_at. |
| server/datastore/mysql/migrations/tables/20260707140752_AddDDMAssetsTable_test.go | Tests uniqueness/FK behavior for the new asset tables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| asset_uuid varchar(37) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| team_id int unsigned NOT NULL, | ||
| identifier varchar(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| name varchar(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL, |
There was a problem hiding this comment.
I was thinking we don't want this to be more explicit, that way we catch bugs earlier in code. With this it would default to team_id 0 (unassigned) which is less visible than empty name. I would like to continue not having default for require fields either way (especially on new tables), lmk.
| declaration_uuid varchar(37) COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| asset_uuid varchar(37) COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| PRIMARY KEY (declaration_uuid, asset_uuid), |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48866 +/- ##
==========================================
- Coverage 68.05% 68.05% -0.01%
==========================================
Files 3693 3694 +1
Lines 234406 234451 +45
Branches 12460 12460
==========================================
+ Hits 159535 159565 +30
- Misses 60543 60552 +9
- Partials 14328 14334 +6
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:
|
Related issue: Resolves #48566
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. Coming in bigger backend story.
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
Summary by CodeRabbit
New Features
Bug Fixes