Skip to content

Fix/expunge historical version exception message #6880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix/expunge historical version exception message #6880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
746518f
fix: improve error handling and logging in expungeHistoricalVersion p…
patrick-werner Apr 17, 2025
9fe2b4d
fix formatting
patrick-werner Apr 17, 2025
c63e5dc
feat: add test for expungeHistoricalVersions method to verify excepti…
patrick-werner Apr 17, 2025
b7de5b6
Merge remote-tracking branch 'origin/master' into fix/expunge-histori…
patrick-werner May 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions ...r-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/JpaResourceExpungeService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;

import java.text.MessageFormat;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -228,8 +229,12 @@ private void expungeHistoricalVersion(
RequestDetails theRequestDetails,
ResourceHistoryTablePk theNextVersionId,
AtomicInteger theRemainingCount) {
ResourceHistoryTable version =
myResourceHistoryTableDao.findById(theNextVersionId).orElseThrow(IllegalArgumentException::new);
ResourceHistoryTable version = myResourceHistoryTableDao
.findById(theNextVersionId)
.orElseThrow(() -> new IllegalArgumentException(MessageFormat.format(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should probably include an error code here - We try to include them on all newly thrown exception messages that might end up visible to an end user. (FYI we use this page to reserve them)

Suggested change
.orElseThrow(() -> new IllegalArgumentException(MessageFormat.format(
.orElseThrow(() -> new IllegalArgumentException(Msg.code(2701) + MessageFormat.format(

"No historical version found for ResourceHistoryTablePk: {0} (partition {1})",
theNextVersionId.getId(), theNextVersionId.getPartitionId())));

IdDt id = version.getIdDt();
ourLog.info("Deleting resource version {}", id.getValue());

Expand Down
33 changes: 33 additions & 0 deletions ...aserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/JpaResourceExpungeServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import ca.uhn.fhir.jpa.dao.data.IResourceHistoryTableDao;
import ca.uhn.fhir.jpa.dao.data.IResourceTableDao;
import ca.uhn.fhir.jpa.model.dao.JpaPid;
import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTablePk;
import ca.uhn.fhir.jpa.model.entity.ResourceTable;
import ca.uhn.fhir.model.primitive.IdDt;
import ca.uhn.fhir.rest.api.server.RequestDetails;
Expand All @@ -16,9 +17,13 @@
import org.mockito.junit.jupiter.MockitoExtension;
import org.slf4j.LoggerFactory;

import java.lang.reflect.Field;
import java.util.Collections;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicInteger;

import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
Expand Down Expand Up @@ -56,4 +61,32 @@ public void testExpungeDoesNotDeleteAllSearchParams() {
myService.expungeCurrentVersionOfResource(myRequestDetails, JpaPid.fromId(1L), new AtomicInteger(1));
verify(myService, never()).deleteAllSearchParams(any());
}


@Test
public void testExpungeHistoricalVersions_missingEntry_throwsWithContextualMessage() throws NoSuchFieldException, IllegalAccessException {
// given a primary key with no existing history
ResourceHistoryTablePk pk = new ResourceHistoryTablePk();
pk.setPartitionIdValue(42);
// set versionId via reflection to avoid null in message
Field versionIdField = ResourceHistoryTablePk.class.getDeclaredField("myVersionId");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we could just add a setter to this field so the test has better type safety... No harm in adding it, it's just not there 'cause it wasn't needed before

versionIdField.setAccessible(true);
versionIdField.set(pk, 123L);

when(myResourceHistoryTableDao.findById(pk)).thenReturn(Optional.empty());

AtomicInteger remaining = new AtomicInteger(1);
IllegalArgumentException ex = assertThrows(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: This is fine, but @fil512 just pointed out to me how much nicer the assertThatThrownBy syntax can be for this kind of assertion the other day:

hapi-fhir/hapi-fhir-base/src/test/java/ca/uhn/fhir/interceptor/executor/InterceptorServiceTest.java

Lines 698 to 700 in 75162c5

assertThatThrownBy(() -> myInterceptorService.runWithFilterHooks(Pointcut.TEST_FILTER, myParams, mySupplier))
.hasMessageContaining("Supplier was not executed in filter")
.hasMessageContaining("InterceptorServiceTest$FilterHooks$1.testFilter");

IllegalArgumentException.class,
() -> myService.expungeHistoricalVersions(myRequestDetails, Collections.singletonList(pk), remaining)
);

String msg = ex.getMessage();
assertTrue(msg.contains("No historical version found"),
"Expected exception message to contain 'No historical version found'");
assertTrue(msg.contains("ResourceHistoryTablePk: 123"),
"Expected message to include 'ResourceHistoryTablePk: 123'");
assertTrue(msg.contains("partition 42"),
"Expected message to include partition id '42'");
}
}
3 changes: 2 additions & 1 deletion hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/expunge/PartitionRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@ public <T> void runInPartitionedThreads(List<T> theResourceIds, Consumer<List<T>
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
ourLog.error("Error while " + myProcessName, e);
throw new InternalErrorException(Msg.code(1085) + e);
Throwable cause = e.getCause() != null ? e.getCause() : e;
throw new InternalErrorException(Msg.code(1085) + cause.getMessage(), cause);
} finally {
executorService.shutdown();
}
Expand Down
Loading