diff --git a/.changeset/dry-ducks-shake.md b/.changeset/dry-ducks-shake.md new file mode 100644 index 000000000..924ad4431 --- /dev/null +++ b/.changeset/dry-ducks-shake.md @@ -0,0 +1,6 @@ +--- +"@hyperdx/common-utils": patch +"@hyperdx/app": patch +--- + +feat: add robust source form validation and error reporting diff --git a/packages/api/src/routers/api/__tests__/sources.test.ts b/packages/api/src/routers/api/__tests__/sources.test.ts index 861f4d1c3..871aa6558 100644 --- a/packages/api/src/routers/api/__tests__/sources.test.ts +++ b/packages/api/src/routers/api/__tests__/sources.test.ts @@ -1,10 +1,10 @@ -import { SourceKind } from '@hyperdx/common-utils/dist/types'; +import { SourceKind, TSourceUnion } from '@hyperdx/common-utils/dist/types'; import { Types } from 'mongoose'; import { getLoggedInAgent, getServer } from '@/fixtures'; import { Source } from '@/models/source'; -const MOCK_SOURCE = { +const MOCK_SOURCE: Omit, 'id'> = { kind: SourceKind.Log, name: 'Test Source', connection: new Types.ObjectId().toString(), @@ -13,6 +13,7 @@ const MOCK_SOURCE = { tableName: 'test_table', }, timestampValueExpression: 'timestamp', + defaultTableSelectExpression: 'body', }; describe('sources router', () => { diff --git a/packages/api/src/routers/api/sources.ts b/packages/api/src/routers/api/sources.ts index 94e6333f2..840a1d97c 100644 --- a/packages/api/src/routers/api/sources.ts +++ b/packages/api/src/routers/api/sources.ts @@ -1,4 +1,7 @@ -import { SourceSchema } from '@hyperdx/common-utils/dist/types'; +import { + SourceSchema, + sourceSchemaWithout, +} from '@hyperdx/common-utils/dist/types'; import express from 'express'; import { z } from 'zod'; import { validateRequest } from 'zod-express-middleware'; @@ -26,19 +29,22 @@ router.get('/', async (req, res, next) => { } }); +const SourceSchemaNoId = sourceSchemaWithout({ id: true }); + router.post( '/', validateRequest({ - body: SourceSchema.omit({ id: true }), + body: SourceSchemaNoId, }), async (req, res, next) => { try { const { teamId } = getNonNullUserWithTeam(req); + // TODO: HDX-1768 Eliminate type assertion const source = await createSource(teamId.toString(), { ...req.body, team: teamId, - }); + } as any); res.json(source); } catch (e) { @@ -59,10 +65,11 @@ router.put( try { const { teamId } = getNonNullUserWithTeam(req); + // TODO: HDX-1768 Eliminate type assertion const source = await updateSource(teamId.toString(), req.params.id, { ...req.body, team: teamId, - }); + } as any); if (!source) { res.status(404).send('Source not found'); diff --git a/packages/app/src/components/SQLInlineEditor.tsx b/packages/app/src/components/SQLInlineEditor.tsx index 2fe5e0139..b5a78d964 100644 --- a/packages/app/src/components/SQLInlineEditor.tsx +++ b/packages/app/src/components/SQLInlineEditor.tsx @@ -106,6 +106,7 @@ type SQLInlineEditorProps = { onLanguageChange?: (language: 'sql' | 'lucene') => void; language?: 'sql' | 'lucene'; onSubmit?: () => void; + error?: React.ReactNode; size?: string; label?: React.ReactNode; disableKeywordAutocomplete?: boolean; @@ -134,6 +135,7 @@ export default function SQLInlineEditor({ onLanguageChange, language, onSubmit, + error, value, size, label, @@ -260,7 +262,7 @@ export default function SQLInlineEditor({ shadow="none" bg="dark.6" style={{ - border: '1px solid var(--mantine-color-gray-7)', + border: `1px solid ${error ? 'var(--mantine-color-red-7)' : 'var(--mantine-color-gray-7)'}`, display: 'flex', alignItems: 'center', minHeight: size === 'xs' ? 30 : 36, @@ -357,7 +359,7 @@ export function SQLInlineEditorControlled({ queryHistoryType, ...props }: Omit & UseControllerProps) { - const { field } = useController(props); + const { field, fieldState } = useController(props); // Guard against wrongly typed values const value = field.value || props.defaultValue; @@ -375,6 +377,7 @@ export function SQLInlineEditorControlled({ onChange={field.onChange} placeholder={placeholder} value={stringValue} + error={fieldState.error?.message} additionalSuggestions={additionalSuggestions} queryHistoryType={queryHistoryType} {...props} diff --git a/packages/app/src/components/SourceForm.tsx b/packages/app/src/components/SourceForm.tsx index 35ec9dbc5..f3d159dfe 100644 --- a/packages/app/src/components/SourceForm.tsx +++ b/packages/app/src/components/SourceForm.tsx @@ -6,10 +6,13 @@ import { UseFormSetValue, UseFormWatch, } from 'react-hook-form'; +import { z } from 'zod'; import { MetricsDataType, SourceKind, + sourceSchemaWithout, TSource, + TSourceUnion, } from '@hyperdx/common-utils/dist/types'; import { Anchor, @@ -18,17 +21,12 @@ import { Divider, Flex, Group, - Menu, Radio, - SegmentedControl, - Select, Slider, Stack, - Switch, Text, Tooltip, } from '@mantine/core'; -import { useDebouncedCallback } from '@mantine/hooks'; import { notifications } from '@mantine/notifications'; import { SourceSelectControlled } from '@/components/SourceSelect'; @@ -117,15 +115,7 @@ function FormRow({ // OR traceModel.logModel = 'log_id_blah' // custom always points towards the url param -export function LogTableModelForm({ - control, - watch, - setValue, -}: { - control: Control; - watch: UseFormWatch; - setValue: UseFormSetValue; -}) { +export function LogTableModelForm({ control, watch }: TableModelProps) { const databaseName = watch(`from.databaseName`, DEFAULT_DATABASE); const tableName = watch(`from.tableName`); const connectionId = watch(`connection`); @@ -135,25 +125,6 @@ export function LogTableModelForm({ return ( <> - - - - - - - - - ; - watch: UseFormWatch; - setValue: UseFormSetValue; -}) { +export function TraceTableModelForm({ control, watch }: TableModelProps) { const databaseName = watch(`from.databaseName`, DEFAULT_DATABASE); const tableName = watch(`from.tableName`); const connectionId = watch(`connection`); return ( - - - - - - - - - ( + render={({ field: { onChange, value } }) => (
; - watch: UseFormWatch; - setValue: UseFormSetValue; -}) { +export function SessionTableModelForm({ control, watch }: TableModelProps) { const databaseName = watch(`from.databaseName`, DEFAULT_DATABASE); const tableName = watch(`from.tableName`); const connectionId = watch(`connection`); - const [showOptionalFields, setShowOptionalFields] = useState(false); - return ( <> - - - - - - - - - ; + watch: UseFormWatch; + setValue: UseFormSetValue; +} + export function MetricTableModelForm({ control, watch, setValue, -}: { - control: Control; - watch: UseFormWatch; - setValue: UseFormSetValue; -}) { +}: TableModelProps) { const databaseName = watch(`from.databaseName`, DEFAULT_DATABASE); const connectionId = watch(`connection`); @@ -780,7 +697,11 @@ export function MetricTableModelForm({ const [prefix, suffix] = name.split('.'); if (prefix === 'metricTables') { const tableName = - value?.metricTables?.[suffix as keyof typeof value.metricTables]; + value.kind === SourceKind.Metric + ? value?.metricTables?.[ + suffix as keyof typeof value.metricTables + ] + : ''; const metricType = suffix as MetricsDataType; const isValid = await isValidMetricTable({ databaseName, @@ -811,16 +732,6 @@ export function MetricTableModelForm({ return ( <> - - - - - - {Object.values(MetricsDataType).map(metricType => ( ; - watch: UseFormWatch; - setValue: UseFormSetValue; + control: Control; + watch: UseFormWatch; + setValue: UseFormSetValue; kind: SourceKind; }) { switch (kind) { @@ -916,37 +827,49 @@ export function TableSourceForm({ const { data: source } = useSource({ id: sourceId }); const { data: connections } = useConnections(); - const { watch, control, setValue, handleSubmit, resetField, formState } = - useForm({ - defaultValues: { - kind: SourceKind.Log, - name: defaultName, - connection: connections?.[0]?.id, - from: { - databaseName: 'default', - tableName: '', - }, + const { + watch, + control, + setValue, + formState, + handleSubmit, + resetField, + setError, + clearErrors, + } = useForm({ + defaultValues: { + kind: SourceKind.Log, + name: defaultName, + connection: connections?.[0]?.id, + from: { + databaseName: 'default', + tableName: '', }, - values: source, - resetOptions: { - keepDirtyValues: true, - keepErrors: true, - }, - }); + }, + // TODO: HDX-1768 remove type assertion + values: source as TSourceUnion, + resetOptions: { + keepDirtyValues: true, + keepErrors: true, + }, + }); useEffect(() => { - const { unsubscribe } = watch(async (value, { name, type }) => { + const { unsubscribe } = watch(async (_value, { name, type }) => { try { + // TODO: HDX-1768 get rid of this type assertion + const value = _value as TSourceUnion; if ( value.connection != null && value.from?.databaseName != null && - value.from.tableName != null && + (value.kind === SourceKind.Metric || value.from.tableName != null) && name === 'from.tableName' && type === 'change' ) { const config = await inferTableSourceConfig({ databaseName: value.from.databaseName, - tableName: value.from.tableName, + tableName: + value.kind !== SourceKind.Metric ? value.from.tableName : '', connectionId: value.connection, }); if (Object.keys(config).length > 0) { @@ -983,10 +906,42 @@ export function TableSourceForm({ const updateSource = useUpdateSource(); const deleteSource = useDeleteSource(); + const sourceFormSchema = sourceSchemaWithout({ id: true }); + const handleError = (error: z.ZodError) => { + const errors = error.errors; + for (const err of errors) { + const errorPath: string = err.path.join('.'); + // TODO: HDX-1768 get rid of this type assertion if possible + setError(errorPath as any, { ...err }); + } + notifications.show({ + color: 'red', + message: ( + + + Failed to create source + + {errors.map((err, i) => ( + + ✖ {err.message} + + ))} + + ), + }); + }; + const _onCreate = useCallback(() => { + clearErrors(); handleSubmit(data => { + const parseResult = sourceFormSchema.safeParse(data); + if (parseResult.error) { + handleError(parseResult.error); + return; + } createSource.mutate( - { source: data }, + // TODO: HDX-1768 get rid of this type assertion + { source: data as TSource }, { onSuccess: data => { onCreate?.(data); @@ -995,21 +950,28 @@ export function TableSourceForm({ message: 'Source created', }); }, - onError: () => { + onError: error => { notifications.show({ color: 'red', - message: 'Failed to create source', + message: `Failed to create source - ${error.message}`, }); }, }, ); })(); - }, [handleSubmit, createSource, onCreate]); + }, [handleSubmit, createSource, onCreate, kind, formState]); const _onSave = useCallback(() => { + clearErrors(); handleSubmit(data => { + const parseResult = sourceFormSchema.safeParse(data); + if (parseResult.error) { + handleError(parseResult.error); + return; + } updateSource.mutate( - { source: data }, + // TODO: HDX-1768 get rid of this type assertion + { source: data as TSource }, { onSuccess: () => { onSave?.(); @@ -1029,6 +991,9 @@ export function TableSourceForm({ })(); }, [handleSubmit, updateSource, onSave]); + const databaseName = watch(`from.databaseName`, DEFAULT_DATABASE); + const connectionId = watch(`connection`); + return (
+ + + + + + + {kind !== SourceKind.Metric && ( + + + + )} = { key: Key; value: Value }; -export const MetricTableSchema = z.object( - Object.values(MetricsDataType).reduce( - (acc, key) => ({ - ...acc, - [key]: z.string(), - }), - {} as Record, - ), -); +export const MetricTableSchema = z + .object( + Object.values(MetricsDataType).reduce( + (acc, key) => ({ + ...acc, + [key]: z.string().optional(), + }), + {} as Record, + ), + ) + .refine( + tables => Object.values(tables).some(table => table && table.length > 0), + { message: 'At least one metric table must be specified' }, + ); export type MetricTable = z.infer; @@ -481,54 +486,176 @@ export enum SourceKind { Metric = 'metric', } -export const SourceSchema = z.object({ +// -------------------------- +// TABLE SOURCE FORM VALIDATION +// -------------------------- + +// Base schema with fields common to all source types +const SourceBaseSchema = z.object({ + id: z.string(), + name: z.string().min(1, 'Name is required'), + kind: z.nativeEnum(SourceKind), + connection: z.string().min(1, 'Server Connection is required'), from: z.object({ - databaseName: z.string(), - tableName: z.string(), + databaseName: z.string().min(1, 'Database is required'), + tableName: z.string().min(1, 'Table is required'), }), - timestampValueExpression: z.string(), - connection: z.string(), + timestampValueExpression: z.string().min(1, 'Timestamp Column is required'), +}); - // Common - kind: z.nativeEnum(SourceKind), - id: z.string(), - name: z.string(), - displayedTimestampValueExpression: z.string().optional(), - implicitColumnExpression: z.string().optional(), +// Log source form schema +const LogSourceAugmentation = { + kind: z.literal(SourceKind.Log), + defaultTableSelectExpression: z.string({ + message: 'Default Table Select Expression is required', + }), + + // Optional fields for logs serviceNameExpression: z.string().optional(), + severityTextExpression: z.string().optional(), bodyExpression: z.string().optional(), - tableFilterExpression: z.string().optional(), eventAttributesExpression: z.string().optional(), resourceAttributesExpression: z.string().optional(), - defaultTableSelectExpression: z.string().optional(), - - // Logs - uniqueRowIdExpression: z.string().optional(), - severityTextExpression: z.string().optional(), + displayedTimestampValueExpression: z.string().optional(), + metricSourceId: z.string().optional(), traceSourceId: z.string().optional(), - - // Traces & Logs traceIdExpression: z.string().optional(), spanIdExpression: z.string().optional(), + implicitColumnExpression: z.string().optional(), + uniqueRowIdExpression: z.string().optional(), + tableFilterExpression: z.string().optional(), +}; - // Sessions - sessionSourceId: z.string().optional(), - - // Traces - durationExpression: z.string().optional(), - durationPrecision: z.number().min(0).max(9).optional(), - parentSpanIdExpression: z.string().optional(), - spanNameExpression: z.string().optional(), - spanEventsValueExpression: z.string().optional(), +// Trace source form schema +const TraceSourceAugmentation = { + kind: z.literal(SourceKind.Trace), + defaultTableSelectExpression: z.string().optional(), - spanKindExpression: z.string().optional(), + // Required fields for traces + durationExpression: z.string().min(1, 'Duration Expression is required'), + durationPrecision: z.number().min(0).max(9).default(3), + traceIdExpression: z.string().min(1, 'Trace ID Expression is required'), + spanIdExpression: z.string().min(1, 'Span ID Expression is required'), + parentSpanIdExpression: z + .string() + .min(1, 'Parent span ID expression is required'), + spanNameExpression: z.string().min(1, 'Span Name Expression is required'), + spanKindExpression: z.string().min(1, 'Span Kind Expression is required'), + + // Optional fields for traces + logSourceId: z.string().optional().nullable(), + sessionSourceId: z.string().optional(), + metricSourceId: z.string().optional(), statusCodeExpression: z.string().optional(), statusMessageExpression: z.string().optional(), - logSourceId: z.string().optional().nullable(), + serviceNameExpression: z.string().optional(), + resourceAttributesExpression: z.string().optional(), + eventAttributesExpression: z.string().optional(), + spanEventsValueExpression: z.string().optional(), + implicitColumnExpression: z.string().optional(), +}; - // OTEL Metrics - metricTables: MetricTableSchema.optional(), - metricSourceId: z.string().optional(), -}); +// Session source form schema +const SessionSourceAugmentation = { + kind: z.literal(SourceKind.Session), + + // Required fields for sessions + eventAttributesExpression: z + .string() + .min(1, 'Log Attributes Expression is required'), + resourceAttributesExpression: z + .string() + .min(1, 'Resource Attributes Expression is required'), + traceSourceId: z + .string({ message: 'Correlated Trace Source is required' }) + .min(1, 'Correlated Trace Source is required'), + + // Optional fields for sessions + implicitColumnExpression: z.string().optional(), +}; + +// Metric source form schema +const MetricSourceAugmentation = { + kind: z.literal(SourceKind.Metric), + // override from SourceBaseSchema + from: z.object({ + databaseName: z.string().min(1, 'Database is required'), + tableName: z.string(), + }), + + // Metric tables - at least one should be provided + metricTables: MetricTableSchema, + resourceAttributesExpression: z + .string() + .min(1, 'Resource Attributes is required'), + + // Optional fields for metrics + logSourceId: z.string().optional(), +}; -export type TSource = z.infer; +// Union of all source form schemas for validation +export const SourceSchema = z.discriminatedUnion('kind', [ + SourceBaseSchema.extend(LogSourceAugmentation), + SourceBaseSchema.extend(TraceSourceAugmentation), + SourceBaseSchema.extend(SessionSourceAugmentation), + SourceBaseSchema.extend(MetricSourceAugmentation), +]); +export type TSourceUnion = z.infer; + +// This function exists to perform schema validation with omission of a certain +// value. It is not possible to do on the discriminatedUnion directly +export function sourceSchemaWithout( + omissions: { [k in keyof z.infer]?: true } = {}, +) { + // TODO: Make these types work better if possible + return z.discriminatedUnion('kind', [ + SourceBaseSchema.omit(omissions).extend(LogSourceAugmentation), + SourceBaseSchema.omit(omissions).extend(TraceSourceAugmentation), + SourceBaseSchema.omit(omissions).extend(SessionSourceAugmentation), + SourceBaseSchema.omit(omissions).extend(MetricSourceAugmentation), + ]); +} + +// Helper types for better union flattening +type AllKeys = T extends any ? keyof T : never; +// This is Claude Opus's explanation of this type magic to extract the required +// parameters: +// +// 1. [K in keyof T]-?: +// Maps over all keys in T. The -? removes the optional modifier, making all +// properties required in this mapped type +// 2. {} extends Pick ? never : K +// Pick creates a type with just property K from T. +// {} extends Pick checks if an empty object can satisfy the picked property. +// If the property is optional, {} can extend it (returns never) +// If the property is required, {} cannot extend it (returns K) +// 3. [keyof T] +// Indexes into the mapped type to get the union of all non-never values +type NonOptionalKeysPresentInEveryUnionBranch = { + [K in keyof T]-?: {} extends Pick ? never : K; +}[keyof T]; + +// Helper to check if a key is required in ALL branches of the union +type RequiredInAllBranches> = T extends any + ? K extends NonOptionalKeysPresentInEveryUnionBranch + ? true + : false + : never; + +// This type gathers the Required Keys across the discriminated union TSourceUnion +// and keeps them as required in a non-unionized type, and also gathers all possible +// optional keys from the union branches and brings them into one unified flattened type. +// This is done to maintain compatibility with the legacy zod schema. +type FlattenUnion = { + // If a key is required in all branches of a union, make it a required key + [K in AllKeys as RequiredInAllBranches extends true + ? K + : never]: T extends infer U ? (K extends keyof U ? U[K] : never) : never; +} & { + // If a key is not required in all branches of a union, make it an optional + // key and join the possible types + [K in AllKeys as RequiredInAllBranches extends true + ? never + : K]?: T extends infer U ? (K extends keyof U ? U[K] : never) : never; +}; +export type TSource = FlattenUnion>;