TRUNK-6616: Create function in ObsService to efficiently return all previous versions of obs#6042
TRUNK-6616: Create function in ObsService to efficiently return all previous versions of obs#6042jwnasambu wants to merge 7 commits into
Conversation
4432b52 to
ffdaa74
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6042 +/- ##
============================================
- Coverage 59.41% 59.35% -0.06%
- Complexity 9517 9520 +3
============================================
Files 731 731
Lines 38222 38239 +17
Branches 5578 5581 +3
============================================
- Hits 22709 22698 -11
- Misses 13475 13496 +21
- Partials 2038 2045 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@chibongho Kindly feel free to review by PR at your convenient time please! |
| private static final String OBS_ID_QUERY_PARAM = "obsId"; | ||
|
|
||
| private static final String GET_SAVED_STATUS_SQL = "select status from obs where obs_id = :" + OBS_ID_QUERY_PARAM; | ||
|
|
There was a problem hiding this comment.
I wouldn't define these as constants like this. It's inconsistent with everything else.
| List<Integer> ids = session.createNativeQuery(cteSql, Integer.class).setParameter("startId", obs.getObsId()).list(); | ||
|
|
||
| List<Obs> history = new ArrayList<>(ids.size()); | ||
| for (Integer id : ids) { |
There was a problem hiding this comment.
We shouldn't loop through the ids like this. Just run "FROM Obs where Obs.id in (:obsIds)" (or whatever the appropriate JPAQL is here. This isn't that efficient because it requires N+1 selects for N obs.
| * history (from that version back to the original, excluding any newer versions that | ||
| * superseded it). | ||
| * @return a List of Obs representing the full version history, including the given Obs itself | ||
| * @since 2.7.0 |
| * Voided obs are intentionally included because previous versions are always voided when a new | ||
| * revision is saved via {@link #saveObs(Obs, String)}. | ||
| * | ||
| * @param obs the <strong>current/latest</strong> Obs whose version history is to be retrieved; must |
There was a problem hiding this comment.
| * @param obs the <strong>current/latest</strong> Obs whose version history is to be retrieved; must | |
| * @param obs the Obs whose version history is to be retrieved; must |
| * Obs via the {@code previousVersion} association: OpenMRS stores | ||
| * {@code previousVersion} on the <em>newer</em> Obs pointing to the <em>older</em> one | ||
| * it superseded. Passing an intermediate version will silently return only a partial | ||
| * history (from that version back to the original, excluding any newer versions that | ||
| * superseded it). |
There was a problem hiding this comment.
| * Obs via the {@code previousVersion} association: OpenMRS stores | |
| * {@code previousVersion} on the <em>newer</em> Obs pointing to the <em>older</em> one | |
| * it superseded. Passing an intermediate version will silently return only a partial | |
| * history (from that version back to the original, excluding any newer versions that | |
| * superseded it). | |
| * Obs via the {@code previousVersion} association. |
| * it superseded. Passing an intermediate version will silently return only a partial | ||
| * history (from that version back to the original, excluding any newer versions that | ||
| * superseded it). | ||
| * @return a List of Obs representing the full version history, including the given Obs itself |
There was a problem hiding this comment.
| * @return a List of Obs representing the full version history, including the given Obs itself | |
| * @return a List of Obs representing the full version history up to and including the supplied obs |
| * @see org.openmrs.api.ObsService#getObsVersionHistory(Obs) | ||
| * @param obs the Obs to retrieve version history for; must not be null and must have a non-null | ||
| * obsId (precondition enforced by the service layer) | ||
| * @since 2.7.0 |
| * @param obs the Obs to retrieve version history for; must not be null and must have a non-null | ||
| * obsId (precondition enforced by the service layer) |
There was a problem hiding this comment.
| * @param obs the Obs to retrieve version history for; must not be null and must have a non-null | |
| * obsId (precondition enforced by the service layer) | |
| * @param obs the Obs to retrieve version history for; must not be null and must have a non-null | |
| * obsId |
There was a problem hiding this comment.
It's a bad idea for the DAO to rely on the service layer being called. It should still check this itself.
039f1aa to
6eee76d
Compare
| String cteSql = "WITH RECURSIVE version_chain (obs_id, previous_version, date_created) AS (" | ||
| + " SELECT obs_id, previous_version, date_created FROM obs WHERE obs_id = :startId" + " UNION ALL" | ||
| + " SELECT o.obs_id, o.previous_version, o.date_created" + " FROM obs o" | ||
| + " INNER JOIN version_chain vc ON o.obs_id = vc.previous_version" + ")" | ||
| + "SELECT obs_id FROM version_chain ORDER BY date_created DESC"; |
There was a problem hiding this comment.
| String cteSql = "WITH RECURSIVE version_chain (obs_id, previous_version, date_created) AS (" | |
| + " SELECT obs_id, previous_version, date_created FROM obs WHERE obs_id = :startId" + " UNION ALL" | |
| + " SELECT o.obs_id, o.previous_version, o.date_created" + " FROM obs o" | |
| + " INNER JOIN version_chain vc ON o.obs_id = vc.previous_version" + ")" | |
| + "SELECT obs_id FROM version_chain ORDER BY date_created DESC"; | |
| String cteSql = "WITH RECURSIVE version_chain (obs_id, previous_version, date_created) AS (" | |
| + " SELECT obs_id, previous_version, date_created FROM obs WHERE obs_id = :startId UNION ALL" | |
| + " SELECT o.obs_id, o.previous_version, o.date_created FROM obs o" | |
| + " INNER JOIN version_chain vc ON o.obs_id = vc.previous_version)" | |
| + "SELECT obs_id FROM version_chain ORDER BY date_created DESC"; |
There was a problem hiding this comment.
There's actually no point to having the ORDER BY here if we have it in the second query.
| * <p> | ||
| * Voided obs are intentionally included because previous versions are always voided when a new | ||
| * revision is saved via {@link #saveObs(Obs, String)}. |
There was a problem hiding this comment.
| * <p> | |
| * Voided obs are intentionally included because previous versions are always voided when a new | |
| * revision is saved via {@link #saveObs(Obs, String)}. |
| * non-null obsId. The traversal walks backwards from this Obs via the | ||
| * {@code previousVersion} association. | ||
| * @return a List of Obs representing the full version history up to and including the supplied obs | ||
| * @since 3.0.0 |
There was a problem hiding this comment.
We'll likely need to backport this, so @since 3.0.0, 2.9.0 and whatever the next version of 2.8 will be, I think.
There was a problem hiding this comment.
Following up on this: the interface still says and the next 2.8.1 release, but ObsDAO got set to 2.8.7, so the two do not match. And 2.8.7 has since been released (it is the current <release> in the openmrs-api maven-metadata, with 2.8.8-SNAPSHOT as the active 2.8 line now), so a backport merging today would first appear in 2.8.8. Could you set both this and ObsDAO to @since 3.0.0, 2.9.0, 2.8.8? The 2.9.0 and 3.0.0 parts are still accurate since neither has shipped.
There was a problem hiding this comment.
@dkayiwa, thanks for the review. I have addressed the proposed changes, and I learned a lot from your feedback. Kindly feel free to review my PR again at your convenience. Thank you!
| @Override | ||
| public List<Obs> getObsVersionHistory(Obs obs) { | ||
| // Use a single recursive CTE to collect all obs_ids in the previousVersion chain, | ||
| // then load the Obs entities in one IN-clause query — O(1) round-trips instead of O(N). |
There was a problem hiding this comment.
| // then load the Obs entities in one IN-clause query — O(1) round-trips instead of O(N). | |
| // then load the Obs entities in one IN-clause query. |
| assertEquals(originalRange.getLowNormal(), newRange.getLowNormal()); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
We shouldn't just have one happy-path chain of data, but also unrelated observations and a couple of different scenarios (e.g., a 15-obs chain and an obs with no previous members)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Obs version-history API method ChangesObs Version-History Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant ObsService as "ObsServiceImpl"
participant ObsDAO as "HibernateObsDAO"
participant DB as "Database"
Client->>ObsService: getObsVersionHistory(obs)
ObsService->>ObsService: validate obs != null and obs.getObsId() != null
ObsService->>ObsDAO: getObsVersionHistory(obs)
ObsDAO->>DB: execute recursive CTE on previous_version chain
DB-->>ObsDAO: list of obs_id(s)
ObsDAO->>DB: SELECT ... WHERE obs_id IN (...) ORDER BY dateCreated DESC, obsId DESC
DB-->>ObsDAO: ordered Obs rows
ObsDAO-->>ObsService: List<Obs>
ObsService-->>Client: List<Obs>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/main/java/org/openmrs/api/db/hibernate/HibernateObsDAO.java`:
- Around line 84-97: The current implementation in HibernateObsDAO builds a CTE
and then orders results by dateCreated, which can misorder revisions with
identical timestamps; modify the CTE (cteSql) to emit a depth/index (e.g.,
level) alongside obs_id, return the list of ids with their depth, then fetch the
Obs entities via the IN query (using session.createQuery as before) and finally
reconstruct/return the List<Obs> in the exact chain order by sorting the fetched
Obs by that depth mapping (preserve mapping from ids to depth and reorder the
query result to follow the previous_version chain), ensuring you still use
obs.getObsId() as the startId parameter.
In `@api/src/test/java/org/openmrs/api/ObsServiceTest.java`:
- Around line 2194-2203: The test currently creates an unsaved Obs (variable
unsaved) and calls obsService.getObsVersionHistory(unsaved) then asserts a
singleton list, but ObsServiceImpl#getObsVersionHistory is specified to throw
IllegalArgumentException for an Obs with null obsId; update the test in
ObsServiceTest to expect/verify that calling getObsVersionHistory with an
unsaved Obs (null obsId) throws IllegalArgumentException instead of asserting a
returned list, so the test aligns with the service contract and the existing
checks in ObsServiceImpl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fabd464-88a6-4bd8-9584-0fee4dcb064f
📒 Files selected for processing (6)
api/src/main/java/org/openmrs/api/ObsService.javaapi/src/main/java/org/openmrs/api/db/ObsDAO.javaapi/src/main/java/org/openmrs/api/db/hibernate/HibernateObsDAO.javaapi/src/main/java/org/openmrs/api/impl/ObsServiceImpl.javaapi/src/test/java/org/openmrs/api/ObsServiceTest.javaapi/src/test/resources/org/openmrs/api/include/ObsServiceTest-PreviousVersions.xml
ccd758e to
129cd47
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/src/test/java/org/openmrs/api/ObsServiceTest.java (1)
2194-2203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis block tests the undocumented (and incorrect) singleton path for unsaved
Obs— remove it.The
Obscreated here hasobsId == null. The public contract documented inObsService#getObsVersionHistorystates the parameter "must have a non-null obsId", so this block is asserting behaviour the API explicitly prohibits. This issue was flagged in a previous review, and while the implementation was subsequently modified to make the test pass, that modification itself introduced the contract inconsistency flagged inObsServiceImpl.java.Once the implementation is simplified to always throw for
obsId == null, this block becomes a failing test. The right fix is to remove it: the dedicated testgetObsVersionHistory_shouldThrowIllegalArgumentExceptionForObsWithoutId(lines 2236–2238) already covers the "unsaved obs must throw" contract.🧹 Proposed change
- // Obs with no previous members (new unsaved obs) - Obs unsaved = new Obs(); - unsaved.setConcept(latestObs.getConcept()); - unsaved.setPerson(latestObs.getPerson()); - unsaved.setObsDatetime(new java.util.Date()); - unsaved.setValueNumeric(5.0); - List<Obs> unsavedVersions = obsService.getObsVersionHistory(unsaved); - assertNotNull(unsavedVersions); - assertEquals(1, unsavedVersions.size()); - assertNull(unsavedVersions.get(0).getObsId());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/java/org/openmrs/api/ObsServiceTest.java` around lines 2194 - 2203, Remove the test block that constructs an unsaved Obs and calls obsService.getObsVersionHistory(unsaved) (the block that creates new Obs, sets concept/person/obsDatetime/valueNumeric, asserts a single-element list and null obsId). The public contract for ObsService#getObsVersionHistory requires a non-null obsId and there is already a dedicated test getObsVersionHistory_shouldThrowIllegalArgumentExceptionForObsWithoutId covering the unsaved-Obs case, so delete this singleton-path assertions to keep tests consistent with the API contract and with ObsServiceImpl behavior.api/src/main/java/org/openmrs/api/ObsService.java (1)
43-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve the
@sinceplaceholder and add a@throwstag.Great addition to the public API! Two small Javadoc items to clean up before merge:
@sincetag (line 52): The string"the next 2.8.x release"is still a placeholder. A concrete version number (e.g.,2.8.1) is needed so tooling and downstream module developers know exactly which releases carry this method. This was pointed out in a previous review and is still unresolved.Missing
@throws: The implementation throwsIllegalArgumentExceptionfor bothnullobs and obs with anullobsId, but that contract is invisible to callers reading only the interface. Adding a@throwstag here makes the precondition explicit at the API boundary.✏️ Suggested Javadoc update
* `@return` a List of Obs representing the full version history up to and including the supplied obs + * `@throws` IllegalArgumentException if {`@code` obs} is null or its {`@code` obsId} is null - * `@since` 3.0.0, 2.9.0, and the next 2.8.x release + * `@since` 3.0.0, 2.9.0, 2.8.x (replace 2.8.x with the actual patch version)As per coding guidelines: "Flag missing
@sincetags on new public API in the api/ module" and checkstyle's JavadocMethod expectation that@throwsappears in public-method docs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/main/java/org/openmrs/api/ObsService.java` around lines 43 - 55, Update the Javadoc for ObsService.getObsVersionHistory: replace the placeholder phrase in the `@since` tag with a concrete release number (e.g., change "the next 2.8.x release" to a specific version like "2.8.1") and add a `@throws` IllegalArgumentException entry that documents the preconditions (that the obs parameter must not be null and must have a non-null obsId, and that the method will throw IllegalArgumentException when those conditions are violated).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/main/java/org/openmrs/api/impl/ObsServiceImpl.java`:
- Around line 99-107: In ObsServiceImpl, enforce the documented contract by
rejecting any Obs with a null obsId instead of permitting a partially-populated
singleton path: in the method handling the obs parameter (the block that
currently checks obs.getObsId() == null and then conditionally tests
concept/person/obsDatetime), remove the concept/person/obsDatetime conditional
and always throw an IllegalArgumentException when obs.getObsId() == null;
update/remove the corresponding test that relied on the partially-populated
singleton behavior in ObsServiceTest and keep the dedicated
getObsVersionHistory_shouldThrowIllegalArgumentExceptionForObsWithoutId test to
validate the new behavior.
---
Duplicate comments:
In `@api/src/main/java/org/openmrs/api/ObsService.java`:
- Around line 43-55: Update the Javadoc for ObsService.getObsVersionHistory:
replace the placeholder phrase in the `@since` tag with a concrete release number
(e.g., change "the next 2.8.x release" to a specific version like "2.8.1") and
add a `@throws` IllegalArgumentException entry that documents the preconditions
(that the obs parameter must not be null and must have a non-null obsId, and
that the method will throw IllegalArgumentException when those conditions are
violated).
In `@api/src/test/java/org/openmrs/api/ObsServiceTest.java`:
- Around line 2194-2203: Remove the test block that constructs an unsaved Obs
and calls obsService.getObsVersionHistory(unsaved) (the block that creates new
Obs, sets concept/person/obsDatetime/valueNumeric, asserts a single-element list
and null obsId). The public contract for ObsService#getObsVersionHistory
requires a non-null obsId and there is already a dedicated test
getObsVersionHistory_shouldThrowIllegalArgumentExceptionForObsWithoutId covering
the unsaved-Obs case, so delete this singleton-path assertions to keep tests
consistent with the API contract and with ObsServiceImpl behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c11371f0-1b67-40c1-af0d-bb1d7215abe6
📒 Files selected for processing (6)
api/src/main/java/org/openmrs/api/ObsService.javaapi/src/main/java/org/openmrs/api/db/ObsDAO.javaapi/src/main/java/org/openmrs/api/db/hibernate/HibernateObsDAO.javaapi/src/main/java/org/openmrs/api/impl/ObsServiceImpl.javaapi/src/test/java/org/openmrs/api/ObsServiceTest.javaapi/src/test/resources/org/openmrs/api/include/ObsServiceTest-PreviousVersions.xml
✅ Files skipped from review due to trivial changes (1)
- api/src/test/resources/org/openmrs/api/include/ObsServiceTest-PreviousVersions.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/main/java/org/openmrs/api/db/hibernate/HibernateObsDAO.java
- api/src/main/java/org/openmrs/api/db/ObsDAO.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/main/java/org/openmrs/api/impl/ObsServiceImpl.java`:
- Around line 97-101: Delete the stale misleading comment that says "For unsaved
Obs instances (obsId == null), return it as a singleton list." in ObsServiceImpl
so it no longer contradicts the actual behavior; the code immediately below (the
check obs.getObsId() == null that throws new IllegalArgumentException("Obs must
have an obsId")) is the source of truth, so remove the two-comment lines to
avoid confusion for future contributors.
- Around line 93-103: Remove the outdated singleton-path assertions from the
test method getObsVersionHistory_shouldHandleMultipleScenarios(): delete the
block that constructs an unsaved Obs, calls
obsService.getObsVersionHistory(unsaved), and asserts a singleton result (the
snippet that creates "unsaved", calls getObsVersionHistory(unsaved), then
assertNotNull/ assertEquals(1)/ assertNull on the returned list). This will
avoid the conflict with the new behavior (and the new test
shouldThrowIllegalArgumentExceptionForObsWithoutId()) which expects
getObsVersionHistory to throw for Obs with no obsId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca2275a8-8752-44b1-8b51-49dfcac217fc
📒 Files selected for processing (1)
api/src/main/java/org/openmrs/api/impl/ObsServiceImpl.java
cb876dd to
8b64425
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/src/main/java/org/openmrs/api/db/ObsDAO.java (1)
92-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
@sincetag is still incomplete — flagged in a prior review and not yet updated.The
ObsServiceinterface (updated in this same PR) now documents@since 3.0.0, 2.9.0, and the next 2.8.x release. The DAO declaration still only lists@since 3.0.0, which is inconsistent — if this method is being backported, the DAO version should track the same set of releases.📝 Suggested update
- * `@since` 3.0.0 + * `@since` 3.0.0, 2.9.0, and the next 2.8.x release🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/main/java/org/openmrs/api/db/ObsDAO.java` around lines 92 - 98, The `@since` Javadoc on the ObsDAO method getObsVersionHistory(Obs) is outdated; update its `@since` tag to match the ObsService declaration (include "3.0.0, 2.9.0, and the next 2.8.x release") so the DAO and service Javadocs are consistent—edit the Javadoc block above List<Obs> getObsVersionHistory(Obs obs); to replace the single "@since 3.0.0" entry with the full comma-separated set used in ObsService.api/src/main/java/org/openmrs/api/db/hibernate/HibernateObsDAO.java (1)
84-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOrdering by
dateCreated DESC, obsId DESCcan silently produce wrong history order.When multiple revisions share the same
dateCreatedmillisecond timestamp — which happens routinely in unit tests that build chains in a tight loop (see the 15-obs chain atObsServiceTest.javalines 2172–2192) — theobsId DESCtie-breaker becomes the sole ordering mechanism. This is an implicit assumption that auto-increment IDs are always monotonically increasing with time, which the JPA spec does not guarantee.The reliable fix is to carry a
depthcounter through the CTE itself and sort on it, so the traversal order is determined structurally by thepreviousVersionchain rather than by wall-clock time:♻️ Proposed fix — depth-based ordering
- String cteSql = "WITH RECURSIVE version_chain (obs_id, previous_version, date_created) AS (" - + " SELECT obs_id, previous_version, date_created FROM obs WHERE obs_id = :startId UNION ALL" - + " SELECT o.obs_id, o.previous_version, o.date_created FROM obs o" - + " INNER JOIN version_chain vc ON o.obs_id = vc.previous_version)" + "SELECT obs_id FROM version_chain"; + String cteSql = "WITH RECURSIVE version_chain (obs_id, previous_version, depth) AS (" + + " SELECT obs_id, previous_version, 0 FROM obs WHERE obs_id = :startId UNION ALL" + + " SELECT o.obs_id, o.previous_version, vc.depth + 1 FROM obs o" + + " INNER JOIN version_chain vc ON o.obs_id = vc.previous_version)" + + " SELECT obs_id FROM version_chain ORDER BY depth"; Session session = sessionFactory.getCurrentSession(); List<Integer> ids = session.createNativeQuery(cteSql, Integer.class).setParameter("startId", obs.getObsId()).list(); if (ids.isEmpty()) { return Arrays.asList(obs); } - return session - .createQuery("FROM Obs o WHERE o.obsId IN (:obsIds) ORDER BY o.dateCreated DESC, o.obsId DESC", Obs.class) - .setParameter("obsIds", ids).getResultList(); + // Load entities without re-ordering, then reconstruct in the CTE traversal order. + List<Obs> loaded = session.createQuery("FROM Obs o WHERE o.obsId IN (:obsIds)", Obs.class) + .setParameter("obsIds", ids).getResultList(); + Map<Integer, Obs> byId = new java.util.HashMap<>(); + for (Obs o : loaded) { + byId.put(o.getObsId(), o); + } + return ids.stream() + .map(byId::get) + .filter(java.util.Objects::nonNull) + .collect(java.util.stream.Collectors.toList());This approach requires adding
java.util.Mapandjava.util.stream.Collectorsto the imports. TheORDER BY depthin the outerSELECTis deterministic (depth 0 = the supplied obs, depth 1 = its direct prior version, etc.) and does not rely on timestamps or ID ordering at all.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/main/java/org/openmrs/api/db/hibernate/HibernateObsDAO.java` around lines 84 - 98, The current native CTE (cteSql / version_chain) and subsequent query rely on ordering by dateCreated and obsId which can be nondeterministic when timestamps tie; modify the CTE to carry a depth column (e.g., add depth INT with depth=0 for the start row and depth+1 for recursion), change the outer SELECT to return obs_id ordered by depth so session.createNativeQuery(cteSql, Integer.class).setParameter("startId", ...) yields ids in structural history order, then pass that ordered list into the HQL query (replace the existing "ORDER BY o.dateCreated DESC, o.obsId DESC" usage) to preserve deterministic history ordering; also add imports for java.util.Map and java.util.stream.Collectors if you need to re-map results into entities or preserve ordering when building the final List<Obs>.
🧹 Nitpick comments (1)
api/src/main/java/org/openmrs/api/impl/ObsServiceImpl.java (1)
93-101: ⚡ Quick winService-layer validation conflicts with the project convention and creates a behavioral discrepancy with the DAO.
Two related concerns here:
Convention: Per the project's established pattern, input validation (null checks and co-dependent field checks) is expected at the DAO layer rather than in service implementations. The
obs == nullandobs.getObsId() == nullguards belong inHibernateObsDAO.getObsVersionHistory.Behavioral mismatch: When
obs.getObsId() == null, the service throwsIllegalArgumentException, but the DAO returnsArrays.asList(obs). Any code that calls the DAO directly (not via the service proxy) gets silently different behavior — that hidden divergence is exactly the kind of bug that's hard to trace. Aligning them so the DAO throws too makes the contract uniform.The suggested combined fix:
- In
HibernateObsDAO: add a null guard forobs(throwsIAE) and change theobs.getObsId() == nullbranch to also throwIAEinstead of returning a singleton.- In
ObsServiceImpl: remove lines 94–99 entirely, letting the DAO enforce the contract.Based on learnings — the OpenMRS Core convention is that
*ServiceImplclasses delegate validation to the DAO rather than duplicating checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/main/java/org/openmrs/api/impl/ObsServiceImpl.java` around lines 93 - 101, Remove the redundant input validation from ObsServiceImpl.getObsVersionHistory (delete the obs == null and obs.getObsId() == null guards) so the service simply delegates to dao.getObsVersionHistory(obs); add equivalent validation into HibernateObsDAO.getObsVersionHistory so it throws IllegalArgumentException when obs is null or when obs.getObsId() is null (instead of returning a singleton list) to align behavior and preserve the project's convention that the DAO enforces input contracts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/main/java/org/openmrs/api/db/hibernate/HibernateObsDAO.java`:
- Around line 75-79: The method HibernateObsDAO.getObsVersionHistory should
defensively check for a null obs parameter before calling obs.getObsId(); add an
initial null-guard that throws a clear IllegalArgumentException (or another
appropriate checked runtime exception) with a descriptive message like "obs must
not be null" so callers get a meaningful error instead of an NPE, then keep the
existing logic that returns Arrays.asList(obs) when obs.getObsId() is null.
---
Duplicate comments:
In `@api/src/main/java/org/openmrs/api/db/hibernate/HibernateObsDAO.java`:
- Around line 84-98: The current native CTE (cteSql / version_chain) and
subsequent query rely on ordering by dateCreated and obsId which can be
nondeterministic when timestamps tie; modify the CTE to carry a depth column
(e.g., add depth INT with depth=0 for the start row and depth+1 for recursion),
change the outer SELECT to return obs_id ordered by depth so
session.createNativeQuery(cteSql, Integer.class).setParameter("startId", ...)
yields ids in structural history order, then pass that ordered list into the HQL
query (replace the existing "ORDER BY o.dateCreated DESC, o.obsId DESC" usage)
to preserve deterministic history ordering; also add imports for java.util.Map
and java.util.stream.Collectors if you need to re-map results into entities or
preserve ordering when building the final List<Obs>.
In `@api/src/main/java/org/openmrs/api/db/ObsDAO.java`:
- Around line 92-98: The `@since` Javadoc on the ObsDAO method
getObsVersionHistory(Obs) is outdated; update its `@since` tag to match the
ObsService declaration (include "3.0.0, 2.9.0, and the next 2.8.x release") so
the DAO and service Javadocs are consistent—edit the Javadoc block above
List<Obs> getObsVersionHistory(Obs obs); to replace the single "@since 3.0.0"
entry with the full comma-separated set used in ObsService.
---
Nitpick comments:
In `@api/src/main/java/org/openmrs/api/impl/ObsServiceImpl.java`:
- Around line 93-101: Remove the redundant input validation from
ObsServiceImpl.getObsVersionHistory (delete the obs == null and obs.getObsId()
== null guards) so the service simply delegates to
dao.getObsVersionHistory(obs); add equivalent validation into
HibernateObsDAO.getObsVersionHistory so it throws IllegalArgumentException when
obs is null or when obs.getObsId() is null (instead of returning a singleton
list) to align behavior and preserve the project's convention that the DAO
enforces input contracts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 002d7c83-2ec9-45ea-9e2f-272d5b692260
📒 Files selected for processing (6)
api/src/main/java/org/openmrs/api/ObsService.javaapi/src/main/java/org/openmrs/api/db/ObsDAO.javaapi/src/main/java/org/openmrs/api/db/hibernate/HibernateObsDAO.javaapi/src/main/java/org/openmrs/api/impl/ObsServiceImpl.javaapi/src/test/java/org/openmrs/api/ObsServiceTest.javaapi/src/test/resources/org/openmrs/api/include/ObsServiceTest-PreviousVersions.xml
|
@ibacher kindly feel free to review my PR please! |
ibacher
left a comment
ibacher
left a comment
There was a problem hiding this comment.
Two small things, but LGTM otherwise.
| throw new IllegalArgumentException("Obs must have an obsId"); | ||
| } | ||
| // Use a single recursive CTE to collect all obs_ids in the previousVersion chain, | ||
| // then load the Obs entities in one IN-clause query. |
There was a problem hiding this comment.
| // then load the Obs entities in one IN-clause query. | |
| // Use a single recursive CTE to collect all obs_ids in the previousVersion chain, | |
| // then load the Obs entities in one IN-clause query. | |
| String cteSql = "WITH RECURSIVE version_chain (obs_id, previous_version, date_created) AS (" |
| * @see org.openmrs.api.ObsService#getObsVersionHistory(Obs) | ||
| * @param obs the Obs to retrieve version history for; must not be null and must have a non-null | ||
| * obsId | ||
| * @since 3.0.0, 2.9.0, 2.8.1 |
There was a problem hiding this comment.
| * @since 3.0.0, 2.9.0, 2.8.1 | |
| * @since 3.0.0, 2.9.0, 2.8.7 |
998808b to
982db99
Compare
|
There was a problem hiding this comment.
@ibacher Kindly help me understand how to fix the build failure above. Initially, it appeared to be caused by an incorrect context configuration. After fixing the issue locally, the build succeeds, but I am still getting a build failure on GitHub. What beats my understanding even before fixing the build failure, locally the build is successful.
62f6019 to
3baa6f9
Compare
|
@jwnasambu The output I think is pretty clear. You need to run |
…revious versions of obs
|
@ibacher Thanks for pointing that out. Although I fixed the issue, I’m still encountering the original error. This is the same error I reported on slack channel https://pastebin.com/0D2WJNbi |
|
@wikumChamith, @ibacher, @dkayiwa Kindly feel free to review my PR at your convenient time please! |
|
|
@chibongho , @ibacher , @wikumChamith , @dkayiwa kindly help me review this PR at your convenient time please. |
|
5 participants
Description of what I changed
I have updated the ObsService to optimize the retrieval of historical obs data. The previous implementation utilized an iterative approach that triggered a separate database query for every preceding version of an observation, leading to significant performance lag. I have replaced it with a single recursive Common Table Expression (CTE) query allowing the system to fetch the entire version history in one efficient database operation. This change eliminates the "N+1 query" bottleneck and ensures the REST API remains responsive even when handling observations with extensive edit histories.
Issue I worked on
https://openmrs.atlassian.net/browse/TRUNK-6616
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream masterSummary by CodeRabbit
New Features
Behavior Changes
Tests