fix(core): apply or reject field type changes on update instead of dropping them (#1397)#1556
Conversation
…opping them (emdash-cms#1397) SchemaRegistry.updateField built its SET clause without type/column_type, and UpdateFieldInput had no `type` field, so `emdash seed --on-conflict update` (seed/apply.ts) couldn't express a field-type change. Re-seeding after changing a field's type reported success and bumped fields.updated while _emdash_fields.type/column_type kept their old values — stale typegen with no warning. Add `type` to UpdateFieldInput and write type/column_type in updateField. A type change that keeps the same column affinity (string -> slug, both TEXT) is applied; one that would change the column type (text -> portableText, TEXT -> JSON) is rejected with a FIELD_TYPE_COLUMN_CHANGE SchemaError, since no in-place column migration exists and rewriting only the metadata would desync column_type from the real ec_* column. The seed update branch now passes the field type through. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: c91fc32 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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 |
github-actions
Bot
added
area/core
size/M
review/needs-review
|
Could not push formatting changes to this fork. The contributor may have "Allow edits by maintainers" disabled. Please run the formatter locally: |
Collapse multi-line .innerJoin() calls onto single lines per the repo's oxfmt config. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
![emdashbot[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound and targets the right problem: SchemaRegistry.updateField previously ignored field-type changes, so emdash seed --on-conflict update silently dropped them. Adding type to UpdateFieldInput, applying it only when the SQLite column affinity is unchanged, and rejecting affinity-changing updates with a clear FIELD_TYPE_COLUMN_CHANGE error is the right fix for #1397 and avoids corrupting the metadata/column mapping.
I checked the diff, the full registry implementation, the API schema/route layer, seed validation/apply, the error-code catalog, and the new tests. The registry and seed layers are correctly implemented and well-tested for both the success and rejection paths. However, the HTTP API contract was not kept in sync: the Zod body schema for PUT /_emdash/api/schema/collections/{slug}/fields/{fieldSlug} still omits type, so the new capability is unreachable from the admin UI or any API caller. That is a blocking inconsistency introduced by the PR.
Other than that, the only notable gap is a minor catalog omission in ErrorCode. Once the API schema accepts type, this should be good to merge.
Findings
-
[needs fixing]
packages/core/src/api/schemas/schema.ts:126-137The
updateFieldBodyschema does not includetype, so Zod strips it from the request body before the route casts the validated body toUpdateFieldInputand passes it tohandleSchemaFieldUpdate. Even thoughUpdateFieldInputandSchemaRegistry.updateFieldnow support type changes, API callers (and the admin UI) cannot actually use them.Add
typeas an optional field-type value:export const updateFieldBody = z .object({ label: z.string().min(1).optional(), type: fieldTypeValues.optional(), required: z.boolean().optional(), unique: z.boolean().optional(), defaultValue: z.unknown().optional(), validation: fieldValidation.nullable(), widget: z.string().optional(), options: fieldWidgetOptions, sortOrder: z.number().int().min(0).optional(), searchable: z.boolean().optional(), translatable: z.boolean().optional(), }) .meta({ id: "UpdateFieldBody" });Please also add a unit test in
packages/core/tests/unit/api/schemas.test.tsasserting thatupdateFieldBody.parsepreserves a validtypevalue. -
[suggestion]
packages/core/src/api/errors.ts:63FIELD_TYPE_COLUMN_CHANGEis returned fromSchemaRegistry.updateFieldbut is not listed in theErrorCodeconstant alongside the other schema codes. Adding it keeps the catalog complete;mapErrorStatusalready falls through to400for it, so no mapping change is required unless you want to make that explicit.
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
github-actions
Bot
added
review/awaiting-author
|
@marcusbellamyshaw-cell can you look at the review feedback? |
|
/review |
![ask-bonk[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core fix is correct and well-tested at the registry and seed layers. However, the previous review (emdashbot, 2026-06-24) flagged a blocking inconsistency that has not been addressed in the two later commits: the PUT /_emdash/api/schema/collections/{slug}/fields/{fieldSlug} Zod body schema still omits type, so the new capability is unreachable from the admin UI and any other HTTP caller. Re-raising that finding plus one minor observation.
Findings
[blocking] packages/core/src/api/schemas/schema.ts:125-138 — updateFieldBody does not accept type.
Zod's default .object() strips unknown keys before the route's as UpdateFieldInput cast in packages/core/src/astro/routes/api/schema/collections/[slug]/fields/[fieldSlug].ts:58. So even though SchemaRegistry.updateField now supports a type change, every HTTP caller — including the admin UI's updateField client in packages/admin/src/lib/api/schema.ts:253, which already serializes the full input — has its type silently stripped before it reaches handleSchemaFieldUpdate. The fix in this PR therefore only works via direct registry/seed callers; the public API surface still has the original #1397 bug.
Suggested change:
export const updateFieldBody = z
.object({
label: z.string().min(1).optional(),
type: fieldTypeValues.optional(),
required: z.boolean().optional(),
// ...rest unchanged
})
.meta({ id: "UpdateFieldBody" });Please also add a unit test in packages/core/tests/unit/api/schemas.test.ts confirming updateFieldBody.parse({ type: "slug" }) preserves type, and ideally an integration test that PUTs { type: "slug" } through the HTTP route and reads back the persisted _emdash_fields.type. Without an end-to-end test exercising the route, future regressions in the API layer will keep hiding under registry-level coverage.
[minor] packages/core/src/api/errors.ts:63 — FIELD_TYPE_COLUMN_CHANGE missing from the ErrorCode catalog.
FIELD_TYPE_COLUMN_CHANGE is thrown by SchemaRegistry.updateField (registry.ts line 549) but isn't listed in the ErrorCode constant alongside the other schema codes. mapErrorStatus's default branch already returns 400 for non-_ERROR codes so HTTP behavior is correct, but adding it here keeps the catalog complete and discoverable for admin-side message mapping.
| } | ||
| nextType = input.type; | ||
| nextColumnType = newColumnType; | ||
| } |
There was a problem hiding this comment.
Optional / minor: when input.type === field.type (no-op type write — what the existing tests for unrelated updates exercise), the new code falls through to writing type/column_type back to their existing values. That's harmless but means every updateField call — including ones that only flip required or widget — now writes two more columns than before. If you want to preserve the previous SET-clause shape for the common case, you can guard the new columns inside if (input.type !== undefined).
Not blocking; flagging only because the PR description claims a no-op write of the existing value, which is accurate but worth confirming is intentional.
|
Review posted successfully. Summary of what I posted:
|
…pe changes reach the registry The updateFieldBody Zod schema omitted `type`, so it was stripped from the request body before the route cast to UpdateFieldInput — making the new field-type-change capability unreachable from the admin UI and any HTTP caller. Add `type` to the schema, add FIELD_TYPE_COLUMN_CHANGE to the ErrorCode catalog, and cover type preservation in the schemas unit test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
review/needs-rereview
|
Addressed both findings in 88a9484:
Targeted suites green ( |
|
@ascorbic — unrelated to this PR, a quick logistics ask while I have you. I'm capped at 10 open PRs against the repo, and I've been at the cap for about a week, so I can't promote my draft (#1588) to a real PR or open any new bug fixes until some of the current ones land. They're all small, single-issue fixes against tracked issues (#1397, #1480, #1178, #1370, #1416, #1368, #1415, #1551, #1463, plus the #1588 draft for the Workers afterDelete/afterUnpublish hook cancellation). Would it be possible to raise my open-PR limit a bit, or is there anything I can do on my end to help get the queue moving? Happy to rebase, split, or reprioritize whichever are easiest to review first. Thanks! |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@marcusbellamyshaw-cell the limit is to keep it manageable for me to review the PRs. When there's a huge number of PRs it makes it hard to review them and to keep them deconflicted. I'm the only person reviewing PRs right now, and I am reviewing them as quickly as I can. The best thing you can do to get them merged quickly is to respond to review comments from me or bots. Thanks for your patience. |
|
Will do 👍 |
2 participants
What does this PR do?
SchemaRegistry.updateFieldbuilt itsSETclause withouttype/column_type, andUpdateFieldInputhad notypefield, soemdash seed --on-conflict update(and the seed update branch inseed/apply.ts) couldn't express a field-type change. Re-seeding after changing a field's type inseed.jsonreported success and bumpedfields.updated, while_emdash_fields.type/column_typekept their old values — typegen then regenerated stale TS types with no warning.This adds
typetoUpdateFieldInputand writestype/column_typeinupdateField:string→slug, bothTEXT) is applied.text→portableText,TEXT→JSON) is rejected with aFIELD_TYPE_COLUMN_CHANGESchemaError, because there is no in-place column migration and silently rewriting only the metadata would desynccolumn_typefrom the realec_*column.The seed update branch now passes the field
typethrough, so a re-seed either applies the change or fails loudly instead of silently dropping it. Backwards-compatible: callers that don't passtypeget a no-op write of the existing value.Closes #1397
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (targeted:schema/registry.test.ts+seed-on-conflict+ seed/schema suites)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
New tests cover both layers:
updateFieldapplies a same-affinity change and rejects an affinity-changing one;applySeed --on-conflict updatepersists a same-affinity change and rejects an affinity-changing one instead of silently reporting success. Schema + seed suites green (248 + 85 tests).