Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ed1f486
move ip utils to their own file
david-crespo Sep 20, 2024
72b506f
validateIpNet util based one existing validateIp
david-crespo Sep 20, 2024
7edb0f3
let's try a more interesting result type on the validator
david-crespo Sep 24, 2024
02a20d1
validate IPs and IP nets on the route form
david-crespo Sep 24, 2024
ee03705
redo validateIp to look more like validateIpNet, esp. including message
david-crespo Sep 24, 2024
1cce7b4
tweak validation logic to get more helpful messages, test it
david-crespo Sep 24, 2024
22747cb
validate transit IPs on network interface edit
david-crespo Sep 24, 2024
56d872b
split parse from validate, it's clean!
david-crespo Sep 24, 2024
da65a40
fix e2e failure but it raises existential questions
david-crespo Sep 24, 2024
feeedd0
test leading zeroes in width
david-crespo Sep 25, 2024
48f2985
pass noop validate functions to comboboxes that get swapped with text…
david-crespo Sep 25, 2024
1efa7ef
validate IPs, IP nets and names on firewall rules form
david-crespo Sep 26, 2024
e54fc38
fix subform revalidation issue
david-crespo Sep 27, 2024
7fd1d1c
really fix subform revalidation and test it like crazy
david-crespo Sep 30, 2024
a048e32
add test for transit IPs form, incorporate PR #2480
david-crespo Oct 1, 2024
c554f31
validate transit ip dupes in validate function
david-crespo Oct 1, 2024
26ee9a9
tests that catch clear button bug, fix bug
david-crespo Oct 1, 2024
92c0d8d
have to do the reset on success effect for transit IPs too
david-crespo Oct 1, 2024
b6bb51a
test IP validation in route form
david-crespo Oct 1, 2024
fb16091
polish up last two todos to do two twos too
david-crespo Oct 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion app/components/form/fields/ComboboxField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import {
useController,
type Control,
type FieldPath,
type FieldPathValue,
type FieldValues,
type Validate,
} from 'react-hook-form'

import { Combobox, type ComboboxBaseProps } from '~/ui/lib/Combobox'
Expand All @@ -25,6 +27,7 @@ export type ComboboxFieldProps<
name: TName
control: Control<TFieldValues>
onChange?: (value: string | null | undefined) => void
validate?: Validate<FieldPathValue<TFieldValues, TName>, TFieldValues>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this lets us validate names in arbitrary-value comboboxes.

} & ComboboxBaseProps

export function ComboboxField<
Expand All @@ -51,9 +54,14 @@ export function ComboboxField<
: allowArbitraryValues
? 'Select an option or enter a custom value'
: 'Select an option',
validate,
...props
}: ComboboxFieldProps<TFieldValues, TName>) {
const { field, fieldState } = useController({ name, control, rules: { required } })
const { field, fieldState } = useController({
name,
control,
rules: { required, validate },
})
return (
<div className="max-w-lg">
<Combobox
Expand Down
82 changes: 58 additions & 24 deletions app/forms/firewall-rules-common.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* Copyright Oxide Computer Company
*/

import { useEffect } from 'react'
import {
useController,
useForm,
Expand All @@ -27,7 +28,7 @@ import { CheckboxField } from '~/components/form/fields/CheckboxField'
import { ComboboxField } from '~/components/form/fields/ComboboxField'
import { DescriptionField } from '~/components/form/fields/DescriptionField'
import { ListboxField } from '~/components/form/fields/ListboxField'
import { NameField } from '~/components/form/fields/NameField'
import { NameField, validateName } from '~/components/form/fields/NameField'
import { NumberField } from '~/components/form/fields/NumberField'
import { RadioField } from '~/components/form/fields/RadioField'
import { TextField, TextFieldInner } from '~/components/form/fields/TextField'
Expand All @@ -39,6 +40,7 @@ import * as MiniTable from '~/ui/lib/MiniTable'
import { TextInputHint } from '~/ui/lib/TextInput'
import { KEYS } from '~/ui/util/keys'
import { ALL_ISH } from '~/util/consts'
import { validateIp, validateIpNet } from '~/util/ip'
import { links } from '~/util/links'
import { capitalize } from '~/util/str'

Expand All @@ -62,7 +64,6 @@ type TargetAndHostFilterType =
type TargetAndHostFormValues = {
type: TargetAndHostFilterType
value: string
subnetVpc?: string
}

// these are part of the target and host filter form;
Expand Down Expand Up @@ -132,8 +133,13 @@ const DynamicTypeAndValueFields = ({
items={items}
allowArbitraryValues
hideOptionalTag
// TODO: validate here, but it's complicated because it's conditional
// on which type is selected
validate={(value) =>
// required: false arg is desirable because otherwise if you put in
// a bad name and submit, causing it to revalidate onChange, then
// clear the field you're left with a BS "Target name is required"
// error message
validateName(value, `${capitalize(sectionType)} name`, false)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think required: false is correct because we don't want this to show a required error when it's left empty after a bad input was attempted.

/>
) : (
<TextField
Expand All @@ -146,8 +152,11 @@ const DynamicTypeAndValueFields = ({
onSubmitTextField(e)
}
}}
// TODO: validate here, but it's complicated because it's conditional
// on which type is selected
validate={(value) =>
(valueType === 'ip' && validateIp(value)) ||
(valueType === 'ip_net' && validateIpNet(value)) ||
undefined
}
/>
)}
</>
Expand Down Expand Up @@ -233,14 +242,14 @@ type CommonFieldsProps = {
error: ApiError | null
}

const targetAndHostDefaultValues: TargetAndHostFormValues = {
type: 'vpc',
value: '',
}

export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => {
const { project, vpc } = useVpcSelector()
const targetAndHostDefaultValues: TargetAndHostFormValues = {
type: 'vpc',
value: '',
// only becomes relevant when the type is 'VPC subnet'; ignored otherwise
subnetVpc: vpc,
}

// prefetchedApiQueries below are prefetched in firewall-rules-create and -edit
const {
data: { items: instances },
Expand All @@ -255,8 +264,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
// Targets
const targetForm = useForm({ defaultValues: targetAndHostDefaultValues })
const targets = useController({ name: 'targets', control }).field
const targetType = targetForm.watch('type')
const targetValue = targetForm.watch('value')
const [targetType, targetValue] = targetForm.watch(['type', 'value'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡

// get the list of items that are not already in the list of targets
const targetItems = {
vpc: availableItems(targets.value, 'vpc', vpcs),
Expand All @@ -272,8 +280,20 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
if (!type || !value) return
if (targets.value.some((t) => t.value === value && t.type === type)) return
targets.onChange([...targets.value, { type, value }])
targetForm.reset()
targetForm.reset(targetAndHostDefaultValues)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't seeing this reset do what I expected, and I figured out why: we call this reset inside the callback to handleSubmit. This does in fact set isSubmitted to false. But after it runs our callback, handleSubmit sets isSubmitted: true. Hence the very brief flash of false, then back to true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by useEffect below.

})
// HACK: we need to reset the target form completely after a successful submit,
// including especially `isSubmitted`, because that governs whether we're in
// the validate regime (which doesn't validate until submit) or the reValidate
// regime (which validate on every keypress). The reset inside `handleSubmit`
// doesn't do that for us because `handleSubmit` set `isSubmitted: true` after
// running the callback. So we have to watch for a successful submit and call
// the reset again here.
// https://github.com/react-hook-form/react-hook-form/blob/9a497a70a/src/logic/createFormControl.ts#L1194-L1203
const { isSubmitSuccessful: targetSubmitSuccessful } = targetForm.formState
useEffect(() => {
if (targetSubmitSuccessful) targetForm.reset(targetAndHostDefaultValues)
}, [targetSubmitSuccessful, targetForm])
Comment on lines +293 to +296
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a clever workaround. A shame that we need to distinguish between the two flows, but premature validation errors are even worse. Nevertheless, this is clever.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking there is probably a way to wrap this up into some kind of subform helper hook.


// Ports
const portRangeForm = useForm({ defaultValues: { portRange: '' } })
Expand All @@ -290,8 +310,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
// Host Filters
const hostForm = useForm({ defaultValues: targetAndHostDefaultValues })
const hosts = useController({ name: 'hosts', control }).field
const hostType = hostForm.watch('type')
const hostValue = hostForm.watch('value')
const [hostType, hostValue] = hostForm.watch(['type', 'value'])
// get the list of items that are not already in the list of host filters
const hostFilterItems = {
vpc: availableItems(hosts.value, 'vpc', vpcs),
Expand All @@ -306,8 +325,13 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
if (!type || !value) return
if (hosts.value.some((t) => t.value === value && t.type === type)) return
hosts.onChange([...hosts.value, { type, value }])
hostForm.reset()
hostForm.reset(targetAndHostDefaultValues)
})
// HACK: see comment above about doing the same for target form
const { isSubmitSuccessful: hostSubmitSuccessful } = hostForm.formState
useEffect(() => {
if (hostSubmitSuccessful) hostForm.reset(targetAndHostDefaultValues)
}, [hostSubmitSuccessful, hostForm])

return (
<>
Expand Down Expand Up @@ -411,13 +435,18 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
control={targetForm.control}
valueType={targetType}
items={targetItems[targetType]}
onTypeChange={() => targetForm.setValue('value', '')}
// HACK: reset the whole subform, keeping type (because we just set
// it). most importantly, this resets isSubmitted so the form can go
// back to validating on submit instead of change
onTypeChange={() =>
targetForm.reset({ type: targetForm.getValues('type'), value: '' })
}
onInputChange={(value) => targetForm.setValue('value', value)}
onSubmitTextField={submitTarget}
/>
<MiniTable.ClearAndAddButtons
addButtonCopy="Add target"
disableClear={!!targetValue}
disableClear={!targetValue}
onClear={() => targetForm.reset()}
onSubmit={submitTarget}
/>
Expand Down Expand Up @@ -468,8 +497,8 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
</div>
<MiniTable.ClearAndAddButtons
addButtonCopy="Add port filter"
disableClear={!!portValue}
onClear={portRangeForm.reset}
disableClear={!portValue}
onClear={() => portRangeForm.reset()}
onSubmit={submitPortRange}
/>
</div>
Expand Down Expand Up @@ -518,13 +547,18 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
control={hostForm.control}
valueType={hostType}
items={hostFilterItems[hostType]}
onTypeChange={() => hostForm.setValue('value', '')}
// HACK: reset the whole subform, keeping type (because we just set
// it). most importantly, this resets isSubmitted so the form can go
// back to validating on submit instead of change
onTypeChange={() =>
hostForm.reset({ type: hostForm.getValues('type'), value: '' })
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of hate this, but it does pretty elegantly express what I want to happen here, namely that we are basically starting over from scratch whenever we change the type. Very weird combination of feeling both wrong and exactly right.

onInputChange={(value) => hostForm.setValue('value', value)}
onSubmitTextField={submitHost}
/>
<MiniTable.ClearAndAddButtons
addButtonCopy="Add host filter"
disableClear={!!hostValue}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WHOOPS

disableClear={!hostValue}
onClear={() => hostForm.reset()}
onSubmit={submitHost}
/>
Expand Down
20 changes: 9 additions & 11 deletions app/forms/ip-pool-range-add.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,14 @@ import { SideModalForm } from '~/components/form/SideModalForm'
import { useIpPoolSelector } from '~/hooks/use-params'
import { addToast } from '~/stores/toast'
import { Message } from '~/ui/lib/Message'
import { parseIp } from '~/util/ip'
import { pb } from '~/util/path-builder'
import { validateIp } from '~/util/str'

const defaultValues: IpRange = {
first: '',
last: '',
}

const invalidAddressError = { type: 'pattern', message: 'Not a valid IP address' }

const ipv6Error = { type: 'pattern', message: 'IPv6 ranges are not yet supported' }

/**
Expand All @@ -35,20 +33,20 @@ const ipv6Error = { type: 'pattern', message: 'IPv6 ranges are not yet supported
* regex twice, though.
*/
function resolver(values: IpRange) {
const first = validateIp(values.first)
const last = validateIp(values.last)
const first = parseIp(values.first)
const last = parseIp(values.last)

const errors: FieldErrors<IpRange> = {}

if (!first.valid) {
errors.first = invalidAddressError
} else if (first.isv6) {
if (first.type === 'error') {
errors.first = { type: 'pattern', message: first.message }
} else if (first.type === 'v6') {
errors.first = ipv6Error
}

if (!last.valid) {
errors.last = invalidAddressError
} else if (last.isv6) {
if (last.type === 'error') {
errors.last = { type: 'pattern', message: last.message }
} else if (last.type === 'v6') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing actually changes in this file, so I didn't change the tests.

errors.last = ipv6Error
}

Expand Down
28 changes: 21 additions & 7 deletions app/forms/network-interface-edit.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*
* Copyright Oxide Computer Company
*/
import { useEffect } from 'react'
import { useForm } from 'react-hook-form'
import * as R from 'remeda'

Expand All @@ -25,6 +26,7 @@ import { FieldLabel } from '~/ui/lib/FieldLabel'
import * as MiniTable from '~/ui/lib/MiniTable'
import { TextInputHint } from '~/ui/lib/TextInput'
import { KEYS } from '~/ui/util/keys'
import { validateIpNet } from '~/util/ip'
import { links } from '~/util/links'

type EditNetworkInterfaceFormProps = {
Expand Down Expand Up @@ -56,13 +58,18 @@ export function EditNetworkInterfaceForm({
const transitIps = form.watch('transitIps') || []

const transitIpsForm = useForm({ defaultValues: { transitIp: '' } })
const transitIpValue = transitIpsForm.watch('transitIp')
const { isSubmitSuccessful: transitIpSubmitSuccessful } = transitIpsForm.formState

const submitTransitIp = () => {
const transitIp = transitIpsForm.getValues('transitIp')
if (!transitIp) return
const submitTransitIp = transitIpsForm.handleSubmit(({ transitIp }) => {
// validate has already checked to make sure it's not in the list
form.setValue('transitIps', [...transitIps, transitIp])
transitIpsForm.reset()
}
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without wrapping this in handleSubmit, it doesn't run the validate function on submit. This way the inner function never gets called unless validation passes.


useEffect(() => {
if (transitIpSubmitSuccessful) transitIpsForm.reset()
}, [transitIpSubmitSuccessful, transitIpsForm])

return (
<SideModalForm
Expand Down Expand Up @@ -92,7 +99,7 @@ export function EditNetworkInterfaceForm({
Transit IPs
</FieldLabel>
<TextInputHint id="transitIp-help-text" className="mb-2">
Enter an IPv4 or IPv6 address.{' '}
An IP network, like 192.168.0.0/16.{' '}
<a href={links.transitIpsDocs} target="_blank" rel="noreferrer">
Learn more about transit IPs.
</a>
Expand All @@ -107,12 +114,19 @@ export function EditNetworkInterfaceForm({
submitTransitIp()
}
}}
validate={(value) => {
const error = validateIpNet(value)
if (error) return error

if (transitIps.includes(value)) return 'Transit IP already in list'
}}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this here instead of in the handleSubmit means we get a beautiful validation error inline on attempts to add a dupe.

placeholder="Enter an IP network"
/>
</div>
<MiniTable.ClearAndAddButtons
addButtonCopy="Add Transit IP"
disableClear={!!transitIpsForm.formState.dirtyFields.transitIp}
onClear={transitIpsForm.reset}
disableClear={!transitIpValue}
onClear={() => transitIpsForm.reset()}
onSubmit={submitTransitIp}
/>
</div>
Expand Down
Loading