From b3c2a6368dbe013c2d7508021f707872c10ce893 Mon Sep 17 00:00:00 2001 From: Carlo <1778532+cdcme@users.noreply.github.com> Date: Mon, 6 Jul 2026 17:05:59 -0400 Subject: [PATCH] Count FMAs by slug (#48783) **Related issue:** Resolves #48528 This keys the count, pagination, and the frontend row-combining on the app's slug token (the prefix before `/`, shared across an app's platform entries but distinct across apps). The count now equals the rows shown in every view (macOS, Windows, All), and name-colliding apps stay as separate rows. # Checklist for submitter - [x] 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. ## Testing - [x] Added/updated automated tests - [x] 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 ## Summary by CodeRabbit * **New Features** * Software listings now group platform-specific installers into a single app row based on the app identifier, improving how macOS and Windows entries appear together. * **Bug Fixes** * Apps with the same display name but different identifiers now stay separate instead of being merged incorrectly. * List counts and pagination now match the combined app view more accurately across the software pages. --- frontend/__mocks__/softwareMock.ts | 1 + .../InstallerActionCell.tests.tsx | 13 ++- frontend/interfaces/software.ts | 1 + .../FleetMaintainedAppsTable.tests.tsx | 48 +++++++++++ .../FleetMaintainedAppsTable.tsx | 21 +++-- server/datastore/mysql/maintained_apps.go | 61 +++++++------- .../datastore/mysql/maintained_apps_test.go | 84 +++++++++++++++---- server/service/integration_enterprise_test.go | 14 ++-- 8 files changed, 183 insertions(+), 60 deletions(-) create mode 100644 frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppsTable/FleetMaintainedAppsTable.tests.tsx diff --git a/frontend/__mocks__/softwareMock.ts b/frontend/__mocks__/softwareMock.ts index 0b89d5321c..a4c68ea27e 100644 --- a/frontend/__mocks__/softwareMock.ts +++ b/frontend/__mocks__/softwareMock.ts @@ -369,6 +369,7 @@ const DEFAULT_FLEET_MAINTAINED_APPS_MOCK: IFleetMaintainedApp = { name: "test app", version: "1.2.3", platform: "darwin", + slug: "test-app/darwin", }; export const createMockFleetMaintainedApp = ( diff --git a/frontend/components/TableContainer/DataTable/InstallerActionCell/InstallerActionCell.tests.tsx b/frontend/components/TableContainer/DataTable/InstallerActionCell/InstallerActionCell.tests.tsx index b718db759d..7db5d91023 100644 --- a/frontend/components/TableContainer/DataTable/InstallerActionCell/InstallerActionCell.tests.tsx +++ b/frontend/components/TableContainer/DataTable/InstallerActionCell/InstallerActionCell.tests.tsx @@ -5,7 +5,11 @@ import InstallerActionCell from "./InstallerActionCell"; describe("InstallerAction cell", () => { it("renders add button if installer is available", async () => { - render(); + render( + + ); expect(screen.getByText(/add/i)).toBeInTheDocument(); }); @@ -17,7 +21,12 @@ describe("InstallerAction cell", () => { it("renders checkmark if installer is already added", async () => { render( ); diff --git a/frontend/interfaces/software.ts b/frontend/interfaces/software.ts index 69749c939a..86d290bde1 100644 --- a/frontend/interfaces/software.ts +++ b/frontend/interfaces/software.ts @@ -891,6 +891,7 @@ export interface IFleetMaintainedApp { name: string; version: string; platform: FleetMaintainedAppPlatform; + slug: string; // "/", e.g. "figma/darwin"; the token uniquely identifies an app across its platform entries software_title_id?: number; // null unless the team already has the software added (as a Fleet-maintained app, App Store (app), or custom package) } diff --git a/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppsTable/FleetMaintainedAppsTable.tests.tsx b/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppsTable/FleetMaintainedAppsTable.tests.tsx new file mode 100644 index 0000000000..119ad3955f --- /dev/null +++ b/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppsTable/FleetMaintainedAppsTable.tests.tsx @@ -0,0 +1,48 @@ +import { IFleetMaintainedApp } from "interfaces/software"; + +import { combineAppsByPlatform } from "./FleetMaintainedAppsTable"; + +const app = (overrides: Partial): IFleetMaintainedApp => ({ + id: 1, + name: "App", + version: "1.0", + platform: "darwin", + slug: "app/darwin", + ...overrides, +}); + +describe("combineAppsByPlatform", () => { + it("combines an app's macOS and Windows entries into a single row", () => { + const combined = combineAppsByPlatform([ + app({ id: 1, name: "Figma", slug: "figma/darwin", platform: "darwin" }), + app({ id: 2, name: "Figma", slug: "figma/windows", platform: "windows" }), + ]); + + expect(combined).toHaveLength(1); + expect(combined[0].name).toBe("Figma"); + expect(combined[0].macos?.id).toBe(1); + expect(combined[0].windows?.id).toBe(2); + }); + + it("keeps two distinct apps that share a display name as separate rows", () => { + // MacPaw Gemini and Google Gemini share the name "Gemini" but have + // different slug tokens, so they must not collapse into one row. + const combined = combineAppsByPlatform([ + app({ id: 1, name: "Gemini", slug: "gemini/darwin", platform: "darwin" }), + app({ + id: 2, + name: "Gemini", + slug: "google-gemini/darwin", + platform: "darwin", + }), + ]); + + expect(combined).toHaveLength(2); + // Each row keeps its own macOS entry (neither Gemini is hidden/overwritten). + expect(combined.map((c) => c.macos?.id).sort()).toEqual([1, 2]); + expect(combined.map((c) => c.macos?.slug).sort()).toEqual([ + "gemini/darwin", + "google-gemini/darwin", + ]); + }); +}); diff --git a/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppsTable/FleetMaintainedAppsTable.tsx b/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppsTable/FleetMaintainedAppsTable.tsx index 48e43164b8..6c29b74ccc 100644 --- a/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppsTable/FleetMaintainedAppsTable.tsx +++ b/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppsTable/FleetMaintainedAppsTable.tsx @@ -42,28 +42,31 @@ const EmptyFleetAppsTable = () => ( /> ); -/** Used to convert FleetMaintainedApp API response which has separate entries - * for Windows FMA and macOS FMA into table friendly format that combines - * entries for the same app for different platforms */ -const combineAppsByPlatform = ( +/** Converts the FleetMaintainedApp API response, which has separate macOS and + * Windows entries, into a table-friendly format that combines an app's entries + * for different platforms into one row. Apps are keyed by their slug token (the + * prefix before "/"), not by name, so two distinct apps that share a display + * name stay as separate rows. */ +export const combineAppsByPlatform = ( fmaList: IFleetMaintainedApp[] ): ICombinedFMA[] => { - const combinedApps: { [name: string]: ICombinedFMA } = {}; + const combinedApps: { [appToken: string]: ICombinedFMA } = {}; fmaList.forEach((app: IFleetMaintainedApp) => { const { name, platform, ...rest } = app; + const appToken = app.slug.split("/")[0]; - if (!combinedApps[name]) { - combinedApps[name] = { name, macos: null, windows: null }; + if (!combinedApps[appToken]) { + combinedApps[appToken] = { name, macos: null, windows: null }; } if (platform === "darwin") { - combinedApps[name].macos = { + combinedApps[appToken].macos = { platform: platform as FleetMaintainedAppPlatform, ...rest, }; } else if (platform === "windows") { - combinedApps[name].windows = { + combinedApps[appToken].windows = { platform: platform as FleetMaintainedAppPlatform, ...rest, }; diff --git a/server/datastore/mysql/maintained_apps.go b/server/datastore/mysql/maintained_apps.go index b32860c734..e011a69d77 100644 --- a/server/datastore/mysql/maintained_apps.go +++ b/server/datastore/mysql/maintained_apps.go @@ -13,8 +13,9 @@ import ( ) // maintainedAppsAllowedOrderKeys allowlists order keys for listing -// Fleet-maintained apps. The list is a combined-by-name view, so name is the -// only meaningful key; it's validation-only, since ORDER BY is hard-coded below. +// Fleet-maintained apps. The list is a combined-by-app view (see +// ListAvailableFleetMaintainedApps), so name is the only meaningful key; it's +// validation-only, since ORDER BY is hard-coded below. var maintainedAppsAllowedOrderKeys = common_mysql.OrderKeyAllowlist{ "name": "fma.name", } @@ -223,12 +224,14 @@ func (ds *Datastore) GetMaintainedAppBySlug(ctx context.Context, slug string, te func (ds *Datastore) ListAvailableFleetMaintainedApps(ctx context.Context, teamID *uint, opt fleet.MaintainedAppListOptions) ([]fleet.MaintainedApp, *fleet.PaginationMetadata, error) { dbReader := ds.reader(ctx) - // We paginate by distinct app NAME, because the UI combines an app's macOS - // and Windows entries into a single row and an app must not be split across a - // page boundary. The count, by contrast, is the total number of apps (each - // platform entry is its own installable app). The team join lets us tell - // whether each app has already been added, which the "available only" filter - // needs. + // We paginate and count by distinct app token (the slug prefix, e.g. "figma" + // in "figma/darwin"), which identifies an app across its platform entries. + // The UI combines an app's macOS and Windows entries into one row, so an app + // must not be split across a page boundary and the count must equal the rows + // shown. Keying on the token rather than the name keeps two distinct apps that + // share a name (e.g. gemini/darwin and google-gemini/darwin) separate. The + // team join tells us whether each app is already added, for the "available + // only" filter. fromClause := `FROM fleet_maintained_apps fma` var fromArgs []any if teamID != nil { @@ -252,14 +255,10 @@ func (ds *Datastore) ListAvailableFleetMaintainedApps(ctx context.Context, teamI where += ` AND team_titles.id IS NULL` } - // Total count of matching apps. We count distinct rows (by primary key), not - // distinct names: an app's macOS and Windows entries are separate installable - // apps and are each counted, even though the UI combines them into one row. - // DISTINCT fma.id also collapses any duplicate rows from the team join's - // fan-out. + // Count by distinct token; DISTINCT also collapses the team join's fan-out. countArgs := append(append([]any{}, fromArgs...), whereArgs...) var filteredCount int - if err := sqlx.GetContext(ctx, dbReader, &filteredCount, `SELECT COUNT(DISTINCT fma.id) `+fromClause+where, countArgs...); err != nil { + if err := sqlx.GetContext(ctx, dbReader, &filteredCount, `SELECT COUNT(DISTINCT SUBSTRING_INDEX(fma.slug, '/', 1)) `+fromClause+where, countArgs...); err != nil { return nil, nil, ctxerr.Wrap(ctx, err, "get fleet maintained apps count") } @@ -290,24 +289,26 @@ func (ds *Datastore) ListAvailableFleetMaintainedApps(ctx context.Context, teamI direction = "DESC" } - // Select the page of app names, fetching one extra to detect a next page. + // Select the page of app tokens, fetching one extra to detect a next page. + // Group by the token and order by the app's name (the token maps to a single + // name), with the token as a deterministic tiebreaker for same-named apps. perPage := opt.GetPerPage() - pageNamesStmt := fmt.Sprintf( - `SELECT DISTINCT fma.name %s%s ORDER BY fma.name %s LIMIT %d OFFSET %d`, - fromClause, where, direction, perPage+1, perPage*opt.Page, + pageTokensStmt := fmt.Sprintf( + `SELECT SUBSTRING_INDEX(fma.slug, '/', 1) AS app_token %s%s GROUP BY app_token ORDER BY MIN(fma.name) %s, app_token %s LIMIT %d OFFSET %d`, + fromClause, where, direction, direction, perPage+1, perPage*opt.Page, ) - pageNamesArgs := append(append([]any{}, fromArgs...), whereArgs...) - var pageNames []string - if err := sqlx.SelectContext(ctx, dbReader, &pageNames, pageNamesStmt, pageNamesArgs...); err != nil { - return nil, nil, ctxerr.Wrap(ctx, err, "selecting fleet maintained app page names") + pageTokensArgs := append(append([]any{}, fromArgs...), whereArgs...) + var pageTokens []string + if err := sqlx.SelectContext(ctx, dbReader, &pageTokens, pageTokensStmt, pageTokensArgs...); err != nil { + return nil, nil, ctxerr.Wrap(ctx, err, "selecting fleet maintained app page tokens") } meta := &fleet.PaginationMetadata{HasPreviousResults: opt.Page > 0, TotalResults: uint(filteredCount)} //nolint:gosec // dismiss G115 - if uint(len(pageNames)) > perPage { //nolint:gosec // dismiss G115 + if uint(len(pageTokens)) > perPage { //nolint:gosec // dismiss G115 meta.HasNextResults = true - pageNames = pageNames[:perPage] + pageTokens = pageTokens[:perPage] } - if len(pageNames) == 0 { + if len(pageTokens) == 0 { // Page is past the last result. return []fleet.MaintainedApp{}, meta, nil } @@ -317,13 +318,13 @@ func (ds *Datastore) ListAvailableFleetMaintainedApps(ctx context.Context, teamI selectStmt := `SELECT fma.id, fma.name, fma.platform, fma.slug, ` var rowsArgs []any if teamID != nil { - selectStmt += teamFMATitlesJoin + ` WHERE fma.name IN (?)` - rowsArgs = []any{teamID, teamID, pageNames} + selectStmt += teamFMATitlesJoin + ` WHERE SUBSTRING_INDEX(fma.slug, '/', 1) IN (?)` + rowsArgs = []any{teamID, teamID, pageTokens} } else { - selectStmt += `NULL software_title_id FROM fleet_maintained_apps fma WHERE fma.name IN (?)` - rowsArgs = []any{pageNames} + selectStmt += `NULL software_title_id FROM fleet_maintained_apps fma WHERE SUBSTRING_INDEX(fma.slug, '/', 1) IN (?)` + rowsArgs = []any{pageTokens} } - selectStmt += fmt.Sprintf(` ORDER BY fma.name %s, fma.platform ASC`, direction) + selectStmt += fmt.Sprintf(` ORDER BY fma.name %s, fma.slug ASC`, direction) selectStmt, rowsArgs, err := sqlx.In(selectStmt, rowsArgs...) if err != nil { diff --git a/server/datastore/mysql/maintained_apps_test.go b/server/datastore/mysql/maintained_apps_test.go index 264fb3d85a..c12a65232a 100644 --- a/server/datastore/mysql/maintained_apps_test.go +++ b/server/datastore/mysql/maintained_apps_test.go @@ -24,6 +24,7 @@ func TestMaintainedApps(t *testing.T) { {"Sync", testSync}, {"ListAndGetAvailableApps", testListAndGetAvailableApps}, {"ListAvailableAppsByNameAndFilters", testListAvailableAppsByNameAndFilters}, + {"ListAvailableAppsSharedName", testListAvailableAppsSharedName}, {"SyncAndRemoveApps", testSyncAndRemoveApps}, {"GetMaintainedAppBySlug", testGetMaintainedAppBySlug}, {"ListAvailableAppsWindows", testListAvailableAppsWindows}, @@ -456,7 +457,7 @@ func testListAndGetAvailableApps(t *testing.T, ds *Datastore) { maintained3.TitleID = nil require.Equal(t, maintained3, gotApp) - // Ordering: the combined-by-name view is only meaningfully sortable by name, + // Ordering: the combined-by-app view is only meaningfully sortable by name, // so "name" is the one allowed order key. expectedApps is declared in // ascending name order, so we derive the expected name sequences from it. appNames := func(apps []fleet.MaintainedApp) []string { @@ -681,11 +682,11 @@ func testListAvailableAppsWindows(t *testing.T, ds *Datastore) { require.Nil(t, apps[1].TitleID) } -// testListAvailableAppsByNameAndFilters verifies that the list paginates by -// distinct app NAME (an app's macOS and Windows entries are combined into one -// logical app in the UI) while the total count is by distinct app row (each -// platform entry counted separately), and that the platform and available-only -// filters work server-side. +// testListAvailableAppsByNameAndFilters verifies that the list paginates and +// counts by distinct app TOKEN (the slug prefix), so an app's macOS and Windows +// entries are combined into one logical app for both pagination and the count +// (matching the single combined row the UI renders), and that the platform and +// available-only filters work server-side. func testListAvailableAppsByNameAndFilters(t *testing.T, ds *Datastore) { ctx := context.Background() @@ -721,20 +722,21 @@ func testListAvailableAppsByNameAndFilters(t *testing.T, ds *Datastore) { return fleet.MaintainedAppListOptions{ListOptions: o} } - // Unfiltered: 6 apps (the count, one per platform entry) across 4 names, 6 - // rows returned. + // Unfiltered: 4 apps (the count, one per app token: alpha, beta, gamma, + // delta) across 4 names, 6 rows returned (the raw per-platform entries the UI + // combines into 4 rows). apps, meta, err := ds.ListAvailableFleetMaintainedApps(ctx, &team.ID, listOpts(fleet.ListOptions{})) require.NoError(t, err) - require.EqualValues(t, 6, meta.TotalResults) + require.EqualValues(t, 4, meta.TotalResults) require.Len(t, apps, 6) require.False(t, meta.HasNextResults) - // Pagination is by app name: a page of 2 names that includes a dual-platform + // Pagination is by app token: a page of 2 apps that includes a dual-platform // app returns ALL of that app's rows, so an app is never split across a page // boundary. Page 0 => Alpha (darwin+windows) + Beta (darwin) = 3 rows. apps, meta, err = ds.ListAvailableFleetMaintainedApps(ctx, &team.ID, listOpts(fleet.ListOptions{PerPage: 2})) require.NoError(t, err) - require.EqualValues(t, 6, meta.TotalResults) + require.EqualValues(t, 4, meta.TotalResults) require.True(t, meta.HasNextResults) require.False(t, meta.HasPreviousResults) require.Equal(t, []string{"Alpha", "Alpha", "Beta"}, appNames(apps)) @@ -774,11 +776,10 @@ func testListAvailableAppsByNameAndFilters(t *testing.T, ds *Datastore) { // Available-only hides Beta (its only platform is added) but keeps the other // three apps, which still have at least one not-yet-added platform. The count - // is the 5 not-yet-added platform entries (Alpha macOS+Windows, Gamma - // Windows, Delta macOS+Windows). + // is the 3 remaining app tokens (Alpha, Gamma, Delta). apps, meta, err = ds.ListAvailableFleetMaintainedApps(ctx, &team.ID, fleet.MaintainedAppListOptions{AvailableOnly: true, ListOptions: fleet.ListOptions{IncludeMetadata: true}}) require.NoError(t, err) - require.EqualValues(t, 5, meta.TotalResults) + require.EqualValues(t, 3, meta.TotalResults) require.NotContains(t, appNames(apps), "Beta") // Without the filter, Beta is still listed (as added). @@ -794,6 +795,61 @@ func testListAvailableAppsByNameAndFilters(t *testing.T, ds *Datastore) { require.ElementsMatch(t, []string{"Alpha", "Alpha", "Delta", "Delta"}, appNames(apps)) } +// testListAvailableAppsSharedName verifies that two distinct apps sharing a +// display name (e.g. "Gemini": gemini/darwin and google-gemini/darwin) are +// counted and listed as two separate apps. Keying on the slug token keeps them +// distinct, so the count matches the row count and neither app is hidden. +func testListAvailableAppsSharedName(t *testing.T, ds *Datastore) { + ctx := context.Background() + + team, err := ds.NewTeam(ctx, &fleet.Team{Name: "Team Shared Name"}) + require.NoError(t, err) + + mkApp := func(name, slug, platform, ident string) { + _, err := ds.UpsertMaintainedApp(ctx, &fleet.MaintainedApp{ + Name: name, Slug: slug, Platform: platform, UniqueIdentifier: ident, + }) + require.NoError(t, err) + } + // Two different macOS apps that share the display name "Gemini". + mkApp("Gemini", "gemini/darwin", "darwin", "com.macpaw.site.Gemini2") + mkApp("Gemini", "google-gemini/darwin", "darwin", "com.google.GeminiMacOS") + + assertTwoGeminis := func(opt fleet.MaintainedAppListOptions) { + apps, meta, err := ds.ListAvailableFleetMaintainedApps(ctx, &team.ID, opt) + require.NoError(t, err) + // Both apps counted (not collapsed by shared name) ... + require.EqualValues(t, 2, meta.TotalResults) + // ... and both rows returned, with distinct slugs. + require.Len(t, apps, 2) + slugs := []string{apps[0].Slug, apps[1].Slug} + require.ElementsMatch(t, []string{"gemini/darwin", "google-gemini/darwin"}, slugs) + require.Equal(t, "Gemini", apps[0].Name) + require.Equal(t, "Gemini", apps[1].Name) + } + + // Count must equal rows unfiltered and with the macOS platform filter. + assertTwoGeminis(fleet.MaintainedAppListOptions{ListOptions: fleet.ListOptions{IncludeMetadata: true}}) + assertTwoGeminis(fleet.MaintainedAppListOptions{Platform: "darwin", ListOptions: fleet.ListOptions{IncludeMetadata: true}}) + + // Paginating one app at a time yields each Gemini on its own page, never + // splitting or dropping one. + page0, meta0, err := ds.ListAvailableFleetMaintainedApps(ctx, &team.ID, fleet.MaintainedAppListOptions{ListOptions: fleet.ListOptions{PerPage: 1, IncludeMetadata: true}}) + require.NoError(t, err) + require.EqualValues(t, 2, meta0.TotalResults) + require.Len(t, page0, 1) + require.True(t, meta0.HasNextResults) + + page1, meta1, err := ds.ListAvailableFleetMaintainedApps(ctx, &team.ID, fleet.MaintainedAppListOptions{ListOptions: fleet.ListOptions{PerPage: 1, Page: 1, IncludeMetadata: true}}) + require.NoError(t, err) + require.EqualValues(t, 2, meta1.TotalResults) + require.Len(t, page1, 1) + require.False(t, meta1.HasNextResults) + require.True(t, meta1.HasPreviousResults) + // The two pages cover the two distinct apps. + require.NotEqual(t, page0[0].Slug, page1[0].Slug) +} + func testSoftwareTitleRenamingWindows(t *testing.T, ds *Datastore) { ctx := context.Background() diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index f86c24ce0c..20c7c058c2 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -21025,11 +21025,15 @@ func (s *integrationEnterpriseTestSuite) TestMaintainedApps() { require.False(t, listMAResp.Meta.HasNextResults) require.Len(t, listMAResp.FleetMaintainedApps, len(expectedApps)) - // Count is the total number of platform-specific maintained apps: an app's - // macOS and Windows entries are counted separately, even though the UI - // combines them into a single row. The full unfiltered list returns one row - // per app, so the count equals the number of rows returned. - require.Equal(t, len(expectedApps), listMAResp.Count) + // Count is the number of distinct apps (by slug token), matching the combined + // rows the UI renders: an app's macOS and Windows entries share a token and + // count once. The unfiltered list returns every platform row for the UI to + // combine, so the row count can exceed the count. + distinctAppTokens := make(map[string]struct{}, len(expectedApps)) + for _, a := range expectedApps { + distinctAppTokens[strings.SplitN(a.Slug, "/", 2)[0]] = struct{}{} + } + require.Equal(t, len(distinctAppTokens), listMAResp.Count) sortFMAs := func(a, b fleet.MaintainedApp) int { if c := cmp.Compare(a.Name, b.Name); c != 0 {