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

[nollymar]

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 finished @freddyDOTCMS's task in 5m 25s —— View job

---

PR Review

I reviewed the latest commit (9746983a) against @fabrizzio-dotCMS's prior review. It addresses all four findings. Details below.

Resolved

• ✅ DeterministicIdentifierAPIImpl.java:329 (prior finding 2) — the seed is now computed from the normalized field. FieldFactoryImpl.dbSaveUpdate builds normalizeData(builder.build()) before seeding (FieldFactoryImpl.java:342-344), so SYSTEM-only types (TagField, BinaryField, ConstantField, RowField, dividers…) seed from their persisted dataType=SYSTEM rather than the raw payload. id = f(normalize(field)) now holds. Covered by Test_TagField_DeterministicId_Is_Independent_Of_Incoming_DataType.
• ✅ DeterministicIdentifierAPIImpl.java:330 (prior finding 3) — the unreachable null != dataType branch is gone; resolveName now unconditionally uses field.dataType().value. Consistent with dataType() being a mandatory Immutables attribute.
• ✅ DeterministicIdentifierAPITest.java:790 (prior finding 4) — the transactionalSave test no longer injects a random UUIDGenerator id. It drives the incoming id through generateDeterministicIdBestEffort(...) and asserts it differs from the receiver's stored id, so it now exercises the seed. A new test (Test_CleanUpFieldJob_SkipsCleanup_WhenColumnRecycledBySameVariable) covers the risky same-variable / same-dataType / recycled-column case with a positive control (a genuine delete on a different variable still clears its column).
• ✅ DeterministicIdentifierAPIImpl.java:329 (prior finding 1 — silent data loss on column recycle) — mitigated at the point of harm rather than in the id space. CleanUpFieldReferencesJob.java:84-94 now skips cleanField when the deleted field's dbColumn is still held by a field with the same variable, so a re-seeded-but-identical field no longer has its content wiped. The job is enqueued via a post-commit listener (FieldAPIImpl.java:863 → triggerCleanUpJob), so type.fields() reads the committed post-save state and reliably sees the recycled column.

Notes (non-blocking)

• 🟡 CleanUpFieldReferencesJob.java:85-89 — the guard resolves finding 1 for the same-variable case, which is the reported bug. Finding 1's cross-version concern (sender on new build seeds variable:type:dataType, receiver on old build holds variable:type) is still not closed by a seed version/fallback — but the guard now prevents the data-loss outcome that made it severe, since that mismatch also lands on the same variable + recycled column. Cross-version push-publish remains out-of-contract; worth a note in the rollback-unsafe review given the id space still changes shape from this build forward. No code change needed here unless you want the explicit version-prefix + legacy-hash fallback.
• The !DataTypes.SYSTEM.value.equals(deletedColumn) clause correctly avoids false-positive skips for SYSTEM-column fields (which all share "system_field"), so the guard only fires for real numbered-column recycling.

No new issues introduced. The blocking findings from the prior review are addressed.

[fabrizzio-dotCMS]

Review — deterministic field-id seed change

Reviewed against eff265d15377f267d7af6771fe87fd54a8a824d0 (substantive commit 9cfca2bcff45; the newer commit is only a merge from main).

The PR appends dataType.value to the deterministic field-id seed in DeterministicIdentifierAPIImpl.resolveName. That changes the computed id of every field whose id is generated from this build onward, with no seed versioning and no fallback to the previous hash. Nothing here blocks merge, but finding 1 is worth an explicit decision before it goes in, and finding 2 is a correctness bug I'd fix in this PR.

---

1. MEDIUM — the new seed silently invalidates field ids generated before this change

dotCMS/src/main/java/com/dotmarketing/business/DeterministicIdentifierAPIImpl.java:329-332

Deterministic ids exist so two instances that independently create the same logical field converge on the same id. Adding dataType.value changes the hash, so the same logical field is A = hash(...:myfield:TextField) when its id was generated before this change and B = hash(...:myfield:TextField:text) after. Nothing maps B back to A.

Scope — this needs two environments on different builds. Existing fields keep their stored ids across an upgrade (FieldFactoryImpl:339 only generates when throwAwayField.id() == null), so a single instance is unaffected, and two instances on the same build agree. The exposure is cross-version push publish (or a content type created independently on both sides by starter / plugin / script, with the two creations straddling this change).

Where it goes wrong, with sender on the new build and receiver on the older one:

1. On the sender, myField (TextField / TEXT) is deleted and re-added identically — this used to be a no-op push, same id on both sides.
2. The sender now pushes B; the receiver still holds A. In ContentTypeAPIImpl.transactionalSave (~:1198) A is absent from newFields, so it calls fieldAPI.delete(oldField), which enqueues CleanUpFieldReferencesJob.
3. The data type did not change, so nextAvailableColumn hands the re-inserted field the same column (text1) the deleted one had.
4. The post-commit job then runs cleanField on it, clearing text1 plus contentlet_as_json #- '{fields,myField}' for every contentlet with mod_date <= deletionDate.

The receiver loses that field's stored content, and it fails quietly: the push reports success and the field is still there in the content type, just empty.

I'm flagging it rather than blocking on it because cross-version push publish is out-of-contract to begin with. Two things still make it worth a deliberate call:

• The failure mode is silent data loss on the receiver — typically delivery. Nobody notices until an editor opens the content.
• It isn't cleanly revertible. Any field whose id was generated while this is on the build already carries the new-recipe hash, so backing the change out doesn't restore the previous state. That shape of change belongs in the rollback-unsafe review (docs/core/ROLLBACK_UNSAFE_CATEGORIES.md) even if the risk is accepted.

If the seed change stays, a version prefix plus a legacy-hash fallback on lookup miss closes it. The alternative is to fix what #36636 actually describes — FieldFactoryImpl.dbSaveUpdate discarding the incoming dataType / dbColumn — which resolves the issue without touching the id space at all. I'd lean to the second, but that's a judgement call, not a blocker.

---

2. MEDIUM — the seed reads the pre-normalization dataType, so the id isn't a function of the field

DeterministicIdentifierAPIImpl.java:331, with dotCMS/src/main/java/com/dotcms/contenttype/business/FieldFactoryImpl.java:341 and :345

The ordering in dbSaveUpdate is:

:341   builder.id(APILocator.getDeterministicIdentifierAPI()
                 .generateDeterministicIdBestEffort(throwAwayField, () -> tryVar));  // seeds from the raw dataType
:345   builder = FieldBuilder.builder(normalizeData(builder.build()));               // dataType is corrected here

normalizeData:246-249 forces dataType = SYSTEM whenever acceptedDataTypes is exactly [SYSTEM] — BinaryField, ColumnField, ConstantField, RowField, TagField, and the divider/tab fields. So for those types the id is seeded from a data type the field never actually ends up having.

Two instances on the same build, where the incoming payload differs only in whether it carries an explicit dataType:

	incoming payload	persisted dataType	seed
A	{"clazz":"...TagField","variable":"tags","dataType":"TEXT"}	system_field	tags:TagField:text
B	{"clazz":"...TagField","variable":"tags"}	system_field	tags:TagField:system_field

Identical persisted rows, different ids. The contract of a deterministic id is id = f(field); as written it's id = f(raw payload) while field = normalize(raw payload), so re-deriving the id from the stored field yields a different id than the one stored — determinism is broken for those types independently of any version skew.

To be clear about what this doesn't cause: it does not feed finding 1's data-loss path. All the affected types get nextAvailableColumn → "system_field" (FieldFactoryImpl:723-725) rather than a numbered column, so there's no column reuse, and CleanUpFieldReferencesJob:82-91 already excludes ConstantField, RelationshipField, CategoryField, HostFolderField and the dividers from cleanField. This is a determinism defect, not a content destroyer.

Fix is small — seed from the normalized field (generate after normalizeData, or normalize before seeding). One caveat: doing so also changes the id of those SYSTEM types relative to what this PR currently produces, i.e. it's a second id-space mutation. So it should land in this PR together with whatever is decided for finding 1 — otherwise three seed recipes end up circulating in the installed base instead of two.

---

3. LOW — the null != dataType fallback branch is unreachable

DeterministicIdentifierAPIImpl.java:330

Field.dataType() is declared public abstract DataTypes dataType() (com/dotcms/contenttype/model/field/Field.java:244) — a mandatory Immutables attribute — and 30 of the 32 concrete field classes override it with @Value.Default. No real Field instance returns null here; the only caller reaching the S_S branch is the Mockito stub at DeterministicIdentifierAPITest.java:853. Dead branch, plus a test documenting a fallback that can't fire.

If it's meant as the compatibility path for finding 1, it doesn't serve that purpose — it keys off null, not off the id's vintage.

---

4. LOW — the new transactionalSave test doesn't exercise the change

dotcms-integration/src/test/java/com/dotmarketing/business/DeterministicIdentifierAPITest.java:755

Test_ContentType_Save_Applies_DataType_Change_When_Field_Id_Differs builds the incoming field with .id(UUIDGenerator.generateUuid()) (:789) instead of letting resolveName produce it. With a random id the test never touches the seed — it only asserts delete-then-insert behaviour transactionalSave already had, and passes with the production change reverted.

It also picks the harmless variant: dataType goes TEXT → INTEGER, so the re-inserted field lands on a different column and nothing is cleared. The risky case is the opposite — divergent id with an unchanged dataType, which is what reuses the column.

(Test_ResolveName_Seed_Includes_DataType_When_Present:822 does cover the seed directly; the gap is specifically the upgrade boundary.) Suggested: drive the incoming id through defaultGenerator.generateDeterministicIdBestEffort(field, field::variable), and add a case where the pre-existing field carries a legacy-seed id with the same dataType — that's the one that must not lose content.

[dsilvam]

@freddyDOTCMS let's please address the code-review comments both from @fabrizzio-dotCMS and Claude and make sure it gets into the merge queue

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[freddyDOTCMS]

Review — deterministic field-id seed change

Reviewed against eff265d15377f267d7af6771fe87fd54a8a824d0 (substantive commit 9cfca2bcff45; the newer commit is only a merge from main).

The PR appends dataType.value to the deterministic field-id seed in DeterministicIdentifierAPIImpl.resolveName. That changes the computed id of every field whose id is generated from this build onward, with no seed versioning and no fallback to the previous hash. Nothing here blocks merge, but finding 1 is worth an explicit decision before it goes in, and finding 2 is a correctness bug I'd fix in this PR.

1. MEDIUM — the new seed silently invalidates field ids generated before this change

dotCMS/src/main/java/com/dotmarketing/business/DeterministicIdentifierAPIImpl.java:329-332

Deterministic ids exist so two instances that independently create the same logical field converge on the same id. Adding dataType.value changes the hash, so the same logical field is A = hash(...:myfield:TextField) when its id was generated before this change and B = hash(...:myfield:TextField:text) after. Nothing maps B back to A.

Scope — this needs two environments on different builds. Existing fields keep their stored ids across an upgrade (FieldFactoryImpl:339 only generates when throwAwayField.id() == null), so a single instance is unaffected, and two instances on the same build agree. The exposure is cross-version push publish (or a content type created independently on both sides by starter / plugin / script, with the two creations straddling this change).

Where it goes wrong, with sender on the new build and receiver on the older one:

1. On the sender, myField (TextField / TEXT) is deleted and re-added identically — this used to be a no-op push, same id on both sides.
2. The sender now pushes B; the receiver still holds A. In ContentTypeAPIImpl.transactionalSave (~:1198) A is absent from newFields, so it calls fieldAPI.delete(oldField), which enqueues CleanUpFieldReferencesJob.
3. The data type did not change, so nextAvailableColumn hands the re-inserted field the same column (text1) the deleted one had.
4. The post-commit job then runs cleanField on it, clearing text1 plus contentlet_as_json #- '{fields,myField}' for every contentlet with mod_date <= deletionDate.

The receiver loses that field's stored content, and it fails quietly: the push reports success and the field is still there in the content type, just empty.

I'm flagging it rather than blocking on it because cross-version push publish is out-of-contract to begin with. Two things still make it worth a deliberate call:

• The failure mode is silent data loss on the receiver — typically delivery. Nobody notices until an editor opens the content.
• It isn't cleanly revertible. Any field whose id was generated while this is on the build already carries the new-recipe hash, so backing the change out doesn't restore the previous state. That shape of change belongs in the rollback-unsafe review (docs/core/ROLLBACK_UNSAFE_CATEGORIES.md) even if the risk is accepted.

If the seed change stays, a version prefix plus a legacy-hash fallback on lookup miss closes it. The alternative is to fix what #36636 actually describes — FieldFactoryImpl.dbSaveUpdate discarding the incoming dataType / dbColumn — which resolves the issue without touching the id space at all. I'd lean to the second, but that's a judgement call, not a blocker.

2. MEDIUM — the seed reads the pre-normalization dataType, so the id isn't a function of the field

DeterministicIdentifierAPIImpl.java:331, with dotCMS/src/main/java/com/dotcms/contenttype/business/FieldFactoryImpl.java:341 and :345

The ordering in dbSaveUpdate is:

:341   builder.id(APILocator.getDeterministicIdentifierAPI()
                 .generateDeterministicIdBestEffort(throwAwayField, () -> tryVar));  // seeds from the raw dataType
:345   builder = FieldBuilder.builder(normalizeData(builder.build()));               // dataType is corrected here

normalizeData:246-249 forces dataType = SYSTEM whenever acceptedDataTypes is exactly [SYSTEM] — BinaryField, ColumnField, ConstantField, RowField, TagField, and the divider/tab fields. So for those types the id is seeded from a data type the field never actually ends up having.

Two instances on the same build, where the incoming payload differs only in whether it carries an explicit dataType:

incoming payload persisted dataType seed
A {"clazz":"...TagField","variable":"tags","dataType":"TEXT"} system_field tags:TagField:text
B {"clazz":"...TagField","variable":"tags"} system_field tags:TagField:system_field
Identical persisted rows, different ids. The contract of a deterministic id is id = f(field); as written it's id = f(raw payload) while field = normalize(raw payload), so re-deriving the id from the stored field yields a different id than the one stored — determinism is broken for those types independently of any version skew.

To be clear about what this doesn't cause: it does not feed finding 1's data-loss path. All the affected types get nextAvailableColumn → "system_field" (FieldFactoryImpl:723-725) rather than a numbered column, so there's no column reuse, and CleanUpFieldReferencesJob:82-91 already excludes ConstantField, RelationshipField, CategoryField, HostFolderField and the dividers from cleanField. This is a determinism defect, not a content destroyer.

Fix is small — seed from the normalized field (generate after normalizeData, or normalize before seeding). One caveat: doing so also changes the id of those SYSTEM types relative to what this PR currently produces, i.e. it's a second id-space mutation. So it should land in this PR together with whatever is decided for finding 1 — otherwise three seed recipes end up circulating in the installed base instead of two.

3. LOW — the null != dataType fallback branch is unreachable

DeterministicIdentifierAPIImpl.java:330

Field.dataType() is declared public abstract DataTypes dataType() (com/dotcms/contenttype/model/field/Field.java:244) — a mandatory Immutables attribute — and 30 of the 32 concrete field classes override it with @Value.Default. No real Field instance returns null here; the only caller reaching the S_S branch is the Mockito stub at DeterministicIdentifierAPITest.java:853. Dead branch, plus a test documenting a fallback that can't fire.

If it's meant as the compatibility path for finding 1, it doesn't serve that purpose — it keys off null, not off the id's vintage.

4. LOW — the new transactionalSave test doesn't exercise the change

dotcms-integration/src/test/java/com/dotmarketing/business/DeterministicIdentifierAPITest.java:755

Test_ContentType_Save_Applies_DataType_Change_When_Field_Id_Differs builds the incoming field with .id(UUIDGenerator.generateUuid()) (:789) instead of letting resolveName produce it. With a random id the test never touches the seed — it only asserts delete-then-insert behaviour transactionalSave already had, and passes with the production change reverted.

It also picks the harmless variant: dataType goes TEXT → INTEGER, so the re-inserted field lands on a different column and nothing is cleared. The risky case is the opposite — divergent id with an unchanged dataType, which is what reuses the column.

(Test_ResolveName_Seed_Includes_DataType_When_Present:822 does cover the seed directly; the gap is specifically the upgrade boundary.) Suggested: drive the incoming id through defaultGenerator.generateDeterministicIdBestEffort(field, field::variable), and add a case where the pre-existing field carries a legacy-seed id with the same dataType — that's the one that must not lose content.

Finding 2 ✅ Fixed

FieldFactoryImpl.java — moved normalizeData before ID generation so the seed reflects the actual persisted dataType:

// Before
builder.id(generateDeterministicIdBestEffort(throwAwayField, () -> tryVar)); // seeds from raw dataType
builder = FieldBuilder.builder(normalizeData(builder.build())); // corrects dataType after

// After
final Field normalizedForSeed = normalizeData(builder.build());
builder.id(generateDeterministicIdBestEffort(normalizedForSeed, () -> tryVar)); // seeds from normalized dataType
builder = FieldBuilder.builder(normalizeData(builder.build())); // idempotent, ID is preserved

Test added: Test_TagField_DeterministicId_Is_Independent_Of_Incoming_DataType — saves a TagField twice, once with explicit dataType=TEXT and once without; asserts both produce the same ID.

---

Finding 3 ✅ Fixed

DeterministicIdentifierAPIImpl.java — removed the dead null branch:

// Before
return null != dataType
? String.format(S_S_S, name, field.typeName(), dataType.value)
: String.format(S_S, name, field.typeName());

// After
return String.format(S_S_S, name, field.typeName(), field.dataType().value);

Tests cleaned up:

• expectedFieldSeed helper — removed the null ternary
• Test_ResolveName_Seed_Includes_DataType_When_Present — removed the Mockito stub that forced dataType() to return null and the assertion that documented the dead branch
• Removed now-unused import static org.mockito.Mockito.mock/when

---

Finding 4 ✅ Fixed

Test_ContentType_Save_Applies_DataType_Change_When_Field_Id_Differs — replaced UUIDGenerator.generateUuid() with generateDeterministicIdBestEffort(incomingTemplate, () -> fieldVarName) so the test actually drives the seed.
Simplified the field list to List.of(recreatedField). Added assertNotEquals before the save to make the precondition explicit. Updated javadoc to cover both same-version and mixed-version scenarios.

Risky case covered by Test_CleanUpFieldJob_SkipsCleanup_WhenColumnRecycledBySameVariable — same dataType, different ID, same column recycled. Two assertions prove correctness together: genuineField (different variable,
genuine delete) content is cleared — proves the job ran; myField (same-variable replacement) content is preserved — proves the guard worked.

The guard added to CleanUpFieldReferencesJob.java is what makes the test pass:

if (deletedColumn != null
&& !DataTypes.SYSTEM.value.equals(deletedColumn)
&& type.fields().stream().anyMatch(f ->
deletedColumn.equalsIgnoreCase(f.dbColumn())
&& field.variable().equalsIgnoreCase(f.variable()))) {
Logger.info(..., "Skipping cleanup — column recycled by same variable");
return;
}

---

Finding 1 ⚠️ Accepted risk, not fixed

No code change made. The reviewer said "flagging rather than blocking" because cross-version push-publish is out-of-contract. Your point that sender and receiver should always be on the same version is the right
justification.

What should still happen before merge:

• Add an entry to docs/core/ROLLBACK_UNSAFE_CATEGORIES.md — the seed change is a one-way mutation. Any rollback after fields are created on the new build will have ID mismatches.
• Add a comment near FieldFactoryImpl.java:315 flagging the dataType/dbColumn discard as the correct long-term fix for Push publish silently discards a field's data-type change when a field is deleted and re-added with the same variable name #36636 so the follow-up work has a clear pointer.