chore(1-3842): don't reorder constraint properties / make id's non-optional (#10160)

This PR takes two steps towards better constraint handling:

## New type: `IConstraintWithId`

Introduces a new type, `IConstraintWithId`. This is the same as an
`IConstraint`, except the constraint id property is required. The idea
is that the list of editable constraints should move towards using this
instead of just `IConstraint`. That should prevent us (on a type-level)
from seeing more of the same kind of errors we saw with the segment
constraints yesterday.

I don't want to go ahead and update all the upstream uses of this to
IConstraintWithId in this PR, so I'll look at that separately.

## API payload constraint replacer

Introduces an api payload constraint "replacer", which we can use for
[JSON.stringify's `replacer`
parameter](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#the_replacer_parameter).
The current implementation works both for strategies and for segments
and has been added to edit + create forms for both of these resources.

This has a couple benefits:
1. We can clearly state exactly how we want them to be rendered,
including property order. I've decided to go with context -> operator ->
value(s) as the main one (check the screenie), as I believe this is the
most logical reading order.
2. We can exclude value/values (whichever one doesn't work with the
operator)
3. It doesn't matter how we treat constraints internally, we can still
present the payload how we want
4. Importantly: this only affects the stringification for the
user-facing API payload, so it's very low risk. It does not affect
anything that we actually send to the api.


Here's what it can look like with ordered properties:

<img width="392" alt="image"
src="https://github.com/user-attachments/assets/f46f77c8-0b5a-4ded-b13a-bb567df60bd3"
/>

frontend/src/component/common/NewConstraintAccordion/ConstraintsList/EditableConstraintsList.tsx

@@ -1,5 +1,5 @@
 import type React from 'react';
-import { useImperativeHandle } from 'react';
+import { useEffect, useImperativeHandle } from 'react';
 import { forwardRef } from 'react';
 import { styled } from '@mui/material';
 import type { IConstraint } from 'interfaces/strategy';
@@ -9,6 +9,7 @@ import { ConstraintsList } from 'component/common/ConstraintsList/ConstraintsLis
 import { EditableConstraint } from 'component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/EditableConstraint';
 import { createEmptyConstraint } from '../../../../utils/createEmptyConstraint.ts';
 import { constraintId } from 'constants/constraintId.ts';
+import { v4 as uuidv4 } from 'uuid';
 export interface IEditableConstraintsListRef {
     addConstraint?: (contextName: string) => void;
 }
@@ -39,6 +40,17 @@ export const EditableConstraintsList = forwardRef<
         },
     }));
 
+    useEffect(() => {
+        if (!constraints.every((constraint) => constraintId in constraint)) {
+            setConstraints(
+                constraints.map((constraint) => ({
+                    [constraintId]: uuidv4(),
+                    ...constraint,
+                })),
+            );
+        }
+    }, [constraints, setConstraints]);
+
     const onDelete = (index: number) => {
         setConstraints(
             produce((draft) => {
@@ -70,7 +82,7 @@ export const EditableConstraintsList = forwardRef<
             <ConstraintsList>
                 {constraints.map((constraint, index) => (
                     <EditableConstraint
-                        key={constraint[constraintId] || index}
+                        key={constraint[constraintId]}
                         constraint={constraint}
                         onDelete={() => onDelete(index)}
                         onUpdate={onAutoSave(constraint[constraintId])}

frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.test.ts

@@ -1,6 +1,7 @@
 import { createEmptyConstraint } from 'utils/createEmptyConstraint';
 import { fromIConstraint, toIConstraint } from './editable-constraint-type.ts';
 import { constraintId } from 'constants/constraintId';
+import type { IConstraint } from 'interfaces/strategy.ts';
 
 test('mapping to and from retains the constraint id', () => {
     const constraint = createEmptyConstraint('context');
@@ -11,7 +12,7 @@ test('mapping to and from retains the constraint id', () => {
 });
 
 test('mapping to an editable constraint adds a constraint id if there is none', () => {
-    const constraint = createEmptyConstraint('context');
+    const constraint: IConstraint = createEmptyConstraint('context');
     delete constraint[constraintId];
 
     const editableConstraint = fromIConstraint(constraint);
@@ -23,22 +24,9 @@ test('mapping to an editable constraint adds a constraint id if there is none',
 });
 
 test('mapping from an empty constraint removes redundant value / values', () => {
-    const constraint = createEmptyConstraint('context');
+    const constraint = { ...createEmptyConstraint('context'), value: '' };
     expect(constraint).toHaveProperty('value');
 
     const transformed = toIConstraint(fromIConstraint(constraint));
     expect(transformed).not.toHaveProperty('value');
 });
-
-test('mapping to constraint returns properties in expected order', () => {
-    const constraint = createEmptyConstraint('context');
-    const transformed = toIConstraint(fromIConstraint(constraint));
-
-    expect(Object.keys(transformed)).toEqual([
-        'values',
-        'inverted',
-        'operator',
-        'contextName',
-        'caseInsensitive',
-    ]);
-});

frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.ts

@@ -90,29 +90,13 @@ export const fromIConstraint = (
 };
 
 export const toIConstraint = (constraint: EditableConstraint): IConstraint => {
-    const {
-        inverted,
-        operator,
-        contextName,
-        caseInsensitive,
-        [constraintId]: id,
-    } = constraint;
-    const baseValues = {
-        inverted,
-        operator,
-        contextName,
-        caseInsensitive,
-        [constraintId]: id,
-    };
     if ('value' in constraint) {
-        return {
-            value: constraint.value,
-            ...baseValues,
-        };
+        return constraint;
     } else {
+        const { values, ...rest } = constraint;
         return {
+            ...rest,
             values: Array.from(constraint.values),
-            ...baseValues,
         };
     }
 };

frontend/src/component/feature/FeatureStrategy/FeatureStrategyCreate/FeatureStrategyCreate.tsx

@@ -37,6 +37,7 @@ import { useDefaultStrategy } from '../../../project/Project/ProjectSettings/Pro
 import { FeatureStrategyForm } from '../FeatureStrategyForm/FeatureStrategyForm.tsx';
 import { NewStrategyVariants } from 'component/feature/StrategyTypes/NewStrategyVariants';
 import { Limit } from 'component/common/Limit/Limit';
+import { apiPayloadConstraintReplacer } from 'utils/api-payload-constraint-replacer.ts';
 
 const useStrategyLimit = (strategyCount: number) => {
     const { uiConfig } = useUiConfig();
@@ -280,7 +281,7 @@ export const formatAddStrategyApiCode = (
     }
 
     const url = `${unleashUrl}/api/admin/projects/${projectId}/features/${featureId}/environments/${environmentId}/strategies`;
-    const payload = JSON.stringify(strategy, undefined, 2);
+    const payload = JSON.stringify(strategy, apiPayloadConstraintReplacer, 2);
 
     return `curl --location --request POST '${url}' \\
     --header 'Authorization: INSERT_API_KEY' \\

frontend/src/component/feature/FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit.tsx

@@ -35,6 +35,7 @@ import {
     getChangeRequestConflictCreatedDataFromScheduleData,
 } from './change-request-conflict-data.ts';
 import { constraintId } from 'constants/constraintId.ts';
+import { apiPayloadConstraintReplacer } from 'utils/api-payload-constraint-replacer.ts';
 
 const useTitleTracking = () => {
     const [previousTitle, setPreviousTitle] = useState<string>('');
@@ -352,7 +353,11 @@ export const formatUpdateStrategyApiCode = (
     };
 
     const url = `${unleashUrl}/api/admin/projects/${projectId}/features/${featureId}/environments/${environmentId}/strategies/${strategyId}`;
-    const payload = JSON.stringify(sortedStrategy, undefined, 2);
+    const payload = JSON.stringify(
+        sortedStrategy,
+        apiPayloadConstraintReplacer,
+        2,
+    );
 
     return `curl --location --request PUT '${url}' \\
     --header 'Authorization: INSERT_API_KEY' \\

frontend/src/component/feature/FeatureStrategy/FeatureStrategyForm/FeatureStrategyForm.tsx

@@ -143,28 +143,6 @@ const StyledAlertBox = styled(Box)(({ theme }) => ({
     },
 }));
 
-const StyledEnvironmentBox = styled(Box)(({ theme }) => ({
-    display: 'flex',
-    alignItems: 'center',
-}));
-
-const EnvironmentIconBox = styled(Box)(({ theme }) => ({
-    transform: 'scale(0.9)',
-    display: 'flex',
-    alignItems: 'center',
-}));
-
-const EnvironmentTypography = styled(Typography, {
-    shouldForwardProp: (prop) => prop !== 'enabled',
-})<{ enabled: boolean }>(({ enabled }) => ({
-    fontWeight: enabled ? 'bold' : 'normal',
-}));
-
-const EnvironmentTypographyHeader = styled(Typography)(({ theme }) => ({
-    marginRight: theme.spacing(0.5),
-    color: theme.palette.text.secondary,
-}));
-
 const StyledTab = styled(Tab)(({ theme }) => ({
     width: '100px',
 }));
@@ -173,11 +151,11 @@ const StyledBadge = styled(Badge)(({ theme }) => ({
     marginLeft: theme.spacing(1),
 }));
 
-const StyledConstraintSeparator = styled(ConstraintSeparator)(({ theme }) => ({
+const StyledConstraintSeparator = styled(ConstraintSeparator)({
     top: '-10px',
     left: '0',
     transform: 'translateY(0)',
-}));
+});
 
 export const FeatureStrategyForm = ({
     projectId,

frontend/src/component/segments/CreateSegment/CreateSegment.tsx

@@ -21,6 +21,7 @@ import { useSegmentValuesCount } from 'component/segments/hooks/useSegmentValues
 import { SEGMENT_CREATE_BTN_ID } from 'utils/testIds';
 import { useSegmentLimits } from 'hooks/api/getters/useSegmentLimits/useSegmentLimits';
 import { useOptionalPathParam } from 'hooks/useOptionalPathParam';
+import { apiPayloadConstraintReplacer } from 'utils/api-payload-constraint-replacer.ts';
 
 interface ICreateSegmentProps {
     modal?: boolean;
@@ -61,7 +62,7 @@ export const CreateSegment = ({ modal }: ICreateSegmentProps) => {
         return `curl --location --request POST '${uiConfig.unleashUrl}/api/admin/segments' \\
 --header 'Authorization: INSERT_API_KEY' \\
 --header 'Content-Type: application/json' \\
---data-raw '${JSON.stringify(getSegmentPayload(), undefined, 2)}'`;
+--data-raw '${JSON.stringify(getSegmentPayload(), apiPayloadConstraintReplacer, 2)}'`;
     };
 
     const handleSubmit = async (e: React.FormEvent) => {

frontend/src/component/segments/EditSegment/EditSegment.tsx

@@ -27,6 +27,7 @@ import { useHighestPermissionChangeRequestEnvironment } from 'hooks/useHighestPe
 import type { ISegment } from 'interfaces/segment.ts';
 import { constraintId } from 'constants/constraintId.ts';
 import { v4 as uuidv4 } from 'uuid';
+import { apiPayloadConstraintReplacer } from 'utils/api-payload-constraint-replacer.ts';
 
 interface IEditSegmentProps {
     modal?: boolean;
@@ -88,7 +89,7 @@ export const EditSegment = ({ modal }: IEditSegmentProps) => {
         }/api/admin/segments/${segmentId}' \\
 --header 'Authorization: INSERT_API_KEY' \\
 --header 'Content-Type: application/json' \\
---data-raw '${JSON.stringify(getSegmentPayload(), undefined, 2)}'`;
+--data-raw '${JSON.stringify(getSegmentPayload(), apiPayloadConstraintReplacer, 2)}'`;
     };
 
     const highestPermissionChangeRequestEnv =

frontend/src/component/segments/SegmentForm.tsx

@@ -1,4 +1,4 @@
-import type { IConstraint } from 'interfaces/strategy';
+import type { IConstraint, IConstraintWithId } from 'interfaces/strategy';
 import { SegmentFormStepOne } from './SegmentFormStepOne.tsx';
 import { SegmentFormStepTwo } from './SegmentFormStepTwo.tsx';
 import type React from 'react';
@@ -14,7 +14,7 @@ interface ISegmentProps {
     name: string;
     description: string;
     project?: string;
-    constraints: IConstraint[];
+    constraints: IConstraintWithId[];
     setName: React.Dispatch<React.SetStateAction<string>>;
     setDescription: React.Dispatch<React.SetStateAction<string>>;
     setProject: React.Dispatch<React.SetStateAction<string | undefined>>;

frontend/src/component/segments/SegmentFormStepTwo.test.tsx

@@ -3,12 +3,12 @@ import { screen, waitFor } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import { testServerRoute, testServerSetup } from 'utils/testServer';
 import { SegmentFormStepTwo } from './SegmentFormStepTwo.tsx';
-import type { IConstraint } from 'interfaces/strategy';
 import { vi } from 'vitest';
 import {
     CREATE_SEGMENT,
     UPDATE_PROJECT_SEGMENT,
 } from 'component/providers/AccessProvider/permissions';
+import type { IConstraintWithId } from 'interfaces/strategy.ts';
 
 const server = testServerSetup();
 
@@ -26,7 +26,7 @@ const setupRoutes = () => {
 
 const defaultProps = {
     project: undefined,
-    constraints: [] as IConstraint[],
+    constraints: [] as IConstraintWithId[],
     setConstraints: vi.fn(),
     setCurrentStep: vi.fn(),
     mode: 'create' as const,

frontend/src/component/segments/SegmentFormStepTwo.tsx

@@ -13,7 +13,7 @@ import {
     UPDATE_SEGMENT,
 } from 'component/providers/AccessProvider/permissions';
 import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext';
-import type { IConstraint } from 'interfaces/strategy';
+import type { IConstraint, IConstraintWithId } from 'interfaces/strategy';
 import { useNavigate } from 'react-router-dom';
 import { EditableConstraintsList } from 'component/common/NewConstraintAccordion/ConstraintsList/EditableConstraintsList';
 import type { IEditableConstraintsListRef } from 'component/common/NewConstraintAccordion/ConstraintsList/EditableConstraintsList';
@@ -33,7 +33,7 @@ import { GO_BACK } from 'constants/navigate';
 
 interface ISegmentFormPartTwoProps {
     project?: string;
-    constraints: IConstraint[];
+    constraints: IConstraintWithId[];
     setConstraints: React.Dispatch<React.SetStateAction<IConstraint[]>>;
     setCurrentStep: React.Dispatch<React.SetStateAction<SegmentFormStep>>;
     mode: SegmentFormMode;

frontend/src/component/segments/hooks/useSegmentForm.ts

@@ -1,6 +1,8 @@
-import type { IConstraint } from 'interfaces/strategy';
+import type { IConstraint, IConstraintWithId } from 'interfaces/strategy';
 import { useEffect, useState } from 'react';
 import { useSegmentValidation } from 'hooks/api/getters/useSegmentValidation/useSegmentValidation';
+import { v4 as uuidv4 } from 'uuid';
+import { constraintId } from 'constants/constraintId';
 
 export const useSegmentForm = (
     initialName = '',
@@ -11,8 +13,13 @@ export const useSegmentForm = (
     const [name, setName] = useState(initialName);
     const [description, setDescription] = useState(initialDescription);
     const [project, setProject] = useState<string | undefined>(initialProject);
-    const [constraints, setConstraints] =
-        useState<IConstraint[]>(initialConstraints);
+    const initialConstraintsWithId = initialConstraints.map((constraint) => ({
+        [constraintId]: uuidv4(),
+        ...constraint,
+    }));
+    const [constraints, setConstraints] = useState<IConstraintWithId[]>(
+        initialConstraintsWithId,
+    );
     const [errors, setErrors] = useState({});
     const nameError = useSegmentValidation(name, initialName);
 
@@ -29,7 +36,7 @@ export const useSegmentForm = (
     }, [initialProject]);
 
     useEffect(() => {
-        setConstraints(initialConstraints);
+        setConstraints(initialConstraintsWithId);
         // eslint-disable-next-line
     }, [JSON.stringify(initialConstraints)]);
 
@@ -61,7 +68,9 @@ export const useSegmentForm = (
         project,
         setProject,
         constraints,
-        setConstraints,
+        setConstraints: setConstraints as React.Dispatch<
+            React.SetStateAction<IConstraint[]>
+        >,
         getSegmentPayload,
         clearErrors,
         errors,

frontend/src/interfaces/strategy.ts

@@ -68,6 +68,10 @@ export interface IConstraint {
     [constraintId]?: string;
 }
 
+export interface IConstraintWithId extends IConstraint {
+    [constraintId]: string;
+}
+
 export interface IFeatureStrategySortOrder {
     id: string;
     sortOrder: number;

frontend/src/utils/api-payload-constraint-replacer.test.ts

@@ -0,0 +1,57 @@
+import type { IConstraint } from 'interfaces/strategy';
+import { serializeConstraint } from './api-payload-constraint-replacer.ts';
+import { constraintId } from 'constants/constraintId';
+
+test('keys are ordered in the expected order', () => {
+    const input: IConstraint = {
+        values: ['something'],
+        inverted: true,
+        operator: 'STR_CONTAINS',
+        contextName: 'context',
+        caseInsensitive: true,
+    };
+
+    const output = serializeConstraint(input);
+
+    expect(Object.entries(output)).toStrictEqual([
+        ['contextName', 'context'],
+        ['operator', 'STR_CONTAINS'],
+        ['values', ['something']],
+        ['caseInsensitive', true],
+        ['inverted', true],
+    ]);
+});
+
+test('only value OR values is present, not both', () => {
+    const input: IConstraint = {
+        value: 'something',
+        values: ['something else'],
+        inverted: true,
+        operator: 'IN',
+        contextName: 'context',
+        caseInsensitive: true,
+    };
+
+    const noValue = serializeConstraint(input);
+    expect(noValue.values).toStrictEqual(['something else']);
+    expect(noValue).not.toHaveProperty('value');
+
+    const noValues = serializeConstraint({
+        ...input,
+        operator: 'SEMVER_EQ',
+    });
+    expect(noValues.value).toStrictEqual('something');
+    expect(noValues).not.toHaveProperty('values');
+});
+
+test('constraint id is not included', () => {
+    const input: IConstraint = {
+        [constraintId]: 'constraint-id',
+        value: 'something',
+        operator: 'IN',
+        contextName: 'context',
+    };
+
+    const output = serializeConstraint(input);
+    expect(constraintId in output).toBeFalsy();
+});

frontend/src/utils/api-payload-constraint-replacer.ts

@@ -0,0 +1,39 @@
+import { isSingleValueOperator } from 'constants/operators';
+import type { IConstraint } from 'interfaces/strategy';
+
+export const serializeConstraint = ({
+    value,
+    values,
+    inverted,
+    operator,
+    contextName,
+    caseInsensitive,
+}: IConstraint): IConstraint => {
+    const makeConstraint = (
+        valueProp: { value: string } | { values: string[] },
+    ): IConstraint => {
+        return {
+            contextName,
+            operator,
+            ...valueProp,
+            caseInsensitive,
+            inverted,
+        };
+    };
+
+    if (isSingleValueOperator(operator)) {
+        return makeConstraint({ value: value || '' });
+    }
+
+    return makeConstraint({ values: values || [] });
+};
+
+export const apiPayloadConstraintReplacer = (key: string, value: any) => {
+    if (key !== 'constraints' || !Array.isArray(value)) {
+        return value;
+    }
+    const orderedConstraints = (value as IConstraint[]).map(
+        serializeConstraint,
+    );
+    return orderedConstraints;
+};

frontend/src/utils/createEmptyConstraint.ts

@@ -1,16 +1,15 @@
 import { constraintId } from 'constants/constraintId';
-import { dateOperators } from 'constants/operators';
-import type { IConstraint } from 'interfaces/strategy';
-import { oneOf } from 'utils/oneOf';
+import { isDateOperator } from 'constants/operators';
+import type { IConstraintWithId } from 'interfaces/strategy';
 import { operatorsForContext } from 'utils/operatorsForContext';
 import { v4 as uuidv4 } from 'uuid';
 
-export const createEmptyConstraint = (contextName: string): IConstraint => {
+export const createEmptyConstraint = (
+    contextName: string,
+): IConstraintWithId => {
     const operator = operatorsForContext(contextName)[0];
 
-    const value = oneOf(dateOperators, operator)
-        ? new Date().toISOString()
-        : '';
+    const value = isDateOperator(operator) ? new Date().toISOString() : '';
 
     return {
         contextName,