feat: add robust source form validation and error reporting #923
Conversation
🦋 Changeset detectedLatest commit: b749dd6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
knudtty
force-pushed
the
aaron/source-form-validation
branch
from
b7d1de4 to
a6f0261
Compare
knudtty
force-pushed
the
aaron/source-form-validation
branch
from
a6f0261 to
5a21bcb
Compare
| // TODO: HDX-1768 Eliminate type assertion | ||
| const source = await createSource(teamId.toString(), { | ||
| ...req.body, | ||
| team: teamId, | ||
| }); | ||
| } as any); |
There was a problem hiding this comment.
Because TSource and the new TSourceUnion are both used in some areas, some type assertions are needed to make things compile right now. I tagged all type assertions with a TODO comment and the linear issue where they were added.
I plan to follow up with a Part 2 PR to switch all instances of TSource to TSourceUnion on the frontend, or just eliminate TSource entirely. This should eliminate nearly all type assertions
| notifications.show({ | ||
| color: 'red', | ||
| message: ( | ||
| <Stack> | ||
| <Text size="sm"> | ||
| <b>Failed to create source</b> | ||
| </Text> | ||
| {errors.map((err, i) => ( | ||
| <Text key={i} size="sm"> | ||
| ✖ {err.message} | ||
| </Text> | ||
| ))} | ||
| </Stack> | ||
| ), | ||
| }); |
There was a problem hiding this comment.
Display a toast with the error information. You can test this out by trying to add a session without a Correlated Trace Source selected, or by any source without a timestamp column defined.
| if (parseResult.error) { | ||
| handleError(parseResult.error); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Does the error handling for creating and saving sources. I tried using zodResolver but couldn't get it working.
| <FormRow label={'Server Connection'}> | ||
| <ConnectionSelectControlled control={control} name={`connection`} /> | ||
| </FormRow> | ||
| <FormRow label={'Database'}> | ||
| <DatabaseSelectControlled | ||
| control={control} | ||
| name={`from.databaseName`} | ||
| connectionId={connectionId} | ||
| /> | ||
| </FormRow> | ||
| {kind !== SourceKind.Metric && ( | ||
| <FormRow label={'Table'}> | ||
| <DBTableSelectControlled | ||
| database={databaseName} | ||
| control={control} | ||
| name={`from.tableName`} | ||
| connectionId={connectionId} | ||
| rules={{ required: 'Table is required' }} | ||
| /> | ||
| </FormRow> | ||
| )} |
There was a problem hiding this comment.
Moved essential common components to a shared location
| export const MetricTableSchema = z | ||
| .object( | ||
| Object.values(MetricsDataType).reduce( | ||
| (acc, key) => ({ | ||
| ...acc, | ||
| [key]: z.string().optional(), | ||
| }), | ||
| {} as Record<MetricsDataType, z.ZodString>, | ||
| ), | ||
| ) | ||
| .refine( | ||
| tables => Object.values(tables).some(table => table && table.length > 0), | ||
| { message: 'At least one metric table must be specified' }, | ||
| ); |
packages/common-utils/src/types.ts
Outdated
| const SourceFormBaseSchema = z.object({ | ||
| id: z.string().optional(), | ||
| 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().min(1, 'Database is required'), | ||
| tableName: z.string().min(1, 'Table is required'), | ||
| }), | ||
| timestampValueExpression: z.string().min(1, 'Timestamp Column is required'), | ||
| }); |
There was a problem hiding this comment.
Common across all source types
packages/common-utils/src/types.ts
Outdated
| export const SourceFormSchema = z.discriminatedUnion('kind', [ | ||
| SourceFormBaseSchema.extend(LogSourceFormAugmentation), | ||
| SourceFormBaseSchema.extend(TraceSourceFormAugmentation), | ||
| SourceFormBaseSchema.extend(SessionSourceFormAugmentation), | ||
| SourceFormBaseSchema.extend(MetricSourceFormAugmentation), | ||
| ]); | ||
| export type TSourceForm = z.infer<typeof SourceFormSchema>; |
There was a problem hiding this comment.
This is where the magic happens. SourceFormSchema and TSourceUnion is a tagged union. You can switch on the 'kind' field of the different source types and typescript will know which fields are valid for that type. I want to replace TSource everywhere possible with TSourceUnion. It will eliminate an entire class of bugs where we wrongly assume a source is a specific kind, but that field doesn't exist on that source because it's a different kind
| 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(), |
There was a problem hiding this comment.
Remove previous zod SourceSchema
| ? never | ||
| : K]?: T extends infer U ? (K extends keyof U ? U[K] : never) : never; | ||
| }; | ||
| export type TSource = FlattenUnion<z.infer<typeof SourceSchema>>; |
There was a problem hiding this comment.
There is some type magic happening, but this preserves TSource compatibility while being based off of the new union zod schema
knudtty
force-pushed
the
aaron/source-form-validation
branch
3 times, most recently
from
980d2ac to
17146f4
Compare
knudtty
force-pushed
the
aaron/source-form-validation
branch
from
17146f4 to
b749dd6
Compare
This PR deprecates the existing zod source schema, which was not adequately handling the different cases for required vs. optional parameters across different source types. The new zod schema is a discriminated union, built from the different SourceKinds. With its inferred type,
TSourceUnion, you can now switch onkindand Typescript will resolve which fields are valid for that kind of source. We should adopt this type across the app wherever we can instead ofTSource.TSourcewas previously inferred from the original source schema and is heavily used across the application and not yet ready for removal. For compatibility,TSourceis now constructed with some type magic from the new TSourceUnion.Fixes HDX-1768
Fixes HDX-1804