From cd9a5074f38e14524af363a0be85d7103409cfb5 Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Tue, 10 Dec 2024 16:44:20 -0500 Subject: [PATCH 01/15] other files update --- .../AbstractSimpleMigrationStrategy.java | 66 ++----- .../MigrationStrategyRepository.java | 23 ++- .../folio/fqm/migration/MigrationUtils.java | 97 +++++++++++ .../strategies/V4DateFieldTimezones.java | 164 ++++++++++++++++++ .../folio/fqm/service/MigrationService.java | 2 +- 5 files changed, 295 insertions(+), 57 deletions(-) create mode 100644 src/main/java/org/folio/fqm/migration/MigrationUtils.java create mode 100644 src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java diff --git a/src/main/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategy.java b/src/main/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategy.java index 1e0ddbb1..dc01594e 100644 --- a/src/main/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategy.java +++ b/src/main/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategy.java @@ -2,8 +2,6 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.node.ArrayNode; -import com.fasterxml.jackson.databind.node.ObjectNode; import java.io.UncheckedIOException; import java.util.ArrayList; import java.util.HashSet; @@ -105,16 +103,27 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn result = result.withEntityTypeId(this.getEntityTypeChanges().getOrDefault(src.entityTypeId(), src.entityTypeId())); - ObjectNode fql = (ObjectNode) objectMapper.readTree(result.fqlQuery()); - fql.set(MigrationService.VERSION_KEY, objectMapper.valueToTree(this.getTargetVersion())); - Map fieldChanges = this.getFieldChanges().getOrDefault(src.entityTypeId(), Map.of()); Map> fieldWarnings = this.getFieldWarnings().getOrDefault(src.entityTypeId(), Map.of()); - if (!fieldChanges.isEmpty() || !fieldWarnings.isEmpty()) { - // map query fields - fql = migrateFqlTree(fieldChanges, fieldWarnings, fql, warnings); + String newFql = MigrationUtils.migrateFql( + result.fqlQuery(), + _v -> this.getTargetVersion(), + (fql, key, value) -> { + if (fieldWarnings.containsKey(key)) { + FieldWarning warning = fieldWarnings.get(key).apply(key, value.toPrettyString()); + + warnings.add(warning); + if (warning instanceof RemovedFieldWarning || warning instanceof QueryBreakingWarning) { + return; + } + } + fql.set(getNewFieldName(fieldChanges, key), value); + } + ); + + if (!fieldChanges.isEmpty() || !fieldWarnings.isEmpty()) { // map fields list result = result.withFields( @@ -139,7 +148,7 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn ); } - result = result.withFqlQuery(objectMapper.writeValueAsString(fql)); + result = result.withFqlQuery(newFql); return result.withWarnings(new ArrayList<>(warnings)); } catch (JsonProcessingException e) { @@ -157,43 +166,4 @@ protected static String getNewFieldName(Map fieldChanges, String return fieldChanges.getOrDefault(oldFieldName, oldFieldName); } } - - protected static ObjectNode migrateFqlTree( - Map fieldChanges, - Map> fieldWarnings, - ObjectNode fql, - Set warnings - ) { - ObjectNode result = new ObjectMapper().createObjectNode(); - // iterate through fields in source - fql - .fields() - .forEachRemaining(entry -> { - if ("$and".equals(entry.getKey())) { - ArrayNode resultContents = new ObjectMapper().createArrayNode(); - ((ArrayNode) entry.getValue()).elements() - .forEachRemaining(node -> { - ObjectNode innerResult = migrateFqlTree(fieldChanges, fieldWarnings, (ObjectNode) node, warnings); - // handle removed fields - if (!innerResult.isEmpty()) { - resultContents.add(innerResult); - } - }); - result.set("$and", resultContents); - } else { - if (fieldWarnings.containsKey(entry.getKey())) { - FieldWarning warning = fieldWarnings - .get(entry.getKey()) - .apply(entry.getKey(), entry.getValue().toPrettyString()); - warnings.add(warning); - if (warning instanceof RemovedFieldWarning || warning instanceof QueryBreakingWarning) { - return; - } - } - result.set(getNewFieldName(fieldChanges, entry.getKey()), entry.getValue()); - } - }); - - return result; - } } diff --git a/src/main/java/org/folio/fqm/migration/MigrationStrategyRepository.java b/src/main/java/org/folio/fqm/migration/MigrationStrategyRepository.java index 2cee508a..714b9b7c 100644 --- a/src/main/java/org/folio/fqm/migration/MigrationStrategyRepository.java +++ b/src/main/java/org/folio/fqm/migration/MigrationStrategyRepository.java @@ -1,24 +1,31 @@ package org.folio.fqm.migration; import java.util.List; +import org.folio.fqm.client.ConfigurationClient; import org.folio.fqm.migration.strategies.V0POCMigration; import org.folio.fqm.migration.strategies.V1ModeOfIssuanceConsolidation; import org.folio.fqm.migration.strategies.V2ResourceTypeConsolidation; import org.folio.fqm.migration.strategies.V3RamsonsFieldCleanup; +import org.folio.fqm.migration.strategies.V4DateFieldTimezones; import org.springframework.stereotype.Component; @Component public class MigrationStrategyRepository { - // prevent re-initialization on each call - private static final List MIGRATION_STRATEGIES = List.of( - new V0POCMigration(), - new V1ModeOfIssuanceConsolidation(), - new V2ResourceTypeConsolidation(), - new V3RamsonsFieldCleanup() - ); + private final List migrationStrategies; + + public MigrationStrategyRepository(ConfigurationClient configurationClient) { + this.migrationStrategies = + List.of( + new V0POCMigration(), + new V1ModeOfIssuanceConsolidation(), + new V2ResourceTypeConsolidation(), + new V3RamsonsFieldCleanup(), + new V4DateFieldTimezones(configurationClient) + ); + } public List getMigrationStrategies() { - return MIGRATION_STRATEGIES; + return this.migrationStrategies; } } diff --git a/src/main/java/org/folio/fqm/migration/MigrationUtils.java b/src/main/java/org/folio/fqm/migration/MigrationUtils.java new file mode 100644 index 00000000..cc3ada37 --- /dev/null +++ b/src/main/java/org/folio/fqm/migration/MigrationUtils.java @@ -0,0 +1,97 @@ +package org.folio.fqm.migration; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonMappingException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.ObjectNode; +import jakarta.validation.constraints.NotNull; +import java.io.UncheckedIOException; +import java.util.function.Function; +import lombok.experimental.UtilityClass; +import lombok.extern.log4j.Log4j2; +import org.apache.commons.lang3.function.TriConsumer; +import org.folio.fqm.service.MigrationService; + +@Log4j2 +@UtilityClass +public class MigrationUtils { + + private static final ObjectMapper objectMapper = new ObjectMapper(); + + /** + * Helper function to transform an FQL query. This changes a version to a new one, and runs a given + * function on each field in the query. See {@link #migrateFqlTree(ObjectNode, TriConsumer)} for more + * details on the field transformation function. + * + * @param fqlQuery The root query to migrate + * @param versionTransformer A function that takes the current (potentially null) version and + * returns the new one to be persisted in the query + * @param handler something that takes the result node, the field name, and the field's query value, + * applies some transformation, and stores the results back in result + * @throws JsonMappingException + * @throws JsonProcessingException + */ + public static String migrateFql( + @NotNull String fqlQuery, + Function versionTransformer, + TriConsumer handler + ) { + try { + ObjectNode fql = (ObjectNode) objectMapper.readTree(fqlQuery); + + fql.set( + MigrationService.VERSION_KEY, + objectMapper.valueToTree(versionTransformer.apply(fql.get(MigrationService.VERSION_KEY).asText())) + ); + + fql = migrateFqlTree(fql, handler); + + return objectMapper.writeValueAsString(fql); + } catch (JsonProcessingException e) { + log.error("Unable to process JSON", e); + throw new UncheckedIOException(e); + } + } + + /** + * Call `handler` for each field in the FQL query tree, returning a new tree. + * Note that `handler` is responsible for inserting what should be left in the tree, if anything; + * if the function is a no-op, an empty FQL tree will be returned. + * + * A true "no-op" here would look like (result, key, value) -> result.set(key, value). + * + * This conveniently handles `$and`s, allowing logic to be handled on fields only. + * + * @param fql the fql node + * @param handler something that takes the result node, the field name, and the field's query value, + * applies some transformation, and stores the results back in result + * @return + */ + private static ObjectNode migrateFqlTree(ObjectNode fql, TriConsumer handler) { + ObjectNode result = new ObjectMapper().createObjectNode(); + // iterate through fields in source + fql + .fields() + .forEachRemaining(entry -> { + if ("$and".equals(entry.getKey())) { + ArrayNode resultContents = new ObjectMapper().createArrayNode(); + ((ArrayNode) entry.getValue()).elements() + .forEachRemaining(node -> { + ObjectNode innerResult = migrateFqlTree((ObjectNode) node, handler); + // handle removed fields + if (!innerResult.isEmpty()) { + resultContents.add(innerResult); + } + }); + result.set("$and", resultContents); + // ensure we don't run this on the _version + } else if (!MigrationService.VERSION_KEY.equals(entry.getKey())) { + handler.accept(result, entry.getKey(), entry.getValue()); + } + }); + + return result; + } +} diff --git a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java new file mode 100644 index 00000000..0cdb95ee --- /dev/null +++ b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java @@ -0,0 +1,164 @@ +package org.folio.fqm.migration.strategies; + +import com.fasterxml.jackson.databind.node.TextNode; + +import org.folio.fql.service.FqlService; +import org.folio.fqm.client.ConfigurationClient; +import org.folio.fqm.migration.MigratableQueryInformation; +import org.folio.fqm.migration.MigrationStrategy; +import org.folio.fqm.migration.MigrationUtils; + +import java.time.LocalDate; +import java.time.ZoneId; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import java.util.concurrent.atomic.AtomicReference; + +import lombok.RequiredArgsConstructor; + +/** + * Version 4 -> 5, handles addition of time component to date queries. These are not required for the query to run, + * however, the query builder now expects/requires this form. Additionally, we need to calculate the "midnight" for + * these dates, to conform them to the tenant timezone (previously we always used UTC). + * + * @see https://folio-org.atlassian.net/browse/MODFQMMGR-594 for the addition of this migration script + * @see https://folio-org.atlassian.net/browse/MODFQMMGR-466 for the original support + * @see https://folio-org.atlassian.net/browse/UIPQB-126 for the query builder addition + * @see https://folio-org.atlassian.net/browse/MODFQMMGR-573 for the addition of tenant TZ logic in FQM + */ +@RequiredArgsConstructor +public class V4DateFieldTimezones implements MigrationStrategy { + + public static final String SOURCE_VERSION = "4"; + public static final String TARGET_VERSION = "5"; + + // must snapshot this point in time, as the entity types stored within may change past this migration + private static final Map> DATE_FIELDS = Map.ofEntries( + Map.entry( + UUID.fromString("8418e512-feac-4a6a-a56d-9006aab31e33"), + Set.of("holdings.created_at", "holdings.updated_at") + ), + Map.entry( + UUID.fromString("6b08439b-4f8e-4468-8046-ea620f5cfb74"), + Set.of("instance.cataloged_date", "instance.created_at", "instance.updated_at") + ), + Map.entry( + UUID.fromString("d0213d22-32cf-490f-9196-d81c3c66e53f"), + Set.of( + "holdings.created_at", + "holdings.updated_at", + "instances.cataloged_date", + "instances.created_at", + "instances.updated_at", + "items.circulation_notes[*]->date", + "items.created_date", + "items.last_check_in_date_time", + "items.updated_date" + ) + ), + Map.entry( + UUID.fromString("d6729885-f2fb-4dc7-b7d0-a865a7f461e4"), + Set.of( + "holdings.created_at", + "holdings.updated_at", + "instance.cataloged_date", + "instance.created_at", + "instance.updated_at", + "items.circulation_notes[*]->date", + "items.created_date", + "items.last_check_in_date_time", + "items.updated_date", + "loans.checkout_date", + "loans.claimed_returned_date", + "loans.created_at", + "loans.declared_lost_date", + "loans.due_date", + "loans.return_date", + "users.expiration_date" + ) + ), + Map.entry( + UUID.fromString("b5ffa2e9-8080-471a-8003-a8c5a1274503"), + Set.of("changelogs[*]->timestamp", "created_at", "edi_job_scheduling_date", "updated_at") + ), + Map.entry( + UUID.fromString("abc777d3-2a45-43e6-82cb-71e8c96d13d2"), + Set.of( + "assigned_to_user.user_created_date", + "assigned_to_user.user_updated_date", + "po_created_by_user.user_created_date", + "po_created_by_user.user_updated_date", + "po_updated_by_user.user_created_date", + "po_updated_by_user.user_updated_date", + "po.created_at", + "po.updated_at", + "pol_created_by_user.user_created_date", + "pol_created_by_user.user_updated_date", + "pol_updated_by_user.user_created_date", + "pol_updated_by_user.user_updated_date", + "pol.created_at", + "pol.eresource_expected_activation", + "pol.physical_expected_receipt_date", + "pol.receipt_date", + "pol.updated_at" + ) + ), + Map.entry( + UUID.fromString("ddc93926-d15a-4a45-9d9c-93eadc3d9bbf"), + Set.of( + "users.created_date", + "users.date_of_birth", + "users.enrollment_date", + "users.expiration_date", + "users.updated_date", + "users.user_created_date", + "users.user_updated_date" + ) + ) + ); + + private final ConfigurationClient configurationClient; + + @Override + public String getLabel() { + return "V4 -> V5 Date field time addition (MODFQMMGR-594)"; + } + + @Override + public boolean applies(String version) { + return SOURCE_VERSION.equals(version); + } + + @Override + public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryInformation query) { + Set fieldsToMigrate = DATE_FIELDS.getOrDefault(query.entityTypeId(), Set.of()); + + AtomicReference timezone = new AtomicReference<>(); + + return query.withFqlQuery( + MigrationUtils.migrateFql( + query.fqlQuery(), + _v -> TARGET_VERSION, + (result, key, value) -> { + if ( + !fieldsToMigrate.contains(key) || // not a date field + value.textValue().contains("T") // already has time component + ) { + result.set(key, value); // no-op + return; + } + + if (timezone.get() == null) { + timezone.set(configurationClient.getTenantTimezone()); + } + + result.set( + key, + new TextNode(LocalDate.parse(value.textValue()).atStartOfDay(timezone.get()).toInstant().toString()) + ); + } + ) + ); + } +} diff --git a/src/main/java/org/folio/fqm/service/MigrationService.java b/src/main/java/org/folio/fqm/service/MigrationService.java index 094afc64..797b1f30 100644 --- a/src/main/java/org/folio/fqm/service/MigrationService.java +++ b/src/main/java/org/folio/fqm/service/MigrationService.java @@ -25,7 +25,7 @@ public class MigrationService { public static final UUID REMOVED_ENTITY_TYPE_ID = UUID.fromString("deadbeef-dead-dead-dead-deaddeadbeef"); - protected static final String CURRENT_VERSION = "4"; + protected static final String CURRENT_VERSION = "5"; // TODO: replace this with current version in the future? protected static final String DEFAULT_VERSION = "0"; From cf3ed93f16ddd491034eb12dc4641800b51acc3e Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Tue, 10 Dec 2024 16:46:52 -0500 Subject: [PATCH 02/15] add comment --- .../migration/strategies/V4DateFieldTimezones.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java index 0cdb95ee..2eb34cea 100644 --- a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java +++ b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java @@ -1,21 +1,18 @@ package org.folio.fqm.migration.strategies; import com.fasterxml.jackson.databind.node.TextNode; - -import org.folio.fql.service.FqlService; -import org.folio.fqm.client.ConfigurationClient; -import org.folio.fqm.migration.MigratableQueryInformation; -import org.folio.fqm.migration.MigrationStrategy; -import org.folio.fqm.migration.MigrationUtils; - import java.time.LocalDate; import java.time.ZoneId; import java.util.Map; import java.util.Set; import java.util.UUID; import java.util.concurrent.atomic.AtomicReference; - import lombok.RequiredArgsConstructor; +import org.folio.fql.service.FqlService; +import org.folio.fqm.client.ConfigurationClient; +import org.folio.fqm.migration.MigratableQueryInformation; +import org.folio.fqm.migration.MigrationStrategy; +import org.folio.fqm.migration.MigrationUtils; /** * Version 4 -> 5, handles addition of time component to date queries. These are not required for the query to run, @@ -134,6 +131,7 @@ public boolean applies(String version) { public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryInformation query) { Set fieldsToMigrate = DATE_FIELDS.getOrDefault(query.entityTypeId(), Set.of()); + // avoid initializing this if we don't actually need it AtomicReference timezone = new AtomicReference<>(); return query.withFqlQuery( From 83544fbd9cd0d74ad3af0db0db4e5671d02da40f Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Tue, 10 Dec 2024 16:48:10 -0500 Subject: [PATCH 03/15] logging --- .../strategies/V4DateFieldTimezones.java | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java index 2eb34cea..3751b2bc 100644 --- a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java +++ b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java @@ -3,11 +3,13 @@ import com.fasterxml.jackson.databind.node.TextNode; import java.time.LocalDate; import java.time.ZoneId; +import java.time.format.DateTimeParseException; import java.util.Map; import java.util.Set; import java.util.UUID; import java.util.concurrent.atomic.AtomicReference; import lombok.RequiredArgsConstructor; +import lombok.extern.log4j.Log4j2; import org.folio.fql.service.FqlService; import org.folio.fqm.client.ConfigurationClient; import org.folio.fqm.migration.MigratableQueryInformation; @@ -24,6 +26,7 @@ * @see https://folio-org.atlassian.net/browse/UIPQB-126 for the query builder addition * @see https://folio-org.atlassian.net/browse/MODFQMMGR-573 for the addition of tenant TZ logic in FQM */ +@Log4j2 @RequiredArgsConstructor public class V4DateFieldTimezones implements MigrationStrategy { @@ -151,10 +154,15 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn timezone.set(configurationClient.getTenantTimezone()); } - result.set( - key, - new TextNode(LocalDate.parse(value.textValue()).atStartOfDay(timezone.get()).toInstant().toString()) - ); + try { + result.set( + key, + new TextNode(LocalDate.parse(value.textValue()).atStartOfDay(timezone.get()).toInstant().toString()) + ); + } catch (DateTimeParseException e) { + log.warn("Could not migrate date {}", value, e); + result.set(key, value); // no-op + } } ) ); From 9f5b14e359a7a049114cf6e60bb589b13f74c5e7 Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Tue, 10 Dec 2024 17:27:39 -0500 Subject: [PATCH 04/15] test fix --- .../folio/fqm/migration/MigrationStrategyRepositoryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/folio/fqm/migration/MigrationStrategyRepositoryTest.java b/src/test/java/org/folio/fqm/migration/MigrationStrategyRepositoryTest.java index 2bbadc92..2e79dcd1 100644 --- a/src/test/java/org/folio/fqm/migration/MigrationStrategyRepositoryTest.java +++ b/src/test/java/org/folio/fqm/migration/MigrationStrategyRepositoryTest.java @@ -25,7 +25,7 @@ class MigrationStrategyRepositoryTest { FqlService fqlService = new FqlService(); - MigrationStrategyRepository migrationStrategyRepository = new MigrationStrategyRepository(); + MigrationStrategyRepository migrationStrategyRepository = new MigrationStrategyRepository(null); MigrationService migrationService = new MigrationService(null, null, new ObjectMapper()); @Test From 5848edd8e2b7679a07f1f01c0d93751593ba2b00 Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Tue, 10 Dec 2024 21:10:45 -0500 Subject: [PATCH 05/15] debug --- .../folio/fqm/migration/strategies/V4DateFieldTimezones.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java index 3751b2bc..79c5aea1 100644 --- a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java +++ b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java @@ -142,6 +142,10 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn query.fqlQuery(), _v -> TARGET_VERSION, (result, key, value) -> { + log.info("key: {}", key); + log.info("value: {}", value); + log.info("value.textValue(): {}", value.textValue()); + log.info("value.asText(): {}", value.asText()); if ( !fieldsToMigrate.contains(key) || // not a date field value.textValue().contains("T") // already has time component From 53271b8401fdf5f89be552f1f658902755b45637 Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Tue, 10 Dec 2024 21:37:04 -0500 Subject: [PATCH 06/15] safer updates --- .../strategies/V4DateFieldTimezones.java | 48 +++++++++++-------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java index 79c5aea1..3401bd02 100644 --- a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java +++ b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java @@ -1,5 +1,6 @@ package org.folio.fqm.migration.strategies; +import com.fasterxml.jackson.databind.node.ObjectNode; import com.fasterxml.jackson.databind.node.TextNode; import java.time.LocalDate; import java.time.ZoneId; @@ -142,31 +143,38 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn query.fqlQuery(), _v -> TARGET_VERSION, (result, key, value) -> { - log.info("key: {}", key); - log.info("value: {}", value); - log.info("value.textValue(): {}", value.textValue()); - log.info("value.asText(): {}", value.asText()); - if ( - !fieldsToMigrate.contains(key) || // not a date field - value.textValue().contains("T") // already has time component - ) { + if (!fieldsToMigrate.contains(key)) { result.set(key, value); // no-op return; } - if (timezone.get() == null) { - timezone.set(configurationClient.getTenantTimezone()); - } + ObjectNode conditions = (ObjectNode) value; - try { - result.set( - key, - new TextNode(LocalDate.parse(value.textValue()).atStartOfDay(timezone.get()).toInstant().toString()) - ); - } catch (DateTimeParseException e) { - log.warn("Could not migrate date {}", value, e); - result.set(key, value); // no-op - } + conditions + .properties() + .forEach(entry -> { + if (entry.getValue().textValue().contains("T")) { + // no-op, we already have a time component + return; + } + + if (timezone.get() == null) { + timezone.set(configurationClient.getTenantTimezone()); + } + + try { + conditions.set( + entry.getKey(), + new TextNode( + LocalDate.parse(entry.getValue().textValue()).atStartOfDay(timezone.get()).toInstant().toString() + ) + ); + } catch (DateTimeParseException e) { + log.warn("Could not migrate date {}", entry, e); + } + }); + + result.set(key, conditions); } ) ); From f194f53393a25038465d171301af5ff73639aaa4 Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Tue, 10 Dec 2024 21:38:23 -0500 Subject: [PATCH 07/15] properly iterate dates --- .../fqm/migration/strategies/V4DateFieldTimezones.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java index 3401bd02..f840640d 100644 --- a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java +++ b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java @@ -153,8 +153,10 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn conditions .properties() .forEach(entry -> { - if (entry.getValue().textValue().contains("T")) { - // no-op, we already have a time component + if ( + !entry.getValue().isTextual() || // no-op, may be a boolean or something else + entry.getValue().textValue().contains("T") // no-op, we already have a time component + ) { return; } From 23edcadb2f79442467bd599efd0e181b997c77bd Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Tue, 10 Dec 2024 21:58:14 -0500 Subject: [PATCH 08/15] more fix --- .../java/org/folio/fqm/migration/MigrationUtils.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/folio/fqm/migration/MigrationUtils.java b/src/main/java/org/folio/fqm/migration/MigrationUtils.java index cc3ada37..880a3eb7 100644 --- a/src/main/java/org/folio/fqm/migration/MigrationUtils.java +++ b/src/main/java/org/folio/fqm/migration/MigrationUtils.java @@ -8,6 +8,7 @@ import com.fasterxml.jackson.databind.node.ObjectNode; import jakarta.validation.constraints.NotNull; import java.io.UncheckedIOException; +import java.util.Optional; import java.util.function.Function; import lombok.experimental.UtilityClass; import lombok.extern.log4j.Log4j2; @@ -43,7 +44,11 @@ public static String migrateFql( fql.set( MigrationService.VERSION_KEY, - objectMapper.valueToTree(versionTransformer.apply(fql.get(MigrationService.VERSION_KEY).asText())) + objectMapper.valueToTree( + versionTransformer.apply( + Optional.ofNullable(fql.get(MigrationService.VERSION_KEY)).map(JsonNode::asText).orElse(null) + ) + ) ); fql = migrateFqlTree(fql, handler); @@ -89,6 +94,9 @@ private static ObjectNode migrateFqlTree(ObjectNode fql, TriConsumer Date: Wed, 11 Dec 2024 13:09:28 -0500 Subject: [PATCH 09/15] Test migration utils --- .../folio/fqm/migration/MigrationUtils.java | 4 +- .../strategies/V4DateFieldTimezones.java | 136 +++++------- .../fqm/migration/MigrationUtilsTest.java | 199 ++++++++++++++++++ 3 files changed, 250 insertions(+), 89 deletions(-) create mode 100644 src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java diff --git a/src/main/java/org/folio/fqm/migration/MigrationUtils.java b/src/main/java/org/folio/fqm/migration/MigrationUtils.java index 880a3eb7..934d6870 100644 --- a/src/main/java/org/folio/fqm/migration/MigrationUtils.java +++ b/src/main/java/org/folio/fqm/migration/MigrationUtils.java @@ -29,7 +29,7 @@ public class MigrationUtils { * @param fqlQuery The root query to migrate * @param versionTransformer A function that takes the current (potentially null) version and * returns the new one to be persisted in the query - * @param handler something that takes the result node, the field name, and the field's query value, + * @param handler something that takes the result node, the field name, and the field's query object, * applies some transformation, and stores the results back in result * @throws JsonMappingException * @throws JsonProcessingException @@ -70,7 +70,7 @@ public static String migrateFql( * This conveniently handles `$and`s, allowing logic to be handled on fields only. * * @param fql the fql node - * @param handler something that takes the result node, the field name, and the field's query value, + * @param handler something that takes the result node, the field name, and the field's query object, * applies some transformation, and stores the results back in result * @return */ diff --git a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java index f840640d..7b3735d7 100644 --- a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java +++ b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezones.java @@ -5,9 +5,7 @@ import java.time.LocalDate; import java.time.ZoneId; import java.time.format.DateTimeParseException; -import java.util.Map; import java.util.Set; -import java.util.UUID; import java.util.concurrent.atomic.AtomicReference; import lombok.RequiredArgsConstructor; import lombok.extern.log4j.Log4j2; @@ -35,88 +33,54 @@ public class V4DateFieldTimezones implements MigrationStrategy { public static final String TARGET_VERSION = "5"; // must snapshot this point in time, as the entity types stored within may change past this migration - private static final Map> DATE_FIELDS = Map.ofEntries( - Map.entry( - UUID.fromString("8418e512-feac-4a6a-a56d-9006aab31e33"), - Set.of("holdings.created_at", "holdings.updated_at") - ), - Map.entry( - UUID.fromString("6b08439b-4f8e-4468-8046-ea620f5cfb74"), - Set.of("instance.cataloged_date", "instance.created_at", "instance.updated_at") - ), - Map.entry( - UUID.fromString("d0213d22-32cf-490f-9196-d81c3c66e53f"), - Set.of( - "holdings.created_at", - "holdings.updated_at", - "instances.cataloged_date", - "instances.created_at", - "instances.updated_at", - "items.circulation_notes[*]->date", - "items.created_date", - "items.last_check_in_date_time", - "items.updated_date" - ) - ), - Map.entry( - UUID.fromString("d6729885-f2fb-4dc7-b7d0-a865a7f461e4"), - Set.of( - "holdings.created_at", - "holdings.updated_at", - "instance.cataloged_date", - "instance.created_at", - "instance.updated_at", - "items.circulation_notes[*]->date", - "items.created_date", - "items.last_check_in_date_time", - "items.updated_date", - "loans.checkout_date", - "loans.claimed_returned_date", - "loans.created_at", - "loans.declared_lost_date", - "loans.due_date", - "loans.return_date", - "users.expiration_date" - ) - ), - Map.entry( - UUID.fromString("b5ffa2e9-8080-471a-8003-a8c5a1274503"), - Set.of("changelogs[*]->timestamp", "created_at", "edi_job_scheduling_date", "updated_at") - ), - Map.entry( - UUID.fromString("abc777d3-2a45-43e6-82cb-71e8c96d13d2"), - Set.of( - "assigned_to_user.user_created_date", - "assigned_to_user.user_updated_date", - "po_created_by_user.user_created_date", - "po_created_by_user.user_updated_date", - "po_updated_by_user.user_created_date", - "po_updated_by_user.user_updated_date", - "po.created_at", - "po.updated_at", - "pol_created_by_user.user_created_date", - "pol_created_by_user.user_updated_date", - "pol_updated_by_user.user_created_date", - "pol_updated_by_user.user_updated_date", - "pol.created_at", - "pol.eresource_expected_activation", - "pol.physical_expected_receipt_date", - "pol.receipt_date", - "pol.updated_at" - ) - ), - Map.entry( - UUID.fromString("ddc93926-d15a-4a45-9d9c-93eadc3d9bbf"), - Set.of( - "users.created_date", - "users.date_of_birth", - "users.enrollment_date", - "users.expiration_date", - "users.updated_date", - "users.user_created_date", - "users.user_updated_date" - ) - ) + // these names are unique enough that we don't need a per-ET listing + private static final Set DATE_FIELDS = Set.of( + "assigned_to_user.user_created_date", + "assigned_to_user.user_updated_date", + "changelogs[*]->timestamp", + "created_at", + "edi_job_scheduling_date", + "holdings.created_at", + "holdings.updated_at", + "instance.cataloged_date", + "instance.created_at", + "instance.updated_at", + "instances.cataloged_date", + "instances.created_at", + "instances.updated_at", + "items.circulation_notes[*]->date", + "items.created_date", + "items.last_check_in_date_time", + "items.updated_date", + "loans.checkout_date", + "loans.claimed_returned_date", + "loans.created_at", + "loans.declared_lost_date", + "loans.due_date", + "loans.return_date", + "po_created_by_user.user_created_date", + "po_created_by_user.user_updated_date", + "po_updated_by_user.user_created_date", + "po_updated_by_user.user_updated_date", + "po.created_at", + "po.updated_at", + "pol_created_by_user.user_created_date", + "pol_created_by_user.user_updated_date", + "pol_updated_by_user.user_created_date", + "pol_updated_by_user.user_updated_date", + "pol.created_at", + "pol.eresource_expected_activation", + "pol.physical_expected_receipt_date", + "pol.receipt_date", + "pol.updated_at", + "updated_at", + "users.created_date", + "users.date_of_birth", + "users.enrollment_date", + "users.expiration_date", + "users.updated_date", + "users.user_created_date", + "users.user_updated_date" ); private final ConfigurationClient configurationClient; @@ -133,8 +97,6 @@ public boolean applies(String version) { @Override public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryInformation query) { - Set fieldsToMigrate = DATE_FIELDS.getOrDefault(query.entityTypeId(), Set.of()); - // avoid initializing this if we don't actually need it AtomicReference timezone = new AtomicReference<>(); @@ -143,7 +105,7 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn query.fqlQuery(), _v -> TARGET_VERSION, (result, key, value) -> { - if (!fieldsToMigrate.contains(key)) { + if (!DATE_FIELDS.contains(key)) { result.set(key, value); // no-op return; } diff --git a/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java b/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java new file mode 100644 index 00000000..49743a91 --- /dev/null +++ b/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java @@ -0,0 +1,199 @@ +package org.folio.fqm.migration; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.jupiter.api.Assertions.fail; + +import com.fasterxml.jackson.databind.node.TextNode; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Function; +import lombok.extern.log4j.Log4j2; +import org.apache.commons.lang3.tuple.Pair; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +@Log4j2 +class MigrationUtilsTest { + + static List functionCallTestCases() { + return List.of( + // query, version transformation expected calls, list of field transformation calls + Arguments.of("{}", null, List.of()), + Arguments.of("{\"_version\":\"1\"}", "1", List.of()), + // basic single-field query + Arguments.of("{\"field\":{\"$eq\":\"foo\"}}", null, List.of(Pair.of("field", "{\"$eq\":\"foo\"}"))), + Arguments.of( + "{\"_version\":\"1\", \"field\":{\"$eq\":\"foo\"}}", + "1", + List.of(Pair.of("field", "{\"$eq\":\"foo\"}")) + ), + // multi-field query, without $and + Arguments.of( + """ + { + "field1": {"$eq": "foo"}, + "field2": {"$le": "bar"}, + "field3": {"$ne": "baz"} + } + """, + null, + List.of( + Pair.of("field1", "{\"$eq\":\"foo\"}"), + Pair.of("field2", "{\"$le\":\"bar\"}"), + Pair.of("field3", "{\"$ne\":\"baz\"}") + ) + ), + Arguments.of( + """ + { + "_version": "newest and coolest", + "field1": {"$eq": "foo"}, + "field2": {"$le": "bar"}, + "field3": {"$ne": "baz"} + } + """, + "newest and coolest", + List.of( + Pair.of("field1", "{\"$eq\":\"foo\"}"), + Pair.of("field2", "{\"$le\":\"bar\"}"), + Pair.of("field3", "{\"$ne\":\"baz\"}") + ) + ), + // multi-operator single-field query + Arguments.of( + """ + {"field1": { + "$le": 500, + "$ge": 100 + }} + """, + null, + List.of(Pair.of("field1", "{\"$le\":500,\"$ge\":100}")) + ), + // query with $and + Arguments.of( + """ + {"$and": [ + { "field": {"$ne": "foo"} }, + { "field": {"$ne": "bar"} } + ]} + """, + null, + List.of(Pair.of("field", "{\"$ne\":\"foo\"}"), Pair.of("field", "{\"$ne\":\"bar\"}")) + ), + // putting everything together + Arguments.of( + """ + { + "_version": "1", + "field1": {"$eq": "foo"}, + "field2": {"$le": "bar"}, + "field3": { + "$ne": "baz", + "$gt": 100 + }, + "$and": [ + {"field4": {"$ne": "foo"}}, + {"field5": { + "$ne": "bar", + "$lt": 100 + }} + ] + } + """, + "1", + List.of( + Pair.of("field1", "{\"$eq\":\"foo\"}"), + Pair.of("field2", "{\"$le\":\"bar\"}"), + Pair.of("field3", "{\"$ne\":\"baz\",\"$gt\":100}"), + Pair.of("field4", "{\"$ne\":\"foo\"}"), + Pair.of("field5", "{\"$ne\":\"bar\",\"$lt\":100}") + ) + ) + ); + } + + @ParameterizedTest + @MethodSource("functionCallTestCases") + void testFunctionCalls(String query, String versionArgument, List> fieldArguments) { + AtomicInteger versionTransformationCalls = new AtomicInteger(0); + Function versionTransformer = input -> { + assertThat(input, is(versionArgument)); + versionTransformationCalls.incrementAndGet(); + return "newVersion"; + }; + + List> fieldArgumentsLeftToGet = new ArrayList<>(fieldArguments); + + MigrationUtils.migrateFql( + query, + versionTransformer, + (node, field, value) -> { + assertThat(node, is(notNullValue())); + assertThat(field, is(notNullValue())); + assertThat(value, is(notNullValue())); + + Pair actual = Pair.of(field, value.toString()); + if (fieldArgumentsLeftToGet.contains(actual)) { + fieldArgumentsLeftToGet.remove(actual); + } else { + fail("Unexpected field transformation call: " + actual.getLeft() + " -> " + actual.getRight()); + } + } + ); + + assertThat(versionTransformationCalls.get(), is(1)); + assertThat(fieldArgumentsLeftToGet, is(empty())); + } + + @Test + void testReturnedResultWithNoFieldsOrVersion() { + assertThat( + MigrationUtils.migrateFql("{}", v -> "new version", (r, k, v) -> {}), + is(equalTo("{\"_version\":\"new version\"}")) + ); + } + + @Test + void testReturnedResultWithVersionAndNoFields() { + assertThat( + MigrationUtils.migrateFql("{\"_version\":\"old\"}", v -> "new version", (r, k, v) -> {}), + is(equalTo("{\"_version\":\"new version\"}")) + ); + } + + @Test + void testReturnedResultWithVersionAndFields() { + assertThat( + MigrationUtils.migrateFql( + "{\"_version\":\"old\",\"test\":{}}", + v -> "new version", + // this is solely responsible for determining what gets set back into the query + // (excluding the special _version) + (result, k, v) -> result.set("foo", new TextNode("bar")) + ), + is(equalTo("{\"_version\":\"new version\",\"foo\":\"bar\"}")) + ); + } + + @Test + void testReturnedResultWithAndOperator() { + assertThat( + MigrationUtils.migrateFql( + "{\"_version\":\"old\",\"$and\":[{\"test\":{}}]}", + v -> "new version", + // this is solely responsible for determining what gets set back into the query + // (excluding the special _version) + (result, k, v) -> result.set("foo", new TextNode("bar")) + ), + is(equalTo("{\"_version\":\"new version\",\"$and\":[{\"foo\":\"bar\"}]}")) + ); + } +} From ecbb6b7030e59006b67f5942c7181d954cf9c808 Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Wed, 11 Dec 2024 13:12:40 -0500 Subject: [PATCH 10/15] add exceptional check --- .../java/org/folio/fqm/migration/MigrationUtilsTest.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java b/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java index 49743a91..539ce599 100644 --- a/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java +++ b/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java @@ -5,9 +5,11 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; +import static org.junit.Assert.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import com.fasterxml.jackson.databind.node.TextNode; +import java.io.UncheckedIOException; import java.util.ArrayList; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; @@ -196,4 +198,9 @@ void testReturnedResultWithAndOperator() { is(equalTo("{\"_version\":\"new version\",\"$and\":[{\"foo\":\"bar\"}]}")) ); } + + @Test + void testInvalidJson() { + assertThrows(UncheckedIOException.class, () -> MigrationUtils.migrateFql("invalid", v -> null, (r, k, v) -> {})); + } } From a78340e022f4b24de21e44fab6f24a310e461f81 Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Wed, 11 Dec 2024 13:31:16 -0500 Subject: [PATCH 11/15] refactor migration constant storage to remove cycle --- docs/Migration.md | 2 ++ .../fqm/config/MigrationConfiguration.java | 23 +++++++++++++++++++ .../AbstractSimpleMigrationStrategy.java | 14 ++++++----- .../MigrationStrategyRepository.java | 4 ++-- .../folio/fqm/migration/MigrationUtils.java | 15 ++++++------ .../migration/strategies/V0POCMigration.java | 2 ++ ....java => V4DateFieldTimezoneAddition.java} | 4 ++-- .../folio/fqm/service/MigrationService.java | 22 ++++++------------ .../AbstractSimpleMigrationStrategyTest.java | 8 ++++--- .../MigrationStrategyRepositoryTest.java | 8 ++++++- .../fqm/migration/MigrationUtilsTest.java | 4 ++-- 11 files changed, 67 insertions(+), 39 deletions(-) create mode 100644 src/main/java/org/folio/fqm/config/MigrationConfiguration.java rename src/main/java/org/folio/fqm/migration/strategies/{V4DateFieldTimezones.java => V4DateFieldTimezoneAddition.java} (97%) diff --git a/docs/Migration.md b/docs/Migration.md index 6b94f85a..b6a779a3 100644 --- a/docs/Migration.md +++ b/docs/Migration.md @@ -33,6 +33,8 @@ Any change to an entity type that results in a field being removed or renamed sh 1. Describe the [warnings](#warnings) in this migration by implementing any of the applicable methods. 1. Add your new strategy to `src/main/java/org/folio/fqm/migration/MigrationStrategyRepository.java`. 1. Update the `CURRENT_VERSION` in `src/main/java/org/folio/fqm/service/MigrationService.java`. +1. If something fancy is being done, or you want to go above and beyond, write a custom test. You can see `src/test/java/org/folio/fqm/migration/strategies/TestTemplate` and `V0POCMigrationTest` for a framework that can easily be extended for common test case formats. + - Implementations of `AbstractSimpleMigrationStrategy` are automatically tested via `MigrationStrategyRepositoryTest`, however, this is just for basic smoke tests and contains no logic to test specifics of an actual migration. Not all use cases can be covered with `AbstractSimpleMigrationStrategy`; in that case, a custom migration will be needed. See [advanced migrations](#advanced-migrations) for more details. diff --git a/src/main/java/org/folio/fqm/config/MigrationConfiguration.java b/src/main/java/org/folio/fqm/config/MigrationConfiguration.java new file mode 100644 index 00000000..060bb142 --- /dev/null +++ b/src/main/java/org/folio/fqm/config/MigrationConfiguration.java @@ -0,0 +1,23 @@ +package org.folio.fqm.config; + +import java.util.UUID; +import org.springframework.context.annotation.Configuration; + +@Configuration +public class MigrationConfiguration { + + public static final String VERSION_KEY = "_version"; + public static final UUID REMOVED_ENTITY_TYPE_ID = UUID.fromString("deadbeef-dead-dead-dead-deaddeadbeef"); + + private static final String CURRENT_VERSION = "5"; + // TODO: replace this with current version in the future? + private static final String DEFAULT_VERSION = "0"; + + public String getCurrentVersion() { + return CURRENT_VERSION; + } + + public String getDefaultVersion() { + return DEFAULT_VERSION; + } +} diff --git a/src/main/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategy.java b/src/main/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategy.java index dc01594e..2f54f805 100644 --- a/src/main/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategy.java +++ b/src/main/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategy.java @@ -14,13 +14,13 @@ import java.util.function.Function; import lombok.extern.log4j.Log4j2; import org.folio.fql.service.FqlService; +import org.folio.fqm.config.MigrationConfiguration; import org.folio.fqm.migration.warnings.EntityTypeWarning; import org.folio.fqm.migration.warnings.FieldWarning; import org.folio.fqm.migration.warnings.QueryBreakingWarning; import org.folio.fqm.migration.warnings.RemovedEntityWarning; import org.folio.fqm.migration.warnings.RemovedFieldWarning; import org.folio.fqm.migration.warnings.Warning; -import org.folio.fqm.service.MigrationService; @Log4j2 public abstract class AbstractSimpleMigrationStrategy implements MigrationStrategy { @@ -84,8 +84,10 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn if (entityTypeWarning.isPresent() && entityTypeWarning.get() instanceof RemovedEntityWarning) { return MigratableQueryInformation .builder() - .entityTypeId(MigrationService.REMOVED_ENTITY_TYPE_ID) - .fqlQuery(objectMapper.writeValueAsString(Map.of(MigrationService.VERSION_KEY, this.getTargetVersion()))) + .entityTypeId(MigrationConfiguration.REMOVED_ENTITY_TYPE_ID) + .fqlQuery( + objectMapper.writeValueAsString(Map.of(MigrationConfiguration.VERSION_KEY, this.getTargetVersion())) + ) .fields(List.of()) .warning(getEntityTypeWarnings().get(src.entityTypeId()).apply(src.fqlQuery())) .build(); @@ -96,7 +98,7 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn if (src.fqlQuery() == null) { result = result.withFqlQuery( - objectMapper.writeValueAsString(Map.of(MigrationService.VERSION_KEY, this.getTargetVersion())) + objectMapper.writeValueAsString(Map.of(MigrationConfiguration.VERSION_KEY, this.getTargetVersion())) ); } @@ -109,7 +111,7 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn String newFql = MigrationUtils.migrateFql( result.fqlQuery(), - _v -> this.getTargetVersion(), + originalVersion -> this.getTargetVersion(), (fql, key, value) -> { if (fieldWarnings.containsKey(key)) { FieldWarning warning = fieldWarnings.get(key).apply(key, value.toPrettyString()); @@ -158,7 +160,7 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn } protected static String getNewFieldName(Map fieldChanges, String oldFieldName) { - if (MigrationService.VERSION_KEY.equals(oldFieldName)) { + if (MigrationConfiguration.VERSION_KEY.equals(oldFieldName)) { return oldFieldName; } else if (fieldChanges.containsKey("*")) { return fieldChanges.get("*").formatted(oldFieldName); diff --git a/src/main/java/org/folio/fqm/migration/MigrationStrategyRepository.java b/src/main/java/org/folio/fqm/migration/MigrationStrategyRepository.java index 714b9b7c..d0f01d77 100644 --- a/src/main/java/org/folio/fqm/migration/MigrationStrategyRepository.java +++ b/src/main/java/org/folio/fqm/migration/MigrationStrategyRepository.java @@ -6,7 +6,7 @@ import org.folio.fqm.migration.strategies.V1ModeOfIssuanceConsolidation; import org.folio.fqm.migration.strategies.V2ResourceTypeConsolidation; import org.folio.fqm.migration.strategies.V3RamsonsFieldCleanup; -import org.folio.fqm.migration.strategies.V4DateFieldTimezones; +import org.folio.fqm.migration.strategies.V4DateFieldTimezoneAddition; import org.springframework.stereotype.Component; @Component @@ -21,7 +21,7 @@ public MigrationStrategyRepository(ConfigurationClient configurationClient) { new V1ModeOfIssuanceConsolidation(), new V2ResourceTypeConsolidation(), new V3RamsonsFieldCleanup(), - new V4DateFieldTimezones(configurationClient) + new V4DateFieldTimezoneAddition(configurationClient) ); } diff --git a/src/main/java/org/folio/fqm/migration/MigrationUtils.java b/src/main/java/org/folio/fqm/migration/MigrationUtils.java index 934d6870..00f0b71e 100644 --- a/src/main/java/org/folio/fqm/migration/MigrationUtils.java +++ b/src/main/java/org/folio/fqm/migration/MigrationUtils.java @@ -6,14 +6,13 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; -import jakarta.validation.constraints.NotNull; import java.io.UncheckedIOException; import java.util.Optional; -import java.util.function.Function; +import java.util.function.UnaryOperator; import lombok.experimental.UtilityClass; import lombok.extern.log4j.Log4j2; import org.apache.commons.lang3.function.TriConsumer; -import org.folio.fqm.service.MigrationService; +import org.folio.fqm.config.MigrationConfiguration; @Log4j2 @UtilityClass @@ -35,18 +34,18 @@ public class MigrationUtils { * @throws JsonProcessingException */ public static String migrateFql( - @NotNull String fqlQuery, - Function versionTransformer, + String fqlQuery, + UnaryOperator versionTransformer, TriConsumer handler ) { try { ObjectNode fql = (ObjectNode) objectMapper.readTree(fqlQuery); fql.set( - MigrationService.VERSION_KEY, + MigrationConfiguration.VERSION_KEY, objectMapper.valueToTree( versionTransformer.apply( - Optional.ofNullable(fql.get(MigrationService.VERSION_KEY)).map(JsonNode::asText).orElse(null) + Optional.ofNullable(fql.get(MigrationConfiguration.VERSION_KEY)).map(JsonNode::asText).orElse(null) ) ) ); @@ -92,7 +91,7 @@ private static ObjectNode migrateFqlTree(ObjectNode fql, TriConsumer TARGET_VERSION, + originalVersion -> TARGET_VERSION, (result, key, value) -> { if (!DATE_FIELDS.contains(key)) { result.set(key, value); // no-op diff --git a/src/main/java/org/folio/fqm/service/MigrationService.java b/src/main/java/org/folio/fqm/service/MigrationService.java index 797b1f30..71922fcc 100644 --- a/src/main/java/org/folio/fqm/service/MigrationService.java +++ b/src/main/java/org/folio/fqm/service/MigrationService.java @@ -5,11 +5,11 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; import java.util.Optional; -import java.util.UUID; import javax.annotation.CheckForNull; import lombok.AllArgsConstructor; import lombok.extern.log4j.Log4j2; import org.folio.fql.service.FqlService; +import org.folio.fqm.config.MigrationConfiguration; import org.folio.fqm.migration.MigratableQueryInformation; import org.folio.fqm.migration.MigrationStrategy; import org.folio.fqm.migration.MigrationStrategyRepository; @@ -21,21 +21,13 @@ @AllArgsConstructor(onConstructor_ = @Autowired) public class MigrationService { - public static final String VERSION_KEY = "_version"; - - public static final UUID REMOVED_ENTITY_TYPE_ID = UUID.fromString("deadbeef-dead-dead-dead-deaddeadbeef"); - - protected static final String CURRENT_VERSION = "5"; - - // TODO: replace this with current version in the future? - protected static final String DEFAULT_VERSION = "0"; - private final FqlService fqlService; + private final MigrationConfiguration migrationConfiguration; private final MigrationStrategyRepository migrationStrategyRepository; private final ObjectMapper objectMapper; public String getLatestVersion() { - return CURRENT_VERSION; + return migrationConfiguration.getCurrentVersion(); } public boolean isMigrationNeeded(@CheckForNull String fqlQuery) { @@ -61,15 +53,15 @@ public MigratableQueryInformation migrate(MigratableQueryInformation migratableQ public String getVersion(@CheckForNull String fqlQuery) { if (fqlQuery == null) { - return DEFAULT_VERSION; + return migrationConfiguration.getDefaultVersion(); } try { return Optional - .ofNullable(((ObjectNode) objectMapper.readTree(fqlQuery)).get(VERSION_KEY)) + .ofNullable(((ObjectNode) objectMapper.readTree(fqlQuery)).get(MigrationConfiguration.VERSION_KEY)) .map(JsonNode::asText) - .orElse(DEFAULT_VERSION); + .orElse(migrationConfiguration.getDefaultVersion()); } catch (JsonProcessingException e) { - return DEFAULT_VERSION; + return migrationConfiguration.getDefaultVersion(); } } } diff --git a/src/test/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategyTest.java b/src/test/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategyTest.java index 1f903d5d..dad33dc2 100644 --- a/src/test/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategyTest.java +++ b/src/test/java/org/folio/fqm/migration/AbstractSimpleMigrationStrategyTest.java @@ -12,6 +12,7 @@ import java.util.function.BiFunction; import java.util.function.Function; import org.folio.fql.service.FqlService; +import org.folio.fqm.config.MigrationConfiguration; import org.folio.fqm.migration.warnings.DeprecatedEntityWarning; import org.folio.fqm.migration.warnings.DeprecatedFieldWarning; import org.folio.fqm.migration.warnings.EntityTypeWarning; @@ -20,7 +21,6 @@ import org.folio.fqm.migration.warnings.RemovedEntityWarning; import org.folio.fqm.migration.warnings.RemovedFieldWarning; import org.folio.fqm.migration.warnings.Warning; -import org.folio.fqm.service.MigrationService; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -276,10 +276,12 @@ static List sourcesForMigrationResults() { List.of("field1", "foo", "field2") ), new MigratableQueryInformation( - MigrationService.REMOVED_ENTITY_TYPE_ID, + MigrationConfiguration.REMOVED_ENTITY_TYPE_ID, "{\"_version\":\"target\"}", List.of(), - List.of(RemovedEntityWarning.withoutAlternative("0b").apply("{\"_version\":\"source\",\"foo\":{\"$eq\":\"foo\"}}")) + List.of( + RemovedEntityWarning.withoutAlternative("0b").apply("{\"_version\":\"source\",\"foo\":{\"$eq\":\"foo\"}}") + ) ) ), // field-level warnings diff --git a/src/test/java/org/folio/fqm/migration/MigrationStrategyRepositoryTest.java b/src/test/java/org/folio/fqm/migration/MigrationStrategyRepositoryTest.java index 2e79dcd1..e3e0de33 100644 --- a/src/test/java/org/folio/fqm/migration/MigrationStrategyRepositoryTest.java +++ b/src/test/java/org/folio/fqm/migration/MigrationStrategyRepositoryTest.java @@ -12,6 +12,7 @@ import lombok.extern.log4j.Log4j2; import org.apache.commons.lang3.tuple.Pair; import org.folio.fql.service.FqlService; +import org.folio.fqm.config.MigrationConfiguration; import org.folio.fqm.service.MigrationService; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; @@ -26,7 +27,12 @@ class MigrationStrategyRepositoryTest { FqlService fqlService = new FqlService(); MigrationStrategyRepository migrationStrategyRepository = new MigrationStrategyRepository(null); - MigrationService migrationService = new MigrationService(null, null, new ObjectMapper()); + MigrationService migrationService = new MigrationService( + null, + new MigrationConfiguration(), + null, + new ObjectMapper() + ); @Test void testHasStrategies() { diff --git a/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java b/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java index 539ce599..b16e24f8 100644 --- a/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java +++ b/src/test/java/org/folio/fqm/migration/MigrationUtilsTest.java @@ -13,7 +13,7 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Function; +import java.util.function.UnaryOperator; import lombok.extern.log4j.Log4j2; import org.apache.commons.lang3.tuple.Pair; import org.junit.jupiter.api.Test; @@ -126,7 +126,7 @@ static List functionCallTestCases() { @MethodSource("functionCallTestCases") void testFunctionCalls(String query, String versionArgument, List> fieldArguments) { AtomicInteger versionTransformationCalls = new AtomicInteger(0); - Function versionTransformer = input -> { + UnaryOperator versionTransformer = input -> { assertThat(input, is(versionArgument)); versionTransformationCalls.incrementAndGet(); return "newVersion"; From a25f18bec56e920a9f1c0bca4e8fafa101d8d39d Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Wed, 11 Dec 2024 13:37:46 -0500 Subject: [PATCH 12/15] Add V4 specific migration test --- .../V4DateFieldTimezoneAdditionTest.java | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 src/test/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAdditionTest.java diff --git a/src/test/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAdditionTest.java b/src/test/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAdditionTest.java new file mode 100644 index 00000000..f35f1e99 --- /dev/null +++ b/src/test/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAdditionTest.java @@ -0,0 +1,88 @@ +package org.folio.fqm.migration.strategies; + +import static org.mockito.Mockito.when; + +import java.time.ZoneId; +import java.util.List; +import java.util.UUID; +import org.folio.fqm.client.ConfigurationClient; +import org.folio.fqm.migration.MigratableQueryInformation; +import org.folio.fqm.migration.MigrationStrategy; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.provider.Arguments; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class V4DateFieldTimezoneAdditionTest extends TestTemplate { + + @Mock + ConfigurationClient configurationClient; + + @BeforeEach + public void setup() { + when(configurationClient.getTenantTimezone()).thenReturn(ZoneId.of("America/New_York")); + } + + @Override + public MigrationStrategy getStrategy() { + return new V4DateFieldTimezoneAddition(configurationClient); + } + + @Override + public List getExpectedTransformations() { + return List.of( + Arguments.of( + "Query with dates", + MigratableQueryInformation + .builder() + .entityTypeId(UUID.fromString("3ad4b672-a69c-5a80-b483-bbc77c29cbfd")) + .fqlQuery( + """ + { + "items.updated_date": { + "$ne": "2024-05-01T12:34:56Z", + "$eq": "2024-07-01", + "$leq": "2024-11-01", + "$empty": false + }, + "items.last_check_in_date_time": { + "$eq": "2024-01-01" + }, + "items.created_date": { + "$eq": "invalid" + }, + "unrelated.field": { + "$contains": "2024-07-01" + } + } + """ + ) + .fields(List.of()) + .build(), + MigratableQueryInformation + .builder() + .entityTypeId(UUID.fromString("3ad4b672-a69c-5a80-b483-bbc77c29cbfd")) + .fqlQuery( + """ + { + "items.updated_date": { + "$ne": "2024-05-01T12:34:56Z", + "$eq": "2024-07-01T04:00:00Z", + "$leq": "2024-11-01T04:00:00Z", + "$empty": false + }, + "items.last_check_in_date_time": { "$eq": "2024-01-01T05:00:00Z" }, + "items.created_date": { "$eq": "invalid" }, + "unrelated.field": { "$contains": "2024-07-01" }, + "_version": "5" + } + """ + ) + .fields(List.of()) + .build() + ) + ); + } +} From a9d742f525e552412081c3b823eae8f1f6275cf6 Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Wed, 11 Dec 2024 13:46:14 -0500 Subject: [PATCH 13/15] fix test --- src/test/java/org/folio/fqm/service/MigrationServiceTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/java/org/folio/fqm/service/MigrationServiceTest.java b/src/test/java/org/folio/fqm/service/MigrationServiceTest.java index 862e6778..2af07dfa 100644 --- a/src/test/java/org/folio/fqm/service/MigrationServiceTest.java +++ b/src/test/java/org/folio/fqm/service/MigrationServiceTest.java @@ -15,6 +15,7 @@ import java.util.List; import lombok.RequiredArgsConstructor; import org.folio.fql.service.FqlService; +import org.folio.fqm.config.MigrationConfiguration; import org.folio.fqm.migration.MigratableQueryInformation; import org.folio.fqm.migration.MigrationStrategy; import org.folio.fqm.migration.MigrationStrategyRepository; @@ -34,6 +35,9 @@ class MigrationServiceTest { @Mock private FqlService fqlService; + @Spy + private MigrationConfiguration migrationConfiguration; + @Mock private MigrationStrategyRepository migrationStrategyRepository; From cd562189b4322d7354819fef4d1fd6e847cd9883 Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Wed, 11 Dec 2024 14:05:53 -0500 Subject: [PATCH 14/15] imports --- .../java/org/folio/fqm/migration/strategies/V0POCMigration.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/org/folio/fqm/migration/strategies/V0POCMigration.java b/src/main/java/org/folio/fqm/migration/strategies/V0POCMigration.java index a3140331..361c9a49 100644 --- a/src/main/java/org/folio/fqm/migration/strategies/V0POCMigration.java +++ b/src/main/java/org/folio/fqm/migration/strategies/V0POCMigration.java @@ -4,8 +4,6 @@ import java.util.UUID; import java.util.function.BiFunction; import java.util.function.Function; -import lombok.AllArgsConstructor; -import lombok.RequiredArgsConstructor; import org.folio.fqm.migration.AbstractSimpleMigrationStrategy; import org.folio.fqm.migration.warnings.EntityTypeWarning; import org.folio.fqm.migration.warnings.FieldWarning; From 7e422ccfb04b7703fe539d95291e80fc09f5f775 Mon Sep 17 00:00:00 2001 From: Noah Overcash Date: Wed, 11 Dec 2024 14:22:01 -0500 Subject: [PATCH 15/15] Use proper suffixes for dates --- .../V4DateFieldTimezoneAddition.java | 5 ++++- .../fqm/service/FqlToSqlConverterService.java | 5 +++-- .../V4DateFieldTimezoneAdditionTest.java | 20 ++++++++++++------- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAddition.java b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAddition.java index f61c3935..836286f8 100644 --- a/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAddition.java +++ b/src/main/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAddition.java @@ -14,6 +14,7 @@ import org.folio.fqm.migration.MigratableQueryInformation; import org.folio.fqm.migration.MigrationStrategy; import org.folio.fqm.migration.MigrationUtils; +import org.folio.fqm.service.FqlToSqlConverterService; /** * Version 4 -> 5, handles addition of time component to date queries. These are not required for the query to run, @@ -130,7 +131,9 @@ public MigratableQueryInformation apply(FqlService fqlService, MigratableQueryIn conditions.set( entry.getKey(), new TextNode( - LocalDate.parse(entry.getValue().textValue()).atStartOfDay(timezone.get()).toInstant().toString() + FqlToSqlConverterService.DATE_TIME_FORMATTER.format( + LocalDate.parse(entry.getValue().textValue()).atStartOfDay(timezone.get()) + ) ) ); } catch (DateTimeParseException e) { diff --git a/src/main/java/org/folio/fqm/service/FqlToSqlConverterService.java b/src/main/java/org/folio/fqm/service/FqlToSqlConverterService.java index 4e4fa06b..4d668ecf 100644 --- a/src/main/java/org/folio/fqm/service/FqlToSqlConverterService.java +++ b/src/main/java/org/folio/fqm/service/FqlToSqlConverterService.java @@ -33,6 +33,7 @@ import java.time.LocalDate; import java.time.LocalDateTime; +import java.time.ZoneId; import java.time.format.DateTimeFormatter; import java.util.List; import java.util.Map; @@ -65,8 +66,8 @@ public class FqlToSqlConverterService { * any record matching the provided date will be retrieved. If datetime is provided in any other form, * an exact comparison will be performed. */ - private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd"); - private static final DateTimeFormatter DATE_TIME_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSS"); + public static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd"); + public static final DateTimeFormatter DATE_TIME_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSS").withZone(ZoneId.of("UTC")); private static final String DATE_REGEX = "^\\d{4}-\\d{2}-\\d{2}$"; private static final String DATE_TIME_REGEX = "^\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d{3}$"; private static final String STRING_TYPE = "stringType"; diff --git a/src/test/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAdditionTest.java b/src/test/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAdditionTest.java index f35f1e99..15e6db5d 100644 --- a/src/test/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAdditionTest.java +++ b/src/test/java/org/folio/fqm/migration/strategies/V4DateFieldTimezoneAdditionTest.java @@ -42,7 +42,7 @@ public List getExpectedTransformations() { """ { "items.updated_date": { - "$ne": "2024-05-01T12:34:56Z", + "$ne": "2024-05-01T12:34:56.000", "$eq": "2024-07-01", "$leq": "2024-11-01", "$empty": false @@ -68,14 +68,20 @@ public List getExpectedTransformations() { """ { "items.updated_date": { - "$ne": "2024-05-01T12:34:56Z", - "$eq": "2024-07-01T04:00:00Z", - "$leq": "2024-11-01T04:00:00Z", + "$ne": "2024-05-01T12:34:56.000", + "$eq": "2024-07-01T04:00:00.000", + "$leq": "2024-11-01T04:00:00.000", "$empty": false }, - "items.last_check_in_date_time": { "$eq": "2024-01-01T05:00:00Z" }, - "items.created_date": { "$eq": "invalid" }, - "unrelated.field": { "$contains": "2024-07-01" }, + "items.last_check_in_date_time": { + "$eq": "2024-01-01T05:00:00.000" + }, + "items.created_date": { + "$eq": "invalid" + }, + "unrelated.field": { + "$contains": "2024-07-01" + }, "_version": "5" } """