Fix/expunge historical version exception message #6880
Conversation
…on message for missing entries
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/expunge/PartitionRunner.java:130
- [nitpick] Consider adding a separator (e.g., ': ') between Msg.code(1085) and cause.getMessage() to improve the clarity of the exception message.
throw new InternalErrorException(Msg.code(1085) + cause.getMessage(), cause);
|
Formatting check succeeded! |
…cal-version-exception-message
|
Formatting check succeeded! |
| myResourceHistoryTableDao.findById(theNextVersionId).orElseThrow(IllegalArgumentException::new); | ||
| ResourceHistoryTable version = myResourceHistoryTableDao | ||
| .findById(theNextVersionId) | ||
| .orElseThrow(() -> new IllegalArgumentException(MessageFormat.format( |
There was a problem hiding this comment.
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)
| .orElseThrow(() -> new IllegalArgumentException(MessageFormat.format( | |
| .orElseThrow(() -> new IllegalArgumentException(Msg.code(2701) + MessageFormat.format( |
| ResourceHistoryTablePk pk = new ResourceHistoryTablePk(); | ||
| pk.setPartitionIdValue(42); | ||
| // set versionId via reflection to avoid null in message | ||
| Field versionIdField = ResourceHistoryTablePk.class.getDeclaredField("myVersionId"); |
There was a problem hiding this comment.
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
| when(myResourceHistoryTableDao.findById(pk)).thenReturn(Optional.empty()); | ||
|
|
||
| AtomicInteger remaining = new AtomicInteger(1); | ||
| IllegalArgumentException ex = assertThrows( |
There was a problem hiding this comment.
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:
Summary
Improve exception handling in the asynchronous Expunge job so that missing historical versions throw a clear, contextual error instead of HAPI‑2223: null.
Changes
In JpaResourceExpungeService.expungeHistoricalVersion(), replace orElseThrow(IllegalArgumentException::new) with a lambda that includes the missing version’s ID and partition in the exception message.
Update PartitionRunner’s error‑handling to extract cause.getMessage() and propagate it, ensuring the logged and thrown InternalErrorException carries the real root‑cause text.
Add a simple unit test to verify that an absent history entry yields a non‑null, descriptive exception message.
Related issue
Closes #6879