1
0
mirror of https://github.com/Unleash/unleash.git synced 2025-08-04 13:48:56 +02:00

chore(1-3639): constraint validation (#9909)

Implements client-side validation of constraint values before you can
add them to a constraint.

I've removed the extra server-side validation that used to happen for
each specific constraint, because the surrounding form itself uses
server side validation to check every constraint every time there's a
change. This is what controls disabling the submit button etc.

I wanna make the next PR a bit of a followup cleanup now that it's
clearer what properties we do and don't need.
<img width="371" alt="image"
src="https://github.com/user-attachments/assets/7c98708f-fcbe-40ca-8590-bb0f5b2ad167"
/>
<img width="361" alt="image"
src="https://github.com/user-attachments/assets/503d4841-d910-4e8e-b0ef-a3d725739534"
/>
This commit is contained in:
Thomas Heartman 2025-05-06 15:21:33 +02:00 committed by GitHub
parent 681079bd08
commit a7118e0c18
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 164 additions and 60 deletions

View File

@ -64,6 +64,9 @@ type Validator =
| 'STRING_ARRAY_VALIDATOR' | 'STRING_ARRAY_VALIDATOR'
| 'DATE_VALIDATOR'; | 'DATE_VALIDATOR';
/**
* @deprecated; remove with `addEditStrategy` flag. This component requires a lot of state and mixes many components. Better off using dedicated pieces where you need them.
*/
export const useConstraintInput = ({ export const useConstraintInput = ({
contextDefinition, contextDefinition,
localConstraint, localConstraint,

View File

@ -3,6 +3,7 @@ import { styled } from '@mui/material';
import { forwardRef, useImperativeHandle, useRef, useState } from 'react'; import { forwardRef, useImperativeHandle, useRef, useState } from 'react';
import { ValueChip } from './ValueList'; import { ValueChip } from './ValueList';
import { AddValuesPopover, type OnAddActions } from './AddValuesPopover'; import { AddValuesPopover, type OnAddActions } from './AddValuesPopover';
import type { ConstraintValidatorOutput } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/constraintValidators';
const StyledChip = styled(ValueChip, { const StyledChip = styled(ValueChip, {
shouldForwardProp: (prop) => prop !== 'hasValue', shouldForwardProp: (prop) => prop !== 'hasValue',
@ -23,10 +24,21 @@ type Props = {
currentValue?: string; currentValue?: string;
helpText?: string; helpText?: string;
inputType: 'text' | 'number'; inputType: 'text' | 'number';
validator: (value: string) => ConstraintValidatorOutput;
}; };
export const AddSingleValueWidget = forwardRef<HTMLDivElement, Props>( export const AddSingleValueWidget = forwardRef<HTMLDivElement, Props>(
({ currentValue, onAddValue, removeValue, helpText, inputType }, ref) => { (
{
currentValue,
onAddValue,
removeValue,
helpText,
inputType,
validator,
},
ref,
) => {
const [open, setOpen] = useState(false); const [open, setOpen] = useState(false);
const positioningRef = useRef<HTMLDivElement>(null); const positioningRef = useRef<HTMLDivElement>(null);
useImperativeHandle( useImperativeHandle(
@ -40,9 +52,15 @@ export const AddSingleValueWidget = forwardRef<HTMLDivElement, Props>(
return; return;
} }
const [isValid, errorMessage] = validator(newValue);
if (isValid) {
onAddValue(newValue); onAddValue(newValue);
setError(''); setError('');
setOpen(false); setOpen(false);
} else {
setError(errorMessage);
return;
}
}; };
return ( return (

View File

@ -4,6 +4,7 @@ import { forwardRef, useImperativeHandle, useRef, useState } from 'react';
import { parseParameterStrings } from 'utils/parseParameter'; import { parseParameterStrings } from 'utils/parseParameter';
import { baseChipStyles } from './ValueList'; import { baseChipStyles } from './ValueList';
import { AddValuesPopover, type OnAddActions } from './AddValuesPopover'; import { AddValuesPopover, type OnAddActions } from './AddValuesPopover';
import type { ConstraintValidatorOutput } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/constraintValidators';
// todo: MUI v6 / v7 upgrade: consider changing this to a Chip to align with the rest of the values and the single value selector. There was a fix introduced in v6 that makes you not lose focus on pressing esc: https://mui.com/material-ui/migration/upgrade-to-v6/#chip talk to Thomas for more info. // todo: MUI v6 / v7 upgrade: consider changing this to a Chip to align with the rest of the values and the single value selector. There was a fix introduced in v6 that makes you not lose focus on pressing esc: https://mui.com/material-ui/migration/upgrade-to-v6/#chip talk to Thomas for more info.
const AddValuesButton = styled('button')(({ theme }) => ({ const AddValuesButton = styled('button')(({ theme }) => ({
@ -29,10 +30,11 @@ const AddValuesButton = styled('button')(({ theme }) => ({
interface AddValuesProps { interface AddValuesProps {
onAddValues: (newValues: string[]) => void; onAddValues: (newValues: string[]) => void;
helpText?: string; helpText?: string;
validator: (...values: string[]) => ConstraintValidatorOutput;
} }
export const AddValuesWidget = forwardRef<HTMLButtonElement, AddValuesProps>( export const AddValuesWidget = forwardRef<HTMLButtonElement, AddValuesProps>(
({ onAddValues, helpText }, ref) => { ({ onAddValues, helpText, validator }, ref) => {
const [open, setOpen] = useState(false); const [open, setOpen] = useState(false);
const positioningRef = useRef<HTMLButtonElement>(null); const positioningRef = useRef<HTMLButtonElement>(null);
useImperativeHandle( useImperativeHandle(
@ -56,9 +58,14 @@ export const AddValuesWidget = forwardRef<HTMLButtonElement, AddValuesProps>(
return; return;
} }
const [isValid, errorMessage] = validator(...newValues);
if (isValid) {
onAddValues(newValues); onAddValues(newValues);
clearInput(); clearInput();
setError(''); setError('');
} else {
setError(errorMessage);
}
}; };
return ( return (

View File

@ -6,12 +6,12 @@ import { styled } from '@mui/material';
import TimezoneCountries from 'countries-and-timezones'; import TimezoneCountries from 'countries-and-timezones';
import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly'; import { ScreenReaderOnly } from 'component/common/ScreenReaderOnly/ScreenReaderOnly';
import { HelpIcon } from 'component/common/HelpIcon/HelpIcon'; import { HelpIcon } from 'component/common/HelpIcon/HelpIcon';
import type { ConstraintValidatorOutput } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/constraintValidators';
interface IDateSingleValueProps { interface IDateSingleValueProps {
setValue: (value: string) => void; setValue: (value: string) => void;
value?: string; value?: string;
error: string; validator: (value: string) => ConstraintValidatorOutput;
setError: React.Dispatch<React.SetStateAction<string>>;
} }
const StyledInput = styled(Input)({ const StyledInput = styled(Input)({
@ -31,9 +31,9 @@ const Container = styled('div')(({ theme }) => ({
export const ConstraintDateInput = ({ export const ConstraintDateInput = ({
setValue, setValue,
value, value,
error, validator,
setError,
}: IDateSingleValueProps) => { }: IDateSingleValueProps) => {
const [error, setError] = useState('');
const timezones = Object.values( const timezones = Object.values(
TimezoneCountries.getAllTimezones({ deprecated: false }) as { TimezoneCountries.getAllTimezones({ deprecated: false }) as {
[timezone: string]: { name: string; utcOffsetStr: string }; [timezone: string]: { name: string; utcOffsetStr: string };
@ -79,8 +79,18 @@ export const ConstraintDateInput = ({
setError(''); setError('');
const parsedDate = parseValidDate(e.target.value); const parsedDate = parseValidDate(e.target.value);
const dateString = parsedDate?.toISOString(); const dateString = parsedDate?.toISOString();
dateString && setPickedDate(dateString); if (!dateString) {
setError('Invalid date format');
} else {
setPickedDate(dateString);
const [isValid, errorMessage] = validator(dateString);
if (isValid) {
dateString && setValue(dateString); dateString && setValue(dateString);
} else {
setError(errorMessage);
}
}
}} }}
InputLabelProps={{ InputLabelProps={{
shrink: true, shrink: true,

View File

@ -1,6 +1,9 @@
import { IconButton, styled } from '@mui/material'; import { IconButton, styled } from '@mui/material';
import GeneralSelect from 'component/common/GeneralSelect/GeneralSelect'; import GeneralSelect from 'component/common/GeneralSelect/GeneralSelect';
import type { Input } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput'; import {
useConstraintInput,
type Input,
} from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput';
import { import {
DATE_AFTER, DATE_AFTER,
dateOperators, dateOperators,
@ -11,7 +14,7 @@ import {
import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext';
import type { IUnleashContextDefinition } from 'interfaces/context'; import type { IUnleashContextDefinition } from 'interfaces/context';
import type { IConstraint } from 'interfaces/strategy'; import type { IConstraint } from 'interfaces/strategy';
import { useEffect, useRef, useState, type FC } from 'react'; import { useEffect, useMemo, useRef, useState, type FC } from 'react';
import { oneOf } from 'utils/oneOf'; import { oneOf } from 'utils/oneOf';
import { import {
CURRENT_TIME_CONTEXT_FIELD, CURRENT_TIME_CONTEXT_FIELD,
@ -32,6 +35,7 @@ import { AddSingleValueWidget } from './AddSingleValueWidget';
import { ConstraintDateInput } from './ConstraintDateInput'; import { ConstraintDateInput } from './ConstraintDateInput';
import { LegalValuesSelector } from './LegalValuesSelector'; import { LegalValuesSelector } from './LegalValuesSelector';
import { resolveLegalValues } from './resolve-legal-values'; import { resolveLegalValues } from './resolve-legal-values';
import { constraintValidator } from './constraint-validator';
const Container = styled('article')(({ theme }) => ({ const Container = styled('article')(({ theme }) => ({
'--padding': theme.spacing(2), '--padding': theme.spacing(2),
@ -177,7 +181,6 @@ type Props = {
setContextName: (contextName: string) => void; setContextName: (contextName: string) => void;
setOperator: (operator: Operator) => void; setOperator: (operator: Operator) => void;
setLocalConstraint: React.Dispatch<React.SetStateAction<IConstraint>>; setLocalConstraint: React.Dispatch<React.SetStateAction<IConstraint>>;
action: string;
onDelete?: () => void; onDelete?: () => void;
toggleInvertedOperator: () => void; toggleInvertedOperator: () => void;
toggleCaseSensitivity: () => void; toggleCaseSensitivity: () => void;
@ -189,11 +192,9 @@ type Props = {
setValue: (value: string) => void; setValue: (value: string) => void;
setValues: (values: string[]) => void; setValues: (values: string[]) => void;
setValuesWithRecord: (values: string[]) => void; setValuesWithRecord: (values: string[]) => void;
setError: React.Dispatch<React.SetStateAction<string>>;
removeValue: (index: number) => void; removeValue: (index: number) => void;
input: Input;
error: string;
}; };
export const EditableConstraint: FC<Props> = ({ export const EditableConstraint: FC<Props> = ({
constraintChanges, constraintChanges,
constraint, constraint,
@ -205,17 +206,19 @@ export const EditableConstraint: FC<Props> = ({
onUndo, onUndo,
toggleInvertedOperator, toggleInvertedOperator,
toggleCaseSensitivity, toggleCaseSensitivity,
input,
contextDefinition, contextDefinition,
constraintValues, constraintValues,
constraintValue, constraintValue,
setValue, setValue,
setValues, setValues,
setValuesWithRecord, setValuesWithRecord,
setError,
removeValue, removeValue,
error,
}) => { }) => {
const { input } = useConstraintInput({
contextDefinition,
localConstraint,
});
const { context } = useUnleashContext(); const { context } = useUnleashContext();
const { contextName, operator } = localConstraint; const { contextName, operator } = localConstraint;
const [showCaseSensitiveButton, setShowCaseSensitiveButton] = const [showCaseSensitiveButton, setShowCaseSensitiveButton] =
@ -279,6 +282,7 @@ export const EditableConstraint: FC<Props> = ({
} }
}; };
const validator = useMemo(() => constraintValidator(input), [input]);
const TopRowInput = () => { const TopRowInput = () => {
switch (inputType.input) { switch (inputType.input) {
case 'date': case 'date':
@ -286,13 +290,13 @@ export const EditableConstraint: FC<Props> = ({
<ConstraintDateInput <ConstraintDateInput
setValue={setValue} setValue={setValue}
value={localConstraint.value} value={localConstraint.value}
error={error} validator={validator}
setError={setError}
/> />
); );
case 'single value': case 'single value':
return ( return (
<AddSingleValueWidget <AddSingleValueWidget
validator={validator}
onAddValue={(newValue) => { onAddValue={(newValue) => {
setValue(newValue); setValue(newValue);
}} }}
@ -311,6 +315,7 @@ export const EditableConstraint: FC<Props> = ({
case 'multiple values': case 'multiple values':
return ( return (
<AddValuesWidget <AddValuesWidget
validator={validator}
helpText='Maximum 100 char length per value' helpText='Maximum 100 char length per value'
ref={addValuesButtonRef} ref={addValuesButtonRef}
onAddValues={(newValues) => { onAddValues={(newValues) => {

View File

@ -1,12 +1,10 @@
import { useCallback, useEffect, useState } from 'react'; import { useCallback, useEffect, useState } from 'react';
import type { IConstraint } from 'interfaces/strategy'; import type { IConstraint } from 'interfaces/strategy';
import { cleanConstraint } from 'utils/cleanConstraint'; import { cleanConstraint } from 'utils/cleanConstraint';
import useFeatureApi from 'hooks/api/actions/useFeatureApi/useFeatureApi';
import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext';
import type { IUnleashContextDefinition } from 'interfaces/context'; import type { IUnleashContextDefinition } from 'interfaces/context';
import type { Operator } from 'constants/operators'; import type { Operator } from 'constants/operators';
import { EditableConstraint } from 'component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint'; import { EditableConstraint } from 'component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint';
import { useConstraintInput } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput';
interface IConstraintAccordionEditProps { interface IConstraintAccordionEditProps {
constraint: IConstraint; constraint: IConstraint;
@ -55,13 +53,6 @@ export const EditableConstraintWrapper = ({
const [contextDefinition, setContextDefinition] = useState( const [contextDefinition, setContextDefinition] = useState(
resolveContextDefinition(context, localConstraint.contextName), resolveContextDefinition(context, localConstraint.contextName),
); );
const { validateConstraint } = useFeatureApi();
const [action, setAction] = useState('');
const { input, validator, setError, error } = useConstraintInput({
contextDefinition,
localConstraint,
});
useEffect(() => { useEffect(() => {
setContextDefinition( setContextDefinition(
@ -69,10 +60,6 @@ export const EditableConstraintWrapper = ({
); );
}, [localConstraint.contextName, context]); }, [localConstraint.contextName, context]);
useEffect(() => {
setError('');
}, [setError]);
const onUndo = () => { const onUndo = () => {
if (constraintChanges.length < 2) return; if (constraintChanges.length < 2) return;
const previousChange = constraintChanges[constraintChanges.length - 2]; const previousChange = constraintChanges[constraintChanges.length - 2];
@ -190,7 +177,6 @@ export const EditableConstraintWrapper = ({
setLocalConstraint={setLocalConstraint} setLocalConstraint={setLocalConstraint}
setContextName={setContextName} setContextName={setContextName}
setOperator={setOperator} setOperator={setOperator}
action={action}
toggleInvertedOperator={setInvertedOperator} toggleInvertedOperator={setInvertedOperator}
toggleCaseSensitivity={setCaseInsensitive} toggleCaseSensitivity={setCaseInsensitive}
onDelete={onDelete} onDelete={onDelete}
@ -199,11 +185,8 @@ export const EditableConstraintWrapper = ({
setValues={setValues} setValues={setValues}
setValuesWithRecord={setValuesWithRecord} setValuesWithRecord={setValuesWithRecord}
setValue={setValue} setValue={setValue}
setError={setError}
constraintValues={constraint?.values || []} constraintValues={constraint?.values || []}
constraintValue={constraint?.value || ''} constraintValue={constraint?.value || ''}
input={input}
error={error}
contextDefinition={contextDefinition} contextDefinition={contextDefinition}
removeValue={removeValue} removeValue={removeValue}
constraint={constraint} constraint={constraint}

View File

@ -0,0 +1,75 @@
// todo: (flag: `addEditStrategy`) see if this type is better duplicated or extracted to somewhere else
import type { Input } from 'component/common/NewConstraintAccordion/ConstraintAccordionEdit/ConstraintAccordionEditBody/useConstraintInput/useConstraintInput';
import { isValid, parseISO } from 'date-fns';
import semver from 'semver';
export type ConstraintValidationResult = [boolean, string];
const numberValidator = (value: string): ConstraintValidationResult => {
const converted = Number(value);
if (typeof converted !== 'number' || Number.isNaN(converted)) {
if (typeof value === 'string' && value.includes(',')) {
return [
false,
'Comma (",") is not valid as a decimal separator. Use a period (".") instead.',
];
}
return [false, 'Value must be a number'];
}
return [true, ''];
};
const stringListValidator = (
...values: string[]
): ConstraintValidationResult => {
const error: ConstraintValidationResult = [
false,
'Values must be a list of strings',
];
if (!Array.isArray(values)) {
return error;
}
if (!values.every((value) => typeof value === 'string')) {
return error;
}
return [true, ''];
};
const semVerValidator = (value: string): ConstraintValidationResult => {
const isCleanValue = semver.clean(value) === value;
if (!semver.valid(value) || !isCleanValue) {
return [false, 'Value is not a valid semver. For example 1.2.4'];
}
return [true, ''];
};
const dateValidator = (value: string): ConstraintValidationResult => {
if (!isValid(parseISO(value))) {
return [false, 'Value must be a valid date matching RFC3339'];
}
return [true, ''];
};
export const constraintValidator = (input: Input) => {
switch (input) {
case 'IN_OPERATORS_LEGAL_VALUES':
case 'STRING_OPERATORS_LEGAL_VALUES':
case 'NUM_OPERATORS_LEGAL_VALUES':
case 'SEMVER_OPERATORS_LEGAL_VALUES':
case 'IN_OPERATORS_FREETEXT':
case 'STRING_OPERATORS_FREETEXT':
return stringListValidator;
case 'DATE_OPERATORS_SINGLE_VALUE':
return dateValidator;
case 'NUM_OPERATORS_SINGLE_VALUE':
return numberValidator;
case 'SEMVER_OPERATORS_SINGLE_VALUE':
return semVerValidator;
}
};

View File

@ -53,8 +53,10 @@ export const MobileNavigationSidebar: FC<{
); );
}; };
export const StretchContainer = styled(Box)<{ mode: string; admin: boolean }>( export const StretchContainer = styled(Box, {
({ theme, mode, admin }) => ({ shouldForwardProp: (propName) =>
propName !== 'mode' && propName !== 'admin',
})<{ mode: string; admin: boolean }>(({ theme, mode, admin }) => ({
backgroundColor: admin backgroundColor: admin
? theme.palette.background.application ? theme.palette.background.application
: theme.palette.background.paper, : theme.palette.background.paper,
@ -68,8 +70,7 @@ export const StretchContainer = styled(Box)<{ mode: string; admin: boolean }>(
overflowAnchor: 'none', overflowAnchor: 'none',
minWidth: mode === 'full' ? theme.spacing(32) : 'auto', minWidth: mode === 'full' ? theme.spacing(32) : 'auto',
width: mode === 'full' ? theme.spacing(32) : 'auto', width: mode === 'full' ? theme.spacing(32) : 'auto',
}), }));
);
const StyledLink = styled(Link)(({ theme }) => focusable(theme)); const StyledLink = styled(Link)(({ theme }) => focusable(theme));
@ -92,7 +93,9 @@ const StyledUnleashLogoOnlyWhite = styled(LogoOnlyWhite)(({ theme }) => ({
})); }));
// This component is needed when the sticky item could overlap with nav items. You can replicate it on a short screen. // This component is needed when the sticky item could overlap with nav items. You can replicate it on a short screen.
const StickyContainer = styled(Box)<{ admin: boolean }>(({ theme, admin }) => ({ const StickyContainer = styled(Box, {
shouldForwardProp: (prop) => prop !== 'admin',
})<{ admin: boolean }>(({ theme, admin }) => ({
position: 'sticky', position: 'sticky',
paddingBottom: theme.spacing(1.5), paddingBottom: theme.spacing(1.5),
paddingTop: theme.spacing(1), paddingTop: theme.spacing(1),