mirror of
https://github.com/hyperdxio/hyperdx
synced 2026-04-21 13:37:15 +00:00
fix: Prevent duplicate tile IDs in dashboard imports
This commit is contained in:
parent
f086842f3c
commit
94c616eddc
7 changed files with 198 additions and 26 deletions
7
.changeset/khaki-shoes-cough.md
Normal file
7
.changeset/khaki-shoes-cough.md
Normal file
|
|
@ -0,0 +1,7 @@
|
|||
---
|
||||
"@hyperdx/common-utils": patch
|
||||
"@hyperdx/api": patch
|
||||
"@hyperdx/app": patch
|
||||
---
|
||||
|
||||
fix: Prevent duplicate tile IDs in dashboard imports
|
||||
|
|
@ -1,4 +1,5 @@
|
|||
import {
|
||||
addDuplicateTileIdIssues,
|
||||
AggregateFunctionSchema,
|
||||
AlertThresholdType,
|
||||
DashboardFilterSchema,
|
||||
|
|
@ -469,20 +470,11 @@ export type ExternalDashboardTileWithId = z.infer<
|
|||
|
||||
export const externalDashboardTileListSchema = z
|
||||
.array(externalDashboardTileSchemaWithOptionalId)
|
||||
.superRefine((tiles, ctx) => {
|
||||
const seen = new Set<string>();
|
||||
for (const tile of tiles) {
|
||||
if (tile.id && seen.has(tile.id)) {
|
||||
ctx.addIssue({
|
||||
code: z.ZodIssueCode.custom,
|
||||
message: `Duplicate tile ID: ${tile.id}. Omit the ID to generate a unique one.`,
|
||||
});
|
||||
}
|
||||
if (tile.id) {
|
||||
seen.add(tile.id);
|
||||
}
|
||||
}
|
||||
});
|
||||
.superRefine((tiles, ctx) =>
|
||||
addDuplicateTileIdIssues(tiles, ctx, {
|
||||
messageSuffix: '. Omit the ID to generate a unique one.',
|
||||
}),
|
||||
);
|
||||
|
||||
// ==============================
|
||||
// Alerts
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { useEffect, useMemo, useRef, useState } from 'react';
|
||||
import { type ReactNode, useEffect, useMemo, useRef, useState } from 'react';
|
||||
import dynamic from 'next/dynamic';
|
||||
import Head from 'next/head';
|
||||
import Link from 'next/link';
|
||||
|
|
@ -59,7 +59,7 @@ function FileSelection({
|
|||
|
||||
const [error, setError] = useState<{
|
||||
message: string;
|
||||
details?: string;
|
||||
details?: ReactNode;
|
||||
} | null>(null);
|
||||
const [errorDetails, { toggle: toggleErrorDetails }] = useDisclosure(false);
|
||||
|
||||
|
|
@ -75,18 +75,38 @@ function FileSelection({
|
|||
setError(null);
|
||||
if (!file) return;
|
||||
|
||||
let data: unknown;
|
||||
try {
|
||||
const text = await file.text();
|
||||
const data = JSON.parse(text);
|
||||
const parsed = DashboardTemplateSchema.parse(data); // throws if invalid
|
||||
onComplete(parsed);
|
||||
} catch (e: any) {
|
||||
data = JSON.parse(text);
|
||||
} catch (e: unknown) {
|
||||
onComplete(null);
|
||||
setError({
|
||||
message: 'Invalid JSON File',
|
||||
details: e instanceof Error ? e.message : 'Failed to parse JSON',
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
const result = DashboardTemplateSchema.safeParse(data);
|
||||
if (!result.success) {
|
||||
onComplete(null);
|
||||
setError({
|
||||
message: 'Failed to Import Dashboard',
|
||||
details: e?.message ?? 'Failed to parse/validate JSON',
|
||||
details: (
|
||||
<Stack gap={0}>
|
||||
{result.error.issues.map(issue => (
|
||||
<Text key={`${issue.path.join('.')}:${issue.message}`} c="red">
|
||||
{issue.message}
|
||||
</Text>
|
||||
))}
|
||||
</Stack>
|
||||
),
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
onComplete(result.data);
|
||||
};
|
||||
|
||||
return (
|
||||
|
|
@ -153,7 +173,7 @@ function FileSelection({
|
|||
{error && (
|
||||
<div>
|
||||
<Text c="red">{error.message}</Text>
|
||||
{error.details && (
|
||||
{error.details != null && (
|
||||
<>
|
||||
<Button
|
||||
variant="transparent"
|
||||
|
|
@ -173,9 +193,7 @@ function FileSelection({
|
|||
{errorDetails ? 'Hide Details' : 'Show Details'}
|
||||
</Group>
|
||||
</Button>
|
||||
<Collapse expanded={errorDetails}>
|
||||
<Text c="red">{error.details}</Text>
|
||||
</Collapse>
|
||||
<Collapse expanded={errorDetails}>{error.details}</Collapse>
|
||||
</>
|
||||
)}
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -84,6 +84,73 @@ test.describe('Dashboard Template Import', { tag: ['@dashboard'] }, () => {
|
|||
},
|
||||
);
|
||||
|
||||
test(
|
||||
'should show a friendly error when the imported file has duplicate tile IDs',
|
||||
{ tag: '@full-stack' },
|
||||
async ({ page }) => {
|
||||
const duplicateId = 'duplicate-tile-id';
|
||||
const dashboardFile = {
|
||||
version: '0.1.0',
|
||||
name: 'Duplicate Tile Dashboard',
|
||||
tiles: [
|
||||
{
|
||||
id: duplicateId,
|
||||
x: 0,
|
||||
y: 0,
|
||||
w: 6,
|
||||
h: 4,
|
||||
config: {
|
||||
name: 'Tile A',
|
||||
source: 'Logs',
|
||||
displayType: 'number',
|
||||
select: [{ aggFn: 'count', valueExpression: '' }],
|
||||
where: '',
|
||||
whereLanguage: 'sql',
|
||||
},
|
||||
},
|
||||
{
|
||||
id: duplicateId,
|
||||
x: 6,
|
||||
y: 0,
|
||||
w: 6,
|
||||
h: 4,
|
||||
config: {
|
||||
name: 'Tile B',
|
||||
source: 'Logs',
|
||||
displayType: 'number',
|
||||
select: [{ aggFn: 'count', valueExpression: '' }],
|
||||
where: '',
|
||||
whereLanguage: 'sql',
|
||||
},
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
await test.step('Navigate to the dashboard file import page', async () => {
|
||||
await dashboardImportPage.gotoImport();
|
||||
await expect(dashboardImportPage.fileUploadDropzone).toBeVisible();
|
||||
});
|
||||
|
||||
await test.step('Upload a dashboard file with duplicate tile IDs', async () => {
|
||||
await dashboardImportPage.uploadDashboardFile(
|
||||
JSON.stringify(dashboardFile),
|
||||
);
|
||||
});
|
||||
|
||||
await test.step('Verify the import error is shown and the mapping step is not reached', async () => {
|
||||
await expect(dashboardImportPage.importErrorTitle).toBeVisible();
|
||||
await expect(dashboardImportPage.mappingStepHeading).toBeHidden();
|
||||
});
|
||||
|
||||
await test.step('Expand error details and verify the duplicate tile ID is reported', async () => {
|
||||
await dashboardImportPage.showErrorDetailsButton.click();
|
||||
await expect(
|
||||
page.getByText(`Duplicate tile ID: ${duplicateId}`),
|
||||
).toBeVisible();
|
||||
});
|
||||
},
|
||||
);
|
||||
|
||||
test(
|
||||
'should show error for invalid template name',
|
||||
{ tag: '@full-stack' },
|
||||
|
|
|
|||
|
|
@ -11,8 +11,11 @@ export class DashboardImportPage {
|
|||
readonly dashboardNameInput: Locator;
|
||||
readonly finishImportButton: Locator;
|
||||
readonly fileUploadDropzone: Locator;
|
||||
readonly fileUploadInput: Locator;
|
||||
readonly templateNotFoundText: Locator;
|
||||
readonly browseAvailableTemplatesLink: Locator;
|
||||
readonly importErrorTitle: Locator;
|
||||
readonly showErrorDetailsButton: Locator;
|
||||
|
||||
constructor(page: Page) {
|
||||
this.page = page;
|
||||
|
|
@ -25,12 +28,25 @@ export class DashboardImportPage {
|
|||
this.fileUploadDropzone = page.getByText('Drag and drop a JSON file here', {
|
||||
exact: false,
|
||||
});
|
||||
this.fileUploadInput = page.locator('input[type="file"]');
|
||||
this.templateNotFoundText = page.getByText(
|
||||
"Oops! We couldn't find that template.",
|
||||
);
|
||||
this.browseAvailableTemplatesLink = page.getByRole('link', {
|
||||
name: 'browsing available templates',
|
||||
});
|
||||
this.importErrorTitle = page.getByText('Failed to Import Dashboard');
|
||||
this.showErrorDetailsButton = page.getByRole('button', {
|
||||
name: 'Show Details',
|
||||
});
|
||||
}
|
||||
|
||||
async uploadDashboardFile(content: string, filename = 'dashboard.json') {
|
||||
await this.fileUploadInput.setInputFiles({
|
||||
name: filename,
|
||||
mimeType: 'application/json',
|
||||
buffer: Buffer.from(content),
|
||||
});
|
||||
}
|
||||
|
||||
async gotoTemplates() {
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ import {
|
|||
BuilderChartConfigWithDateRange,
|
||||
Connection,
|
||||
DashboardSchema,
|
||||
DashboardTemplateSchema,
|
||||
MetricsDataType,
|
||||
SourceKind,
|
||||
TSource,
|
||||
|
|
@ -933,6 +934,56 @@ describe('utils', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('DashboardTemplateSchema duplicate tile IDs', () => {
|
||||
const makeTemplateTile = (id: string) => ({
|
||||
id,
|
||||
x: 0,
|
||||
y: 0,
|
||||
w: 6,
|
||||
h: 4,
|
||||
config: {
|
||||
name: `Tile ${id}`,
|
||||
source: 'source1',
|
||||
displayType: 'number',
|
||||
select: [{ aggFn: 'count', valueExpression: '' }],
|
||||
where: '',
|
||||
whereLanguage: 'sql',
|
||||
},
|
||||
});
|
||||
|
||||
it('accepts tiles with unique IDs', () => {
|
||||
const template = {
|
||||
version: '0.1.0',
|
||||
name: 'Unique Tiles',
|
||||
tiles: [
|
||||
makeTemplateTile('tile-a'),
|
||||
makeTemplateTile('tile-b'),
|
||||
makeTemplateTile('tile-c'),
|
||||
],
|
||||
};
|
||||
expect(() => DashboardTemplateSchema.parse(template)).not.toThrow();
|
||||
});
|
||||
|
||||
it('rejects tiles with duplicate IDs and surfaces the duplicate ID', () => {
|
||||
const template = {
|
||||
version: '0.1.0',
|
||||
name: 'Duplicate Tiles',
|
||||
tiles: [
|
||||
makeTemplateTile('dup'),
|
||||
makeTemplateTile('unique'),
|
||||
makeTemplateTile('dup'),
|
||||
],
|
||||
};
|
||||
|
||||
const result = DashboardTemplateSchema.safeParse(template);
|
||||
expect(result.success).toBe(false);
|
||||
if (!result.success) {
|
||||
const messages = result.error.issues.map(i => i.message);
|
||||
expect(messages).toContain('Duplicate tile ID: dup');
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('isJsonExpression', () => {
|
||||
it('should return false for expressions without dots', () => {
|
||||
expect(isJsonExpression('col')).toBe(false);
|
||||
|
|
|
|||
|
|
@ -910,6 +910,27 @@ export const PresetDashboardFilterSchema = DashboardFilterSchema.extend({
|
|||
|
||||
export type PresetDashboardFilter = z.infer<typeof PresetDashboardFilterSchema>;
|
||||
|
||||
export function addDuplicateTileIdIssues(
|
||||
tiles: { id?: string }[],
|
||||
ctx: z.RefinementCtx,
|
||||
options?: { messageSuffix?: string },
|
||||
) {
|
||||
const suffix = options?.messageSuffix ?? '';
|
||||
const seen = new Set<string>();
|
||||
for (let i = 0; i < tiles.length; i++) {
|
||||
const id = tiles[i].id;
|
||||
if (!id) continue;
|
||||
if (seen.has(id)) {
|
||||
ctx.addIssue({
|
||||
code: z.ZodIssueCode.custom,
|
||||
message: `Duplicate tile ID: ${id}${suffix}`,
|
||||
path: [i, 'id'],
|
||||
});
|
||||
}
|
||||
seen.add(id);
|
||||
}
|
||||
}
|
||||
|
||||
export const DashboardSchema = z.object({
|
||||
id: z.string(),
|
||||
name: z.string().min(1),
|
||||
|
|
@ -939,7 +960,7 @@ export const DashboardTemplateSchema = DashboardWithoutIdSchema.omit({
|
|||
version: z.string().min(1),
|
||||
description: z.string().optional(),
|
||||
tags: z.array(z.string()).optional(),
|
||||
tiles: z.array(TileTemplateSchema),
|
||||
tiles: z.array(TileTemplateSchema).superRefine(addDuplicateTileIdIssues),
|
||||
filters: z.array(DashboardFilterSchema).optional(),
|
||||
});
|
||||
export type DashboardTemplate = z.infer<typeof DashboardTemplateSchema>;
|
||||
|
|
|
|||
Loading…
Reference in a new issue