From 3cfb0250a7a0fc9f69af59110c6d1d4b96221644 Mon Sep 17 00:00:00 2001 From: Robin Arnold Date: Fri, 12 Aug 2022 15:38:25 +0100 Subject: [PATCH 1/3] issue #3690 fix wall clock time check Signed-off-by: Robin Arnold --- .../persistence/FHIRPersistenceSupport.java | 41 ++++++++++++++----- .../fhir/persistence/ResourceResult.java | 6 +++ 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/FHIRPersistenceSupport.java b/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/FHIRPersistenceSupport.java index 0ef45bf9ef7..782869a781d 100644 --- a/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/FHIRPersistenceSupport.java +++ b/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/FHIRPersistenceSupport.java @@ -11,6 +11,7 @@ import java.time.Instant; import java.time.ZoneOffset; import java.util.List; +import java.util.Objects; import java.util.logging.Level; import java.util.logging.Logger; import java.util.zip.GZIPInputStream; @@ -138,6 +139,9 @@ public static int getMetaVersionId(Resource resource) throws FHIRPersistenceExce * @return */ public static Instant getLastUpdatedFromResource(Resource resource) { + Objects.requireNonNull(resource.getMeta(), "Resource must have a meta element"); + Objects.requireNonNull(resource.getMeta().getLastUpdated(), "Resource must have a lastUpdated value"); + org.linuxforhealth.fhir.model.type.Instant lastUpdated = resource.getMeta().getLastUpdated(); return lastUpdated.getValue().toInstant(); } @@ -146,10 +150,14 @@ public static Instant getLastUpdatedFromResource(Resource resource) { * Get the lastUpdated time to use for the next version of a resource. If a current * version of the resource exists, pass its lastUpdated time as the currenLastUpdated * parameter (all times should be UTC). + * If the gap falls within a grace period of 2 seconds, then we make the new lastUpdated + * time equal the current time + 1 ms to maintain consistent ordering. Clusters should easily + * be able to maintain clocks synchronized within 2 seconds if they are configured with a + * reliable network time service. * @param currentLastUpdated * @return */ - public static org.linuxforhealth.fhir.model.type.Instant getNewLastUpdatedInstant(Instant currentLastUpdated) { + public static org.linuxforhealth.fhir.model.type.Instant getNewLastUpdatedInstant(Instant currentLastUpdated) throws FHIRPersistenceException { Instant lastUpdated = Instant.now(); // Clocks may drift or not be perfectly aligned in clusters. Updates for a given // resource may occur back-to-back and be processed on different nodes. We need @@ -157,16 +165,27 @@ public static org.linuxforhealth.fhir.model.type.Instant getNewLastUpdatedInstan // lastUpdated time so that these changes always appear in order. This can also // happen when virtual machines are migrated between nodes. if (currentLastUpdated != null && !lastUpdated.isAfter(currentLastUpdated)) { - logger.warning("Current clock time " + lastUpdated - + " is not after the lastUpdated time of a current resource version: " + currentLastUpdated - + ". If you see this message frequently, consider improving the accuracy of clock " - + "synchronization in your cluster. Using current lastUpdated plus 1ms instead of " - + "current time."); - - // our solution is to simply adjust lastUpdated so that it falls 1ms - // after the current lastUpdated value, thereby ensuring that - // lastUpdated time always follows version order - lastUpdated = currentLastUpdated.plusMillis(1); + // The wall clock time is before the lastUpdated time of the current version + // in the database. If the gap is under 2 seconds, we simply make the + // new time = old time + 1ms to guarantee we have a reasonable + // ordering of lastUpdated values. Otherwise, it's an error + if (lastUpdated.isAfter(currentLastUpdated.minusSeconds(2))) { + logger.warning("Current clock time " + lastUpdated + + " is not after the lastUpdated time of a current resource version: " + currentLastUpdated + + ". If you see this message frequently, consider improving the accuracy of clock " + + "synchronization in your cluster. Using current lastUpdated plus 1ms instead of " + + "current time."); + + // our solution is to simply adjust lastUpdated so that it falls 1ms + // after the current lastUpdated value, thereby ensuring that + // lastUpdated time always follows version order + lastUpdated = currentLastUpdated.plusMillis(1); + } else { + // should result in 500 server error, which is appropriate in this case + logger.severe("Excessive clock drift - current time is before lastUpdated of current resource. " + + "Check server nodes are configured with a reliable network time service. "); + throw new FHIRPersistenceException("Current time is before lastUpdated of current resource version."); + } } return org.linuxforhealth.fhir.model.type.Instant.of(lastUpdated.atZone(ZoneOffset.UTC)); } diff --git a/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/ResourceResult.java b/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/ResourceResult.java index 204239bca1c..907f9044099 100644 --- a/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/ResourceResult.java +++ b/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/ResourceResult.java @@ -185,6 +185,12 @@ public Builder deleted(boolean flag) { */ public Builder resource(T resource) { this.resource = resource; + + if (resource != null && resource.getMeta() != null && resource.getMeta().getLastUpdated() != null) { + // if we've been given a resource with a lastUpdated field, make sure this lastUpdated value is set + // from it so we know it matches. + this.lastUpdated = FHIRPersistenceSupport.getLastUpdatedFromResource(resource); + } return this; } From f958823b749d0e2336c2c326c77f94d5fd5fcb8f Mon Sep 17 00:00:00 2001 From: Robin Arnold Date: Fri, 12 Aug 2022 17:22:42 +0100 Subject: [PATCH 2/3] issue #3690 use lastUpdated from DTO because undelete will not have a resource Signed-off-by: Robin Arnold --- .../jdbc/impl/FHIRPersistenceJDBCImpl.java | 1 + .../fhir/persistence/ResourceResult.java | 6 ------ .../fhir/persistence/SingleResourceResult.java | 11 +++++++++++ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/fhir-persistence-jdbc/src/main/java/org/linuxforhealth/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java b/fhir-persistence-jdbc/src/main/java/org/linuxforhealth/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java index f70186bfde9..228c86fd76e 100644 --- a/fhir-persistence-jdbc/src/main/java/org/linuxforhealth/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java +++ b/fhir-persistence-jdbc/src/main/java/org/linuxforhealth/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java @@ -1352,6 +1352,7 @@ public SingleResourceResult read(FHIRPersistenceContext .resource(resource) .deleted(resourceIsDeleted) // true if we read something and the is_deleted flag was set .version(resourceDTO != null ? resourceDTO.getVersionId() : 0) + .lastUpdated(resourceDTO != null ? resourceDTO.getLastUpdated().toInstant() : null) .interactionStatus(InteractionStatus.READ) .outcome(getOutcomeIfResourceNotFound(resourceDTO, resourceType.getSimpleName(), logicalId)) .build(); diff --git a/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/ResourceResult.java b/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/ResourceResult.java index 907f9044099..204239bca1c 100644 --- a/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/ResourceResult.java +++ b/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/ResourceResult.java @@ -185,12 +185,6 @@ public Builder deleted(boolean flag) { */ public Builder resource(T resource) { this.resource = resource; - - if (resource != null && resource.getMeta() != null && resource.getMeta().getLastUpdated() != null) { - // if we've been given a resource with a lastUpdated field, make sure this lastUpdated value is set - // from it so we know it matches. - this.lastUpdated = FHIRPersistenceSupport.getLastUpdatedFromResource(resource); - } return this; } diff --git a/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/SingleResourceResult.java b/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/SingleResourceResult.java index a2003a50318..0536a0466fd 100644 --- a/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/SingleResourceResult.java +++ b/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/SingleResourceResult.java @@ -176,6 +176,7 @@ public SingleResourceResult replace(X newResource) { .interactionStatus(this.interactionStatus) .ifNoneMatchVersion(getIfNoneMatchVersion()) .resource(newResource) // the updated resource value + .lastUpdated(getLastUpdated()) .outcome(getOutcome()) .deleted(isDeleted()) .version(getVersion()) @@ -256,6 +257,16 @@ public Builder resource(T resource) { return this; } + /** + * The lastUpdated time read from the database. + * @param lastUpdated + * @return + */ + public Builder lastUpdated(Instant lastUpdated) { + resourceResultBuilder.lastUpdated(lastUpdated); + return this; + } + /** * An OperationOutcome that represents the outcome of the interaction * From 6d4bf34c86dd2cbefd22dfbde16de6870be20b50 Mon Sep 17 00:00:00 2001 From: Robin Arnold Date: Fri, 12 Aug 2022 17:25:11 +0100 Subject: [PATCH 3/3] issue #3690 javadoc from review comment Signed-off-by: Robin Arnold --- .../linuxforhealth/fhir/persistence/FHIRPersistenceSupport.java | 1 + 1 file changed, 1 insertion(+) diff --git a/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/FHIRPersistenceSupport.java b/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/FHIRPersistenceSupport.java index 782869a781d..9f93aa27a78 100644 --- a/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/FHIRPersistenceSupport.java +++ b/fhir-persistence/src/main/java/org/linuxforhealth/fhir/persistence/FHIRPersistenceSupport.java @@ -155,6 +155,7 @@ public static Instant getLastUpdatedFromResource(Resource resource) { * be able to maintain clocks synchronized within 2 seconds if they are configured with a * reliable network time service. * @param currentLastUpdated + * @throws FHIRPersistenceException if current time is 2 or more seconds before currentLastUpdated * @return */ public static org.linuxforhealth.fhir.model.type.Instant getNewLastUpdatedInstant(Instant currentLastUpdated) throws FHIRPersistenceException {