dhis2 · muilpp · Dec 4, 2024 · Dec 3, 2024 · Dec 3, 2024 · Dec 3, 2024
diff --git a/...ice-core/src/main/resources/org/hisp/dhis/eventchangelog.hiberante/EventChangeLog.hbm.xml b/...ice-core/src/main/resources/org/hisp/dhis/eventchangelog.hiberante/EventChangeLog.hbm.xml
@@ -18,7 +18,7 @@
     <many-to-one name="dataElement" class="org.hisp.dhis.dataelement.DataElement" column="dataelementid"
       foreign-key="fk_eventchangelog_dataelementid" />
 
-    <property name="eventProperty" length="100" />
+    <property name="eventField" length="100" />
 
     <property name="previousValue" length="50000" />
 

@@ -99,7 +99,7 @@ public void addDataValueChangeLog(
 
   @Override
   @Transactional
-  public void addPropertyChangeLog(
+  public void addFieldChangeLog(
       @Nonnull Event currentEvent, @Nonnull Event event, @Nonnull String username) {
     logIfChanged(
         "occurredAt", Event::getOccurredDate, this::formatDate, currentEvent, event, username);
@@ -133,7 +133,7 @@ public Set<Pair<String, Class<?>>> getFilterableFields() {
   }
 
   private <T> void logIfChanged(
-      String propertyName,
+      String fieldName,
       Function<Event, T> valueExtractor,
       Function<T, String> formatter,
       Event currentEvent,
@@ -148,34 +148,27 @@ private <T> void logIfChanged(
 
       EventChangeLog eventChangeLog =
           new EventChangeLog(
-              event,
-              null,
-              propertyName,
-              currentValue,
-              newValue,
-              changeLogType,
-              new Date(),
-              userName);
+              event, null, fieldName, currentValue, newValue, changeLogType, new Date(), userName);
 
       hibernateEventChangeLogStore.addEventChangeLog(eventChangeLog);
     }
   }
 
   private ChangeLogType getChangeLogType(String oldValue, String newValue) {
-    if (isNewProperty(oldValue, newValue)) {
+    if (isNewField(oldValue, newValue)) {
       return ChangeLogType.CREATE;
-    } else if (isUpdateProperty(oldValue, newValue)) {
+    } else if (isUpdateField(oldValue, newValue)) {
       return ChangeLogType.UPDATE;
     } else {
       return ChangeLogType.DELETE;
     }
   }
 
-  private boolean isNewProperty(String originalValue, String payloadValue) {
+  private boolean isNewField(String originalValue, String payloadValue) {
     return originalValue == null && payloadValue != null;
   }
 
-  private boolean isUpdateProperty(String originalValue, String payloadValue) {
+  private boolean isUpdateField(String originalValue, String payloadValue) {
     return originalValue != null && payloadValue != null;
   }
 

@@ -46,7 +46,7 @@ public class EventChangeLog {
 
   private DataElement dataElement;
 
-  private String eventProperty;
+  private String eventField;
 
   private String previousValue;
 
@@ -63,40 +63,40 @@ public class EventChangeLog {
   public EventChangeLog(
       Event event,
       DataElement dataElement,
-      String eventProperty,
+      String eventField,
       String previousValue,
       String currentValue,
       ChangeLogType changeLogType,
       Date created,
       String createdByUsername) {
-    this(event, dataElement, eventProperty, previousValue, currentValue, changeLogType, created);
+    this(event, dataElement, eventField, previousValue, currentValue, changeLogType, created);
     this.createdByUsername = createdByUsername;
   }
 
   public EventChangeLog(
       Event event,
       DataElement dataElement,
-      String eventProperty,
+      String eventField,
       String previousValue,
       String currentValue,
       ChangeLogType changeLogType,
       Date created,
       UserInfoSnapshot createdBy) {
-    this(event, dataElement, eventProperty, previousValue, currentValue, changeLogType, created);
+    this(event, dataElement, eventField, previousValue, currentValue, changeLogType, created);
     this.createdBy = createdBy;
   }
 
   private EventChangeLog(
       Event event,
       DataElement dataElement,
-      String eventProperty,
+      String eventField,
       String previousValue,
       String currentValue,
       ChangeLogType changeLogType,
       Date created) {
     this.event = event;
     this.dataElement = dataElement;
-    this.eventProperty = eventProperty;
+    this.eventField = eventField;
     this.previousValue = previousValue;
     this.currentValue = currentValue;
     this.changeLogType = changeLogType;

@@ -58,7 +58,7 @@ void addDataValueChangeLog(
       ChangeLogType changeLogType,
       String userName);
 
-  void addPropertyChangeLog(
+  void addFieldChangeLog(
       @Nonnull Event currentEvent, @Nonnull Event event, @Nonnull String userName);
 
   void deleteTrackedEntityDataValueChangeLog(Event event);

@@ -56,7 +56,7 @@ public class HibernateEventChangeLogStore {
   private static final String COLUMN_CHANGELOG_CREATED = "ecl.created";
   private static final String COLUMN_CHANGELOG_USER = "ecl.createdByUsername";
   private static final String COLUMN_CHANGELOG_DATA_ELEMENT = "d.uid";
-  private static final String COLUMN_CHANGELOG_PROPERTY = "ecl.eventProperty";
+  private static final String COLUMN_CHANGELOG_FIELD = "ecl.eventField";
 
   private static final String DEFAULT_ORDER =
       COLUMN_CHANGELOG_CREATED + " " + SortDirection.DESC.getValue();
@@ -73,13 +73,13 @@ public class HibernateEventChangeLogStore {
           entry("createdAt", COLUMN_CHANGELOG_CREATED),
           entry("username", COLUMN_CHANGELOG_USER),
           entry("dataElement", COLUMN_CHANGELOG_DATA_ELEMENT),
-          entry("property", COLUMN_CHANGELOG_PROPERTY));
+          entry("field", COLUMN_CHANGELOG_FIELD));
 
   private static final Map<Pair<String, Class<?>>, String> FILTERABLE_FIELDS =
       Map.ofEntries(
           entry(Pair.of("username", String.class), COLUMN_CHANGELOG_USER),
           entry(Pair.of("dataElement", UID.class), COLUMN_CHANGELOG_DATA_ELEMENT),
-          entry(Pair.of("property", String.class), COLUMN_CHANGELOG_PROPERTY));
+          entry(Pair.of("field", String.class), COLUMN_CHANGELOG_FIELD));
 
   private final EntityManager entityManager;
 
@@ -102,7 +102,7 @@ public Page<EventChangeLog> getEventChangeLogs(
         """
           select ecl.event,
                  ecl.dataElement,
-                 ecl.eventProperty,
+                 ecl.eventField,
                  ecl.previousValue,
                  ecl.currentValue,
                  ecl.changeLogType,
@@ -147,7 +147,7 @@ public Page<EventChangeLog> getEventChangeLogs(
                 row -> {
                   Event e = (Event) row[0];
                   DataElement dataElement = (DataElement) row[1];
-                  String eventProperty = (String) row[2];
+                  String eventField = (String) row[2];
                   String previousValue = (String) row[3];
                   String currentValue = (String) row[4];
                   ChangeLogType changeLogType = (ChangeLogType) row[5];
@@ -160,7 +160,7 @@ public Page<EventChangeLog> getEventChangeLogs(
                   return new EventChangeLog(
                       e,
                       dataElement,
-                      eventProperty,
+                      eventField,
                       previousValue,
                       currentValue,
                       changeLogType,

@@ -215,6 +215,6 @@ private Set<UID> validateTrackedEntityAttributes(TrackedEntity trackedEntity)
       throw new NotFoundException(TrackedEntity.class, trackedEntity.getUid());
     }
 
-    return attributes.stream().map(a -> UID.of(a.getUid())).collect(Collectors.toSet());
+    return attributes.stream().map(UID::of).collect(Collectors.toSet());
   }
 }
@@ -170,7 +170,7 @@ protected void updateDataValues(
       Event currentEntity,
       UserDetails user) {
     handleDataValues(entityManager, preheat, event.getDataValues(), payloadEntity, user);
-    eventChangeLogService.addPropertyChangeLog(currentEntity, payloadEntity, user.getUsername());
+    eventChangeLogService.addFieldChangeLog(currentEntity, payloadEntity, user.getUsername());
   }
 
   private void handleDataValues(

diff --git a/...s/org/hisp/dhis/db/migration/2.42/V2_42_33__Rename_event_change_log_property_to_field.sql b/...s/org/hisp/dhis/db/migration/2.42/V2_42_33__Rename_event_change_log_property_to_field.sql
@@ -0,0 +1,3 @@
+-- DHIS2-18549
+
+alter table eventchangelog rename eventproperty to eventfield;
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		-- DHIS2-18549

		alter table eventchangelog rename eventproperty to eventfield;
Copy link Contributor ameenhere Dec 3, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Since we have not released v42, can we include this change to the original migration script and remove this script? Instead of first creating something else and then altering it? Copy link Contributor teleivo Dec 3, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. What you are proposing would cause issues on play dev as the we would change the checksum of a migration that ran already. We would need to fix that manually AFAIK. Copy link Contributor Author muilpp Dec 3, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. this is not released, but it's been in master for a few days now. I guess some people will have pulled from it already, if I use the original migration script, won't it create a conflict in Flyway for these people? Copy link Contributor ameenhere Dec 3, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. It would create some conflicts yes. For devs who already have applied the old migration, they will have to either manually fix it or start from an older snapshot. I doubt it would be an issue in play dev, as that always uses a db snapshot from a while ago. Possibly v41 db snapshot. We also have the flyway repair option in dhis config to make the fixes slightly easier. For master play dev, I think we do have flyway repair always set to true (I think). Main intention for this suggestion is to avoid these developmental evolution "within a release" forced onto production migrations. It is better we take some developer convenience hit, rather than having more steps in the migration process than needed. Imagine the table already having millions of entries after the original migration, and in this second migration, we do something extra in that large table which could have been combined in the original migration. Either way, my feeling is to fix (and communicate to devs how to fix) the conflicts for this. What do you think? I could be convinced otherwise :D Copy link Contributor Author muilpp Dec 3, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. I get the concern about adding more steps to the migration, but isn’t it pretty much transparent to the implementations? They’d just update to v42, and Flyway would handle the updates automatically. Whether it’s all in one script or split into a few shouldn’t really matter to them. I see your point of view and would agree if the second script was performing an operation on the data, such as adding a new column to an existing index or modifying the data in some significant way. However, in this case, it's simply renaming a column. Even if the table contains millions of entries, the execution time should stay the same. Would you agree? Copy link Contributor ameenhere Dec 4, 2024 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. To clarify, I was not suggesting to add a "Alter table rename..." to the original migration script. Rather change the create table script to have the correct column names the first time. Whether it’s all in one script or split into a few shouldn’t really matter to them. It will matter to them if we do split migrations would cost more time right? It may not be significant for this case though. Since the renaming column is potentially a constant time operation. But that constant time is still an additional time. If the original query, when creating the table the first time, if it had the right column names, this renaming wouldn't have been necessary. In this case though, assuming (and hoping) the additional time is very very small, let us go with this separate step 👍 😄 muilpp reacted with thumbs up emoji