Repository navigation
fix(core): save a blank url, datetime or number value as null - #3790
DiogoDuart3 wants to merge 1 commit into
Conversation
Once an optional url or datetime field had been filled in and then cleared, the admin sent "" for it. The content schema accepts null or a valid value for these types, so every save and autosave of the entry failed with "<field> must be a valid URL." or "has an invalid value", and the editor could not recover without the field being refilled. The pre-validation normalizer that already turns a blank string into null for array-valued fields now does the same for url, datetime, number and integer fields. None of these types has a meaningful blank string, so this covers every client (admin, REST, MCP) and repairs an entry that already stores "" on its next save. Required fields still reject the value, now reported only as `required` instead of also as an invalid URL. String, text and slug fields keep accepting "". Claude-Session: https://claude.ai/code/session_011SKvTndrtRUPdctoaRyUw9
🦋 Changeset detectedLatest commit: ed5c63f The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
The two red checks here are not from the change: "EmDashBot review" stopped with "model over capacity after 3 attempt(s)", and "label" failed removing a label that did not exist. The test suite has not run on this PR yet. Could a maintainer reapply the |
There was a problem hiding this comment.
This is the right fix in the right place. Clearing an optional URL, datetime, or numeric field leaves the client with no meaningful way to express "empty" other than "", and the runtime’s existing normalizeFieldValues step is exactly where that translation already happens for array-valued fields. Expanding the same blank-to-null normalization to url, datetime, number, and integer is consistent, keeps validation rules unchanged, and applies to every write path because it sits inside the shared handleContentCreate / handleContentUpdate class methods (MCP, REST admin, and direct runtime calls all flow through those).
What I checked:
- Traced
normalizeFieldValuescall sites inhandleContentCreateandhandleContentUpdate; all write paths hit it before validation. - Verified that
zod-generator.tsuses.nullish()for non-required fields, sonullround-trips cleanly after normalization. - Confirmed that required fields still produce a single
requiredissue because the empty-string is turned intonullbefore the required-field pass runs. - Verified
BLANK_AS_NULL_FIELD_TYPEShas no leftover references after the rename. - Review SQL in the new tests: uses
sql.ref()and parameterized values, consistent with AGENTS.md. - Did not run the test suite; my review is static.
Headline issue: the renamed constant still carries its old doc comment, which is now false. I also flagged that the method docstring omits integer, and that a few test assertions assume item.data returns explicit null for cleared fields, while ContentRepository.mapRow strips null values from data. If the tests pass as reported, that serialization detail must be guaranteed elsewhere, but I’d tighten the assertions so they don’t depend on that invariant.
| @@ -304,7 +304,15 @@ const DRAFT_ONLY_UPDATE_KEYS = new Set([ | |||
| ]); | |||
|
|
|||
| /** Field types whose schema is an array, so a stored blank string can never validate. */ | |||
There was a problem hiding this comment.
[needs fixing] The doc comment was left unchanged when ARRAY_FIELD_TYPES was renamed and expanded to BLANK_AS_NULL_FIELD_TYPES. The set now contains non-array scalar types, so the comment is factually wrong and will mislead future readers.
| /** Field types whose schema is an array, so a stored blank string can never validate. */ | |
| /** Field types whose schema has no meaningful blank string, so blank input is normalized to null before validation. */ |
| * Normalize field values in content data before validation. | ||
| * Turns a blank string in an array-valued field into `null`, and fills | ||
| * Turns a blank string into `null` in a field whose type has no blank value | ||
| * (array, url, datetime and number fields), and fills | ||
| * missing image/file dimensions, storageKey, mimeType, and filename from providers. |
There was a problem hiding this comment.
[suggestion] The docstring lists the affected types as “array, url, datetime and number fields” but omits integer, which is also in BLANK_AS_NULL_FIELD_TYPES. Update the parenthetical to be complete.
| * Normalize field values in content data before validation. | |
| * Turns a blank string in an array-valued field into `null`, and fills | |
| * Turns a blank string into `null` in a field whose type has no blank value | |
| * (array, url, datetime and number fields), and fills | |
| * missing image/file dimensions, storageKey, mimeType, and filename from providers. | |
| * (array, url, datetime, number and integer fields), and fills |
| expect(updated.data.item.data.website).toBeNull(); | ||
| expect(updated.data.item.data.starts_at).toBeNull(); |
There was a problem hiding this comment.
[suggestion] These assertions assume item.data returns an explicit null for a cleared scalar field. ContentRepository.mapRow intentionally excludes null values from data, so in the normal case data.website/data.starts_at will be undefined, which would make toBeNull() fail. If the tests pass currently, the response shape is preserving nulls through some other path I did not trace; to avoid coupling the test to that, consider asserting directly against the database (as the first case does) or using a looser check such as expect(value).not.toBe("") / expect(value).toBeFalsy().
|
|
||
| expect(updated.success).toBe(true); | ||
| if (!updated.success) return; | ||
| expect(updated.data.item.data.website).toBeNull(); |
There was a problem hiding this comment.
[suggestion] Same shape concern as lines 72–73: this relies on item.data.website being returned as explicit null. Consider asserting against the database row or using a check that does not depend on whether nulls are kept in the serialized data object.
|
Friendly ping: this still merges cleanly into |
What does this PR do?
If an editor fills in an optional
urlordatetimefield and then clears it, the admin sends""for that field. Validation accepts onlynullor a valid value for these types, so every save and autosave of the entry then fails. The toast reads "Website must be a valid URL." for a url field and "Starts at has an invalid value." for a datetime field. The entry cannot be saved again until the field is refilled.#3354 added a blank-to-
nullstep forportableText,multiSelectandrepeaterinnormalizeFieldValues, which runs before validation on create and update. That PR namedurl,datetime,numberandintegeras uncovered, and this PR adds them to the same step. None of these types has a meaningful blank string. The fix applies to every client (admin, REST, MCP), and an entry that already stores""is repaired on its next save.required, where before a required url field reported bothinvalid_formatandrequired.string,textandslugfields still store"".selectis left out. fix(core): save a blank rich text, multi-select or repeater value as null #3354 listed it too, but aselectwithout options validates as a plain string, where""is a real value, so turning it intonullwould change stored data.Reproduction on a fresh
demos/simple:/_emdash/api/setup/dev-bypass?redirect=/_emdash/admin.eventswith optional fieldswebsite(URL) andstarts_at(Date/time).invalid_formatand the toast says "Website must be a valid URL."invalid_union.With this change, both saves return 200 and the fields are stored as
null.No existing issue.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
Not applicable (no UI change).
The new test is
packages/core/tests/integration/content/blank-scalar-field.test.ts, which runs on SQLite and Postgres. Without the runtime change, 4 of its 5 cases fail. The fifth, which checks that a blank string field is kept, passes either way.min: 1) and integer values storesNULLin all four columns.null.""written directly into its url column is read back, re-sent as the admin does, and saved withnull.""is rejected with exactly onerequiredissue.Results:
packages/corefull suite, 759 files and 9155 tests passed.blank-array-field.test.tsrun withEMDASH_TEST_PGpassed (15 passed, 1 skipped by design).pnpm build,pnpm typecheck,pnpm lintandpnpm format:checkare clean.pnpm test:e2e field-widgets autosave content-crud form-data-loss: 28 passed. The full e2e suite was not run.Note: #3129 (open) edits
normalizeFieldValuestoo, so whichever PR merges second needs a small rebase.https://claude.ai/code/session_011SKvTndrtRUPdctoaRyUw9