diff --git a/.changeset/hdx-2150-deep-review-followups.md b/.changeset/hdx-2150-deep-review-followups.md new file mode 100644 index 0000000000..ff2c07d39a --- /dev/null +++ b/.changeset/hdx-2150-deep-review-followups.md @@ -0,0 +1,23 @@ +--- +'@hyperdx/api': patch +'@hyperdx/common-utils': patch +--- + +External Dashboards API: tighten validation around container/tab references +on the v2 dashboards routes. + +- Cap tile `containerId` and `tabId` at 256 characters to mirror the + internal `DashboardContainer` schema and the `DASHBOARD_CONTAINER_ID_MAX` + constant, now exported from `@hyperdx/common-utils`. +- Cap a single dashboard payload at 500 tiles via the new + `DASHBOARD_MAX_TILES` constant to keep one request from pushing tens of + MB into Mongo. +- Treat empty-string `containerId` / `tabId` on legacy Mongo docs as + absent on read, so dashboards predating the containers feature still + round-trip through the external schema's `min(1)` cap. +- Extract the cross-tile container/tab consistency check into a shared + `validateDashboardContainersConsistency` helper so the canonical + schema and the request body schema agree on what a valid payload is. +- OpenAPI now publishes the matching `maxLength` and `maxItems` bounds + on `DashboardContainer.id`, `DashboardContainerTab.id`, the + `containers` array, and the request `tiles` array. diff --git a/.changeset/hdx-2150-deep-review-roundtrip.md b/.changeset/hdx-2150-deep-review-roundtrip.md new file mode 100644 index 0000000000..915a3f66b9 --- /dev/null +++ b/.changeset/hdx-2150-deep-review-roundtrip.md @@ -0,0 +1,32 @@ +--- +'@hyperdx/api': patch +'@hyperdx/common-utils': patch +--- + +External Dashboards API: fix `PUT` round-trip when the request body omits +`containers`, and self-heal orphan `containerId` / `tabId` references on +read. + +- Move tile-level container/tab reference resolution out of the request + body schema and into the `POST` and `PUT` handlers, so a `PUT` whose + body omits `containers` validates tile refs against the existing + dashboard's containers (the documented "preserve on omit" branch) + rather than against an empty fallback. Without this, a `PUT` that + changes only `tiles` while keeping a tile homed in a real preserved + container was rejected with `Tile references unknown containerId`. +- Split the shared validation helper into a structure-only pass + (`validateDashboardContainersStructure`) and a tile-ref pass + (`validateDashboardTileContainerRefs`) on + `@hyperdx/common-utils`. The composite + `validateDashboardContainersConsistency` now wraps both, so existing + callers keep their current behavior. +- On read, drop `tile.containerId` / `tile.tabId` when the ref does not + resolve to a container (or tab) in the same dashboard. A pre-existing + doc with an orphan ref now round-trips on `GET` as if the ref were + absent, so the next `PUT` validates instead of failing with + `Tile references unknown containerId`. Each drop is logged with the + dashboard id, tile id, and the offending ref. +- Document in the OpenAPI `PUT /api/v2/dashboards/{id}` description that + the endpoint does not support optimistic concurrency. Concurrent PUTs + may silently overwrite each other; clients should serialize edits to + a given dashboard. diff --git a/.changeset/hdx-2150-external-api-containers-tabs.md b/.changeset/hdx-2150-external-api-containers-tabs.md new file mode 100644 index 0000000000..0b5bb6bac9 --- /dev/null +++ b/.changeset/hdx-2150-external-api-containers-tabs.md @@ -0,0 +1,13 @@ +--- +'@hyperdx/api': minor +--- + +External Dashboards API now round-trips the new dashboard organization +layer added in #2015: `containers` on the dashboard, optional `tabs` on each +container, and `containerId` / `tabId` on each tile. Create, get, list, and +update all preserve the structure. The body validates that tile +`containerId` references resolve to a real container, that tile `tabId` +references resolve to a tab inside that container, and that tab ids are +unique within a container. Container id uniqueness is already enforced by +the shared schema. Dashboards saved without `containers` round-trip +unchanged. diff --git a/.changeset/hdx-2150-validators-into-own-module.md b/.changeset/hdx-2150-validators-into-own-module.md new file mode 100644 index 0000000000..2957f40f90 --- /dev/null +++ b/.changeset/hdx-2150-validators-into-own-module.md @@ -0,0 +1,14 @@ +--- +'@hyperdx/api': patch +'@hyperdx/common-utils': patch +--- + +Internal refactor: move `validateDashboardContainersStructure` and +`validateDashboardTileContainerRefs` (and their two helper types) out +of `@hyperdx/common-utils/dist/types` into a new +`@hyperdx/common-utils/dist/dashboardValidation` module. The `types` +file now only contains types and type guards, matching the rest of the +codebase. The previously exported `validateDashboardContainersConsistency` +composite was only used by its own unit test and is dropped; production +code in the v2 dashboards router uses the two underlying helpers +directly. No behaviour change for callers of the external API. diff --git a/packages/api/openapi.json b/packages/api/openapi.json index 265b09de1b..f7dbfd407b 100644 --- a/packages/api/openapi.json +++ b/packages/api/openapi.json @@ -1809,6 +1809,80 @@ } } }, + "DashboardContainerTab": { + "type": "object", + "description": "A single tab inside a dashboard container. Tiles join a tab via tabId.", + "required": [ + "id", + "title" + ], + "properties": { + "id": { + "type": "string", + "minLength": 1, + "maxLength": 256, + "description": "Unique identifier for the tab within its container.", + "example": "errors" + }, + "title": { + "type": "string", + "minLength": 1, + "maxLength": 256, + "description": "Display title for the tab.", + "example": "Errors" + } + } + }, + "DashboardContainer": { + "type": "object", + "description": "A grouping container for tiles on a dashboard. Tiles join a container via containerId.", + "required": [ + "id", + "title", + "collapsed" + ], + "properties": { + "id": { + "type": "string", + "minLength": 1, + "maxLength": 256, + "description": "Unique identifier for the container within the dashboard.", + "example": "service-health" + }, + "title": { + "type": "string", + "minLength": 1, + "maxLength": 256, + "description": "Display title for the container.", + "example": "Service Health" + }, + "collapsed": { + "type": "boolean", + "description": "Persisted default collapse state. Per-viewer state lives in the URL.", + "example": false + }, + "collapsible": { + "type": "boolean", + "description": "Whether the user can collapse the group.", + "default": true, + "example": true + }, + "bordered": { + "type": "boolean", + "description": "Whether to show a visual border around the group.", + "default": true, + "example": true + }, + "tabs": { + "type": "array", + "description": "Optional tabs. 2+ entries renders a tab bar; 0-1 entries renders a plain group header. Tiles join a tab via tabId.", + "maxItems": 20, + "items": { + "$ref": "#/components/schemas/DashboardContainerTab" + } + } + } + }, "TileBase": { "type": "object", "description": "Common fields shared by tile input and output", @@ -1854,6 +1928,20 @@ "config": { "$ref": "#/components/schemas/TileConfig", "description": "Chart configuration for the tile. The displayType field determines which variant is used. Replaces the deprecated \"series\" and \"asRatio\" fields." + }, + "containerId": { + "type": "string", + "minLength": 1, + "maxLength": 256, + "description": "References a DashboardContainer by id. Tiles without containerId render in the default ungrouped area.", + "example": "service-health" + }, + "tabId": { + "type": "string", + "minLength": 1, + "maxLength": 256, + "description": "References a tab inside the tile's container by id. Requires containerId to be set, and the container to declare a matching tab.", + "example": "errors" } } }, @@ -2057,6 +2145,14 @@ "items": { "$ref": "#/components/schemas/SavedFilterValue" } + }, + "containers": { + "type": "array", + "description": "Optional grouping containers. Each tile may join a container via tile.containerId, and a tab inside it via tile.tabId.", + "maxItems": 50, + "items": { + "$ref": "#/components/schemas/DashboardContainer" + } } } }, @@ -2076,6 +2172,7 @@ "tiles": { "type": "array", "description": "List of tiles/charts to include in the dashboard.", + "maxItems": 500, "items": { "$ref": "#/components/schemas/TileInput" } @@ -2118,6 +2215,14 @@ "items": { "$ref": "#/components/schemas/SavedFilterValue" } + }, + "containers": { + "type": "array", + "description": "Optional grouping containers. Each tile may join a container via tile.containerId, and a tab inside it via tile.tabId.", + "maxItems": 50, + "items": { + "$ref": "#/components/schemas/DashboardContainer" + } } } }, @@ -2136,6 +2241,7 @@ }, "tiles": { "type": "array", + "maxItems": 500, "items": { "$ref": "#/components/schemas/TileInput" }, @@ -2180,6 +2286,14 @@ "items": { "$ref": "#/components/schemas/SavedFilterValue" } + }, + "containers": { + "type": "array", + "description": "Optional grouping containers. Each tile may join a container via tile.containerId, and a tab inside it via tile.tabId.", + "maxItems": 50, + "items": { + "$ref": "#/components/schemas/DashboardContainer" + } } } }, @@ -4214,7 +4328,7 @@ }, "put": { "summary": "Update Dashboard", - "description": "Updates an existing dashboard", + "description": "Updates an existing dashboard.\n\n**Concurrency:** This endpoint does not support optimistic\nconcurrency control. Concurrent PUT requests for the same\ndashboard may silently overwrite each other, which can leave\norphan tile-to-container references on layout-shape edits.\nClients should serialize edits to a given dashboard.\n", "operationId": "updateDashboard", "tags": [ "Dashboards" diff --git a/packages/api/src/routers/external-api/__tests__/dashboards.test.ts b/packages/api/src/routers/external-api/__tests__/dashboards.test.ts index be2cd5f869..ef409a729e 100644 --- a/packages/api/src/routers/external-api/__tests__/dashboards.test.ts +++ b/packages/api/src/routers/external-api/__tests__/dashboards.test.ts @@ -3574,6 +3574,761 @@ describe('External API v2 Dashboards - new format', () => { }); }); + describe('Containers and tabs', () => { + const buildTile = ( + sourceId: string, + overrides: Partial = {}, + ): ExternalDashboardTileWithId => ({ + id: new ObjectId().toString(), + name: 'Tile', + x: 0, + y: 0, + w: 6, + h: 3, + config: { + displayType: 'line', + sourceId, + select: [{ aggFn: 'count', where: '' }], + }, + ...overrides, + }); + + it('round-trips containers, tabs, and tile containerId/tabId on create and update', async () => { + const sourceId = traceSource._id.toString(); + const groupedTabA = buildTile(sourceId, { + name: 'In Group, Tab A', + containerId: 'service-health', + tabId: 'errors', + }); + const groupedTabB = buildTile(sourceId, { + name: 'In Group, Tab B', + containerId: 'service-health', + tabId: 'latency', + }); + const groupedNoTab = buildTile(sourceId, { + name: 'In Plain Group', + containerId: 'overview', + }); + const ungrouped = buildTile(sourceId, { name: 'Ungrouped' }); + + const containers = [ + { + id: 'service-health', + title: 'Service Health', + collapsed: false, + collapsible: true, + bordered: true, + tabs: [ + { id: 'errors', title: 'Errors' }, + { id: 'latency', title: 'Latency' }, + ], + }, + { + id: 'overview', + title: 'Overview', + collapsed: true, + }, + ]; + + const createResponse = await authRequest('post', BASE_URL) + .send({ + name: 'Containers Round-Trip', + tiles: [groupedTabA, groupedTabB, groupedNoTab, ungrouped], + tags: ['containers-test'], + containers, + }) + .expect(200); + + expect(createResponse.body.data.containers).toEqual(containers); + const createdTilesByName = Object.fromEntries( + createResponse.body.data.tiles.map((t: ExternalDashboardTileWithId) => [ + t.name, + t, + ]), + ); + expect(createdTilesByName['In Group, Tab A']).toMatchObject({ + containerId: 'service-health', + tabId: 'errors', + }); + expect(createdTilesByName['In Group, Tab B']).toMatchObject({ + containerId: 'service-health', + tabId: 'latency', + }); + expect(createdTilesByName['In Plain Group']).toMatchObject({ + containerId: 'overview', + }); + expect(createdTilesByName['In Plain Group'].tabId).toBeUndefined(); + expect(createdTilesByName.Ungrouped.containerId).toBeUndefined(); + expect(createdTilesByName.Ungrouped.tabId).toBeUndefined(); + + const dashboardId = createResponse.body.data.id; + + const getResponse = await authRequest( + 'get', + `${BASE_URL}/${dashboardId}`, + ).expect(200); + expect(getResponse.body.data.containers).toEqual(containers); + + // Update: rename a tab, drop the second container, re-home tiles. + const updatedContainers = [ + { + id: 'service-health', + title: 'Service Health', + collapsed: true, + tabs: [ + { id: 'errors', title: 'Error Rate' }, + { id: 'latency', title: 'Latency' }, + ], + }, + ]; + const reHomedUngrouped = { + ...createdTilesByName.Ungrouped, + containerId: 'service-health', + tabId: 'errors', + }; + const droppedContainerTile = { + ...createdTilesByName['In Plain Group'], + containerId: undefined, + tabId: undefined, + }; + + const updateResponse = await authRequest( + 'put', + `${BASE_URL}/${dashboardId}`, + ) + .send({ + name: 'Containers Round-Trip', + tiles: [ + createdTilesByName['In Group, Tab A'], + createdTilesByName['In Group, Tab B'], + droppedContainerTile, + reHomedUngrouped, + ], + tags: ['containers-test'], + containers: updatedContainers, + }) + .expect(200); + + expect(updateResponse.body.data.containers).toEqual(updatedContainers); + const updatedTilesByName = Object.fromEntries( + updateResponse.body.data.tiles.map((t: ExternalDashboardTileWithId) => [ + t.name, + t, + ]), + ); + expect(updatedTilesByName['In Plain Group'].containerId).toBeUndefined(); + expect(updatedTilesByName.Ungrouped).toMatchObject({ + containerId: 'service-health', + tabId: 'errors', + }); + }); + + it('round-trips a container with no optional fields set', async () => { + const sourceId = traceSource._id.toString(); + const containers = [ + { + id: 'minimal', + title: 'Minimal', + collapsed: false, + }, + ]; + + const response = await authRequest('post', BASE_URL) + .send({ + name: 'Minimal Container Dashboard', + tiles: [buildTile(sourceId)], + tags: [], + containers, + }) + .expect(200); + + expect(response.body.data.containers).toEqual(containers); + const [container] = response.body.data.containers; + expect(container.collapsible).toBeUndefined(); + expect(container.bordered).toBeUndefined(); + expect(container.tabs).toBeUndefined(); + }); + + it('rejects a tile that references an unknown containerId', async () => { + const sourceId = traceSource._id.toString(); + const response = await authRequest('post', BASE_URL) + .send({ + name: 'Bad Container Reference', + tiles: [buildTile(sourceId, { containerId: 'does-not-exist' })], + tags: [], + containers: [ + { id: 'real-container', title: 'Real', collapsed: false }, + ], + }) + .expect(400); + + expect(response.body.message).toContain( + 'unknown containerId "does-not-exist"', + ); + }); + + it('rejects a tile that references an unknown tabId', async () => { + const sourceId = traceSource._id.toString(); + const response = await authRequest('post', BASE_URL) + .send({ + name: 'Bad Tab Reference', + tiles: [ + buildTile(sourceId, { + containerId: 'service-health', + tabId: 'ghost', + }), + ], + tags: [], + containers: [ + { + id: 'service-health', + title: 'Service Health', + collapsed: false, + tabs: [{ id: 'errors', title: 'Errors' }], + }, + ], + }) + .expect(400); + + expect(response.body.message).toContain('unknown tabId "ghost"'); + }); + + it('rejects a tile that supplies tabId without containerId', async () => { + const sourceId = traceSource._id.toString(); + const response = await authRequest('post', BASE_URL) + .send({ + name: 'Tab Without Container', + tiles: [buildTile(sourceId, { tabId: 'errors' })], + tags: [], + containers: [ + { + id: 'service-health', + title: 'Service Health', + collapsed: false, + tabs: [{ id: 'errors', title: 'Errors' }], + }, + ], + }) + .expect(400); + + expect(response.body.message).toContain( + 'tabId requires containerId to be set', + ); + }); + + it('rejects duplicate container ids', async () => { + const sourceId = traceSource._id.toString(); + const response = await authRequest('post', BASE_URL) + .send({ + name: 'Duplicate Containers', + tiles: [buildTile(sourceId)], + tags: [], + containers: [ + { id: 'dupe', title: 'A', collapsed: false }, + { id: 'dupe', title: 'B', collapsed: false }, + ], + }) + .expect(400); + + expect(response.body.message).toContain('Container IDs must be unique'); + }); + + it('rejects duplicate tab ids within a container', async () => { + const sourceId = traceSource._id.toString(); + const response = await authRequest('post', BASE_URL) + .send({ + name: 'Duplicate Tabs', + tiles: [buildTile(sourceId)], + tags: [], + containers: [ + { + id: 'service-health', + title: 'Service Health', + collapsed: false, + tabs: [ + { id: 'errors', title: 'Errors' }, + { id: 'errors', title: 'Errors Two' }, + ], + }, + ], + }) + .expect(400); + + expect(response.body.message).toContain( + 'Duplicate tab id "errors" in container "service-health"', + ); + }); + + it('round-trips a dashboard with no containers (backward compat)', async () => { + const sourceId = traceSource._id.toString(); + const response = await authRequest('post', BASE_URL) + .send({ + name: 'No Containers', + tiles: [buildTile(sourceId)], + tags: [], + }) + .expect(200); + + expect(response.body.data.containers).toBeUndefined(); + + const dashboardId = response.body.data.id; + const getResponse = await authRequest( + 'get', + `${BASE_URL}/${dashboardId}`, + ).expect(200); + expect(getResponse.body.data.containers).toBeUndefined(); + }); + + // An explicit empty array is semantically equivalent to no organization + // layer. The conversion only emits the field when at least one container + // is present, so the response normalizes [] back to absent. This matches + // the behavior of optional list fields elsewhere in the API. + it('normalizes an explicitly empty containers array to absent on read', async () => { + const sourceId = traceSource._id.toString(); + const response = await authRequest('post', BASE_URL) + .send({ + name: 'Empty Containers Array', + tiles: [buildTile(sourceId)], + tags: [], + containers: [], + }) + .expect(200); + + expect(response.body.data.containers).toBeUndefined(); + + const getResponse = await authRequest( + 'get', + `${BASE_URL}/${response.body.data.id}`, + ).expect(200); + expect(getResponse.body.data.containers).toBeUndefined(); + }); + + it('rejects duplicate container ids on PUT (update path)', async () => { + // The duplicate-id refine has to fire on the PUT body schema as + // well as POST. Without this guard a client could downgrade an + // already-valid dashboard to one with duplicate ids by editing it. + const sourceId = traceSource._id.toString(); + const createResponse = await authRequest('post', BASE_URL) + .send({ + name: 'PUT Duplicate Containers', + tiles: [buildTile(sourceId)], + tags: [], + containers: [ + { id: 'a', title: 'A', collapsed: false }, + { id: 'b', title: 'B', collapsed: false }, + ], + }) + .expect(200); + const dashboardId = createResponse.body.data.id; + + const response = await authRequest('put', `${BASE_URL}/${dashboardId}`) + .send({ + name: 'PUT Duplicate Containers', + tiles: [buildTile(sourceId)], + tags: [], + containers: [ + { id: 'dupe', title: 'A', collapsed: false }, + { id: 'dupe', title: 'B', collapsed: false }, + ], + }) + .expect(400); + + expect(response.body.message).toContain('Container IDs must be unique'); + }); + + it('rejects a tile with containerId set when the dashboard omits containers entirely', async () => { + // Without an explicit `containers: []` and without resolving the + // `data.containers ?? []` default, the tile-level superRefine + // would NPE on `containerById.get`. This guards that the + // containerId still has to resolve even when the field is + // absent. + const sourceId = traceSource._id.toString(); + const response = await authRequest('post', BASE_URL) + .send({ + name: 'Tile with containerId, no containers field', + tiles: [buildTile(sourceId, { containerId: 'service-health' })], + tags: [], + }) + .expect(400); + expect(response.body.message).toContain( + 'unknown containerId "service-health"', + ); + }); + + it('does not require tabId when a tile is in a tabbed container (tile renders without a tab)', async () => { + // The contract is: tabId is only required if the tile WANTS to + // be inside a specific tab. A tile with containerId set to a + // container that has tabs but no tabId of its own renders in the + // container shell rather than under any tab. This guards that + // the schema doesn't accidentally force tabId onto every tile in + // a tabbed container. + const sourceId = traceSource._id.toString(); + await authRequest('post', BASE_URL) + .send({ + name: 'Tabbed container with tile that has no tabId', + tiles: [buildTile(sourceId, { containerId: 'service-health' })], + tags: [], + containers: [ + { + id: 'service-health', + title: 'Service Health', + collapsed: false, + tabs: [ + { id: 'errors', title: 'Errors' }, + { id: 'latency', title: 'Latency' }, + ], + }, + ], + }) + .expect(200); + }); + + it('rejects an empty-string containerId or tabId on a tile', async () => { + const sourceId = traceSource._id.toString(); + const containerResp = await authRequest('post', BASE_URL) + .send({ + name: 'Empty containerId', + tiles: [buildTile(sourceId, { containerId: '' })], + tags: [], + }) + .expect(400); + expect(containerResp.body.message).toContain('tiles.0.containerId'); + + const tabResp = await authRequest('post', BASE_URL) + .send({ + name: 'Empty tabId', + tiles: [ + buildTile(sourceId, { containerId: 'service-health', tabId: '' }), + ], + tags: [], + containers: [ + { + id: 'service-health', + title: 'Service Health', + collapsed: false, + tabs: [{ id: 'errors', title: 'Errors' }], + }, + ], + }) + .expect(400); + expect(tabResp.body.message).toContain('tiles.0.tabId'); + }); + + // The cap mirrors `DASHBOARD_CONTAINER_ID_MAX` in + // `packages/common-utils/src/types.ts`. The 256-char id sits at the + // boundary; 257 chars must reject. + it('accepts a 256-char containerId, rejects 257', async () => { + const sourceId = traceSource._id.toString(); + const idAtMax = 'a'.repeat(256); + const idTooLong = 'a'.repeat(257); + + await authRequest('post', BASE_URL) + .send({ + name: 'Containers id at boundary', + tiles: [buildTile(sourceId, { containerId: idAtMax })], + tags: [], + containers: [{ id: idAtMax, title: 'At boundary', collapsed: false }], + }) + .expect(200); + + const overResp = await authRequest('post', BASE_URL) + .send({ + name: 'Containers id over boundary', + tiles: [buildTile(sourceId, { containerId: idTooLong })], + tags: [], + containers: [ + { id: idTooLong, title: 'Over boundary', collapsed: false }, + ], + }) + .expect(400); + expect(overResp.body.message).toContain('tiles.0.containerId'); + }); + + it('accepts a 256-char tabId, rejects 257', async () => { + const sourceId = traceSource._id.toString(); + const tabAtMax = 'b'.repeat(256); + const tabTooLong = 'b'.repeat(257); + + await authRequest('post', BASE_URL) + .send({ + name: 'Tab id at boundary', + tiles: [ + buildTile(sourceId, { + containerId: 'service-health', + tabId: tabAtMax, + }), + ], + tags: [], + containers: [ + { + id: 'service-health', + title: 'Service Health', + collapsed: false, + tabs: [{ id: tabAtMax, title: 'At boundary' }], + }, + ], + }) + .expect(200); + + const overResp = await authRequest('post', BASE_URL) + .send({ + name: 'Tab id over boundary', + tiles: [ + buildTile(sourceId, { + containerId: 'service-health', + tabId: tabTooLong, + }), + ], + tags: [], + containers: [ + { + id: 'service-health', + title: 'Service Health', + collapsed: false, + tabs: [{ id: tabTooLong, title: 'Over boundary' }], + }, + ], + }) + .expect(400); + expect(overResp.body.message).toContain('tiles.0.tabId'); + }); + + // Cap of 500 mirrors `DASHBOARD_MAX_TILES`. Tested at boundary so the + // limit doesn't accidentally drift. + it('rejects a payload of 501 tiles', async () => { + const sourceId = traceSource._id.toString(); + const tooManyTiles = Array.from({ length: 501 }, (_, i) => + buildTile(sourceId, { name: `Tile ${i}` }), + ); + + const resp = await authRequest('post', BASE_URL) + .send({ + name: 'Too many tiles', + tiles: tooManyTiles, + tags: [], + }) + .expect(400); + // Zod surfaces the path; we just want a 400 with a clear pointer. + expect(resp.body.message).toContain('tiles'); + }); + + // Older Mongo docs predate the containers feature and `tiles: Mixed` + // doesn't enforce `min(1)`. A doc with `containerId: ""` left over + // from earlier code paths must round-trip on read as if absent so + // a subsequent PUT can validate. Insert directly into Mongo so we + // bypass the create-path schema (which now rejects empty strings). + it('treats an empty-string containerId on a legacy doc as absent on read', async () => { + const sourceId = traceSource._id.toString(); + const tile = buildTile(sourceId, { name: 'Legacy empty containerId' }); + const created = await authRequest('post', BASE_URL) + .send({ + name: 'Legacy doc round-trip', + tiles: [tile], + tags: [], + }) + .expect(200); + const dashboardId = created.body.data.id; + + // Mutate Mongo directly to simulate the legacy state. + await Dashboard.updateOne( + { _id: dashboardId }, + { $set: { 'tiles.0.containerId': '', 'tiles.0.tabId': '' } }, + ); + + const getResp = await authRequest( + 'get', + `${BASE_URL}/${dashboardId}`, + ).expect(200); + const [returnedTile] = getResp.body.data.tiles; + expect(returnedTile.containerId).toBeUndefined(); + expect(returnedTile.tabId).toBeUndefined(); + }); + + // P0/P1-1 regression: PUT must preserve the existing containers + // array when the body omits the field. Tile-level container ref + // resolution runs against the effective container set (body + // containers OR existing doc containers), not against the body's + // containers in isolation. Without this guard, a PUT that updates + // only `tiles` and references a real preserved container is + // rejected with "Tile references unknown containerId" because the + // body's containers fall back to an empty array. + it('preserves existing containers on PUT when the body omits the field', async () => { + const sourceId = traceSource._id.toString(); + const containers = [ + { + id: 'service-health', + title: 'Service Health', + collapsed: false, + tabs: [{ id: 'errors', title: 'Errors' }], + }, + ]; + + const created = await authRequest('post', BASE_URL) + .send({ + name: 'Preserve containers on PUT', + tiles: [ + buildTile(sourceId, { + name: 'In container', + containerId: 'service-health', + tabId: 'errors', + }), + ], + tags: [], + containers, + }) + .expect(200); + const dashboardId = created.body.data.id; + const [createdTile] = created.body.data.tiles; + + // PUT keeps the same tile (still pointing at service-health/errors) + // but does not include `containers` in the body. The handler must + // fall back to the existing containers and resolve the tile ref + // against them, so the request succeeds. + const putResp = await authRequest('put', `${BASE_URL}/${dashboardId}`) + .send({ + name: 'Preserve containers on PUT', + tiles: [createdTile], + tags: [], + }) + .expect(200); + + // Containers were preserved on the doc and round-trip on the + // response, even though the request body didn't carry them. + expect(putResp.body.data.containers).toEqual(containers); + const [putTile] = putResp.body.data.tiles; + expect(putTile.containerId).toBe('service-health'); + expect(putTile.tabId).toBe('errors'); + }); + + // P0/P1-1 negative case: even when the body omits `containers`, a + // tile that references a containerId not present in the existing + // dashboard's containers must still be rejected. The fallback only + // permits real containers, not arbitrary ones. + it('rejects a PUT that references an unknown containerId when body omits containers', async () => { + const sourceId = traceSource._id.toString(); + const created = await authRequest('post', BASE_URL) + .send({ + name: 'PUT unknown containerId', + tiles: [buildTile(sourceId, { name: 'Real tile' })], + tags: [], + containers: [{ id: 'real', title: 'Real', collapsed: false }], + }) + .expect(200); + const dashboardId = created.body.data.id; + + const resp = await authRequest('put', `${BASE_URL}/${dashboardId}`) + .send({ + name: 'PUT unknown containerId', + tiles: [ + buildTile(sourceId, { + name: 'Bad ref', + containerId: 'does-not-exist', + }), + ], + tags: [], + }) + .expect(400); + expect(resp.body.message).toContain( + 'unknown containerId "does-not-exist"', + ); + }); + + // Critical P2-4 regression: a Mongo doc with a tile.containerId + // that no longer matches any container in the doc (a container + // got removed without re-homing the tile, or the doc predates + // the containers feature) must round-trip as if the ref were + // absent. Without this self-heal, the GET response would carry + // a containerId that the next PUT body schema rejects, breaking + // the round-trip for legacy data. + it('drops orphan tile.containerId on read when no container matches', async () => { + const sourceId = traceSource._id.toString(); + const created = await authRequest('post', BASE_URL) + .send({ + name: 'Orphan containerId heal', + tiles: [buildTile(sourceId, { name: 'Real tile' })], + tags: [], + containers: [ + { + id: 'real', + title: 'Real', + collapsed: false, + tabs: [{ id: 'errors', title: 'Errors' }], + }, + ], + }) + .expect(200); + const dashboardId = created.body.data.id; + + // Mutate Mongo directly to plant an orphan ref. `tiles` is + // `Mixed`, so the model layer doesn't enforce ref consistency; + // historical writes (or future bugs) can leave the doc in this + // shape. + await Dashboard.updateOne( + { _id: dashboardId }, + { + $set: { + 'tiles.0.containerId': 'ghost-container', + 'tiles.0.tabId': 'ghost-tab', + }, + }, + ); + + const getResp = await authRequest( + 'get', + `${BASE_URL}/${dashboardId}`, + ).expect(200); + const [returnedTile] = getResp.body.data.tiles; + expect(returnedTile.containerId).toBeUndefined(); + expect(returnedTile.tabId).toBeUndefined(); + }); + + // Critical P2-4 regression (tab-only orphan): a tile that points + // at a real container but a tab that no longer exists in that + // container must drop only `tabId`, keeping `containerId`. + // Without this, a stale tabId would fail the next PUT body schema + // even though the container itself is fine. + it('drops orphan tile.tabId on read when no tab matches', async () => { + const sourceId = traceSource._id.toString(); + const created = await authRequest('post', BASE_URL) + .send({ + name: 'Orphan tabId heal', + tiles: [ + buildTile(sourceId, { + name: 'Real tile', + containerId: 'real', + tabId: 'errors', + }), + ], + tags: [], + containers: [ + { + id: 'real', + title: 'Real', + collapsed: false, + tabs: [{ id: 'errors', title: 'Errors' }], + }, + ], + }) + .expect(200); + const dashboardId = created.body.data.id; + + // Replace the tile's tabId with one that doesn't exist in the + // container. + await Dashboard.updateOne( + { _id: dashboardId }, + { $set: { 'tiles.0.tabId': 'ghost-tab' } }, + ); + + const getResp = await authRequest( + 'get', + `${BASE_URL}/${dashboardId}`, + ).expect(200); + const [returnedTile] = getResp.body.data.tiles; + expect(returnedTile.containerId).toBe('real'); + expect(returnedTile.tabId).toBeUndefined(); + }); + }); + describe('DELETE /:id', () => { it('should delete a dashboard', async () => { const dashboard = await createTestDashboard(); diff --git a/packages/api/src/routers/external-api/v2/dashboards.ts b/packages/api/src/routers/external-api/v2/dashboards.ts index 22dcdd9e78..2d7f09df22 100644 --- a/packages/api/src/routers/external-api/v2/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/dashboards.ts @@ -5,13 +5,14 @@ import { z } from 'zod'; import { deleteDashboard } from '@/controllers/dashboard'; import { getSources } from '@/controllers/sources'; -import Dashboard from '@/models/dashboard'; +import Dashboard, { IDashboard } from '@/models/dashboard'; import { validateRequestWithEnhancedErrors as validateRequest } from '@/utils/enhancedErrors'; import logger from '@/utils/logger'; import { ExternalDashboardTileWithId, objectIdSchema } from '@/utils/zod'; import { cleanupDashboardAlerts, + collectTileContainerRefIssues, convertExternalFiltersToInternal, convertExternalTilesToInternal, convertToExternalDashboard, @@ -25,6 +26,23 @@ import { updateDashboardBodySchema, } from './utils/dashboards'; +/** + * Projection used by the GET-list and GET-by-id Dashboard endpoints, kept in + * one place so adding a new field doesn't need touching both call sites. + * Mirrors the shape consumed by `convertToExternalDashboard`. + */ +const EXTERNAL_DASHBOARD_PROJECTION = { + _id: 1, + name: 1, + tiles: 1, + tags: 1, + filters: 1, + savedQuery: 1, + savedQueryLanguage: 1, + savedFilterValues: 1, + containers: 1, +} as const; + async function getSourceConnectionMismatches( team: string | mongoose.Types.ObjectId, tiles: ExternalDashboardTileWithId[], @@ -922,6 +940,67 @@ async function getSourceConnectionMismatches( * search: '#/components/schemas/SearchChartConfig' * markdown: '#/components/schemas/MarkdownChartConfig' * + * DashboardContainerTab: + * type: object + * description: A single tab inside a dashboard container. Tiles join a tab via tabId. + * required: + * - id + * - title + * properties: + * id: + * type: string + * minLength: 1 + * maxLength: 256 + * description: Unique identifier for the tab within its container. + * example: "errors" + * title: + * type: string + * minLength: 1 + * maxLength: 256 + * description: Display title for the tab. + * example: "Errors" + * + * DashboardContainer: + * type: object + * description: A grouping container for tiles on a dashboard. Tiles join a container via containerId. + * required: + * - id + * - title + * - collapsed + * properties: + * id: + * type: string + * minLength: 1 + * maxLength: 256 + * description: Unique identifier for the container within the dashboard. + * example: "service-health" + * title: + * type: string + * minLength: 1 + * maxLength: 256 + * description: Display title for the container. + * example: "Service Health" + * collapsed: + * type: boolean + * description: Persisted default collapse state. Per-viewer state lives in the URL. + * example: false + * collapsible: + * type: boolean + * description: Whether the user can collapse the group. + * default: true + * example: true + * bordered: + * type: boolean + * description: Whether to show a visual border around the group. + * default: true + * example: true + * tabs: + * type: array + * description: Optional tabs. 2+ entries renders a tab bar; 0-1 entries renders a plain group header. Tiles join a tab via tabId. + * maxItems: 20 + * items: + * $ref: '#/components/schemas/DashboardContainerTab' + * * TileBase: * type: object * description: Common fields shared by tile input and output @@ -961,6 +1040,18 @@ async function getSourceConnectionMismatches( * config: * $ref: '#/components/schemas/TileConfig' * description: Chart configuration for the tile. The displayType field determines which variant is used. Replaces the deprecated "series" and "asRatio" fields. + * containerId: + * type: string + * minLength: 1 + * maxLength: 256 + * description: References a DashboardContainer by id. Tiles without containerId render in the default ungrouped area. + * example: "service-health" + * tabId: + * type: string + * minLength: 1 + * maxLength: 256 + * description: References a tab inside the tile's container by id. Requires containerId to be set, and the container to declare a matching tab. + * example: "errors" * * TileOutput: * description: Response format for dashboard tiles @@ -1107,6 +1198,12 @@ async function getSourceConnectionMismatches( * description: Optional default dashboard filter values restored when loading the dashboard. * items: * $ref: '#/components/schemas/SavedFilterValue' + * containers: + * type: array + * description: Optional grouping containers. Each tile may join a container via tile.containerId, and a tab inside it via tile.tabId. + * maxItems: 50 + * items: + * $ref: '#/components/schemas/DashboardContainer' * * CreateDashboardRequest: * type: object @@ -1122,6 +1219,7 @@ async function getSourceConnectionMismatches( * tiles: * type: array * description: List of tiles/charts to include in the dashboard. + * maxItems: 500 * items: * $ref: '#/components/schemas/TileInput' * tags: @@ -1153,6 +1251,12 @@ async function getSourceConnectionMismatches( * description: Optional default dashboard filter values to persist on the dashboard. * items: * $ref: '#/components/schemas/SavedFilterValue' + * containers: + * type: array + * description: Optional grouping containers. Each tile may join a container via tile.containerId, and a tab inside it via tile.tabId. + * maxItems: 50 + * items: + * $ref: '#/components/schemas/DashboardContainer' * * UpdateDashboardRequest: * type: object @@ -1167,6 +1271,7 @@ async function getSourceConnectionMismatches( * example: "Updated Dashboard Name" * tiles: * type: array + * maxItems: 500 * items: * $ref: '#/components/schemas/TileInput' * description: Full list of tiles for the dashboard. Existing tiles are matched by ID; tiles with an ID that does not match an existing tile will be assigned a new generated ID. @@ -1199,6 +1304,12 @@ async function getSourceConnectionMismatches( * description: Optional default dashboard filter values to persist on the dashboard. * items: * $ref: '#/components/schemas/SavedFilterValue' + * containers: + * type: array + * description: Optional grouping containers. Each tile may join a container via tile.containerId, and a tab inside it via tile.tabId. + * maxItems: 50 + * items: + * $ref: '#/components/schemas/DashboardContainer' * * DashboardResponse: * allOf: @@ -1302,16 +1413,7 @@ router.get('/', async (req, res, next) => { const dashboards = await Dashboard.find( { team: teamId }, - { - _id: 1, - name: 1, - tiles: 1, - tags: 1, - filters: 1, - savedQuery: 1, - savedQueryLanguage: 1, - savedFilterValues: 1, - }, + EXTERNAL_DASHBOARD_PROJECTION, ).sort({ name: -1 }); res.json({ @@ -1419,16 +1521,7 @@ router.get( const dashboard = await Dashboard.findOne( { team: teamId, _id: req.params.id }, - { - _id: 1, - name: 1, - tiles: 1, - tags: 1, - filters: 1, - savedQuery: 1, - savedQueryLanguage: 1, - savedFilterValues: 1, - }, + EXTERNAL_DASHBOARD_PROJECTION, ); if (dashboard == null) { @@ -1595,8 +1688,23 @@ router.post( savedQuery, savedQueryLanguage, savedFilterValues, + containers, } = req.body; + // Tile-level container/tab ref resolution: container structure + // (duplicate ids, tab uniqueness) was checked by the body schema; + // tile-resolution runs here against the request body's containers + // (POST has no existing dashboard to fall back to). + const tileRefIssues = collectTileContainerRefIssues( + containers ?? [], + tiles, + ); + if (tileRefIssues.length > 0) { + return res.status(400).json({ + message: tileRefIssues.join('; '), + }); + } + const [missingSources, missingConnections, sourceConnectionMismatches] = await Promise.all([ getMissingSources(teamId, tiles, filters), @@ -1642,6 +1750,7 @@ router.post( savedQueryLanguage: normalizedSavedQueryLanguage, savedFilterValues, team: teamId, + ...(containers !== undefined ? { containers } : {}), }).save(); res.json({ @@ -1658,7 +1767,14 @@ router.post( * /api/v2/dashboards/{id}: * put: * summary: Update Dashboard - * description: Updates an existing dashboard + * description: | + * Updates an existing dashboard. + * + * **Concurrency:** This endpoint does not support optimistic + * concurrency control. Concurrent PUT requests for the same + * dashboard may silently overwrite each other, which can leave + * orphan tile-to-container references on layout-shape edits. + * Clients should serialize edits to a given dashboard. * operationId: updateDashboard * tags: [Dashboards] * parameters: @@ -1818,6 +1934,7 @@ router.put( savedQuery, savedQueryLanguage, savedFilterValues, + containers, } = req.body ?? {}; const [missingSources, missingConnections, sourceConnectionMismatches] = @@ -1848,9 +1965,17 @@ router.put( }); } + // PUT performs a read-modify-write on the dashboard doc with no + // optimistic-concurrency check (no `If-Match` / `updatedAt` + // guard). Concurrent PUTs may silently overwrite each other, + // which can leave orphan tile->container refs on layout-shape + // edits. The endpoint contract is at-most-one-writer; the + // OpenAPI description above documents this. Adding ETag-style + // concurrency would be a breaking change to the request shape + // and is tracked separately. const existingDashboard = await Dashboard.findOne( { _id: dashboardId, team: teamId }, - { tiles: 1, filters: 1 }, + { tiles: 1, filters: 1, containers: 1 }, ).lean(); const existingTileIds = new Set( (existingDashboard?.tiles ?? []).map((t: { id: string }) => t.id), @@ -1859,12 +1984,35 @@ router.put( (existingDashboard?.filters ?? []).map((f: { id: string }) => f.id), ); + // Tile-level container/tab ref resolution: container structure + // was checked by the body schema; tile-resolution runs here + // against the effective container set so an omitted `containers` + // body field (which preserves the existing array on write) is + // resolved against the doc's existing containers rather than an + // empty fallback. Without this, a PUT that updates only `tiles` + // and leaves a tile pointing at a real preserved container + // would be rejected with "Tile references unknown containerId". + const effectiveContainers = + containers ?? existingDashboard?.containers ?? []; + const tileRefIssues = collectTileContainerRefIssues( + effectiveContainers, + tiles, + ); + if (tileRefIssues.length > 0) { + return res.status(400).json({ + message: tileRefIssues.join('; '), + }); + } + const internalTiles = convertExternalTilesToInternal( tiles, existingTileIds, ); - const setPayload: Record = { + // Typed as `Partial` (the canonical Mongo doc shape) so + // that misnamed or wrong-shape fields fail at compile time. The + // legacy `Record` accepted anything. + const setPayload: Partial = { name, tiles: internalTiles, tags: tags && uniq(tags), @@ -1888,6 +2036,9 @@ router.put( if (savedFilterValues !== undefined) { setPayload.savedFilterValues = savedFilterValues; } + if (containers !== undefined) { + setPayload.containers = containers; + } const updatedDashboard = await Dashboard.findOneAndUpdate( { _id: dashboardId, team: teamId }, diff --git a/packages/api/src/routers/external-api/v2/utils/__tests__/dashboards.test.ts b/packages/api/src/routers/external-api/v2/utils/__tests__/dashboards.test.ts index 46e40ee481..3a67c0f7a2 100644 --- a/packages/api/src/routers/external-api/v2/utils/__tests__/dashboards.test.ts +++ b/packages/api/src/routers/external-api/v2/utils/__tests__/dashboards.test.ts @@ -1,7 +1,20 @@ +import { + validateDashboardContainersStructure, + validateDashboardTileContainerRefs, +} from '@hyperdx/common-utils/dist/dashboardValidation'; import { isBuilderSavedChartConfig } from '@hyperdx/common-utils/dist/guards'; import { DisplayType } from '@hyperdx/common-utils/dist/types'; +import mongoose from 'mongoose'; +import { z } from 'zod'; -import { ConfigTile, convertToInternalTileConfig } from '../dashboards'; +import { DashboardDocument } from '@/models/dashboard'; + +import { + collectTileContainerRefIssues, + ConfigTile, + convertToExternalDashboard, + convertToInternalTileConfig, +} from '../dashboards'; function makeMarkdownTile( markdown?: string, @@ -57,9 +70,12 @@ describe('convertToInternalTileConfig', () => { expect(result.y).toBe(8); expect(result.w).toBe(24); expect(result.h).toBe(3); - // name is picked by convertToInternalTileConfig but not part of the - // Tile type — verify it round-trips via the runtime object. - expect(result).toHaveProperty('name', 'How to Use'); + // The internal Tile shape carries name on config, not at the top + // level. The previous `pick(...)` swept name onto the top-level + // object (a stale extra field that the renderer never reads); the + // destructure refactor fixes that. + expect(result.config.name).toBe('How to Use'); + expect(result).not.toHaveProperty('name'); }); it('does not set a real sourceId on the internal config', () => { @@ -75,3 +91,293 @@ describe('convertToInternalTileConfig', () => { }); }); }); + +describe('dashboard container validation helpers', () => { + // Drives the two helpers in sequence (mirroring production usage: + // schema-level structure check, then handler-level tile-ref check) + // through z.superRefine and inspects the resulting ZodError so we + // can assert path + message without standing up the full express + + // DB stack. + function runHelper( + containers: Parameters[0], + tiles: T[], + ): z.ZodIssue[] { + const schema = z + .object({ + containers: z.array(z.unknown()).optional() as unknown as z.ZodTypeAny, + tiles: z.array(z.unknown()) as unknown as z.ZodTypeAny, + }) + .superRefine((data, ctx) => { + const { containerById, hasDuplicateContainerId } = + validateDashboardContainersStructure( + (data.containers ?? []) as typeof containers, + ctx, + ); + // Skip tile-ref resolution when container ids weren't unique: + // the duplicate-id error is enough to flag, and tile-level + // errors against a last-write-wins map would just stack noise. + if (hasDuplicateContainerId) return; + validateDashboardTileContainerRefs( + containerById, + (data.tiles ?? []) as T[], + ctx, + ); + }); + const result = schema.safeParse({ containers, tiles }); + return result.success ? [] : result.error.issues; + } + + it('raises no issues for empty inputs', () => { + expect(runHelper([], [])).toEqual([]); + }); + + it('raises no issues for valid containers and tile refs', () => { + expect( + runHelper( + [ + { + id: 'a', + title: 'A', + collapsed: false, + tabs: [{ id: 't1', title: 'T1' }], + }, + ], + [{ containerId: 'a', tabId: 't1' }], + ), + ).toEqual([]); + }); + + it('flags duplicate container ids and stops tile resolution', () => { + const issues = runHelper( + [ + { id: 'a', title: 'A', collapsed: false }, + { id: 'a', title: 'B', collapsed: false }, + ], + // Tile would otherwise raise an unknown-containerId error too; + // verifying the helper short-circuits keeps that error from + // stacking on top of the duplicate-id error. + [{ containerId: 'ghost' }], + ); + expect(issues).toHaveLength(1); + expect(issues[0].path).toEqual(['containers', 1, 'id']); + expect(issues[0].message).toContain('Container IDs must be unique'); + }); + + it('flags duplicate tab ids inside a container', () => { + const issues = runHelper( + [ + { + id: 'a', + title: 'A', + collapsed: false, + tabs: [ + { id: 't1', title: 'T1' }, + { id: 't1', title: 'T1 again' }, + ], + }, + ], + [], + ); + expect(issues).toHaveLength(1); + expect(issues[0].path).toEqual(['containers', 0, 'tabs', 1, 'id']); + expect(issues[0].message).toContain('Duplicate tab id'); + }); + + it('flags an unknown containerId on a tile', () => { + const issues = runHelper( + [{ id: 'a', title: 'A', collapsed: false }], + [{ containerId: 'b' }], + ); + expect(issues).toHaveLength(1); + expect(issues[0].path).toEqual(['tiles', 0, 'containerId']); + expect(issues[0].message).toContain('unknown containerId'); + }); + + it('flags a tabId without containerId', () => { + const issues = runHelper([], [{ tabId: 't1' }]); + expect(issues).toHaveLength(1); + expect(issues[0].path).toEqual(['tiles', 0, 'tabId']); + expect(issues[0].message).toContain('tabId requires containerId'); + }); + + it('flags an unknown tabId on a tile', () => { + const issues = runHelper( + [ + { + id: 'a', + title: 'A', + collapsed: false, + tabs: [{ id: 't1', title: 'T1' }], + }, + ], + [{ containerId: 'a', tabId: 'ghost' }], + ); + expect(issues).toHaveLength(1); + expect(issues[0].path).toEqual(['tiles', 0, 'tabId']); + expect(issues[0].message).toContain('unknown tabId'); + }); +}); + +describe('collectTileContainerRefIssues', () => { + // The handler-level wrapper around the canonical helper. Tests + // exercise the wrapper rather than the helper itself because the + // wrapper formats Zod issues into `path: message` strings, which is + // what the PUT/POST handlers send back to clients. + it('returns no issues when refs resolve', () => { + expect( + collectTileContainerRefIssues( + [{ id: 'real', title: 'Real', collapsed: false }], + [ + { + id: 't', + x: 0, + y: 0, + w: 1, + h: 1, + name: '', + containerId: 'real', + }, + ], + ), + ).toEqual([]); + }); + + it('returns a formatted message for an unknown containerId', () => { + const issues = collectTileContainerRefIssues( + [], + [ + { + id: 't', + x: 0, + y: 0, + w: 1, + h: 1, + name: '', + containerId: 'ghost', + }, + ], + ); + expect(issues).toEqual([ + 'tiles.0.containerId: Tile references unknown containerId "ghost"', + ]); + }); + + it('returns a formatted message for tabId without containerId', () => { + const issues = collectTileContainerRefIssues( + [], + [ + { + id: 't', + x: 0, + y: 0, + w: 1, + h: 1, + name: '', + tabId: 'ghost', + }, + ], + ); + expect(issues).toEqual([ + 'tiles.0.tabId: tabId requires containerId to be set', + ]); + }); +}); + +describe('convertToExternalDashboard orphan-ref heal', () => { + // `tiles` is `Mixed` in Mongoose, so the model layer can't enforce + // ref consistency. These tests cover the read-path heal: a doc that + // somehow ends up with a tile pointing at a missing container or + // tab must round-trip on read as if the ref were absent. Without + // this, the next PUT body schema rejects the round-tripped tile + // and the dashboard becomes uneditable from the external API. + function makeDoc( + overrides: Partial = {}, + ): DashboardDocument { + return { + _id: new mongoose.Types.ObjectId(), + name: 'Test', + tiles: [], + tags: [], + filters: [], + savedQuery: null, + savedQueryLanguage: null, + savedFilterValues: [], + ...overrides, + } as unknown as DashboardDocument; + } + + function makeTile( + overrides: Partial = {}, + ): DashboardDocument['tiles'][number] { + return { + id: 't1', + x: 0, + y: 0, + w: 1, + h: 1, + config: { + displayType: DisplayType.Markdown, + markdown: '', + source: '', + where: '', + select: [], + name: '', + }, + ...overrides, + }; + } + + it('drops containerId on read when no container matches', () => { + const doc = makeDoc({ + tiles: [makeTile({ containerId: 'ghost', tabId: 'whatever' })], + containers: [{ id: 'real', title: 'Real', collapsed: false }], + }); + const ext = convertToExternalDashboard(doc); + expect(ext.tiles[0].containerId).toBeUndefined(); + expect(ext.tiles[0].tabId).toBeUndefined(); + }); + + it('drops tabId on read when no tab matches but the container resolves', () => { + const doc = makeDoc({ + tiles: [makeTile({ containerId: 'real', tabId: 'ghost-tab' })], + containers: [ + { + id: 'real', + title: 'Real', + collapsed: false, + tabs: [{ id: 'errors', title: 'Errors' }], + }, + ], + }); + const ext = convertToExternalDashboard(doc); + expect(ext.tiles[0].containerId).toBe('real'); + expect(ext.tiles[0].tabId).toBeUndefined(); + }); + + it('drops tabId on read when tabId is set without containerId', () => { + const doc = makeDoc({ + tiles: [makeTile({ tabId: 'ghost-tab' })], + containers: [{ id: 'real', title: 'Real', collapsed: false }], + }); + const ext = convertToExternalDashboard(doc); + expect(ext.tiles[0].containerId).toBeUndefined(); + expect(ext.tiles[0].tabId).toBeUndefined(); + }); + + it('keeps both containerId and tabId on read when both resolve', () => { + const doc = makeDoc({ + tiles: [makeTile({ containerId: 'real', tabId: 'errors' })], + containers: [ + { + id: 'real', + title: 'Real', + collapsed: false, + tabs: [{ id: 'errors', title: 'Errors' }], + }, + ], + }); + const ext = convertToExternalDashboard(doc); + expect(ext.tiles[0].containerId).toBe('real'); + expect(ext.tiles[0].tabId).toBe('errors'); + }); +}); diff --git a/packages/api/src/routers/external-api/v2/utils/dashboards.ts b/packages/api/src/routers/external-api/v2/utils/dashboards.ts index 0212f9b9d1..15c1e3c4af 100644 --- a/packages/api/src/routers/external-api/v2/utils/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/utils/dashboards.ts @@ -1,8 +1,15 @@ import { displayTypeSupportsRawSqlAlerts } from '@hyperdx/common-utils/dist/core/utils'; +import { + validateDashboardContainersStructure, + validateDashboardTileContainerRefs, +} from '@hyperdx/common-utils/dist/dashboardValidation'; import { isRawSqlSavedChartConfig } from '@hyperdx/common-utils/dist/guards'; import { AggregateFunctionSchema, BuilderSavedChartConfig, + DASHBOARD_MAX_CONTAINERS, + DashboardContainer, + DashboardContainerSchema, DisplayType, RawSqlSavedChartConfig, SavedChartConfig, @@ -77,6 +84,7 @@ export type ExternalDashboard = { savedQuery?: string | null; savedQueryLanguage?: string | null; savedFilterValues?: DashboardDocument['savedFilterValues']; + containers?: DashboardContainer[]; }; // -------------------------------------------------------------------------------- @@ -281,6 +289,8 @@ const convertToExternalTileChartConfig = ( function convertTileToExternalChart( tile: DashboardDocument['tiles'][number], + containerById: Map, + dashboardId: string, ): ExternalDashboardTileWithId | undefined { // Returned in case of a failure converting the saved chart config const defaultTileConfig: ExternalDashboardTileConfig = @@ -297,27 +307,97 @@ function convertTileToExternalChart( select: [DEFAULT_SELECT_ITEM], }; + // Treat empty-string container/tab refs as absent so legacy Mongo docs + // (the underlying `tiles` field is `Mixed`, so older entries may carry + // `containerId: ""`) round-trip through the external schema, which now + // enforces `min(1)`. Without this, a GET that hit a legacy doc would + // return a tile that the next PUT couldn't validate. + let containerId = + typeof tile.containerId === 'string' && tile.containerId.length > 0 + ? tile.containerId + : undefined; + let tabId = + typeof tile.tabId === 'string' && tile.tabId.length > 0 + ? tile.tabId + : undefined; + + // Self-heal orphan refs on read. A doc may carry a containerId that + // points at a container that has since been removed (or never + // existed: legacy docs predating the containers feature can have any + // value in this `Mixed`-typed field). Round-trip these as if absent + // so a subsequent PUT validates instead of failing schema with + // "Tile references unknown containerId". Same idea for tabId. + if (containerId !== undefined) { + const container = containerById.get(containerId); + if (!container) { + logger.warn( + { dashboardId, tileId: tile.id, containerId }, + 'Tile references unknown containerId; dropping ref on read', + ); + containerId = undefined; + tabId = undefined; + } else if ( + tabId !== undefined && + !container.tabs?.some(t => t.id === tabId) + ) { + logger.warn( + { dashboardId, tileId: tile.id, containerId, tabId }, + 'Tile references unknown tabId; dropping tabId on read', + ); + tabId = undefined; + } + } else if (tabId !== undefined) { + // tabId without containerId is invalid in the schema; the legacy + // doc would fail a subsequent PUT, so drop it on read. + logger.warn( + { dashboardId, tileId: tile.id, tabId }, + 'Tile has tabId without containerId; dropping tabId on read', + ); + tabId = undefined; + } + + const { id, x, y, w, h } = tile; return { - ...pick(tile, ['id', 'x', 'y', 'w', 'h']), + id, + x, + y, + w, + h, name: tile.config.name ?? '', config: convertToExternalTileChartConfig(tile.config) ?? defaultTileConfig, + ...(containerId !== undefined ? { containerId } : {}), + ...(tabId !== undefined ? { tabId } : {}), }; } export function convertToExternalDashboard( dashboard: DashboardDocument, ): ExternalDashboard { + const containers = dashboard.containers ?? []; + // Dedupe by id when building the lookup map: a doc with duplicate + // container ids can only resolve tile refs against one of them, and + // last-write-wins is consistent with how Mongo would have persisted + // the array. Tile-resolution ambiguity in this case is logged when + // the tile ref turns out to point at a missing container. + const containerById = new Map( + containers.map(c => [c.id, c]), + ); + const dashboardId = dashboard._id.toString(); return { - id: dashboard._id.toString(), + id: dashboardId, name: dashboard.name, tiles: dashboard.tiles - .map(convertTileToExternalChart) + .map(tile => convertTileToExternalChart(tile, containerById, dashboardId)) .filter(t => t !== undefined), tags: dashboard.tags || [], filters: dashboard.filters?.map(translateFilterToExternalFilter) || [], savedQuery: dashboard.savedQuery ?? null, savedQueryLanguage: dashboard.savedQueryLanguage ?? null, savedFilterValues: dashboard.savedFilterValues ?? [], + // Mongoose persists missing arrays as []. Only emit containers when + // the user actually saved one or more, so dashboards without the + // organization layer round-trip with the field absent. + ...(containers.length > 0 ? { containers } : {}), }; } @@ -488,8 +568,23 @@ export function convertToInternalTileConfig( const strippedConfig = _.omitBy(internalConfig, _.isNil) as SavedChartConfig; + // Mirror the spread-conditional pattern used in `convertTileToExternalChart`: + // destructure statically (compile-time narrowing) and include the optional + // refs only when set, so a tile without a containerId never persists + // `containerId: undefined` to Mongo. The previous `pick(...)` over the + // external tile included `name`, but the internal `Tile` type stores the + // name on `config`, not at the top level (`strippedConfig` carries it). + // Stripping the top-level `name` brings the runtime shape back in line + // with `DashboardDocument['tiles'][number]`. + const { id, x, y, w, h, containerId, tabId } = externalTile; return { - ...pick(externalTile, ['id', 'x', 'y', 'w', 'h', 'name']), + id, + x, + y, + w, + h, + ...(containerId !== undefined ? { containerId } : {}), + ...(tabId !== undefined ? { tabId } : {}), config: strippedConfig, }; } @@ -583,6 +678,13 @@ const dashboardBodyBaseShape = { savedFilterValues: z .array(externalDashboardSavedFilterValueSchema) .optional(), + // The internal `DashboardContainerSchema` already caps individual + // container/tab/title sizes; the array cap mirrors what the editor + // would ever generate. + containers: z + .array(DashboardContainerSchema) + .max(DASHBOARD_MAX_CONTAINERS) + .optional(), }; // -------------------------------------------------------------------------------- @@ -701,9 +803,45 @@ function buildDashboardBodySchema(filterSchema: z.ZodTypeAny): z.ZodEffects< path: ['savedQueryLanguage'], }); } + + // Schema-level: only structural checks on containers (duplicate + // ids, per-container tab-id uniqueness). Cross-tile resolution + // moved to the request handler so a PUT can fall back to the + // existing dashboard's containers when the body omits the field + // (otherwise a tile that references a real preserved container + // would be rejected against an empty `data.containers ?? []`). + validateDashboardContainersStructure(data.containers ?? [], ctx); }); } +/** + * Cross-tile container/tab reference resolution against an effective + * container set. Used by the POST and PUT handlers in + * `routers/external-api/v2/dashboards.ts`: POST validates against the + * request body's containers, PUT validates against the request body's + * containers when present, falling back to the existing dashboard's + * containers when the body omits the field. Returns a list of + * `path: message` strings shaped to mirror the body-schema validation + * error format used by `validateRequestWithEnhancedErrors`. + */ +export function collectTileContainerRefIssues( + containers: DashboardContainer[], + tiles: ExternalDashboardTileWithId[], +): string[] { + const schema = z.object({}).superRefine((_, ctx) => { + const containerById = new Map( + containers.map(c => [c.id, c]), + ); + validateDashboardTileContainerRefs(containerById, tiles, ctx); + }); + const result = schema.safeParse({}); + if (result.success) return []; + return result.error.issues.map(issue => { + const path = issue.path.length > 0 ? `${issue.path.join('.')}: ` : ''; + return `${path}${issue.message}`; + }); +} + export const createDashboardBodySchema = buildDashboardBodySchema( externalDashboardFilterSchema, ); diff --git a/packages/api/src/utils/externalApi.ts b/packages/api/src/utils/externalApi.ts index fc456b68fb..d01f273a1f 100644 --- a/packages/api/src/utils/externalApi.ts +++ b/packages/api/src/utils/externalApi.ts @@ -34,7 +34,7 @@ const pickIfTruthy = (obj: T, keys: K[]): Partial => { export function translateExternalChartToTileConfig( chart: SeriesTile, ): DashboardDocument['tiles'][number] { - const { id, name, x, y, w, h, series, asRatio } = chart; + const { id, name, x, y, w, h, series, asRatio, containerId, tabId } = chart; if (series.length === 0) { throw new Error('Chart must have at least one series'); @@ -203,6 +203,8 @@ export function translateExternalChartToTileConfig( w, h, config, + ...(containerId !== undefined ? { containerId } : {}), + ...(tabId !== undefined ? { tabId } : {}), }; } diff --git a/packages/api/src/utils/zod.ts b/packages/api/src/utils/zod.ts index 84181e5844..3ee8a7c019 100644 --- a/packages/api/src/utils/zod.ts +++ b/packages/api/src/utils/zod.ts @@ -2,6 +2,8 @@ import { addDuplicateTileIdIssues, AggregateFunctionSchema, AlertThresholdType, + DASHBOARD_CONTAINER_ID_MAX, + DASHBOARD_MAX_TILES, DashboardFilterSchema, MetricsDataType, NumberFormatSchema, @@ -420,6 +422,11 @@ export const externalDashboardTileSchema = z }) .optional(), config: externalDashboardTileConfigSchema.optional(), + // Bounds match the internal `DashboardContainerSchema` cap (see + // `DASHBOARD_CONTAINER_ID_MAX` in common-utils/src/types.ts) so a + // valid container id from the editor always fits. + containerId: z.string().min(1).max(DASHBOARD_CONTAINER_ID_MAX).optional(), + tabId: z.string().min(1).max(DASHBOARD_CONTAINER_ID_MAX).optional(), }) .superRefine((data, ctx) => { if (data.series && data.config) { @@ -471,6 +478,11 @@ export type ExternalDashboardTileWithId = z.infer< export const externalDashboardTileListSchema = z .array(externalDashboardTileSchemaWithOptionalId) + // Cap the per-dashboard tile fan-out so an external-API caller can't push + // a payload tens of MB into Mongo in one request. The 500 limit sits well + // above any real dashboard; the dashboard editor's add-tile affordance + // is one-at-a-time. + .max(DASHBOARD_MAX_TILES) .superRefine((tiles, ctx) => addDuplicateTileIdIssues(tiles, ctx, { messageSuffix: '. Omit the ID to generate a unique one.', diff --git a/packages/common-utils/src/dashboardValidation.ts b/packages/common-utils/src/dashboardValidation.ts new file mode 100644 index 0000000000..46dd4a5447 --- /dev/null +++ b/packages/common-utils/src/dashboardValidation.ts @@ -0,0 +1,129 @@ +import { z } from 'zod'; + +import { DashboardContainerSchema } from './types'; + +// Inputs shared by the internal `DashboardSchema` refinement and the +// external API body schema: the validation only depends on the +// containerId/tabId reference shape, not on Builder-vs-RawSql config. +type ContainerForValidation = z.infer; +type TileForValidation = { containerId?: string; tabId?: string }; + +/** + * Pass 1: container-id uniqueness and per-container tab-id uniqueness. + * + * Returns the container-by-id map and a flag indicating whether any + * duplicate container ids were seen, so callers that intend to do + * tile-ref resolution next can short-circuit on the duplicate-id case + * (resolving against a last-write-wins map would mask the duplicate + * with cascading "unknown containerId" errors). + * + * Issues raised: + * - Duplicate container ids (path `[i].id`). + * - Duplicate tab ids within a container (path + * `[i].tabs[j].id`). + */ +export function validateDashboardContainersStructure( + containers: ContainerForValidation[], + ctx: z.RefinementCtx, + paths?: { containersPath?: (string | number)[] }, +): { + containerById: Map; + hasDuplicateContainerId: boolean; +} { + const containersPath = paths?.containersPath ?? ['containers']; + + // The container-by-id map is built INSIDE this pass and is only used + // by the tile-resolution helper below; building a Map up-front would + // last-write-win on duplicate ids, masking the duplicate before this + // loop reports it. + const containerById = new Map(); + const seenContainerIds = new Set(); + let hasDuplicateContainerId = false; + containers.forEach((container, containerIdx) => { + if (seenContainerIds.has(container.id)) { + hasDuplicateContainerId = true; + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Container IDs must be unique: "${container.id}"`, + path: [...containersPath, containerIdx, 'id'], + }); + } else { + seenContainerIds.add(container.id); + containerById.set(container.id, container); + } + + if (container.tabs) { + const seenTabIds = new Set(); + container.tabs.forEach((tab, tabIdx) => { + if (seenTabIds.has(tab.id)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Duplicate tab id "${tab.id}" in container "${container.id}"`, + path: [...containersPath, containerIdx, 'tabs', tabIdx, 'id'], + }); + } + seenTabIds.add(tab.id); + }); + } + }); + + return { containerById, hasDuplicateContainerId }; +} + +/** + * Pass 2: each tile's containerId resolves to a real container, and + * each tile's tabId resolves to a tab in that container. Caller is + * responsible for skipping this pass when container ids aren't unique + * (the structure helper returns `hasDuplicateContainerId` for that + * reason). + * + * Issues raised: + * - A tile's `containerId` references an unknown container (path + * `[k].containerId`). + * - A tile's `tabId` is set without `containerId` (path + * `[k].tabId`). + * - A tile's `tabId` references an unknown tab (path + * `[k].tabId`). + */ +export function validateDashboardTileContainerRefs( + containerById: Map, + tiles: T[], + ctx: z.RefinementCtx, + paths?: { tilesPath?: (string | number)[] }, +): void { + const tilesPath = paths?.tilesPath ?? ['tiles']; + + tiles.forEach((tile, tileIdx) => { + if (tile.containerId !== undefined) { + const container = containerById.get(tile.containerId); + if (!container) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Tile references unknown containerId "${tile.containerId}"`, + path: [...tilesPath, tileIdx, 'containerId'], + }); + } + } + + if (tile.tabId !== undefined) { + if (tile.containerId === undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'tabId requires containerId to be set', + path: [...tilesPath, tileIdx, 'tabId'], + }); + return; + } + const container = containerById.get(tile.containerId); + if (!container) return; + const tab = container.tabs?.find(t => t.id === tile.tabId); + if (!tab) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Tile references unknown tabId "${tile.tabId}" in container "${tile.containerId}"`, + path: [...tilesPath, tileIdx, 'tabId'], + }); + } + } + }); +} diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index aeb9fde36c..230bfe42fb 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -905,9 +905,11 @@ export const TileSchema = z.object({ w: z.number(), h: z.number(), config: SavedChartConfigSchema, - containerId: z.string().optional(), + // `min(1)` matches the external API; an empty string isn't a valid + // id and would silently pass `tile.containerId !== undefined` checks. + containerId: z.string().min(1).optional(), // For tiles inside a tab container: which tab this tile belongs to - tabId: z.string().optional(), + tabId: z.string().min(1).optional(), }); export const TileTemplateSchema = TileSchema.extend({ @@ -919,14 +921,33 @@ export const TileTemplateSchema = TileSchema.extend({ export type Tile = z.infer; +// Reasonable bounds on identifiers and titles. The UI never asks the +// user to type either an id or a title longer than ~64 chars; capping +// at 256 leaves room for slugified or composed ids without inviting +// Mongo-doc bloat. Exported so the external-API tile schema can apply +// the same cap to tile.containerId / tile.tabId. +export const DASHBOARD_CONTAINER_ID_MAX = 256; +const DASHBOARD_CONTAINER_TITLE_MAX = 256; +// Caps the per-container tab fan-out. The tab bar visually breaks +// past ~10 tabs and the editor offers no bulk-add affordance. Used +// only by this schema; not exported. +const DASHBOARD_CONTAINER_MAX_TABS = 20; +// Caps the per-dashboard container fan-out. +export const DASHBOARD_MAX_CONTAINERS = 50; +// Caps the per-dashboard tile fan-out. The dashboard editor's add-tile +// affordance is one-at-a-time, but external-API callers can POST a list +// in one request; without a cap a payload could push tens of MB into +// Mongo and run out of memory rendering it. +export const DASHBOARD_MAX_TILES = 500; + export const DashboardContainerTabSchema = z.object({ - id: z.string().min(1), - title: z.string().min(1), + id: z.string().min(1).max(DASHBOARD_CONTAINER_ID_MAX), + title: z.string().min(1).max(DASHBOARD_CONTAINER_TITLE_MAX), }); export const DashboardContainerSchema = z.object({ - id: z.string().min(1), - title: z.string().min(1), + id: z.string().min(1).max(DASHBOARD_CONTAINER_ID_MAX), + title: z.string().min(1).max(DASHBOARD_CONTAINER_TITLE_MAX), collapsed: z.boolean(), // Whether the group can be collapsed (default true) collapsible: z.boolean().optional(), @@ -934,7 +955,10 @@ export const DashboardContainerSchema = z.object({ bordered: z.boolean().optional(), // Optional tabs: 2+ entries → tab bar renders, 0-1 → plain group header. // Tiles reference a specific tab via tabId. - tabs: z.array(DashboardContainerTabSchema).optional(), + tabs: z + .array(DashboardContainerTabSchema) + .max(DASHBOARD_CONTAINER_MAX_TABS) + .optional(), }); export type DashboardContainer = z.infer; @@ -985,6 +1009,15 @@ export function addDuplicateTileIdIssues( } } +// `DashboardSchema` is intentionally left as a `ZodObject` (no parent-level +// `.superRefine`) so existing call sites that chain `.omit()`, `.partial()`, +// or `.extend()` keep working (see `routers/api/dashboards.ts` PATCH body +// and `DashboardWithoutIdSchema` / `DashboardTemplateSchema` below). +// Cross-tile container/tab reference validation lives in +// `./dashboardValidation` and is applied at the external-API request body +// schema (`buildDashboardBodySchema` in `v2/utils/dashboards.ts`), which is +// the only public surface that accepts arbitrary tile + container payloads +// in one call. export const DashboardSchema = z.object({ id: z.string(), name: z.string().min(1), @@ -996,13 +1029,20 @@ export const DashboardSchema = z.object({ savedFilterValues: z.array(FilterSchema).optional(), containers: z .array(DashboardContainerSchema) - .refine( - containers => { - const ids = containers.map(c => c.id); - return new Set(ids).size === ids.length; - }, - { message: 'Container IDs must be unique' }, - ) + .max(DASHBOARD_MAX_CONTAINERS) + .superRefine((containers, ctx) => { + const seen = new Set(); + containers.forEach((c, i) => { + if (seen.has(c.id)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Container IDs must be unique: "${c.id}"`, + path: [i, 'id'], + }); + } + seen.add(c.id); + }); + }) .optional(), }); export const DashboardWithoutIdSchema = DashboardSchema.omit({ id: true });