Self-service categories - migration - #46488
Conversation
- add LegacySoftwareCategoryNames map + TranslateLegacySoftwareCategoryNames helper in server/fleet/software.go; call it inside GetSoftwareCategoryIDs and GetSoftwareCategoryNameToIDMap so callers passing pre-migration plain names (FMA inputs, existing admin YAMLs) still resolve to the renamed rows - testing_utils schema loader: pass --default-character-set=utf8mb4 to the mysql CLI; without it the seeded emoji rows in schema.sql get double-encoded on import - update affected test assertions and fixtures to expect the emoji-prefixed category names directly - nolint:gosec G115 on uint conversions in the migration test
Schema.sql conflict was due to migration_status_tables row from the merged branch's migrations; bumped 20260529184804 to 20260529201240 and regenerated schema.sql so all merged migrations appear in order.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/39018-self-service-categories #46488 +/- ##
===================================================================
Coverage 66.78% 66.79%
===================================================================
Files 2804 2805 +1
Lines 223567 223664 +97
Branches 11308 11308
===================================================================
+ Hits 149310 149389 +79
- Misses 60695 60706 +11
- Partials 13562 13569 +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:
|
|
Some tests are failing due to unrelated reasons. The fix for them is not in this branch. I updated |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![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.
|
@claude review once |
| return []uint{}, nil | ||
| } | ||
| names = fleet.TranslateLegacySoftwareCategoryNames(names) | ||
|
|
||
| stmt := `SELECT id FROM software_categories WHERE name IN (?)` |
There was a problem hiding this comment.
🔴 Both GetSoftwareCategoryIDs (software.go:6766) and GetSoftwareCategoryNameToIDMap (software.go:6789) query software_categories by name with no team_id filter, but the migration backfills N+1 rows per default name (team_id=0 + one per pre-existing team). After upgrading a deployment that has any teams, GetSoftwareCategoryIDs returns N+1 IDs per requested name, breaking the len(catIDs) == len(payload.Categories) validation at 5 call sites and rejecting valid category assignments with "some or all of the categories provided don't exist" (ee/server/service/software_installers.go:411, :2723; ee/server/service/vpp.go:348, :936, :1276). GetSoftwareCategoryNameToIDMap silently overwrites in its result loop, so FMA installer category links (ee/server/service/maintained_apps.go:187) end up pointing at a random team's category row — silent cross-team data corruption. Both queries need a team_id parameter and filter (with a team_id=0 fallback for the legacy plain-name translation).
Extended reasoning...
Why this fails after the migration
The new migration's backfill step at server/datastore/mysql/migrations/tables/20260529203429_AddTeamIDToSoftwareCategories.go does:
INSERT INTO software_categories (name, team_id)
SELECT sc.name, t.id FROM software_categories sc
CROSS JOIN teams t WHERE sc.team_id = 0So in a production upgrade with N existing teams, each default category name (e.g. 🌎 Browsers) ends up with N+1 rows: one at team_id=0 and one per pre-existing team. The migration test itself asserts this — assertTeamCategories requires "team %d should have all 6 default categories".
Failure mode 1 — length-mismatch validation rejects valid input
GetSoftwareCategoryIDs at server/datastore/mysql/software.go:6770:
stmt := `SELECT id FROM software_categories WHERE name IN (?)`No team_id predicate. With N≥1 teams, SELECT id ... WHERE name IN ('🌎 Browsers') returns N+1 rows. All five callers then do:
catIDs, err := svc.ds.GetSoftwareCategoryIDs(ctx, payload.Categories)
if len(catIDs) != len(payload.Categories) {
return ctxerr.Wrap(..., "some or all of the categories provided don't exist")
}at ee/server/service/software_installers.go:411, :2723; ee/server/service/vpp.go:348, :936, :1276. Length never matches → BadRequest for any installer category assignment.
Failure mode 2 — silent cross-team data corruption
GetSoftwareCategoryNameToIDMap (software.go:6789-6816) has the same name-only query, then:
for _, cat := range categories {
result[cat.Name] = cat.ID
}When the SELECT returns N+1 rows for the same name, this loop overwrites — whichever row MySQL returns last wins (no ORDER BY, so effectively non-deterministic). The follow-up legacy-translation loop then propagates that arbitrary ID under the plain-name key too.
ee/server/service/maintained_apps.go:187 calls this for FMA installer category linking. The link rows in software_installer_software_categories get inserted pointing at the wrong team's category row — invisible failure. The UX guarantee from the test ("deleting team A's category cascades to team A's installer links only") silently breaks.
Step-by-step proof
- Production deploys 4.86 with 3 teams (IDs 1, 2, 3).
software_categorieshas 6 rows (team_id=0). - Migration 20260529203429 runs.
- After backfill,
software_categoriescontains 4×6=24 rows.SELECT * FROM software_categories WHERE name = '🌎 Browsers'returns 4 rows (team_id=0,1,2,3 with IDs 2, 8, 14, 20 for instance). - Admin edits a software installer on team 1, sends
PATCH /api/.../titles/X/packagewithcategories=['🌎 Browsers']. updateSoftwareInstallercallsGetSoftwareCategoryIDs(ctx, ['🌎 Browsers'])→ returns[2, 8, 14, 20](len=4).len(catIDs) != len(payload.Categories)→ 4 != 1 → returns 400 "some or all of the categories provided don't exist".- Identically for VPP batch-associate and in-house installer flows.
For maintained_apps.go: an FMA installer is created on team 1 with Categories=['💻 Productivity']. GetSoftwareCategoryNameToIDMap runs the bare query, gets rows for team_id=0/1/2/3, loops and overwrites. The final result['💻 Productivity'] is whatever MySQL returned last (likely team_id=3's row, ID 21). The installer's link row gets inserted with software_category_id=21, but the installer is on team 1. If team 3 is later deleted, the FK cascade removes the link — even though the installer is on team 1.
Why CI didn't catch it
schema.sql is a post-migration dump containing only the 6 team_id=0 rows. Integration tests create teams after loading schema.sql, and NewTeam (server/datastore/mysql/teams.go:33) has no per-team category backfill — that's a separate gap. Only the migration itself, against pre-existing teams in a production database, creates the per-team copies. The migration test proves backfill happens, but doesn't exercise these getters.
Fix
Both GetSoftwareCategoryIDs and GetSoftwareCategoryNameToIDMap need a team_id parameter, the SELECT needs AND team_id = ?, and the legacy plain-name translation path needs a team_id=0 fallback (since the renamed default rows live at team_id=0). All five software_installers.go/vpp.go call sites and the maintained_apps.go caller need to thread the relevant team through.
There was a problem hiding this comment.
This is valid feedback, but it will be addressed in the next PR for the API endpoints for #46391
Claude:
Point 1 (lookup signatures):** Endpoints branch has
GetSoftwareCategoryIDs(ctx, teamID uint, names)andGetSoftwareCategoryNameToIDMap(ctx, teamID uint, names). All 5 call sites insoftware_installers.go/vpp.go/maintained_apps.gopass the team_id through. Plus theLegacySoftwareCategoryNamesmap gives the dual-keyed result we already discussed for the plain→emoji translation fallback. So the multi-row collapse can't happen there.
| if _, err := tx.Exec(` | ||
| INSERT INTO software_categories (name, team_id) | ||
| SELECT sc.name, t.id | ||
| FROM software_categories sc | ||
| CROSS JOIN teams t | ||
| WHERE sc.team_id = 0 | ||
| ORDER BY t.id, FIELD(sc.name, | ||
| '🌎 Browsers', | ||
| '👬 Communication', | ||
| '🧰 Developer tools', | ||
| '💻 Productivity', | ||
| '🔐 Security', | ||
| '🛟 Support') | ||
| `); err != nil { | ||
| return errors.Wrap(err, "backfilling per-fleet default categories") |
There was a problem hiding this comment.
🟡 The migration's second step backfills 6 default categories per team via CROSS JOIN teams, but ds.NewTeam (server/datastore/mysql/teams.go:33) has no matching hook, so teams created after this migration runs won't have their own copies of the defaults. This leaves the schema invariant 'every team has its own copies of the 6 defaults' broken from day one for any post-migration team — the PR description's promise that defaults are duplicated 'for every fleet so that default categories can be edited and deleted by admins' holds only for teams that existed at migration time. Marked as a nit because no read path enforces team_id yet (categories continue to resolve to the team_id=0 row by name), so this isn't user-visible until per-team isolation lands; suggest pairing the team-creation backfill with that follow-up.
Extended reasoning...
What the migration sets up. This PR adds team_id to software_categories, renames the 6 defaults to emoji-prefixed names at team_id=0, then issues a one-time INSERT ... SELECT ... CROSS JOIN teams that duplicates those 6 defaults for every team that exists at migration time, and re-points existing installer/VPP/in-house link rows from the team_id=0 row to the team's own row. The migration test (20260529203429_AddTeamIDToSoftwareCategories_test.go) verifies this for two pre-existing teams.
What it doesn't set up. ds.NewTeam (server/datastore/mysql/teams.go:33-67) still only INSERTs into the teams table and calls saveTeamSecretsDB. No hook in this PR mirrors the migration's per-team backfill at team-creation time. NewSoftwareCategory (software.go:6752) only accepts a name (no team_id argument), so no API surface in the PR can create per-team default rows either.
Step-by-step. Apply the migration on a fresh dev DB → 6 rows at team_id=0 plus N×6 rows for the N existing teams. Now POST /api/latest/fleet/teams to create team_new. Re-query SELECT name FROM software_categories WHERE team_id = team_new.id → zero rows. The migration test exercises pre-existing teams only; if you add an execNoErrLastID(t, db,``INSERT INTO teams (name) VALUES (?)``, "team-c") after applyNext and re-run assertTeamCategories(teamC), it fails: Len(rows, 6) returns 0.
Why the refutation's scope point is fair. Today's read paths (GetSoftwareCategoryIDs, GetSoftwareCategoryNameToIDMap in software.go) don't filter by team_id — they resolve names to whichever row matches, which in practice is the team_id=0 row. So a brand-new team without its own copies still resolves "💻 Productivity" to the global id=1, and installer/VPP/in-house links land on the shared row. No end-user-visible breakage yet.
Why it's still worth flagging. The migration explicitly establishes the invariant 'every team has its own 6 defaults'. The PR description states 'duplicates that for every fleet so that default categories can be edited and deleted by admins' — a contract that silently fails for every post-migration team. The moment a follow-up PR lands that (a) filters reads by team_id, (b) adds an admin edit/delete category API, or (c) re-runs a similar name-join re-pointing pass, new-team installer links have nothing to bind to and the failure becomes user-visible (or worse — editing the "team's" default would mutate the shared team_id=0 row across every team). Pairing the team-creation backfill with that follow-up keeps the invariant intact from the moment per-team isolation matters.
Suggested fix. Extend ds.NewTeam to insert the 6 default rows for the new team's id within the same transaction, mirroring the migration's backfill:
INSERT INTO software_categories (name, team_id)
SELECT name, ? FROM software_categories WHERE team_id = 0Place it inside the existing withRetryTxx callback alongside saveTeamSecretsDB so a partial creation can't leave a team without its defaults.
There was a problem hiding this comment.
This is valid feedback, but it will be addressed in the next PR for the API endpoints for #46391
Claude:
Point 2 (NewTeam seeding):** Endpoints branch's
ds.NewTeamends withds.BatchNewSoftwareCategories(ctx, team.ID, fleet.DefaultSelfServiceCategoryNames)so every team created post-migration is automatically seeded with its own copy of the defaults.
57d78c5
into
feat/39018-self-service-categories
2 participants
Related issue: Resolves #46390
Migration to add a non nullable team_id column. I chose this approach so that server code doesn't have to deal with a potentially null team id for categories. It first adds team_id=0 to all the existing categories, then duplicates that for every fleet so that default categories can be edited and deleted by admins.
It also renames the default categories to include the emojis in their name, which required updating some test expectations, and also mapping default names to the new ones for Fleet maintained apps. If we don't do that, FMA's manifests would have to all be updated right after 4.87 releases and every user would have to migrate immediately. This would also break existing gitops files if the names aren't mapped. The alternative would be to keep the names unchanged, and add custom logic in various places to insert the emojis in the backend and frontend.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
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
Database migrations
COLLATE utf8mb4_unicode_ci).