From 8e8186baa0782dccebbb95d5f5e7ae81caa34d55 Mon Sep 17 00:00:00 2001 From: Robin Arnold Date: Wed, 12 Jan 2022 13:52:36 +0000 Subject: [PATCH] issue #2026 #3174 #3175 history updates for QA Signed-off-by: Robin Arnold --- docs/src/pages/Conformance.md | 4 +- .../jdbc/impl/FHIRPersistenceJDBCImpl.java | 3 +- .../ibm/fhir/server/util/FHIRRestHelper.java | 44 +++++++++++++++---- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/docs/src/pages/Conformance.md b/docs/src/pages/Conformance.md index a31c94c1159..3145b5e4e17 100644 --- a/docs/src/pages/Conformance.md +++ b/docs/src/pages/Conformance.md @@ -133,7 +133,7 @@ There are three possible traversal paths through the data which are requested us Option 1 is useful for users interested in finding the most recent changes that have been applied to the server. Option 2 is useful for system-to-system synchronization because it represents the change history of the server. Option 3 is also useful for system-to-system synchronization cases, but guarantees uniqueness across pages when following the `next` links (versus option 2 which may repeat entries at the time window boundary when multiple resources share the same modification timestamp). -To return all changes that have occurred since a known point in time, use both `_since` and `_sort=_lastUpdated` query parameters: +The `_since` and `_before` search parameters can be used to specify a time range filter. They are both defined as [instant](https://www.hl7.org/fhir/datatypes.html#instant) datatypes which must include a time specified to at least second accuracy and include a timezone. To return all changes that have occurred since a known point in time, use both `_since` and `_sort=_lastUpdated` query parameters: ``` curl -k -u ':' 'https://:/fhir-server/api/v4/_history?_since=2021-02-21T00:00:00Z&_sort=_lastUpdated' @@ -165,6 +165,8 @@ As mentioned above, to simplify client implementations in system-to-system synch In this case, the IBM FHIR Server uses `_changeIdMarker` as a custom paging attribute which references a single `Bundle.entry.id` value. This value can be used by clients to checkpoint where they are in the sequence of changes, and ask for only changes that come after the given id. The simplest way to do this is to follow the `next` link returned in the response Bundle. If the next link is not present in the response Bundle, the end has been reached for the current point in time. Note that `_lastUpdated` and `Bundle.entry.id` values are not perfectly correlated - clients should not mix ordering. The ids used for `Bundle.entry.id` are guaranteed to be unique within a single IBM FHIR Server database (tenant/datasource). +The `_changeIdMarker` query parameter is intended to be used only as part of a `next` link defined within a search result. It is not intended for general use by an end user. + ### Whole System History - Type Filters The query parameter `_type` can be used to limit which resource types are returned from a `_history` request. For example, the following request will return only Patient and Observation resources: diff --git a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java index 1719eecf4df..b33d22cd607 100644 --- a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java +++ b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java @@ -2907,7 +2907,8 @@ private String getResourcePayloadKeyFromContext(FHIRPersistenceContext context) public List readResourcesForRecords(List records) throws FHIRPersistenceException { // TODO support async read from payloadPersistence after issue #2900 is merged. - // Make sure we read deleted resources + // Make sure we read deleted resources...this is important because the result list must + // line up row-for-row with the provided records list FHIRPersistenceContext readContext = FHIRPersistenceContextFactory.createPersistenceContext(null, true); List result = new ArrayList<>(records.size()); for (ResourceChangeLogRecord r: records) { diff --git a/fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java b/fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java index 4ca3604dd58..3cd78d9a066 100644 --- a/fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java +++ b/fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java @@ -93,6 +93,7 @@ import com.ibm.fhir.persistence.HistorySortOrder; import com.ibm.fhir.persistence.InteractionStatus; import com.ibm.fhir.persistence.ResourceChangeLogRecord; +import com.ibm.fhir.persistence.ResourceChangeLogRecord.ChangeType; import com.ibm.fhir.persistence.ResourceEraseRecord; import com.ibm.fhir.persistence.SingleResourceResult; import com.ibm.fhir.persistence.context.FHIRHistoryContext; @@ -3026,10 +3027,18 @@ public Bundle doHistory(MultivaluedMap queryParameters, String r historyContext.getHistorySortOrder()); } - if (historyContext.getReturnPreference() == HTTPReturnPreference.REPRESENTATION) { + if (historyContext.getReturnPreference() == HTTPReturnPreference.REPRESENTATION + || historyContext.getReturnPreference() == HTTPReturnPreference.OPERATION_OUTCOME) { // vread the resources so that we can include them in the response bundle - log.fine("Fetching resources for history response bundle, count=" + records.size()); + if (log.isLoggable(Level.FINE)) { + log.fine("Fetching resources for history response bundle, count=" + records.size()); + } resources = persistence.readResourcesForRecords(records); + + if (resources.size() != records.size()) { + // very unlikely...unless we overlap with a patient erase operation + throw new FHIRPersistenceException("Could not read all required resource records"); + } } } catch (FHIRPersistenceDataAccessException x) { log.log(Level.SEVERE, "Error reading history; params = {" + historyContext + "}", @@ -3050,7 +3059,7 @@ public Bundle doHistory(MultivaluedMap queryParameters, String r // output them in the same order. for (int i=0; i queryParameters, String r entryBuilder.fullUrl(Url.of(changeRecord.getResourceTypeName() + "/" + changeRecord.getLogicalId())); entryBuilder.request(requestBuilder.build()); entryBuilder.response(responseBuilder.build()); - entryBuilder.resource(resource); // remember resource may be null if returnPreference=minimal + + // history bundle entry should not include any resource for deleted entries, + // also resource may be null if returnPreference=minimal + if (changeRecord.getChangeType() != ChangeType.DELETE && resource != null) { + entryBuilder.resource(resource); + } bundleBuilder.entry(entryBuilder.build()); } @@ -3135,9 +3149,9 @@ public Bundle doHistory(MultivaluedMap queryParameters, String r case ASC_LAST_UPDATED: nextRequest.append("&_sort=" + HistorySortOrder.ASC_LAST_UPDATED.toString()); nextRequest.append("&_since=").append(lastChangeTime.atZone(UTC).format(DateTime.PARSER_FORMATTER)); - if (before != null) { + if (historyContext.getBefore() != null && historyContext.getBefore().getValue() != null) { // make sure we keep including the before filter specified in the original request - nextRequest.append("&_before=").append(before.atZone(UTC).format(DateTime.PARSER_FORMATTER)); + nextRequest.append("&_before=").append(historyContext.getBefore().getValue().format(DateTime.PARSER_FORMATTER)); } if (changeIdMarker != null) { @@ -3148,9 +3162,9 @@ public Bundle doHistory(MultivaluedMap queryParameters, String r case DESC_LAST_UPDATED: nextRequest.append("&_sort=" + HistorySortOrder.DESC_LAST_UPDATED.toString()); nextRequest.append("&_before=").append(lastChangeTime.atZone(UTC).format(DateTime.PARSER_FORMATTER)); - if (since != null) { + if (historyContext.getSince() != null && historyContext.getSince().getValue() != null) { // make sure we keep including the since filter specified in the original request - nextRequest.append("&_since=").append(since.atZone(UTC).format(DateTime.PARSER_FORMATTER)); + nextRequest.append("&_since=").append(historyContext.getSince().getValue().format(DateTime.PARSER_FORMATTER)); } if (changeIdMarker != null) { // good way to exclude this resource on the next call @@ -3160,6 +3174,16 @@ public Bundle doHistory(MultivaluedMap queryParameters, String r case NONE: nextRequest.append("&_sort=" + HistorySortOrder.NONE.toString()); nextRequest.append("&_changeIdMarker=").append(changeIdMarker); + // since/before range filters should not be common when using system-defined + // sort order (HistorySortOrder.NONE) but are supported. Note the before + // value will interact with _excludeTransactionTimeoutWindow + if (historyContext.getSince() != null && historyContext.getSince().getValue() != null) { + nextRequest.append("&_since=").append(historyContext.getSince().getValue().format(DateTime.PARSER_FORMATTER)); + } + + if (historyContext.getBefore() != null && historyContext.getBefore().getValue() != null) { + nextRequest.append("&_before=").append(historyContext.getBefore().getValue().format(DateTime.PARSER_FORMATTER)); + } break; } @@ -3187,6 +3211,10 @@ public Bundle doHistory(MultivaluedMap queryParameters, String r selfRequest.append("&_since=").append(historyContext.getSince().getValue().format(DateTime.PARSER_FORMATTER)); } + if (historyContext.getBefore() != null && historyContext.getBefore().getValue() != null) { + selfRequest.append("&_before=").append(historyContext.getBefore().getValue().format(DateTime.PARSER_FORMATTER)); + } + if (resourceType == null && historyContext.getResourceTypes().size() > 0) { selfRequest.append("&_type="); selfRequest.append(String.join(",", historyContext.getResourceTypes()));