Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'unique' to model schema - new feature that checks unique constraints when saving/publishing #135

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
+254 −43
Open

Add 'unique' to model schema - new feature that checks unique constraints when saving/publishing #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5f214b9
find published rows on the same table and rotate
hyunnbunt Feb 7, 2025
cace0dc
Refactoring: duplicatedFields function
hyunnbunt Feb 7, 2025
917e7a5
code refatoring, filtered deleted rows when getting published rows fr…
hyunnbunt Feb 7, 2025
dfedaf0
A Sentry report has been added for queries that return 500 rows or more.
hyunnbunt Feb 7, 2025
65832f4
changed sentry event condition
hyunnbunt Feb 8, 2025
bf66115
changed rules of entry input forms
hyunnbunt Feb 11, 2025
df9b8de
throw BadRequestError with stringified list of fields
hyunnbunt Feb 12, 2025
0962c4d
separated uniqueConstraintCheck
hyunnbunt Feb 17, 2025
921b0cc
refactored - value names changed
hyunnbunt Feb 17, 2025
ac695f5
added uniquenessCheck in performSave
hyunnbunt Feb 20, 2025
2269c96
added checkUnique
hyunnbunt Feb 20, 2025
bd62b65
updateCmsRow reverted changes
hyunnbunt Feb 20, 2025
5e277df
api endpoint change
hyunnbunt Feb 20, 2025
b1e201c
change api method for uniqueness check to GET
hyunnbunt Feb 20, 2025
183fe75
refactoring
hyunnbunt Feb 20, 2025
c61e32f
minor changes
hyunnbunt Feb 20, 2025
ca55b90
unique check changes (publish/save use same module)
hyunnbunt Feb 24, 2025
263849c
DbMgr code duplication removed
hyunnbunt Feb 25, 2025
5ae63b3
Form.Item validateStatus and help attribute added
hyunnbunt Feb 25, 2025
add000b
update uniqueChangedFields state when Form changed
hyunnbunt Feb 25, 2025
a5c2d7c
removed unneccessary comments
hyunnbunt Feb 25, 2025
5105369
catch error code change on publish
hyunnbunt Feb 25, 2025
8c7af54
try block change
hyunnbunt Feb 25, 2025
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
132 changes: 98 additions & 34 deletions platform/wab/src/wab/client/components/cms/CmsEntryDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ export function renderContentEntryFormFields(
table: ApiCmsTable,
database: ApiCmsDatabase,
locales: string[],
disabled: boolean
disabled: boolean,
notValidUniqueFields: string[]
) {
return (
<>
Expand All @@ -145,6 +146,7 @@ export function renderContentEntryFormFields(
formItemProps: deriveFormItemPropsFromField(field),
typeName: field.type,
required: field.required,
uniqueNotValid: notValidUniqueFields.includes(field.identifier),
...(isCmsTextLike(field)
? {
maxChars: field.maxChars,
Expand Down Expand Up @@ -187,6 +189,16 @@ function CmsEntryDetailsForm_(
const mutateRow_ = useMutateRow();
const mutateTableRows = useMutateTableRows();

const [uniqueFieldsIdentifier, setUniqueFieldsIdentifier] = React.useState<
string[]
>([]);
const [uniqueChangedFields, setUniqueChangedFields] = React.useState<
Dict<unknown>
>({});
const [notValidUniqueFields, setNotVaildUniqueFields] = React.useState<
string[]
>([]);

const mutateRow = async () => {
const newRow = await mutateRow_(table.id, row.id);
if (newRow) {
Expand All @@ -199,7 +211,9 @@ function CmsEntryDetailsForm_(
const [form] = useForm();

const hasFormError = React.useCallback(() => {
return form.getFieldsError().some((f) => f.errors.length > 0);
return form.getFieldsError().some((f) => {
return f.errors.length > 0;
});
}, [form]);

const dataEquals = (
Expand Down Expand Up @@ -259,6 +273,10 @@ function CmsEntryDetailsForm_(
await validateFields();
};

const isUniqueFieldChanged = () => {
return Object.keys(uniqueChangedFields).length > 0;
};

const warnConflict = () => {
notification.error({
message: "Update conflict detected",
Expand Down Expand Up @@ -293,10 +311,32 @@ function CmsEntryDetailsForm_(
setInConflict(true);
};

async function checkUniqueness() {
try {
const opts = { uniqueChangedFields: uniqueChangedFields };
const checkedNotValid = await api.checkUnique(row.id, opts);
Copy link
Contributor

Choose a reason for hiding this comment

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

Change return type of checkUnique to make this simpler.

notValidUniqueFields = { a: UniqueFieldCheck, b: UniqueFieldCheck }
checkedNotValid = await api.checkUnique(...) // { b: true, c: false }
Object.entries(checkedNotValid).forEach(([identifier, check]) => {
  if (check.ok) {
    notValidUniqueFields.remove(identifier);
  } else {
    notValidUniqueFields.set(identifier, check);
  }
});

const checkedValid = Object.keys(uniqueChangedFields).filter(
(identifier) => !checkedNotValid.includes(identifier)
);
const checkedValidRemoved = notValidUniqueFields.filter(
(notValid) => !checkedValid.includes(notValid)
);
setNotVaildUniqueFields([
...new Set([...checkedValidRemoved, ...checkedNotValid]),
]);
setUniqueChangedFields({});
Copy link
Contributor

Choose a reason for hiding this comment

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

setState(prev => {
  return { ...prev, newStuff }
})

// type { "uniqueField": "a" }
// start request unique check for { "uniqueField": "a" }
// type { "uniqueField": "ab" }
// finish request unique check for { "uniqueField": "a" } <- this should be ignored
// start request unique check for { "uniqueField": "ab" }'

type UniqueFieldStatus {
  value: string
  status: "not started" | "pending" | "ok" | "violation"
  conflictEntryIds: string[]
}

// {
//    foo: { value: "abc", status: "not started" }
// }
const [uniqueFieldStatus, setUniqueFieldStatus] = setState<{ [fieldId: string]: UniqueFieldStatus>({});
...
onValuesChanged(values) {
  setUniqueFieldStatuses(prevStatuses => {
    const copy = { ...prevStatuses };
    values.forEach(([fieldId, value]) => {
      if (copy[fieldId].value === value) {
        // do nothing, because we already have the same value
      } else  {
        copy[fieldId] = value;
      }
    })
    return copy;
  });
}

checkUniqueness() {
  const checkedNotValid = await api.checkUnique(row.id, opts);
  setUniqueFieldStatus(prevStatus => {
    // left as reader's exercise
  });
}

} catch (err) {
console.log(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove try/catch before merging

}
}

async function performSave() {
const { identifier, ...draftData } = form.getFieldsValue();
try {
setSaving(true);
if (isUniqueFieldChanged()) {
await checkUniqueness();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think checkUniqueness should be in performSave. Rather, you should implement checkUniqueness as a parallel process.

}
await api.updateCmsRow(row.id, {
identifier,
draftData,
Expand All @@ -319,7 +359,6 @@ function CmsEntryDetailsForm_(
await form.validateFields();
return true;
} catch (err) {
console.error("Validation failed:", err);
return !(err.errorFields?.length > 0);
}
};
Expand Down Expand Up @@ -352,6 +391,14 @@ function CmsEntryDetailsForm_(
spawn(validateFields());
}, [row, validateFields]);

React.useEffect(() => {
setUniqueFieldsIdentifier(
table.schema.fields
.filter((field) => field.unique)
.map((field) => field.identifier)
);
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

uniqueFields =  table.schema.fields
        .filter((field) => field.unique)
        .map((field) => field.identifier)

OR

const uniqueFields = useMemo(() => {
  return table.schema.fields
        .filter((field) => field.unique)
        .map((field) => field.identifier)
}, [table]);


useBeforeUnload(() => {
return hasChanges();
}, "You have unsaved changes, are you sure?");
Expand Down Expand Up @@ -405,11 +452,18 @@ function CmsEntryDetailsForm_(
}}
labelCol={{ span: 8 }}
wrapperCol={{ span: 16 }}
onValuesChange={(changedValues, allValues) => {
onValuesChange={(changedValues: Dict<Dict<unknown>>, allValues) => {
if (Object.keys(changedValues).length > 0) {
setHasUnsavedChanges(hasChanges());
setHasUnpublishedChanges(hasPublishableChanges());
console.log({ changedFields: changedValues, allFields: allValues });
const changedField = Object.values(changedValues)[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't assume there is only 1 element in the changed values

if (uniqueFieldsIdentifier.includes(Object.keys(changedField)[0])) {
setUniqueChangedFields({
...uniqueChangedFields,
...changedField,
});
}
}
}}
className={"max-scrollable fill-width"}
Expand Down Expand Up @@ -476,35 +530,43 @@ function CmsEntryDetailsForm_(
setPublishing(true);
const { identifier, ...draftData } =
form.getFieldsValue();
await api.updateCmsRow(row.id, {
identifier,
data: draftData,
draftData: null,
revision,
});
await mutateRow();
setPublishing(false);
setHasUnpublishedChanges(false);
await message.success({
content: "Your changes have been published.",
duration: 5,
});
const hooks = table.settings?.webhooks?.filter(
(hook) => hook.event === "publish"
);
if (hooks && hooks.length > 0) {
const hooksResp = await api.triggerCmsTableWebhooks(
table.id,
"publish"
);
const failed = hooksResp.responses.filter(
(r) => r.status !== 200
try {
await api.updateCmsRow(row.id, {
identifier,
data: draftData,
draftData: null,
revision,
});
await mutateRow();
setPublishing(false);
setHasUnpublishedChanges(false);
await message.success({
content: "Your changes have been published.",
duration: 5,
});
const hooks = table.settings?.webhooks?.filter(
(hook) => hook.event === "publish"
);
if (failed.length > 0) {
await message.warning({
content: "Some publish hooks failed.",
duration: 5,
});
if (hooks && hooks.length > 0) {
const hooksResp = await api.triggerCmsTableWebhooks(
table.id,
"publish"
);
const failed = hooksResp.responses.filter(
(r) => r.status !== 200
);
if (failed.length > 0) {
await message.warning({
content: "Some publish hooks failed.",
duration: 5,
});
}
}
} catch (err) {
setPublishing(false);
console.log(err);
if (err.statusCode === 400) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use 409

The HTTP 409 Conflict status code indicates that a request could not be completed due to a conflict with the current state of the target resource, often occurring during operations like file uploads or updates. This error typically requires resolving the conflict before the request can be successfully processed.

setNotVaildUniqueFields(JSON.parse(err.message));
Copy link
Contributor

Choose a reason for hiding this comment

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

Before assuming that the error is about uniqueness, you need a way to guarantee that it is a uniqueness issue, because we might have other kinds of errors, such as auth error, etc.

Make a new error type in ApiSchema.

export type UniqueViolationError = {
  type: "unique-violation",
  violations: UniqueViolation[],
}

// this is called a typeguard
export function isUniqueViolationError(err: unknown): err is UniqueViolationError {
  return typeof err === "object" && err && err.type === "unique-violation"
}

catch (err) {
  if (isUniqueViolationError(err)) {
    setViolations(...)
  } else {
   throw err;
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the backend throw this UniqueViolationError type object, but on client side, the error is always UnknownApiErrors. And I found this comment in api.ts :
// The error was JSON-parsible, but not one of the known
// ApiErrors. So it is now just a JSON object, not an Error.
// We create an UnknownApiError for it instead.
Should I define a new error class extending ApiErrors, as AuthError is defined (in shared/ApiErrors/errors.ts)?

}
}
}
Expand All @@ -514,7 +576,8 @@ function CmsEntryDetailsForm_(
isPublishing ||
isSaving ||
hasUnsavedChanges ||
hasFormError()
hasFormError() ||
isUniqueFieldChanged()
}
tooltip={
hasFormError()
Expand Down Expand Up @@ -640,7 +703,8 @@ function CmsEntryDetailsForm_(
table!,
database,
database.extraData.locales,
inConflict
inConflict,
notValidUniqueFields
)}
</div>
}
Expand Down
47 changes: 43 additions & 4 deletions platform/wab/src/wab/client/components/cms/CmsInputs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,14 @@ const FormNameContext = createContext<
{ name: NamePathz; label: ReactNode } | undefined
>(undefined);

export class FormValidationError extends Error {
type: string;
constructor(message: string, type: string) {
super(message);
this.type = type;
}
}

function MaybeFormItem({
typeName,
name,
Expand All @@ -245,9 +253,30 @@ function MaybeFormItem({
name: NamePathz;
maxChars?: number;
minChars?: number;
// unique?: boolean;
uniqueNotValid?: boolean;
}) {
type FieldStatus = "success" | "warning" | "error" | "validating" | undefined;
const [fieldStatus, setFieldStatus] = React.useState<FieldStatus>("success");
const [helperText, setHelperText] = React.useState(" ");
const commonRules = [
Comment on lines +259 to 262
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to get rid of these states and use the rules framework. Here's the docs: https://ant.design/components/form#rule

{ required: props.required, message: "Field is required" },
{
validator: async (_, value) => {
if (props.required && value.length === 0) {
setFieldStatus("error");
setHelperText("Field is required");
return Promise.reject();
}
if (props.uniqueNotValid) {
setFieldStatus("warning");
setHelperText("This field should be unique to publish this entry");
return Promise.resolve();
}
setFieldStatus("success");
setHelperText("");
return Promise.resolve();
},
},
];
const typeSpecificRules =
[CmsMetaType.TEXT, CmsMetaType.RICH_TEXT].includes(typeName) &&
Expand All @@ -256,13 +285,19 @@ function MaybeFormItem({
: [];

const rules = [...commonRules, ...typeSpecificRules];

return typeName === CmsMetaType.LIST ? (
<FormNameContext.Provider value={{ name, label }}>
{props.children as any}
</FormNameContext.Provider>
) : (
<Form.Item name={name} label={label} {...props} rules={rules} />
<Form.Item
name={name}
label={label}
{...props}
rules={rules}
help={helperText}
validateStatus={fieldStatus}
/>
);
}

Expand All @@ -276,7 +311,6 @@ export function CmsObjectInput(props: any) {
} = useContentEntryFormContext();
assert(typeMeta.type === CmsMetaType.OBJECT, "Must be rendering an object");
const form = Form.useFormInstance();

return (
<div
style={
Expand Down Expand Up @@ -572,6 +606,8 @@ interface MaybeLocalizedInputProps {
fieldPathSuffix: string[];
formItemProps: FormItemProps;
typeName: CmsTypeName;
// unique: boolean;
uniqueNotValid: boolean;
}

export function renderMaybeLocalizedInput({
Expand All @@ -584,6 +620,7 @@ export function renderMaybeLocalizedInput({
formItemProps,
typeName,
required,
uniqueNotValid,
}: MaybeLocalizedInputProps) {
return (
<ContentEntryFormContext.Consumer>
Expand All @@ -608,6 +645,7 @@ export function renderMaybeLocalizedInput({
minChars={minChars}
required={required}
typeName={typeName}
uniqueNotValid={uniqueNotValid}
{...formItemProps}
name={[...fieldPath, "", ...fieldPathSuffix]}
>
Expand Down Expand Up @@ -643,6 +681,7 @@ export function renderMaybeLocalizedInput({
maxChars={maxChars}
minChars={minChars}
required={required}
uniqueNotValid={uniqueNotValid}
typeName={typeName}
name={[...fieldPath, locale, ...fieldPathSuffix]}
noStyle
Expand Down
6 changes: 6 additions & 0 deletions platform/wab/src/wab/client/components/cms/CmsModelDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,11 @@ function renderModelFieldForm(
<ValueSwitch />
</Form.Item>
)}
{![CmsMetaType.LIST, CmsMetaType.OBJECT].includes(selectedType) && (
<Form.Item label={"Unique"} name={[...fieldPath, "unique"]} required>
<ValueSwitch />
</Form.Item>
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you check the unique switch, then change the type to a list or object?

Similar for localized in the future.

)}
<Form.Item
label={"Helper text"}
name={[...fieldPath, "helperText"]}
Expand Down Expand Up @@ -647,6 +652,7 @@ function ModelFields({
helperText: "",
required: false,
hidden: false,
unique: false,
type: CmsMetaType.TEXT,
defaultValueByLocale: {},
})
Expand Down
2 changes: 2 additions & 0 deletions platform/wab/src/wab/server/AppServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import {
upsertDatabaseTables,
} from "@/wab/server/routes/cms";
import {
checkUniqueness,
cloneDatabase,
cloneRow,
cmsFileUpload,
Expand Down Expand Up @@ -828,6 +829,7 @@ export function addCmsEditorRoutes(app: express.Application) {
app.put("/api/v1/cmse/rows/:rowId", withNext(updateRow));
app.delete("/api/v1/cmse/rows/:rowId", withNext(deleteRow));
app.post("/api/v1/cmse/rows/:rowId/clone", withNext(cloneRow));
app.post("/api/v1/cmse/rows/:rowId/uniqueness", withNext(checkUniqueness));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call it /check-unique-fields

app.get("/api/v1/cmse/row-revisions/:revId", withNext(getRowRevision));

app.post(
Expand Down
Loading
Loading