Skip to content

fix(push-publish): apply field data-type change on the receiver (#36636) - #36746

Open
nollymar wants to merge 2 commits into
mainfrom
worktree-issue-36636-field-datatype-change
Open

fix(push-publish): apply field data-type change on the receiver (#36636)#36746
nollymar wants to merge 2 commits into
mainfrom
worktree-issue-36636-field-datatype-change

Conversation

@nollymar

@nollymar nollymar commented Jul 27, 2026

Copy link
Copy Markdown
Member

Summary

Fixes #36636.

DeterministicIdentifierAPIImpl.resolveName now includes the field's dataType in the id seed (variable : typeName : dataType.value). When a Content Type field is deleted on the sender and immediately re-created with the same velocity variable but a different data type, the receiver's ContentTypeAPIImpl.transactionalSave now sees the incoming field as a different id from the old one and routes it through delete-then-insert — instead of update, which would overwrite the incoming dataType/dbColumn with the existing field's values in FieldFactoryImpl.dbSaveUpdate.

Test plan

  • DeterministicIdentifierAPITest — added coverage for the new dataType-in-seed input.
  • Manual push-publish reproduction per issue Push publish silently discards a field's data-type change when a field is deleted and re-added with the same variable name #36636 (Postgres, JSON storage):
    • Create a Content Type with a Text field myField on the sender and content records carrying values for it.
    • Delete myField and re-create it with a different data type. Set a new value on the content.
    • Push publish the Content Type to the receiver.
    • Verify the receiver's field is saved with the new data type (previously it kept the sender's original — this PR fixes that).
    • Push publish the affected content records to the receiver (a separate step — a Content Type push does not enqueue that type's content records).
    • Verify the receiver's content shows the new value.

Data impact (unchanged from the issue's analysis)

Changing a field's data type is inherently destructive for that field's stored content:

  • The recreated field of a different type gets a new empty column via FieldFactoryImpl.nextAvailableColumn, so existing values in the old column are not migrated.
  • On the sender, CleanUpFieldReferencesJob wipes the old column and the contentlet_as_json entry for that variable when the field is deleted, so the sender loses the pre-existing values at that moment.
  • On the receiver, a Content Type push applies the new field but does not carry content — the receiver's content records keep their pre-change state (missing the new-type value) until the content itself is pushed. A follow-up content push is required to reflect the new values on the receiver.

This PR does not attempt to migrate values; it just ensures the data-type change is applied on the receiver instead of silently dropped.

🤖 Generated with Claude Code

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @fabrizzio-dotCMS's task in 4m 36s —— View job


Code Review — PR #36746

  • Read the diff (DeterministicIdentifierAPIImpl + test)
  • Trace blast radius of the seed change through FieldFactoryImpl / push-publish
  • Assess test coverage

New Issues

No blocking issues found. The change is small, well-scoped, and backed by meaningful tests.

Notes (non-blocking):

  • 🟡 Medium: DeterministicIdentifierAPIImpl.java:329-332 — The seed change is global to all field id generation, not just the delete-recreate scenario. This is safe in practice because a field's deterministic id is only computed when throwAwayField.id() == null (FieldFactoryImpl.java:339), and push-publish bundles carry existing fields with their already-stored ids — so pre-existing fields are matched by id and never re-hashed on the receiver. Worth confirming you're comfortable that any newly created field on an upgraded node now hashes differently than the same field created on a pre-upgrade node (mixed-version cluster edge case). Both sides on the same version stay consistent, so normal push-publish is unaffected.

  • 🟡 Medium: DeterministicIdentifierAPIImpl.java:329field.dataType() also returns non-null values for non-data fields (SYSTEM for constant/binary/divider/tab per DataTypes.getDataType, NONE for others), so their seeds now include system_field/none. Harmless — it just changes the hash consistently on both sides — but the id for these field types will differ from ids generated by the old code. Same mixed-version caveat as above; no action needed if a coordinated upgrade is assumed.

Observations

  • The null-dataType fallback to the legacy variable:typeName seed (line 330-332) is a good touch — it keeps ids stable for any field whose dataType() is null and is covered by the noDataTypeField mock assertion at DeterministicIdentifierAPITest.java:852-859.
  • Test coverage is solid: Test_ResolveName_Seed_Includes_DataType_When_Present (unit-level seed), Test_Delete_And_Recreate_Field_With_Different_DataType_Generates_New_Id (id divergence), and Test_ContentType_Save_Applies_DataType_Change_When_Field_Id_Differs (the actual receiver flow — asserts the old field is dropped and the new data type is applied). These map directly to the failure mode in Push publish silently discards a field's data-type change when a field is deleted and re-added with the same variable name #36636.
  • The existingField update path in FieldFactoryImpl.dbSaveUpdate (lines 315-316) still force-copies the old dataType/dbColumn; this PR correctly sidesteps that by making the incoming field resolve to a different id so it takes the insert path rather than update. That reasoning holds.

Nothing here blocks merge.

worktree-issue-36636-field-datatype-change

@github-actions github-actions Bot added the Area : Backend PR changes Java/Maven backend code label Jul 27, 2026
@nollymar
nollymar force-pushed the worktree-issue-36636-field-datatype-change branch from 10740da to cf9be88 Compare July 27, 2026 18:37
@nollymar
nollymar force-pushed the worktree-issue-36636-field-datatype-change branch from cf9be88 to 2a24c8a Compare July 27, 2026 18:49
When a Content Type field is deleted on the sending instance and
immediately re-created with the same velocity variable but a different
data type (e.g. Text → Whole Number), push publishing silently discarded
the change on the receiver.

The receiver's ContentTypeAPIImpl.transactionalSave diffs incoming
fields against existing ones by id. But
DeterministicIdentifierAPIImpl.resolveName was seeding the field id
with variable + typeName only, so a re-created field kept the same id
as the old one and was routed through the update branch of
FieldFactoryImpl.dbSaveUpdate — which overwrites the incoming
dataType/dbColumn with the existing field's values.

Include the field's dataType in the deterministic id seed so a
same-variable, different-dataType field gets a different id and is
routed through delete-then-insert instead.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@nollymar
nollymar force-pushed the worktree-issue-36636-field-datatype-change branch from 2a24c8a to 9cfca2b Compare July 27, 2026 20:55
@nollymar nollymar changed the title fix(push-publish): apply field data-type change on the receiver + preserve value (#36636) fix(push-publish): apply field data-type change on the receiver (#36636) Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Push publish silently discards a field's data-type change when a field is deleted and re-added with the same variable name

2 participants