Merge Android datastore into main Fleet datastore - #32233
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #32233 +/- ##
==========================================
- Coverage 64.03% 64.01% -0.03%
==========================================
Files 1988 1985 -3
Lines 194446 194360 -86
Branches 6551 6513 -38
==========================================
- Hits 124522 124426 -96
+ Misses 60225 60224 -1
- Partials 9699 9710 +11
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:
|
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good overall.
The main tests are failing in CI. Please take a look.
Also, we can remove this mock:
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAndroid datastore is merged into the main MySQL datastore: receiver/type renames to AndroidDatastore, constructor rename, wiring updates, tests refactored to use main datastore, Android-specific schema and testing utils removed, CI/hooks/Makefile/tools updated to drop Android schema handling, Android mocks replaced/removed, and dependency rules adjusted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Tests/Service
participant DS as AndroidDSWithMock
participant Mock as ds_mock.Store (mock)
participant MySQL as mysql.AndroidDatastore
participant DB as MySQL DB
rect rgb(245,248,255)
note over DS: Routing after merge (new)
Test->>DS: AppConfig()
DS->>Mock: AppConfig()
Mock-->>DS: *fleet.AppConfig
DS-->>Test: *fleet.AppConfig
end
rect rgb(245,255,245)
note over DS,MySQL: Device ops via main datastore (new)
Test->>DS: CreateDeviceTx(ctx, tx, device)
DS->>MySQL: CreateDeviceTx(ctx, tx, device)
MySQL->>DB: INSERT/UPSERT android_devices
DB-->>MySQL: result/ID
MySQL-->>DS: *android.Device
DS-->>Test: *android.Device
end
rect rgb(255,248,245)
note over DS,MySQL: Enterprise ops via main datastore (new)
Test->>DS: GetEnterpriseBySignupToken(token)
DS->>MySQL: GetEnterpriseBySignupToken(token)
alt found
MySQL->>DB: SELECT ...
DB-->>MySQL: row
MySQL-->>DS: *android.EnterpriseDetails
DS-->>Test: details
else not found
MySQL-->>DS: notFound error
DS-->>Test: notFound error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
server/datastore/mysql/android.go (1)
100-105: Critical fix needed: handle zero LastInsertId in NewAndroidHostThe current implementation of NewAndroidHost (server/datastore/mysql/android.go lines 97–105) uses result.LastInsertId() directly to set host.Host.ID. Under MySQL’s INSERT … ON DUPLICATE KEY UPDATE path, LastInsertId() returns 0 for updates, leaving host.Host.ID == 0 and causing all subsequent operations—display name upsert, label membership, CreateDeviceTx, etc.—to act on a nonexistent host (ID 0).
Apply this patch around line 100 in server/datastore/mysql/android.go:
- id, _ := result.LastInsertId() - host.Host.ID = uint(id) // nolint:gosec + id, _ := result.LastInsertId() + if id == 0 { + // Duplicate key update path: fetch the existing host ID by node_key. + if err := sqlx.GetContext(ctx, tx, &id, + `SELECT id FROM hosts WHERE node_key = ?`, host.NodeKey); err != nil { + return ctxerr.Wrap(ctx, err, "lookup host id after upsert") + } + } + host.Host.ID = uint(id) // nolint:gosecAdditionally, add an integration test to cover this code path:
- Pre-insert a host row with the same node_key.
- Call NewAndroidHost with identical node_key.
- Assert that returned host.Host.ID is nonzero and downstream operations succeed.
server/datastore/mysql/android_enterprises.go (1)
51-62: Ensure deterministic enterprise selectionI verified that
enterprise_idis not constrained as UNIQUE inserver/datastore/mysql/schema.sql, soSELECT … FROM android_enterprises WHERE enterprise_id != '' LIMIT 1can return an arbitrary row when multiple entries exist. Please address this by either:
- Enforcing uniqueness
Add a UNIQUE index onenterprise_id(or on a generated “active” flag) inserver/datastore/mysql/schema.sqlto guarantee a single matching row.- Making selection deterministic
Modify the query inserver/datastore/mysql/android_enterprises.go(lines 51–62) to includeORDER BY, for example:- stmt := `SELECT id, enterprise_id FROM android_enterprises WHERE enterprise_id != '' LIMIT 1` + stmt := `SELECT id, enterprise_id + FROM android_enterprises + WHERE enterprise_id != '' + ORDER BY id DESC + LIMIT 1`These changes ensure predictable behavior even if multiple non-empty enterprises exist.
server/mdm/android/tests/testing_utils.go (2)
235-241: Fix unused parameter (won’t compile) and document intentThe name parameter is now unused; Go rejects unused parameters.
Apply this minimal fix:
func CreateNamedMySQLDS(t *testing.T, name string) *mysql.Datastore { if _, ok := os.LookupEnv("MYSQL_TEST"); !ok { t.Skip("MySQL tests are disabled") } - // use the standard Fleet datastore for Android integration tests - return mysql.CreateMySQLDS(t) + // use the standard Fleet datastore for Android integration tests + // dbName no longer needed after consolidation. + _ = name + return mysql.CreateMySQLDS(t) }If uniqueness by dbName is still required elsewhere, we can switch to a named factory if available or derive a suffix from name when creating the DS. Let me know and I’ll propose that patch.
99-112: Fix Android tests to respect the provided DB nameThe current
CreateNamedMySQLDSinserver/mdm/android/tests/testing_utils.goignores thenameparameter and always callsmysql.CreateMySQLDS(t), which (due to its fixed call‐stack offset) will resolve to a constant database name when invoked from this wrapper. This can cause parallel Android suites to stomp on each other’s database.• Location:
server/mdm/android/tests/testing_utils.go:235
• Issue: the wrapper must delegate thenamethrough to the MySQL test harness rather than dropping it.Suggested diff:
func CreateNamedMySQLDS(t *testing.T, name string) *mysql.Datastore { if _, ok := os.LookupEnv("MYSQL_TEST"); !ok { t.Skip("MySQL tests are disabled") } - // use the standard Fleet datastore for Android integration tests - return mysql.CreateMySQLDS(t) + // forward the explicit DB name so each suite gets its own database + return mysql.CreateNamedMySQLDS(t, name) }This ensures the Android integration tests run against a database named exactly by
dbName, preserving isolation when suites execute in parallel.
♻️ Duplicate comments (2)
server/mdm/android/arch_test.go (1)
27-32: Arch rule scope is correct (allow tests, forbid prod deps on MySQL).Allow-listing
server/mdm/android/tests...while forbiddingserver/datastore/mysql...keeps prod Android code decoupled and still lets integration tests hit the main datastore. Matches the intent discussed in prior comments.server/mdm/android/tests/testing_utils.go (1)
220-223: Avoid disabling timeouts entirely for SSE; use generous finite values insteadZeroing Read/Write timeouts can mask hung tests. Prefer large, finite values that won’t interfere with SSE but still fail eventually.
Apply this diff:
- // Disable timeouts for SSE connections in tests - server.Config.ReadTimeout = 0 - server.Config.WriteTimeout = 0 + // Use generous timeouts for SSE without risking indefinite hangs + server.Config.ReadTimeout = 2 * time.Minute + server.Config.WriteTimeout = 2 * time.MinuteAdd the import:
import ( "context" "net/http" "net/http/httptest" "os" "sync" "testing" + "time"
🧹 Nitpick comments (15)
tools/dbutils/schema_generator.go (2)
43-45: Clarify the CLI usage error.The panic message is vague. Make it explicit that exactly one argument (the dumpfile path) is required.
- if len(os.Args) != 2 { - panic("not enough arguments") - } + if len(os.Args) != 2 { + panic("usage: schema_generator <dumpfile-path> (expects exactly 1 argument)") + }
90-91: Use non-executable file perms for schema.sql.
0o655marks the file executable for group/others, which isn’t needed for a .sql dump. Prefer0644.- panicif(os.WriteFile(os.Args[1], stdoutBuf.Bytes(), 0o655)) + panicif(os.WriteFile(os.Args[1], stdoutBuf.Bytes(), 0o644)).github/workflows/test-db-changes.yml (1)
95-99: Fix typo in failure message.Minor nit: “uncommited” → “uncommitted”.
- echo "❌ fail: uncommited changes in schema.sql" + echo "❌ fail: uncommitted changes in schema.sql"git-hooks/backend/hooks/db-schema (1)
7-9: Polish the error output (path + typo).The message still says “uncommited changes in schema.sql”. Consider including the full path and fix the typo to make CI/local failures clearer.
- echo "❌ fail: uncommited changes in schema.sql" - echo "please run 'make dump-test-schema' and commit the changes" + echo "❌ fail: uncommitted changes in server/datastore/mysql/schema.sql" + echo "please run 'make dump-test-schema' and commit the changes"server/mdm/android/service/enterprises_test.go (2)
153-179: Centralize mock wiring to reduce repetition and improve readability.The inline assignments are fine, but extracting them into a small helper keeps tests concise and makes reuse trivial if other tests need the same setup.
func InitCommonDSMocks() *AndroidMockDS { ds := AndroidMockDS{} - // Set up basic Android datastore mocks directly on the Fleet datastore mock - ds.Store.CreateEnterpriseFunc = func(ctx context.Context, _ uint) (uint, error) { - return 1, nil - } - ds.Store.UpdateEnterpriseFunc = func(ctx context.Context, enterprise *android.EnterpriseDetails) error { - return nil - } - ds.Store.GetEnterpriseFunc = func(ctx context.Context) (*android.Enterprise, error) { - return &android.Enterprise{}, nil - } - ds.Store.GetEnterpriseByIDFunc = func(ctx context.Context, ID uint) (*android.EnterpriseDetails, error) { - return &android.EnterpriseDetails{}, nil - } - ds.Store.GetEnterpriseBySignupTokenFunc = func(ctx context.Context, signupToken string) (*android.EnterpriseDetails, error) { - if signupToken == "signup_token" { - return &android.EnterpriseDetails{}, nil - } - return nil, ¬FoundError{} - } - ds.Store.DeleteAllEnterprisesFunc = func(ctx context.Context) error { - return nil - } - ds.Store.DeleteOtherEnterprisesFunc = func(ctx context.Context, ID uint) error { - return nil - } + setAndroidStoreMocks(&ds) ds.Store.AppConfigFunc = func(_ context.Context) (*fleet.AppConfig, error) { return &fleet.AppConfig{}, nil }Add this helper in the same file (outside the selected range):
func setAndroidStoreMocks(ds *AndroidMockDS) { ds.Store.CreateEnterpriseFunc = func(ctx context.Context, _ uint) (uint, error) { return 1, nil } ds.Store.UpdateEnterpriseFunc = func(ctx context.Context, _ *android.EnterpriseDetails) error { return nil } ds.Store.GetEnterpriseFunc = func(ctx context.Context) (*android.Enterprise, error) { return &android.Enterprise{}, nil } ds.Store.GetEnterpriseByIDFunc = func(ctx context.Context, _ uint) (*android.EnterpriseDetails, error) { return &android.EnterpriseDetails{}, nil } ds.Store.GetEnterpriseBySignupTokenFunc = func(ctx context.Context, signupToken string) (*android.EnterpriseDetails, error) { if signupToken == "signup_token" { return &android.EnterpriseDetails{}, nil } return nil, ¬FoundError{} } ds.Store.DeleteAllEnterprisesFunc = func(ctx context.Context) error { return nil } ds.Store.DeleteOtherEnterprisesFunc = func(ctx context.Context, _ uint) error { return nil } }
215-219: Optional: prefer prod-style NotFound for parity with service code paths.The custom notFoundError type is adequate for tests because fleet.IsNotFound(err) hinges on IsNotFound(). If you want closer parity with production behavior, consider returning common_mysql.NotFound("Enterprise").WithName(signupToken) from the mock instead of a bespoke error type and drop notFoundError entirely.
server/datastore/mysql/android_device_test.go (2)
78-101: Stabilize the timestamp to avoid edge-case flakiness.Using time.Now().UTC().Truncate(time.Millisecond) is usually fine; capturing it once improves determinism if assertions happen across DB roundtrips on busy CI agents.
- created.AndroidPolicyID = ptr.Uint(5) - created.LastPolicySyncTime = ptr.Time(time.Now().UTC().Truncate(time.Millisecond)) + created.AndroidPolicyID = ptr.Uint(5) + now := time.Now().UTC().Truncate(time.Millisecond) + created.LastPolicySyncTime = ptr.Time(now)
103-115: Minor readability: avoid reassigning the input parameter inside the tx closure.Returning into a separate variable makes the ownership/lifetime clearer.
-func (ds *Datastore) createDevice(ctx context.Context, device *android.Device) (*android.Device, error) { - err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { - var err error - device, err = ds.CreateDeviceTx(ctx, tx, device) - return err - }) - if err != nil { - return nil, err - } - return device, nil -} +func (ds *Datastore) createDevice(ctx context.Context, device *android.Device) (*android.Device, error) { + var created *android.Device + err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { + var err error + created, err = ds.CreateDeviceTx(ctx, tx, device) + return err + }) + if err != nil { + return nil, err + } + return created, nil +}server/datastore/mysql/android_enterprise_test.go (1)
125-134: Add one more negative: empty/blank signup token.Consider asserting that GetEnterpriseBySignupToken rejects empty or whitespace-only tokens to guard input validation and avoid accidental full scans.
Example:
_, err := ds.GetEnterpriseBySignupToken(testCtx(), "") assert.True(t, fleet.IsNotFound(err))server/datastore/mysql/android_mysql.go (2)
15-20: Add compile-time interface assertion for safetyTo ensure future refactors don’t silently drop methods required by android.Datastore, add a compile-time assertion.
Apply this diff:
type AndroidDatastore struct { logger log.Logger primary *sqlx.DB replica fleet.DBReader // so it cannot be used to perform writes } + +// Ensure AndroidDatastore implements android.Datastore at compile time. +var _ android.Datastore = (*AndroidDatastore)(nil)
34-39: Guard against nil replica in reader()If ds.replica is nil (misconfiguration or certain test setups), reads will panic. Favor a safe fallback to primary.
Apply this diff:
-func (ds *AndroidDatastore) reader(ctx context.Context) fleet.DBReader { +func (ds *AndroidDatastore) reader(ctx context.Context) fleet.DBReader { if ctxdb.IsPrimaryRequired(ctx) { return ds.primary } - return ds.replica + if ds.replica == nil { + return ds.primary + } + return ds.replica }server/datastore/mysql/android_hosts.go (2)
39-50: Deletion choice is nondeterministicGiven the SELECT order is unspecified without ORDER BY, existing[0] could be any duplicate. The proposed ORDER BY ASC above makes you consistently delete the oldest row and update the newer one.
58-72: Insert looks correct; consider minor readability tweakSQL is valid as-is. Optionally, keep VALUES placeholders on one line to reduce diff churn.
Apply this diff (cosmetic):
- stmt := `INSERT INTO android_devices (host_id, device_id, enterprise_specific_id, android_policy_id, last_policy_sync_time) VALUES (?, ?, ?, ?, -?)` + stmt := `INSERT INTO android_devices (host_id, device_id, enterprise_specific_id, android_policy_id, last_policy_sync_time) + VALUES (?, ?, ?, ?, ?)`server/datastore/mysql/android_enterprises.go (2)
14-23: CreateEnterprise path looks goodInsert is parameterized and returns the ID. Optional: check and return LastInsertId error explicitly.
Apply this diff (optional):
- id, _ := res.LastInsertId() + id, err := res.LastInsertId() + if err != nil { + return 0, ctxerr.Wrap(ctx, err, "getting android_enterprises last insert ID") + }
64-83: Fix copy/paste error in wrapped messageError text says “inserting enterprise” in an UPDATE path.
Apply this diff:
- if err != nil { - return ctxerr.Wrap(ctx, err, "inserting enterprise") - } + if err != nil { + return ctxerr.Wrap(ctx, err, "updating enterprise") + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
.github/workflows/test-db-changes.yml(1 hunks)Makefile(2 hunks)git-hooks/backend/hooks/db-schema(1 hunks)server/datastore/mysql/android.go(2 hunks)server/datastore/mysql/android_device_test.go(2 hunks)server/datastore/mysql/android_enterprise_test.go(5 hunks)server/datastore/mysql/android_enterprises.go(7 hunks)server/datastore/mysql/android_hosts.go(5 hunks)server/datastore/mysql/android_mysql.go(3 hunks)server/datastore/mysql/mysql.go(1 hunks)server/mdm/android/arch_test.go(1 hunks)server/mdm/android/mock/android.go(0 hunks)server/mdm/android/mock/datastore.go(0 hunks)server/mdm/android/mock/datastore_setup.go(0 hunks)server/mdm/android/mysql/schema.sql(0 hunks)server/mdm/android/mysql/testing_utils.go(0 hunks)server/mdm/android/service/enterprises_test.go(3 hunks)server/mdm/android/tests/testing_utils.go(5 hunks)tools/ci/rules.go(0 hunks)tools/dbutils/schema_generator.go(1 hunks)
💤 Files with no reviewable changes (6)
- server/mdm/android/mock/android.go
- server/mdm/android/mysql/schema.sql
- tools/ci/rules.go
- server/mdm/android/mock/datastore_setup.go
- server/mdm/android/mysql/testing_utils.go
- server/mdm/android/mock/datastore.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:
tools/dbutils/schema_generator.goserver/datastore/mysql/android_enterprises.goserver/datastore/mysql/android.goserver/mdm/android/arch_test.goserver/datastore/mysql/android_device_test.goserver/datastore/mysql/android_hosts.goserver/datastore/mysql/mysql.goserver/mdm/android/service/enterprises_test.goserver/datastore/mysql/android_mysql.goserver/datastore/mysql/android_enterprise_test.goserver/mdm/android/tests/testing_utils.go
🧠 Learnings (4)
📚 Learning: 2025-08-01T15:08:16.858Z
Learnt from: sgress454
PR: fleetdm/fleet#31508
File: server/datastore/mysql/schema.sql:102-116
Timestamp: 2025-08-01T15:08:16.858Z
Learning: The schema.sql file in server/datastore/mysql/ is auto-generated from migrations for use with tests, so it cannot be manually edited. Any changes must be made through migrations.
Applied to files:
.github/workflows/test-db-changes.ymlMakefile
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/datastore/mysql/android_enterprise_test.goserver/mdm/android/tests/testing_utils.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/datastore/mysql/android_enterprise_test.goserver/mdm/android/tests/testing_utils.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet, tests may validly use testing.T.Context() when the module/toolchain targets Go 1.24+. Do not flag t.Context() usage in this codebase if go.mod/toolchain indicates Go >= 1.24.
Applied to files:
server/datastore/mysql/android_enterprise_test.go
🔇 Additional comments (24)
server/datastore/mysql/mysql.go (1)
269-269: Wiring Android datastore viaNewAndroidDatastorelooks good.Embedding
android.Datastoreand initializing it here keeps construction localized without changing the public API surface.server/datastore/mysql/android.go (2)
119-123: Switch tods.CreateDeviceTxis correct and preserves transactional consistency.This call uses the enclosing transaction (
tx) and the unified Android datastore underneath. No issues spotted.
206-209: Update path usesds.UpdateDeviceTxwithin the same transaction — good.Keeping device updates in the same
withRetryTxxscope avoids partial host/device state.git-hooks/backend/hooks/db-schema (2)
6-6: LGTM: focus the diff on the single consolidated schema file.Limiting the check to server/datastore/mysql/schema.sql aligns with the merge of Android schema into the main datastore.
6-11: No lingering Android schema references detectedRan the provided ripgrep check against
server/mdm/android/mysql/schema.sqland confirmed zero matches. All hooks, workflows, and targets have been updated accordingly.Makefile (2)
235-235: Correct: single-target schema generation now matches the consolidated schema.Updating only ./server/datastore/mysql/schema.sql via dbutils is consistent with the “one datastore, one schema.sql” direction and with the auto-generated rule in this repo.
310-310: Approve: MySQL test bucket trimmed and Android tests moved
The Makefile’sMYSQL_PKGS_TO_TEST := ./server/datastore/mysql/...is correct. Android datastore tests have been relocated underserver/datastore/mysql. Verification confirms:
- server/datastore/mysql/android_device_test.go
- server/datastore/mysql/android_enterprise_test.go
No tests remain under
server/mdm/android/mysql.server/mdm/android/service/enterprises_test.go (1)
11-11: Good: use concrete android types in tests.Pulling in server/mdm/android provides the canonical Enterprise and EnterpriseDetails types for the mocks.
server/datastore/mysql/android_device_test.go (1)
21-31: Rename and new case look good; coverage expanded to updates.TestAndroidDevices consolidates device CRUD tests under the MySQL package and adds an explicit “UpdateDevice” case, which is valuable.
server/datastore/mysql/android_enterprise_test.go (2)
13-24: Good rename and additional coverage.Renaming to TestAndroidEnterprises clarifies scope, and adding DeleteEnterprises plus GetEnterpriseBySignupToken exercises critical paths after the merge.
96-123: Scenario coverage is solid; keep the “no enterprise_id” case.Validating DeleteOtherEnterprises with a record lacking enterprise_id is valuable to ensure filtering logic behaves as intended.
server/datastore/mysql/android_mysql.go (2)
22-29: Constructor rename looks goodNewAndroidDatastore wiring is straightforward and returns the android.Datastore interface as before. No concerns.
43-49: Writer and WithRetryTxx align with existing patternsReturning the primary for writes and delegating retries to common_mysql is consistent with the main datastore.
server/datastore/mysql/android_hosts.go (4)
52-56: LGTMDirect, parameterized delete by primary key.
94-97: LGTMThin wrapper is fine.
74-92: Alldbstruct tags are correctly defined onandroid.Device
TheDevicestruct in server/mdm/android/android.go (lines 43–48) includes matchingdbtags for all named parameters used inupdateDevice(id,host_id,device_id,enterprise_specific_id,android_policy_id,last_policy_sync_time), sosqlx.Namedwill bind as expected.
12-37: Lock rows during duplicate-resolution to avoid races; make deletion deterministic
- The SELECT used to detect existing devices should lock the matching rows within the transaction to prevent concurrent inserts/updates from causing flakiness or duplicate errors.
- Add
ORDER BY id ASCso that when two rows are returned you deterministically delete the oldest row.- Verified that
server/datastore/mysql/schema.sqldefines unique indexes on bothdevice_idandenterprise_specific_id(indexesidx_android_devices_device_idandidx_android_devices_enterprise_specific_id), so locking these rows is safe and will target the correct records.Apply this diff:
--- a/server/datastore/mysql/android_hosts.go +++ b/server/datastore/mysql/android_hosts.go @@ -12,7 +12,12 @@ func (ds *AndroidDatastore) CreateDeviceTx(ctx context.Context, tx sqlx.ExtContext - stmt := `SELECT id, device_id, enterprise_specific_id FROM android_devices WHERE device_id = ? OR enterprise_specific_id = ?` + stmt := ` + SELECT id, device_id, enterprise_specific_id + FROM android_devices + WHERE device_id = ? OR enterprise_specific_id = ? + ORDER BY id ASC + FOR UPDATE`server/datastore/mysql/android_enterprises.go (4)
25-36: LGTMScoped by ID with correct not-found handling.
38-49: LGTMSignup-token lookup is properly scoped.
85-92: Caution: destructive deleteThis deletes every row except the one provided. Ensure callers only pass the intended survivor and consider wrapping higher-level flows in a transaction to avoid partial state if follow-up steps fail.
Would you like me to wrap the call sites in a WithRetryTxx transaction and propose a patch?
94-101: LGTMAppropriate for test cleanup paths.
server/mdm/android/tests/testing_utils.go (3)
44-47: Wrapper correctly routes AppConfig to the mockThis disambiguation avoids accidental calls into the real datastore. Good.
48-83: Device/enterprise wrappers look correctForwarding to the embedded real datastore is clear and avoids circular mocking. No issues.
176-178: LGTMClosing via Datastore.Close() is the right companion to the constructor change.
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good overall.
The SSE changes are due to a flaky test, right? I recommend moving those to a separate PR so we don't mix different issues here.
@getvictor Exactly, yes. I've reverted those before going from draft —> open. 👍 Thanks! |
4 participants
Resolves #31218
Summary
server/mdm/android/mysqltoserver/datastore/mysqlwithandroid_prefixTest plan
Summary by CodeRabbit
No user-facing changes.