chore: add safeguards when using setRawDoc #906
Conversation
packages/root-cms/core/client.ts
Outdated
| } | ||
| if (sys.publishingLocked.until !== undefined) { | ||
| const until = sys.publishingLocked.until; | ||
| if ( |
There was a problem hiding this comment.
this timestamp conversion logic seems to be duplicated here and above, maybe move the logic to a normalization function, e.g.
until: convertToTimetamp(until)
packages/root-cms/core/client.ts
Outdated
| } | ||
| // Otherwise, it's invalid. | ||
| else { | ||
| throw new Error( |
There was a problem hiding this comment.
this error might catch callers off guard, because when creating a new doc you should be able to save only the fields and the system should automatically set a few default sys values if it's not provided.
- if createdAt and modifiedAt are undefined, it should be set to the current time
- if createdBy and modifiedBy are undefined, set it to the current user (i don't think there's currently a way to figure out "who" the current user is, so maybe default it to
root-cms-clientif not specified via a argument option) - if locales is empty, set it to [<default locale> or 'en']
| * }, { mode: 'draft' }); | ||
| * ``` | ||
| */ | ||
| async setRawDoc( |
There was a problem hiding this comment.
one other validation that should be added to this is to ensure the slug/collection/id fields are correct in the updated doc.
there are cases where we might to something like:
setRawDoc('Pages', 'bar', getRawDoc('Pages', 'foo'))
in this case a user would be incorrectly setting the new doc to id = 'Pages/foo' which is incorrect
|
done - updated all the feedback. LMK if you'd prefer different defaults or different comments! |
packages/root-cms/core/client.ts
Outdated
|
|
||
| // Set default values for required timestamp fields if not provided. | ||
| const now = Timestamp.now(); | ||
| if (result.createdAt === undefined || result.createdAt === null) { |
There was a problem hiding this comment.
the === undefined || === null pattern is repeated a few times, i think from a readability perspective i'd prefer this wrapped in a helper function, e.g. i think closure library uses isDef() for this.
i'd also prefer to "read" this with the positive value first and defaulting after, e.g. "if defined, do this, otherwise default to some value".
if (isDef(result.createdAt)) {
// normalize value
} else {
// set default value
}
packages/root-cms/core/client.ts
Outdated
| if (!result.createdBy || typeof result.createdBy !== 'string') { | ||
| result.createdBy = 'root-cms-client'; | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: remove this newline so it feels like the comment above applies to both codeblocks.
|
done |
packages/root-cms/core/client.ts
Outdated
| ) { | ||
| // Ensure id, collection, and slug fields match the parameters to prevent data inconsistencies. | ||
| const expectedId = `${collectionId}/${slug}`; | ||
| if (data.id !== undefined && data.id !== expectedId) { |
There was a problem hiding this comment.
these if() statements are unnecessary, you can go ahead and override all of these values to ensure correctness.
packages/root-cms/core/client.ts
Outdated
|
|
||
| // Validate and normalize sys fields to prevent data integrity issues. | ||
| if (data.sys) { | ||
| data.sys = validateSysFields(data.sys); |
There was a problem hiding this comment.
in case the caller only passes the fields to setRawDoc() maybe we should go ahead and default to an empty object here, otherwise an error would be thrown in validateSysFields().
|
done! |
| } | ||
|
|
||
| // Ensure id, collection, and slug fields match the parameters to prevent data inconsistencies. | ||
| const expectedId = `${collectionId}/${slug}`; |
There was a problem hiding this comment.
for the slug can you pass it through a normalizer function (there might be one in here already), i.e. convert / to --?
packages/root-cms/core/client.ts
Outdated
|
|
||
| // Validate and normalize sys fields to prevent data integrity issues. | ||
| // Default to empty object if sys is not provided. | ||
| if (!data.sys) { |
There was a problem hiding this comment.
remove this if() statement and update to data.sys = validateSysFields(data.sys || {}) below
|
updated; and made a few updates elsewhere to use |
2 participants
I keep accidentally malforming timestamps when writing scripts that use
setRawDocwhich prevents the Root CMS UI from working. This adds some basic safeguards when usingsetRawDocto the structure/type of the Root CMS system fields to help avoid that.