🪟🐛 client side validation of cron expressions #17887
Conversation
github-actions
bot
added
area/platform
josephkmh
added
team/compose
area/frontend
josephkmh
changed the title
josephkmh
changed the title
| .string() | ||
| .required("form.empty.error") | ||
| .test("validCron", "form.cronExpression.error", (expression) => | ||
| !expression ? false : validateCronExpression(expression) |
There was a problem hiding this comment.
Should this check (!expression) be part of validateCronExpression to make this simpler?
There was a problem hiding this comment.
Yeah that's probably a good call. Yup.string.required() unfortunately doesn't narrow the type (it's a bug), so it's still string | undefined when it reaches the .test() method.
| @@ -29,6 +29,7 @@ import { ValuesProps } from "hooks/services/useConnectionHook"; | |||
| import { useCurrentWorkspace } from "services/workspaces/WorkspacesService"; | |||
|
|
|||
| import calculateInitialCatalog from "./calculateInitialCatalog"; | |||
| import { validateCronExpression } from "./ScheduleField"; | |||
There was a problem hiding this comment.
This utility is pretty generic, maybe it's best on src/utils/cron ?
There was a problem hiding this comment.
Sure, I can move it there. I wonder if over time src/utils might get bloated if we move all pure/generic functions there, but we can cross that bridge when we get there.
| @@ -204,6 +203,11 @@ const ScheduleField: React.FC = () => { | |||
| onChange={(item: DropDownOptionDataItem) => onCronChange(item, field, form, "cronTimeZone")} | |||
| /> | |||
| </div> | |||
| {(form.errors?.scheduleData as ConnectionScheduleData)?.cron?.cronExpression && ( | |||
There was a problem hiding this comment.
This value could be set as a var before the return so that it's easier to use:
const cronExpression = (form.errors?.scheduleData as ConnectionScheduleData)?.cron?.cronExpression
There was a problem hiding this comment.
Yeah it's kind of a nasty expression, I'll extract it to make the JSX cleaner.
airbyte-webapp/src/views/Connection/ConnectionForm/ScheduleField/validateCronExpression.ts
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/views/Connection/ConnectionForm/ScheduleField/ScheduleField.tsx
Show resolved
Hide resolved
| const cronValidationError = useMemo(() => { | ||
| return (errors?.scheduleData as ConnectionScheduleData)?.cron?.cronExpression; | ||
| }, [errors]); |
There was a problem hiding this comment.
If we want to get rid of the typecasting here (which imho looks a bit weird, given that TS thinks scheduleData is string), we could directly pull the error of that field (and get rid of the whole useMemo here):
const [, { error: cronValidationError }] = useField("scheduleData.cron.cronExpression");There was a problem hiding this comment.
Nice, that is cleaner, I'll use that. It still is not super type safe (useField() will accept any string without complaining) but it's not as ugly 😅
I think to get proper typing we would need to change the yup schema away from overloading scheduleData with the ambiguous .mixed().when(...), but I didn't want to blow up the PR.
What
Closes https://github.com/airbytehq/airbyte-cloud/issues/2772
Adds frontend validation of cron expressions
How
I tested various cron parsing libraries, but unfortunately did not find any that work perfectly with the quartz syntax.
Instead of parsing the cron expression, I'm relying on a pretty loose regex for the frontend validation. This will give the user quick feedback if, for example, the cron is too short or has invalid characters. I have included tests for all examples on the quartz docs as well as every single cron expression one of our cloud customers is currently using.
It is possible that an invalid cron expression makes it through the frontend validation. In that case, the backend will return an error which we display to the user.