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.

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 13 commits into
base: master
Choose a base branch
from

Conversation

hyunnbunt
Copy link
Contributor

[Unique constraints]
A field can be set to have unique constraint.
When saving an entry's draft data, the unique constraint violation is checked (an endpoint is added)
Upon a publish request, the unique constraint violation is checked for all unique fields. (publish endpoint handles this)

[Details]
The unique constraint only applies to the published entries. It does not affect to draft data of an entry. During auto-saving, the violation status of the most recent values entered by the user in the unique fields are checked. If a violation occurs, a message indicating that publishing is not possible is displayed in the input box, and the publish button is disabled. But Saving is not interrupted.
When a publish request is received, the server checks for violations of all unique fields in the requested entry. If a violation occurs, an error is returned, including a list of the violated fields in the error message. The client uses this information to display a message in the input box indicating that publishing is not possible for the violated fields and disables the publish button.

Copy link

vercel bot commented Feb 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
plasmic-cms-i18n ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 13, 2025 6:55am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
react-email-demo ⬜️ Ignored (Inspect) Mar 13, 2025 6:55am

Copy link

vercel bot commented Feb 24, 2025

@hyunnbunt is attempting to deploy a commit to the Plasmic Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@jaslong jaslong left a comment

Choose a reason for hiding this comment

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

Only reviewed the backend for now, will review frontend later

@@ -7303,11 +7367,9 @@ export class DbMgr implements MigrationDbMgr {
})
: undefined;

await this.entMgr.save(
return await this.entMgr.save(
Copy link
Member

Choose a reason for hiding this comment

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

What does save return here? It seems to be a different from previously, when it was just row.

async getPublishedRows(tableId: CmsTableId) {
const publishedRows = await this.entMgr.find(CmsRow, {
tableId: tableId,
draftData: null,
Copy link
Member

Choose a reason for hiding this comment

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

Can't a published row also have draft data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I misunderstood that modifying a published entry would immediately cancel the published status. So, when users modify the entry and request to publish it again, the previous data is overwritten! Got it and fixed it!

@@ -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
Member

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

@@ -1745,6 +1745,13 @@ export abstract class SharedApi {
return (await this.post(`/cmse/rows/${rowId}/clone`, opts)) as ApiCmseRow;
}

async checkUnique(
rowId: CmsRowId,
opts: { uniqueChangedFields: Dict<unknown> }
Copy link
Member

Choose a reason for hiding this comment

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

The naming for uniqueChangedFields is specific to the current use case (keyword being "changed"). Also, "fields" is usually used to refer to the schema. I think it's ok to call this data on the server, to be consistent with for example the publish API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for explaining it in detail! I've fixed it!

@@ -289,6 +289,17 @@ export async function cloneRow(req: Request, res: Response) {
res.json(clonedRow);
}

export async function checkUniqueness(req: Request, res: Response) {
const mgr = userDbMgr(req);
const row = await mgr.getCmsRowById(req.params.rowId as CmsRowId);
Copy link
Member

Choose a reason for hiding this comment

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

Why is row being fetched, but then only the id is being used later?

Copy link
Member

Choose a reason for hiding this comment

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

This should be removed. The client can pass both tableId and rowId.

},
});
}
const violated: string[] = [];
Copy link
Member

@jaslong jaslong Feb 25, 2025

Choose a reason for hiding this comment

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

Can we return something better than just the field identifier? How about data like:

interface UniqueFieldCheck {
  /** Identifier of schema field. */
  fieldIdentifier: string;
  /** The value that had a violation. */
  value: unknown;
  /** If there are no conflicts. */
  ok: boolean;
  /** Identifiers of rows that conflicted. */
  conflictRowIds: string[];
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Is it okay to add this interface to the same file(DbMgr.ts)?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can change this function to getConflictingCmsRows(data: { [fieldId]: unknown }) that returns CmsRow[]. Then in the routes file you can convert it to UniqueFieldCheck which should live in ApiSchema.

publishedRow.data &&
String(Object.values(publishedRow.data)[0][identifier] ?? "") ===
String(value ?? "")
) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the above lines? I don't understand:

  1. What Object.values(publishedRow.data)[0][identifier] ?? "" evaluates to.
  2. Why everything is being converted to a string before checking for equality.

It might help to break up this if into multiple if statements and use more variables.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Handles locales? Nah, can either be localized or unique, not both.
  2. Should use some sort of deep equality check (look at lodash.equals).
  3. Handle empty data during comparison. Make a normalizeData function to handle these kind of weird cases, then in the future we can apply this function to the rest of the code to make sure data integrity.

Comment on lines 7239 to 7240
Object.entries(fields).forEach(([identifier, value]) => {
for (const publishedRow of publishedRows) {
Copy link
Member

Choose a reason for hiding this comment

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

Slightly weird to use both foreach and for loop in the same function. It would be better to stick with one style. Generally we try to be functional, so I think it would be best to do something like:

return Object.entries(fields)
  .map(([identifier, value], () => {
    const conflictRowIds = publishedRows
      .filter(row => /* whether row conflicts with value */);
    if (conflictRowIds.length > 0) {
      return {
        identifier,
        value,
        conflictRowIds
      }
    }
  });

Comment on lines 259 to 258
type FieldStatus = "success" | "warning" | "error" | "validating" | undefined;
const [fieldStatus, setFieldStatus] = React.useState<FieldStatus>("success");
const [helperText, setHelperText] = React.useState(" ");
const commonRules = [
Copy link
Member

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

{![CmsMetaType.LIST, CmsMetaType.OBJECT].includes(selectedType) && (
<Form.Item label={"Unique"} name={[...fieldPath, "unique"]} required>
<ValueSwitch />
</Form.Item>
Copy link
Member

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.

.filter((field) => field.unique)
.map((field) => field.identifier)
);
}, []);
Copy link
Member

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]);

if (Object.keys(changedValues).length > 0) {
setHasUnsavedChanges(hasChanges());
setHasUnpublishedChanges(hasPublishableChanges());
console.log({ changedFields: changedValues, allFields: allValues });
const changedField = Object.values(changedValues)[0];
Copy link
Member

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

} catch (err) {
setPublishing(false);
console.log(err);
if (err.statusCode === 400) {
Copy link
Member

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.

setPublishing(false);
console.log(err);
if (err.statusCode === 400) {
setNotVaildUniqueFields(JSON.parse(err.message));
Copy link
Member

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)?

Copy link
Member

Choose a reason for hiding this comment

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

You need to update transformErrors to covert it from the generic error to your new error type. It's because when data is sent from server to client, the type information is lost. So you transformErrors will manually convert it back to the correct type.

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

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.

]);
setUniqueChangedFields({});
} catch (err) {
console.log(err);
Copy link
Member

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 checkUniqueness() {
try {
const opts = { uniqueChangedFields: uniqueChangedFields };
const checkedNotValid = await api.checkUnique(row.id, opts);
Copy link
Member

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);
  }
});

setNotVaildUniqueFields([
...new Set([...checkedValidRemoved, ...checkedNotValid]),
]);
setUniqueChangedFields({});
Copy link
Member

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
  });
}

Copy link
Member

@jaslong jaslong left a comment

Choose a reason for hiding this comment

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

backend review first!

name = "unique-violation";
statusCode = 409;
violations: UniqueFieldCheck[];
constructor(violations: UniqueFieldCheck[]) {
Copy link
Member

Choose a reason for hiding this comment

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

There is a shorthand for the following code:

readonly violations: UniqueFieldCheck[]
constructor(violations: UniqueFieldCheck[]) {
  this.violations = violations
}

can be shortened to

constructor(public readonly violations: UniqueFieldCheck[]) {}

Comment on lines 4 to 6
name = "unique-violation";
statusCode = 409;
violations: UniqueFieldCheck[];
Copy link
Member

Choose a reason for hiding this comment

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

These fields should be readonly unless you expect them to be changed

value: unknown;
/** If there are no conflicts. */
ok: boolean;
conflictRowIds: string[];
Copy link
Member

Choose a reason for hiding this comment

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

Use the type CmsRowId instead of string for a bit better type safety and clarity.

@@ -1710,6 +1710,10 @@ export abstract class SharedApi {
return (await this.post(`/cmse/rows/${rowId}/clone`, opts)) as ApiCmseRow;
}

async checkUnique(rowId: CmsRowId, opts: { data: Dict<unknown> }) {
return await this.post(`cmse/rows/${rowId}/check-unique-fields`, opts);
Copy link
Member

Choose a reason for hiding this comment

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

I think it may make more sense to make this a table operation, like cmse/tables${tableId}, since it involves the whole table. You'll still want to pass in the rowId in opts though.

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 was curious about this part, thanks!

@@ -289,6 +289,17 @@ export async function cloneRow(req: Request, res: Response) {
res.json(clonedRow);
}

export async function checkUniqueness(req: Request, res: Response) {
const mgr = userDbMgr(req);
const row = await mgr.getCmsRowById(req.params.rowId as CmsRowId);
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed. The client can pass both tableId and rowId.

.filter((field) => !field.hidden && field.unique)
.map((field) => field.identifier);
if (uniqueIdentifiers.length === 0) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

I think checkUniqueData always returns array? It might be better to return an empty array here for consistency. Then you can set the return type of this function to Promise<UniqueFieldCheck[]>.

BTW as a good rule of thumb, always prefer explicit return types. It makes it faster for future readers to understand the function purpose much faster.

@@ -7262,6 +7343,30 @@ export class DbMgr implements MigrationDbMgr {
);
};

if ("data" in opts && "draftData" in opts && opts.data && !opts.draftData) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this can simply be moved in the if ("data" in opts) block below. Also, I think it's safer to use the data after the mergedData call.

opts.data
);
if (!uniqueFields) {
console.log("There're no unique constraints in this table");
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the !uniqueFields check and console.log after you stop returning undefined

message: "unique violations",
statusCode: 409,
};
throw uniqueViolationError;
Copy link
Member

Choose a reason for hiding this comment

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

This should be throw new UniqueViolationError(uniqueFields)

const violationFields = uniqueFields.filter(
(uniqueField) => !uniqueField.ok
);
if (violationFields.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Could be simplified to uniqueFields.some(field => !field.ok)

Copy link
Member

@jaslong jaslong left a comment

Choose a reason for hiding this comment

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

frontend time :)

validator: (_, value) => {
if (props.uniqueViolation) {
return Promise.reject(
"This field should have unique data to publish entry"
Copy link
Member

Choose a reason for hiding this comment

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

Can we link to the conflicting rows somehow?

Comment on lines +158 to +159
uniqueFieldStatus[field.identifier] &&
uniqueFieldStatus[field.identifier].status === "violation",
Copy link
Member

Choose a reason for hiding this comment

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

Simplify to uniqueFieldStatus[field.identifier]?.status === "violation"

@@ -175,6 +187,7 @@ function CmsEntryDetailsForm_(

const [isSaving, setSaving] = React.useState(false);
const [isPublishing, setPublishing] = React.useState(false);
const [isCheckingUniqueness, setCheckingUniqueness] = React.useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

I think this state can be removed since it can be computed based on uniqueFieldStatus: Object.values(uniqueFieldStatus).some(field => field.status === "pending"). It should be similar to isUniqueFieldUpdated and isUniqueViolationField!

Comment on lines +264 to +278
const isUniqueFieldUpdated = () => {
return (
Object.values(uniqueFieldStatus).filter(
(uniqueStatus) => uniqueStatus.status === "not started"
).length > 0
);
};

const hasUniqueViolationField = () => {
return (
Object.values(uniqueFieldStatus).filter(
(fieldStatus) => fieldStatus.status === "violation"
).length > 0
);
};
Copy link
Member

Choose a reason for hiding this comment

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

These shouldn't be functions since they are always evaluated anyway, and if it's a function and you use it twice, they'll be evaluated twice too. Change these to simple const isUniqueFieldUpdated = Object.values(...)....

Or put them into a useMemo to guarantee they are only recomputed when necessary.

Also, I would move these computations next to uniqueFieldStatus since they are so directly related (or maybe move the uniqueFieldStatus down to here).

@@ -293,6 +326,72 @@ function CmsEntryDetailsForm_(
setInConflict(true);
};

function getDefaultLocale(data: Dict<Dict<unknown>>) {
Copy link
Member

Choose a reason for hiding this comment

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

This is generally useful, and also used on the server. Can you move it to a file like src/wab/shared/cms.ts? The CmsLocaleRowData type we talked about would probably also go there!

function initializeUniqueStatus() {
const uniqueStatus = {};
if (row.draftData) {
const uniqueEntries = getUniqueEntries(getDefaultLocale(row.draftData));
Copy link
Member

Choose a reason for hiding this comment

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

Since unique fields should only work on default locale, maybe you can change getUniqueEntries to accept row.draftData then getDefaultLocale inside the function. That way it's impossible to accidentally pass in a non-default locale data into getUniqueEntries.

uniqueEntries.forEach(([identifier, value]) => {
uniqueStatus[identifier] = {
value: value,
status: "not started",
Copy link
Member

Choose a reason for hiding this comment

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

Should we also check if the data equals the published data? If they are equal, then the status should be "ok".

If the user updates the field value before the response,
the status would changed to "not started", and the previous response is ignored. */
setUniqueFieldStatus((prev) => {
const copy = { ...prev };
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this is only a "shallow copy:"

const b = {}
const prev = { a: b }
const copy = { ...prev }
prev.a === copy.a // this is true

This means we are actually mutating the original data on line 368. This is bad in React and could lead to the component not updating properly. To resolve this, you need to copy the entire fieldStatus object too:

copy[fieldIdentifier] = { ...copy[fieldIdentifier], status: "pending" }

const copy = { ...prev };
uniqueFieldsChecked.forEach((uniqueCheck) => {
const identifier = uniqueCheck.fieldIdentifier;
if (copy[identifier].status === "pending") {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure checking that status === "pending" matters. The more important thing to check is that the field value still matches. Otherwise, maybe you run a check for "a", but then the user changes the field to "b". Then, the check for "a" finishes, and you accidentally mark "b" as "ok".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I thought when a user changes the field to "b" while running a check for "a", the status of the field is changed from "pending" to "not started", because 'onValuesChange' of the Form changes the field status to "not started".
I wanted to design it in this way :
The status of the field is "pending" at first, before the check. While the field is being checked, if the user changes the field, the field status is now "not started". After the response came, I check the status if it's still "pending", to ignore "not started" (updated) field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it more accurate to directly compare whether the field value is the same?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, consider this sequence:

  1. a: not started
  2. a: pending
  3. b: not started
  4. b: pending
  5. a: ok
  6. b: network error

Now the field is "b" while the value is "ok"

}
}
} catch (err) {
setPublishing(false);
Copy link
Member

Choose a reason for hiding this comment

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

Almost perfect try/catch. Nice!

One little thing is to move the setPublishing(false) to a finally block. Finally is always executed, no matter if the try or catch code is run. Put code there that should always happen no matter what.

try {
  ...
} catch {
  ...
} finally {
  setPublshing(false);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants