Skip to content

Commit cf9be88

Browse files
nollymarclaude
andcommitted
fix(push-publish): apply field data-type change on the receiver + preserve value (#36636)
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 — and, once that's fixed, the pushed contentlet's value was wiped by the async cleanup job. Both are addressed here. 1. Deterministic id now includes the field's dataType (DeterministicIdentifierAPIImpl.resolveName). The receiver's ContentTypeAPIImpl.transactionalSave now sees the incoming field as a different id from the old field, routing it through delete-then-insert instead of update — which had been overwriting the incoming dataType/dbColumn with the existing field's values in FieldFactoryImpl.dbSaveUpdate. 2. CleanUpFieldReferencesJob now skips cleanup when the deleted field's variable is owned by a different field id on the same Content Type. Without this, the JSON cleanup at ESContentFactoryImpl#getJsonFieldQueries would strip the newly-inserted field's value from contentlet_as_json for every push-published contentlet, because the sender's mod_date predates the receiver's deletionDate and defeats the existing time guard. Also fixes the @BeforeClass ordering in CleanUpFieldReferencesJobTest so the class can run in isolation — IntegrationTestInitService's static block initializes the CDI container, and must run before any APILocator lookup that resolves CDI-managed beans such as IndexAPIImpl → OSIndexAPIImpl. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent f92f939 commit cf9be88

4 files changed

Lines changed: 336 additions & 6 deletions

File tree

dotCMS/src/main/java/com/dotmarketing/business/DeterministicIdentifierAPIImpl.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import com.dotcms.business.CloseDBIfOpened;
66
import com.dotcms.contenttype.business.ContentTypeAPI;
77
import com.dotcms.contenttype.model.field.BinaryField;
8+
import com.dotcms.contenttype.model.field.DataTypes;
89
import com.dotcms.contenttype.model.field.Field;
910
import com.dotcms.contenttype.model.type.BaseContentType;
1011
import com.dotcms.contenttype.model.type.ContentType;
@@ -62,6 +63,7 @@ public class DeterministicIdentifierAPIImpl implements DeterministicIdentifierAP
6263

6364
static final String NON_DETERMINISTIC_IDENTIFIER = "[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}";
6465
public static final String S_S = "%s:%s";
66+
public static final String S_S_S = "%s:%s:%s";
6567

6668
final IdentifierFactory identifierFactory = FactoryLocator.getIdentifierFactory();
6769
final Predicate<String> testIdentifier = identifierFactory::isIdentifier;
@@ -321,8 +323,13 @@ String resolveName(final Field field, final Supplier<String> fieldVarName) {
321323
if(UtilMethods.isNotSet(name)){
322324
name = field.variable();
323325
}
324-
//amplify the dispersion of the seed by adding the field type
325-
return String.format(S_S, name, field.typeName());
326+
//amplify the dispersion of the seed by adding the field type and, when present, the data
327+
//type, so a field re-created with the same variable but a different data type gets a
328+
//different deterministic id
329+
final DataTypes dataType = field.dataType();
330+
return null != dataType
331+
? String.format(S_S_S, name, field.typeName(), dataType.value)
332+
: String.format(S_S, name, field.typeName());
326333
}
327334

328335
/**

dotCMS/src/main/java/com/dotmarketing/quartz/job/CleanUpFieldReferencesJob.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,24 @@ public void run(final JobExecutionContext jobContext) throws JobExecutionExcepti
7575
try {
7676
final ContentType type = contentTypeAPI.find(field.contentTypeId());
7777

78+
// If the deleted field's velocity variable is now owned by a different field
79+
// on the same Content Type (delete + re-add with the same variable but a
80+
// different data type / id — see issue #36636), skip cleanup. Otherwise the
81+
// JSON cleanup at ESContentFactoryImpl#getJsonFieldQueries would strip the
82+
// new field's value from contentlet_as_json, because it keys by velocity
83+
// variable name.
84+
final boolean variableReassigned = type.fields().stream().anyMatch(
85+
f -> f.variable().equalsIgnoreCase(field.variable())
86+
&& !f.id().equalsIgnoreCase(field.id()));
87+
88+
if (variableReassigned) {
89+
Logger.info(CleanUpFieldReferencesJob.class, () -> String.format(
90+
"Skipping cleanup for deleted field '%s' (id=%s) on Content Type "
91+
+ "'%s': variable is now owned by a different field id.",
92+
field.variable(), field.id(), type.variable()));
93+
return;
94+
}
95+
7896
final Structure structure = new StructureTransformer(type).asStructure();
7997

8098
com.dotmarketing.portlets.structure.model.Field legacyField = new LegacyFieldTransformer(field).asOldField();

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

Lines changed: 194 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,18 @@
99
import static org.junit.Assert.assertFalse;
1010
import static org.junit.Assert.assertNotEquals;
1111
import static org.junit.Assert.assertTrue;
12+
import static org.mockito.Mockito.mock;
13+
import static org.mockito.Mockito.when;
1214

15+
import com.dotcms.contenttype.business.ContentTypeAPI;
16+
import com.dotcms.contenttype.business.FieldAPI;
1317
import com.dotcms.contenttype.model.field.ColumnField;
18+
import com.dotcms.contenttype.model.field.DataTypes;
1419
import com.dotcms.contenttype.model.field.Field;
20+
import com.dotcms.contenttype.model.field.FieldBuilder;
1521
import com.dotcms.contenttype.model.field.RelationshipField;
1622
import com.dotcms.contenttype.model.field.RowField;
23+
import com.dotcms.contenttype.model.field.TextField;
1724
import com.dotcms.contenttype.model.type.BaseContentType;
1825
import com.dotcms.contenttype.model.type.ContentType;
1926
import com.dotcms.datagen.ContentTypeDataGen;
@@ -40,8 +47,10 @@
4047
import com.dotmarketing.portlets.templates.model.Template;
4148
import com.dotmarketing.portlets.workflows.business.SystemWorkflowConstants;
4249
import com.dotmarketing.util.Config;
50+
import com.dotmarketing.util.UUIDGenerator;
4351
import com.dotmarketing.util.UUIDUtil;
4452
import com.dotmarketing.util.WebKeys;
53+
import com.liferay.portal.model.User;
4554
import com.liferay.util.FileUtil;
4655
import com.tngtech.java.junit.dataprovider.DataProvider;
4756
import com.tngtech.java.junit.dataprovider.DataProviderRunner;
@@ -77,6 +86,16 @@ private static void prepareIfNecessary() throws Exception {
7786

7887
private final DeterministicIdentifierAPIImpl defaultGenerator = new DeterministicIdentifierAPIImpl();
7988

89+
/**
90+
* Expected deterministic id seed for a field: {@code variable:typeName:dataType} when the field
91+
* has a data type, {@code variable:typeName} otherwise.
92+
*/
93+
private static String expectedFieldSeed(final Field field) {
94+
return null != field.dataType()
95+
? String.format("%s:%s:%s", field.variable(), field.typeName(), field.dataType().value)
96+
: String.format("%s:%s", field.variable(), field.typeName());
97+
}
98+
8099

81100
/**
82101
* Given Scenario: We have a fileAsset with missing physical file
@@ -334,7 +353,7 @@ public void Test_Generate_Content_Type_Identifier(final ContentTypeTestCase test
334353
final String fieldIdentifier2 = defaultGenerator.generateDeterministicIdBestEffort(field, field::variable);
335354
//Test it is idempotent
336355
assertEquals(fieldIdentifier1, fieldIdentifier2);
337-
final String expected = String.format("%s:%s", field.variable(), field.typeName());
356+
final String expected = expectedFieldSeed(field);
338357
assertEquals(expected, defaultGenerator.resolveName(field, field::variable));
339358
}
340359

@@ -588,7 +607,7 @@ public void test_resolveName_seedShouldContainFieldType(){
588607

589608
//verify the seed contains the field type
590609
for(final Field field : contentType.fields()){
591-
final String expected = String.format("%s:%s", field.variable(), field.typeName());
610+
final String expected = expectedFieldSeed(field);
592611
assertEquals(expected, defaultGenerator.resolveName(field, field::variable));
593612
}
594613

@@ -667,4 +686,177 @@ public void Test_Generate_Folder_Identifier() throws DotDataException {
667686
}
668687
}
669688

689+
/**
690+
* Method to test: {@link DeterministicIdentifierAPIImpl#generateDeterministicIdBestEffort(Field, Supplier)}
691+
* Given Scenario: A Text field is created with data type TEXT, then deleted, then re-created with the
692+
* same variable name but data type INTEGER (the scenario behind issue #36636).
693+
* ExpectedResult: The re-created field must get a DIFFERENT deterministic id (the data type is part of
694+
* the seed) and its data type must be INTEGER.
695+
*/
696+
@Test
697+
public void Test_Delete_And_Recreate_Field_With_Different_DataType_Generates_New_Id() throws Exception {
698+
prepareIfNecessary();
699+
final boolean generateConsistentIdentifiers = Config
700+
.getBooleanProperty(GENERATE_DETERMINISTIC_IDENTIFIERS, true);
701+
try {
702+
Config.setProperty(GENERATE_DETERMINISTIC_IDENTIFIERS, true);
703+
704+
final User systemUser = APILocator.systemUser();
705+
final FieldAPI fieldAPI = APILocator.getContentTypeFieldAPI();
706+
final String fieldVarName = "myField" + System.currentTimeMillis();
707+
708+
final Field textField = new FieldDataGen()
709+
.type(TextField.class)
710+
.name(fieldVarName)
711+
.velocityVarName(fieldVarName)
712+
.dataType(DataTypes.TEXT)
713+
.next();
714+
715+
final ContentType contentType = new ContentTypeDataGen()
716+
.workflowId(SystemWorkflowConstants.SYSTEM_WORKFLOW_ID)
717+
.baseContentType(BaseContentType.CONTENT)
718+
.field(textField)
719+
.nextPersisted();
720+
try {
721+
final Field originalField = fieldAPI
722+
.byContentTypeIdAndVar(contentType.id(), fieldVarName);
723+
assertEquals(DataTypes.TEXT, originalField.dataType());
724+
725+
fieldAPI.delete(originalField);
726+
727+
final Field recreatedField = fieldAPI.save(FieldBuilder.builder(TextField.class)
728+
.name(fieldVarName)
729+
.variable(fieldVarName)
730+
.contentTypeId(contentType.id())
731+
.dataType(DataTypes.INTEGER)
732+
.build(), systemUser);
733+
734+
assertEquals(DataTypes.INTEGER, recreatedField.dataType());
735+
assertNotEquals(
736+
"A field re-created with the same variable but a different data type must get a different deterministic id",
737+
originalField.id(), recreatedField.id());
738+
} finally {
739+
ContentTypeDataGen.remove(contentType);
740+
}
741+
} finally {
742+
Config.setProperty(GENERATE_DETERMINISTIC_IDENTIFIERS, generateConsistentIdentifiers);
743+
}
744+
}
745+
746+
/**
747+
* Method to test: {@link com.dotcms.contenttype.business.ContentTypeAPI#save(ContentType, List)}
748+
* Given Scenario: Replicates the push-publish receiver flow for issue #36636. A Content Type holds a
749+
* Text field with data type TEXT; an incoming save carries the same field variable re-created under a
750+
* DIFFERENT id (as the sender's bundle does once the data type is part of the deterministic id seed)
751+
* with data type INTEGER.
752+
* ExpectedResult: The old field is removed and the data-type change is applied, never silently dropped.
753+
*/
754+
@Test
755+
public void Test_ContentType_Save_Applies_DataType_Change_When_Field_Id_Differs() throws Exception {
756+
prepareIfNecessary();
757+
final boolean generateConsistentIdentifiers = Config
758+
.getBooleanProperty(GENERATE_DETERMINISTIC_IDENTIFIERS, true);
759+
try {
760+
Config.setProperty(GENERATE_DETERMINISTIC_IDENTIFIERS, true);
761+
762+
final User systemUser = APILocator.systemUser();
763+
final FieldAPI fieldAPI = APILocator.getContentTypeFieldAPI();
764+
final ContentTypeAPI contentTypeAPI = APILocator.getContentTypeAPI(systemUser);
765+
final String fieldVarName = "myField" + System.currentTimeMillis();
766+
767+
final Field textField = new FieldDataGen()
768+
.type(TextField.class)
769+
.name(fieldVarName)
770+
.velocityVarName(fieldVarName)
771+
.dataType(DataTypes.TEXT)
772+
.next();
773+
774+
final ContentType contentType = new ContentTypeDataGen()
775+
.workflowId(SystemWorkflowConstants.SYSTEM_WORKFLOW_ID)
776+
.baseContentType(BaseContentType.CONTENT)
777+
.field(textField)
778+
.nextPersisted();
779+
try {
780+
final Field originalField = fieldAPI
781+
.byContentTypeIdAndVar(contentType.id(), fieldVarName);
782+
assertEquals(DataTypes.TEXT, originalField.dataType());
783+
784+
// the incoming (pushed) field: same variable, different id, different data type
785+
final Field recreatedField = FieldBuilder.builder(TextField.class)
786+
.name(fieldVarName)
787+
.variable(fieldVarName)
788+
.contentTypeId(contentType.id())
789+
.id(UUIDGenerator.generateUuid())
790+
.dataType(DataTypes.INTEGER)
791+
.build();
792+
793+
final List<Field> newFields = contentType.fields().stream()
794+
.map(field -> fieldVarName.equalsIgnoreCase(field.variable())
795+
? recreatedField : field)
796+
.collect(Collectors.toList());
797+
798+
contentTypeAPI.save(contentType, newFields);
799+
800+
final Field savedField = fieldAPI
801+
.byContentTypeIdAndVar(contentType.id(), fieldVarName);
802+
assertEquals(DataTypes.INTEGER, savedField.dataType());
803+
assertEquals(recreatedField.id(), savedField.id());
804+
assertNotEquals(originalField.id(), savedField.id());
805+
} finally {
806+
ContentTypeDataGen.remove(contentType);
807+
}
808+
} finally {
809+
Config.setProperty(GENERATE_DETERMINISTIC_IDENTIFIERS, generateConsistentIdentifiers);
810+
}
811+
}
812+
813+
/**
814+
* Method to test: {@link DeterministicIdentifierAPIImpl#resolveName(Field, Supplier)}
815+
* Given Scenario: Two fields share the same variable and field type but differ on data type; a third
816+
* field has no data type at all.
817+
* ExpectedResult: The seed must include the data type when the field has one (so the resulting
818+
* deterministic ids differ) and must fall back to the legacy {@code variable:typeName} format when the
819+
* data type is absent.
820+
*/
821+
@Test
822+
public void Test_ResolveName_Seed_Includes_DataType_When_Present() {
823+
final String fieldVarName = "seedField";
824+
825+
final Field textDataTypeField = FieldBuilder.builder(TextField.class)
826+
.name(fieldVarName)
827+
.variable(fieldVarName)
828+
.contentTypeId("fakeContentTypeId")
829+
.dataType(DataTypes.TEXT)
830+
.build();
831+
832+
final Field integerDataTypeField = FieldBuilder.builder(TextField.class)
833+
.name(fieldVarName)
834+
.variable(fieldVarName)
835+
.contentTypeId("fakeContentTypeId")
836+
.dataType(DataTypes.INTEGER)
837+
.build();
838+
839+
final String textSeed = defaultGenerator
840+
.resolveName(textDataTypeField, textDataTypeField::variable);
841+
final String integerSeed = defaultGenerator
842+
.resolveName(integerDataTypeField, integerDataTypeField::variable);
843+
844+
assertEquals(String.format("%s:%s:%s", fieldVarName, textDataTypeField.typeName(),
845+
DataTypes.TEXT.value), textSeed);
846+
assertEquals(String.format("%s:%s:%s", fieldVarName, integerDataTypeField.typeName(),
847+
DataTypes.INTEGER.value), integerSeed);
848+
assertNotEquals(
849+
"Fields with the same variable but different data types must produce different seeds",
850+
textSeed, integerSeed);
851+
852+
// a field without a data type keeps the legacy variable:typeName seed
853+
final Field noDataTypeField = mock(Field.class);
854+
when(noDataTypeField.variable()).thenReturn(fieldVarName);
855+
when(noDataTypeField.typeName()).thenReturn(textDataTypeField.typeName());
856+
when(noDataTypeField.dataType()).thenReturn(null);
857+
858+
assertEquals(String.format("%s:%s", fieldVarName, textDataTypeField.typeName()),
859+
defaultGenerator.resolveName(noDataTypeField, noDataTypeField::variable));
860+
}
861+
670862
}

0 commit comments

Comments
 (0)