Clean up android app config data layer, fix no-team persist bug - #37740
Conversation
…store methods Still need to make another pass on this to clean up remaining struct fields, as we only actually need the config JSON for Gets
…for the by-VPP-app-team-ID data store method One more refactoring step: dropping the struct declaration, since it's now only used in tests
Zed + Opus 4.5. Prompt: Remove indirection through fleet.AndroidAppConfiguration in all tests in server/datastore/mysql/android_test.go, then remove the struct definition.
…37729) Zed + Opus 4.5. Prompt: Switch updateAndroidAppConfigurationTx's teamID parameter from a *uint to a uint, updating callers as needed.
…int" and use that to clean up the team_id cast
|
Assign of @RachelElysia is for QA as she has an Android env set up and I don't. Test coverage is better now than it was before, so this should just pass cleanly 🤞 @lucasmrod @getvictor Adding y'all as reviewers as there's currently no one but me around in SW until next Monday and y'all have worked closest to where this code is. Thx! |
iansltx
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37740 +/- ##
==========================================
- Coverage 65.86% 65.86% -0.01%
==========================================
Files 2362 2362
Lines 187339 187302 -37
Branches 7908 7908
==========================================
- Hits 123395 123369 -26
+ Misses 52663 52659 -4
+ Partials 11281 11274 -7
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:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughRefactoring of Android app configuration datastore APIs to use Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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: 0
🧹 Nitpick comments (1)
server/fleet/datastore.go (1)
2398-2410: Datastore Android app config interface matches JSON‑centric refactor; consider clarifying teamID semantics and namingThe updated methods:
GetAndroidAppConfiguration(ctx, applicationID string, teamID uint) (*json.RawMessage, error)GetAndroidAppConfigurationByAppTeamID(ctx, vppAppTeamID uint) (*json.RawMessage, error)HasAndroidAppConfigurationChanged(ctx, applicationID string, teamID uint, newConfig json.RawMessage) (bool, error)BulkGetAndroidAppConfigurations(ctx, appIDs []string, teamID uint) (map[string]json.RawMessage, error)DeleteAndroidAppConfiguration(ctx, adamID string, teamID uint) errorlook consistent with the MySQL implementation and the new tests (JSON payloads and explicit
teamID).Two small polish suggestions:
- Consider documenting explicitly in the comment that
teamID == 0represents “No team”, since that’s central to this bugfix and easy to mis‑handle in future call sites.- For
DeleteAndroidAppConfiguration, renamingadamIDtoapplicationID(or vice‑versa across the group) would make the parameter naming consistent with the other methods and reduce confusion between Apple/Android identifiers.Functionally this interface change looks good; these are just minor clarity improvements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
server/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/vpp.goserver/datastore/mysql/vpp_test.goserver/fleet/android.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/ptr/ptr.goserver/worker/software_worker.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/ptr/ptr.goserver/datastore/mysql/vpp.goserver/fleet/android.goserver/datastore/mysql/vpp_test.goserver/worker/software_worker.goserver/datastore/mysql/android.goserver/fleet/datastore.goserver/datastore/mysql/android_test.goserver/mock/datastore_mock.go
🧠 Learnings (5)
📓 Common learnings
Learnt from: getvictor
Repo: fleetdm/fleet PR: 32515
File: server/datastore/mysql/migrations/tables/20250902112642_MigratePrimoFailingPoliciesAutomations.go:110-114
Timestamp: 2025-09-02T20:44:52.724Z
Learning: The app_config_json table in Fleet's MySQL database is guaranteed to have only 1 row, so UPDATE statements without WHERE clauses are safe and intentional for this table.
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36978
File: server/mdm/android/service/profiles.go:441-449
Timestamp: 2025-12-11T23:02:59.284Z
Learning: Fleet’s server/mdm/android/service.Service has fields ds (type fleet.AndroidDatastore) and fleetDS (type fleet.Datastore). The fleet.Datastore interface embeds AndroidDatastore (server/fleet/datastore.go), so assigning r.DS (a fleet.Datastore) to Service.ds is valid in Go due to interface-to-interface assignment using the dynamic type.
📚 Learning: 2025-08-22T01:14:05.454Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 32173
File: server/datastore/mysql/policies.go:360-365
Timestamp: 2025-08-22T01:14:05.454Z
Learning: The sqlx library in Go (and the underlying database/sql package) can handle pointer types as query parameters correctly. Non-nil pointers are automatically dereferenced to get their values, and nil pointers are converted to SQL NULL values. This is standard, documented behavior - not a limitation or bug. Passing pointer parameters like `*uint`, `*string`, etc. to sqlx query methods is perfectly valid.
Applied to files:
server/ptr/ptr.go
📚 Learning: 2025-12-11T23:02:59.284Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36978
File: server/mdm/android/service/profiles.go:441-449
Timestamp: 2025-12-11T23:02:59.284Z
Learning: Fleet’s server/mdm/android/service.Service has fields ds (type fleet.AndroidDatastore) and fleetDS (type fleet.Datastore). The fleet.Datastore interface embeds AndroidDatastore (server/fleet/datastore.go), so assigning r.DS (a fleet.Datastore) to Service.ds is valid in Go due to interface-to-interface assignment using the dynamic type.
Applied to files:
server/datastore/mysql/android.goserver/fleet/datastore.goserver/datastore/mysql/android_test.goserver/mock/datastore_mock.go
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/datastore/mysql/android_test.goserver/mock/datastore_mock.go
📚 Learning: 2025-10-03T18:16:11.482Z
Learnt from: MagnusHJensen
Repo: fleetdm/fleet PR: 33805
File: server/service/integration_mdm_test.go:1248-1251
Timestamp: 2025-10-03T18:16:11.482Z
Learning: In server/service/integration_mdm_test.go, the helper createAppleMobileHostThenEnrollMDM(platform string) is exclusively for iOS/iPadOS hosts (mobile). Do not flag macOS model/behavior issues based on changes within this helper; macOS provisioning uses different helpers such as createHostThenEnrollMDM.
Applied to files:
server/mock/datastore_mock.go
🧬 Code graph analysis (7)
server/datastore/mysql/vpp.go (1)
server/ptr/ptr.go (1)
ValOrZero(92-100)
server/datastore/mysql/vpp_test.go (2)
server/fleet/vpp.go (1)
VPPAppID(9-14)server/fleet/datastore.go (1)
IsNotFound(2822-2828)
server/worker/software_worker.go (1)
server/ptr/ptr.go (1)
RawMessage(62-64)
server/datastore/mysql/android.go (2)
server/ptr/ptr.go (2)
RawMessage(62-64)UintOrNilIfZero(27-32)server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)
server/fleet/datastore.go (1)
server/ptr/ptr.go (1)
RawMessage(62-64)
server/datastore/mysql/android_test.go (1)
server/ptr/ptr.go (2)
RawMessage(62-64)T(86-88)
server/mock/datastore_mock.go (1)
server/ptr/ptr.go (1)
RawMessage(62-64)
🔇 Additional comments (18)
server/ptr/ptr.go (1)
24-32: LGTM!The helper function is well-documented with a clear reference to the bug it addresses. The implementation correctly returns
nilfor zero values, enabling proper SQL NULL handling for "No team" scenarios.server/worker/software_worker.go (1)
137-141: LGTM!The nil check and dereference are correct for the new
*json.RawMessagereturn type fromGetAndroidAppConfigurationByAppTeamID. The logic correctly preserves the nil-config case where noconfigByAppIDmapping is created.server/datastore/mysql/vpp.go (3)
103-105: LGTM!The dereference aligns with the API change where
GetAndroidAppConfigurationnow returns*json.RawMessage. The nil check on line 103 ensures safe dereferencing.
524-528: LGTM!Using
ptr.ValOrZero(teamID)correctly converts the nullable team ID to a uint value (0 for "No team"), which the refactoredupdateAndroidAppConfigurationTxnow expects. This is part of the fix for issue #37729.
664-668: LGTM!Consistent with the change in
SetTeamVPPApps- usingptr.ValOrZero(teamID)to convert the nullable team ID to uint for the updatedupdateAndroidAppConfigurationTxsignature.server/datastore/mysql/vpp_test.go (2)
2598-2601: LGTM!Correctly dereferences the
*json.RawMessagereturned by the updatedGetAndroidAppConfigurationAPI.
2623-2634: Good test coverage for the new lookup method.The test correctly validates
GetAndroidAppConfigurationByAppTeamIDby:
- Asserting consistency with
GetAndroidAppConfigurationresults when config exists- Properly checking for
NotFoundErrorwhen config is nilserver/fleet/android.go (1)
200-204: LGTM!Minor formatting improvement separating the specific "unknown field" error case from the general JSON parsing error.
server/datastore/mysql/android_test.go (2)
2343-2400: Android app config insert/update/delete tests correctly exercise the teamID=0 (“No team”) pathThe new tests around
updateAndroidAppConfigurationTxandGet/BulkGet/DeleteAndroidAppConfigurationforteamID == 0provide good coverage of the No‑team behavior and the JSON‐centric API:
- Using
json.RawMessageplusrequire.JSONEqand string comparisons againstBulkGetAndroidAppConfigurationsverifies both content and keying.- Calling the transactional helper with
teamID0 ensures the underlying bug (0 vs nil handling) is covered at the data layer.- The map length + value equality checks are sufficient to prove the expected appID key is present.
No issues from a correctness standpoint; these tests should effectively guard the No‑team regression going forward.
2429-2470: Team‑scoped and change‑detection tests for Android app configs look solid, with clear edge‑case coverageThe added/updated tests for:
testAndroidAppConfigurationCascadeDeleteTeamtestAndroidAppConfigurationGlobalVsTeamtestHasAndroidAppConfigurationChangedcollectively verify:
- Team‑scoped configs are removed via FK cascade when a team is deleted, and lookups by
(applicationID, teamID)correctly return “not found”.- Global (
teamID == 0) vs team‑specific configs coexist and are independently retrievable, with JSON equality checked viarequire.JSONEq.HasAndroidAppConfigurationChangedhandles nuanced cases:
- Differences in structure and value (empty string,
{},false, emptymanagedConfiguration, extra keys, completely different fields).- Equality for an unchanged config.
- Presence/absence semantics when no row exists (empty new config vs non‑empty).
This is a nice, comprehensive matrix for the change‑detection helper and the new teamID semantics; I don’t see gaps that would block this PR.
Also applies to: 2541-2621
server/mock/datastore_mock.go (2)
1562-1572: Android app config mock function types correctly updated to new APIThe updated function types now take
teamID uintand usejson.RawMessage/map[string]json.RawMessage, matching the refactored datastore API and allowingteamID == 0to flow through as a valid value. This keeps the mock aligned with the production interface for the “No team” fix.
9621-9660: DataStore Android app config methods follow existing mock patternThe new
DataStoremethods for Android app configuration correctly:
- Lock/unlock and set the
*FuncInvokedflags, and- Delegate to the corresponding
*Funcfields with the new(applicationID/adamID, teamID, newConfig)signatures.This is consistent with the rest of the generated mocks and should work seamlessly with the updated tests.
server/datastore/mysql/android.go (6)
1678-1704: LGTM! Parameter refactoring is clean.The signature change from
globalOrTeamIDtoteamID uintand the consistent binding at line 1695 properly support the "No team" (teamID=0) case. The query correctly filters byglobal_or_team_idwhich is the NOT NULL column that stores 0 for "No team".
1706-1720: LGTM! Return type refactoring is correct.The method now returns
*json.RawMessageinstead of a structured type, and the query has been optimized to select only theconfigurationcolumn. The filtering byapplication_idandglobal_or_team_id(line 1708) ensures a unique result.
1722-1741: LGTM! JOIN-based retrieval is correct.The alternate lookup method properly joins
vpp_apps_teamsand filters by the uniquevat.idparameter. The return type change to*json.RawMessageis consistent with the refactoring pattern.
1743-1775: LGTM! Bulk retrieval correctly refactored.The method signature and return type have been properly updated to use
teamID uintand returnmap[string]json.RawMessage. The anonymous struct at lines 1761-1764 appropriately captures the query results, and the filtering byapplication_id IN (?) AND global_or_team_id = ?ensures correct scoping.
1805-1826: LGTM! Deletion properly scoped.The method correctly uses
teamID uintand filters by bothapplication_idandglobal_or_team_idto ensure the deletion is properly scoped. The row count check at lines 1816-1823 appropriately detects if no record was found.
1829-1848: LGTM! Core bug fix for #37729 is correct.The signature change from
teamID *uinttoteamID uint(line 1829) and the binding logic at line 1843 properly address the "No team" persist bug:
ptr.UintOrNilIfZero(teamID)is used for the nullableteam_idcolumn (converts 0 to NULL)teamIDis used directly for the NOT NULLglobal_or_team_idcolumn (0 represents "No team")This dual-column approach allows team_id=0 configurations to be persisted correctly. Input validation at line 1830 and parameterized queries provide proper safeguards.
|
Just tested this branch, lgtm! Screen.Recording.2025-12-30.at.10.31.11.AM.mov |
Resolves #37729. Unreleased, so no changes file. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) ## Testing - [x] Added/updated automated tests - [ ] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Refactored Android app configuration storage and retrieval to use JSON-based format instead of structured objects. * Updated configuration lookups to use team-based identification consistently. * Added new method for retrieving Android app configurations by app team ID. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 participants
Resolves #37729. Unreleased, so no changes file.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Added/updated automated tests
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.