Audit 49 : Add the fetch Patient Audit Revisions API by sudhanshu-raj · Pull Request #45 · openmrs/openmrs-module-auditlogweb

sudhanshu-raj · 2026-05-15T21:37:54Z

Description of what I changed

Created the API to fetch the patient audit revision history in detailed way by including the changed related audited entities. Also added the separate endpoint to fetch the single entity audit revision by their rev id with entity id. I tried to make the change as generic so that it would work for other entity also.

I changed PR title to what ticket has bcz I wanted to change the ticket title but I can't and this one best fits the changes..

Issue I worked on

see https://openmrs.atlassian.net/browse/AUDIT-

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 --amend
I 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 --amend
I ran mvn clean package right before creating this pull request and
added 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 master

…PI to fetch single revision by revision id of entity with their id

sudhanshu-raj · 2026-05-15T21:42:11Z

Hi @ManojLL @wikumChamith , let me know if there is any change on implementation or are we going good with this ?

ManojLL · 2026-05-16T02:00:57Z

+
+@RestController
+@RequiredArgsConstructor
+@RequestMapping("/rest/" + RestConstants.VERSION_1 + "/auditlogs/patient")


Suggested change

@RequestMapping("/rest/" + RestConstants.VERSION_1 + "/auditlogs/patient")

@RequestMapping("/rest/" + RestConstants.VERSION_1 + "/auditlogs/patients")

Here we fetching the detailed revisions for single patient, right ? So do we still need patients ?

resource names are typically plural because the endpoint represents a collection of resources

corrected !

ManojLL · 2026-05-16T02:03:06Z

+    @GetMapping("/fetchByEntity")
+    public AuditLogDetailDTO getAuditLogByEntity(
+            @RequestParam() String entityName,
+            @RequestParam() String entityId,
+            @RequestParam() Integer revisionId
+    ){
+        if (entityName == null || entityId == null || revisionId == null
+                || entityName.isEmpty() || entityId.isEmpty()) {
+            throw new IllegalArgumentException("Missing the one or more required parameter");
+        }
+
+        Class<?> entityClass = UtilClass.resolveAuditedEntityClass(entityName);
+        if (entityClass == null) {
+            throw new IllegalArgumentException("Cannot find class for " + entityName);
+        }
+
+        Object entityIdVal;
+        if (Role.class.isAssignableFrom(entityClass) || GlobalProperty.class.isAssignableFrom(entityClass)) {
+            entityIdVal = entityId;


why we need this api? instead of adding a new api we can add id filter to existing get all audit logs api

This API is only for to fetch the audit revision for a particular entity only, and to do this we need to add two more filters to existing API method and more checks, existing API fits best to return the list of audit entities and on this new one we are returning only the revision for one entity , so I thought it more cleaner way to create new API to handle this . Let me know if I'm wrong.

can there be multiple revisions for a single entity ID?

Yes, a single entity id can have many revisions but here we fetching the changes for particular revision id , the main purpose of adding this api is because when we return the related changed entities metadata on patient api then from that api response we can easily fetch what all got changed for that related entity .

we can use fetch by revision id for this case right?

ya, revision id + entity name + entity id to get the actual change for entity revision

can't we do the same thing only using revision id?

There can be more than one changes on different entity for a single revision id , so we need more details.

rename the API endpoint to /auditlogs/{revisionId} and pass revisionId as a path variable, while keeping entityName and entityId as query parameters

CC @wikumChamith

ManojLL · 2026-05-16T02:05:30Z

-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.mockStatic;
-import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.*;


remove wild card imports

wikumChamith · 2026-05-16T09:50:27Z

 	</modules>

 	<properties>
-		<openmrsPlatformVersion>2.7.0</openmrsPlatformVersion>


This feels like something we should do in a separate PR. If we're updating the platform version, we'll also have to bump the module's major version too.

Back to 2.7.0 now

wikumChamith · 2026-05-16T09:53:22Z


+    @ExceptionHandler(ObjectNotFoundException.class)
+    public ResponseEntity<Map<String, String>> handleObjectNotFound(ObjectNotFoundException ex) {
+        return buildResponseEntity("Bad Request", ex.getMessage(), HttpStatus.NOT_FOUND);


This gives mixed signals, the body says "Bad Request" while the status is 404

My bad, replaced with BAD_REQUEST now

Is this fixed?

wikumChamith · 2026-05-16T09:54:54Z

        dto.setRevisionID(entity.getRevisionEntity().getId());
        dto.setEntityType(currentEntity.getClass().getSimpleName());
-        dto.setEventType(auditType);
+        dto.setEventType(String.valueOf(entity.getRevisionType()));


Why are we changing this?

So existing auditType returns the human readable string like Record was added , etc but we want clean rest api readable format so that's why returning the the revision type directly wrapping in string something like ADD , MOD , DEL.

wikumChamith · 2026-05-16T09:57:21Z

+     * @return a list of {@link AuditEntity} records for this patient
+     */
+    @Authorized(AuditLogConstants.VIEW_AUDIT_LOGS)
+    List<AuditEntity<?>> getEntityAuditRevisionsById(Integer patientId, Class<?> entityClass, int page, int size, String sortOrder);


Shouldn't this be entityId instead of patientId ?

Ahh I missed it, corrected

wikumChamith · 2026-05-16T10:00:08Z

+@Data
+@AllArgsConstructor
+@NoArgsConstructor
+public class AuditEntityDetailsDTO {


Can't we make the existing DTO carry an optional List<RelatedEntityDto> relatedEntities and reuse AuditLogResponseDto?

That's nyc approach , corrected

ManojLL · 2026-05-16T13:28:02Z

+        try {
+            auditEntity = auditService.getAuditEntityRevisionById(entityClass, entityIdVal, revisionId);
+        } catch (ObjectNotFoundException ex) {
+            log.debug("Audit for entity {} with id {} not found", entityName, entityId);


do we need this log here?

Not really, i have added for testing only will remove it

wikumChamith · 2026-05-18T05:05:39Z

+     * @param sortOrder "asc" or "desc" by revision timestamp
+     * @return a paginated list of {@link AuditEntity} records for the patient
+     */
+    public List<AuditEntity<?>> getRevisionsForEntityById(Integer patientId, Class<?> entityClass, int page, int size, String sortOrder) {


patientId or entityId?

ohh again 🙂, done !

wikumChamith · 2026-05-18T05:06:16Z

+     * @param patientId the integer primary key of the Patient
+     * @return the total number of recorded revisions for this patient
+     */
+    public long countRevisionsForEntityById(Integer patientId, Class<?> entityClass) {


ManojLL · 2026-05-22T06:40:38Z

 @RequestMapping("/rest/" + RestConstants.VERSION_1 + "/auditlogs")
 public class AuditLogRestController {

+    private final Logger log = LoggerFactory.getLogger(AuditLogRestController.class);


do we need this log here?

It's not use of now , I removed it

…DaoTest.java Added new line at AuditDaoTest #390 Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

…lClass.java Removed debug log Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

…edEntityDto.java Removed extra empty lines in RelatedEntityDto Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

…LogDetailDTO.java Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

…LogRestControllerTest.java Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

…LogRestController.java Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

ManojLL · 2026-05-23T15:39:29Z

+    /**
+     * Tests {@code PatientLogRestController#getPatientAuditLogs}
+     * It verifies negative/zero pagination values are normalized to defaults.
+     */


remove comments from tests

ManojLL · 2026-05-23T15:41:17Z

+                entityId = Integer.parseInt(entityId.toString());
+            } catch (NumberFormatException e){
+                //If this exception occurred then it may be the uuid, which we're trying to convert to int, so leave it as string
+                log.debug("Entity ID is not an integer, leaving it as type string");


do we need this debug log?

Sure , we can remove it , comments are enough

wikumChamith · 2026-05-25T05:49:18Z

+
+    @ExceptionHandler(ObjectNotFoundException.class)
+    public ResponseEntity<Map<String, String>> handleObjectNotFound(ObjectNotFoundException ex) {
+        return buildResponseEntity("Bad Request", ex.getMessage(), HttpStatus.BAD_REQUEST);


Why not HttpStatus.NOT_FOUND?

wikumChamith · 2026-05-25T05:57:40Z

+        } catch (Exception ex) {
+            if (isMissingAuditTableException(ex)) {
+                log.warn("Skipping revisions for class {} due to missing audit table: {}", entityClass.getName(), ex.getMessage());
+            } else {
+                log.error("Unexpected error while fetching revisions for class {}: {}", entityClass.getName(), ex.getMessage(), ex);
+            }
+            return new ArrayList<>();
+        }


This is problematic. Both of these branches return an empty list, which could make a DB outage look identical to "no history."

Additionally, if the user asks, "What happened to Patient 42?," a missing Patient_AUD table is not the same as saying "nothing happened to Patient 42."

We need to find a better way to handle this.

Fixed it, now throwing the AuditLogUnavailableException which will propagate to the controller and will show the error message instead of any actual result.

wikumChamith · 2026-05-25T06:01:00Z

+        if (identifier != null && !identifier.isEmpty()) {
+            List<Patient> byIdentifier = patientService.getPatients(null, identifier, null, false);
+            if (!byIdentifier.isEmpty()) {
+                return byIdentifier.get(0);


Identifier matches can be ambiguous (e.g., getPatients(null, "123", null, false) can match across different identifier types), and name lookup will collide for any common name. Returning the first match silently in an audit feature can surface the wrong patient's history.

Yes, it can not be optimal to search on Identifier or patient name to fetch the revision history for a particular patient , because both case can give more than one output. Instead we need to search on unique value, like we already using uuid, and we can also use patient id too .

CC : @ManojLL , let me know if we can replace identifier and name with only patient id or have any better idea

wikumChamith · 2026-05-27T05:23:49Z

+        AuditEntity<?> auditEntity;
+        try {
+            auditEntity = auditService.getAuditEntityRevisionById(entityClass, entityIdVal, revisionId);
+        } catch (ObjectNotFoundException ex) {


Is this the correct exception to catch?

Ohh i thought this could be , but corrected now with NoResultException first

wikumChamith · 2026-05-27T05:24:42Z


+    @ExceptionHandler(ObjectNotFoundException.class)
+    public ResponseEntity<Map<String, String>> handleObjectNotFound(ObjectNotFoundException ex) {
+        return buildResponseEntity("Bad Request", ex.getMessage(), HttpStatus.NOT_FOUND);


Is this fixed?

…ed rest exception handler

sudhanshu-raj added 2 commits May 16, 2026 01:29

Added api to fetch detailed audit revisions for patient and another A…

0573206

…PI to fetch single revision by revision id of entity with their id

Test cases added and code optimized

0604350

ManojLL suggested changes May 16, 2026

View reviewed changes

AUDIT-49 : Removed wildcard import and renames the api

85d28b1

wikumChamith reviewed May 16, 2026

View reviewed changes

sudhanshu-raj added 2 commits May 16, 2026 18:01

AUDIT-49: Switch back to exisiting DTO and other minor updates

706fcb2

AUDIT-49:Fixed patient api name

5881a81

ManojLL reviewed May 16, 2026

View reviewed changes

AUDIT-49: Added test cases for API's

440ecd7

sudhanshu-raj marked this pull request as ready for review May 16, 2026 22:11

AUDIT-49: Back to OpenMRS 2.7.x and handles api param responses

15836c7

wikumChamith reviewed May 18, 2026

View reviewed changes

sudhanshu-raj added 2 commits May 18, 2026 14:53

AUDIT-49: Renamed the function params

2e653bd

AUDIT-49:Replaced fetchEntityRevision with path variable

2b452b7

ManojLL suggested changes May 22, 2026

View reviewed changes

sudhanshu-raj and others added 7 commits May 22, 2026 12:41

Update api/src/test/java/org/openmrs/module/auditlogweb/api/dao/Audit…

5b53659

…DaoTest.java Added new line at AuditDaoTest #390 Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

Update api/src/main/java/org/openmrs/module/auditlogweb/api/utils/Uti…

d78ec2a

…lClass.java Removed debug log Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

Update api/src/main/java/org/openmrs/module/auditlogweb/api/dto/Relat…

4a56375

…edEntityDto.java Removed extra empty lines in RelatedEntityDto Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

Update api/src/main/java/org/openmrs/module/auditlogweb/api/dto/Audit…

c56c869

…LogDetailDTO.java Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

Update api/src/main/java/org/openmrs/module/auditlogweb/api/dto/Audit…

14d18a9

…LogDetailDTO.java Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

Update omod/src/test/java/org/openmrs/module/auditlogweb/rest/Patient…

d094368

…LogRestControllerTest.java Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

Update omod/src/main/java/org/openmrs/module/auditlogweb/rest/Patient…

4dcd121

…LogRestController.java Co-authored-by: Manoj Lakshan <48247516+ManojLL@users.noreply.github.com>

sudhanshu-raj force-pushed the AUDIT-49 branch from 38397b0 to 4dcd121 Compare May 22, 2026 07:58

AUDIT-49: Removed unused logger

e23e254

ManojLL suggested changes May 23, 2026

View reviewed changes

AUDIT-49 : Removed test comments and debug log

58e3e2b

ManojLL requested a review from wikumChamith May 25, 2026 01:02

wikumChamith reviewed May 25, 2026

View reviewed changes

sudhanshu-raj added 2 commits May 25, 2026 15:31

AUDIT-49: Http status for ObjectNotFoundException corrected

261bfd1

AUDIT-49:Handling DAO method exceptions

2ea2587

wikumChamith reviewed May 27, 2026

View reviewed changes

AUDIT-49: Corrected exception handler #getAuditLogByEntity and improv…

2cdd562

…ed rest exception handler

	@RequestMapping("/rest/" + RestConstants.VERSION_1 + "/auditlogs/patient")
	@RequestMapping("/rest/" + RestConstants.VERSION_1 + "/auditlogs/patients")

Conversation

sudhanshu-raj commented May 15, 2026

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Uh oh!

sudhanshu-raj commented May 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManojLL May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sudhanshu-raj May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ManojLL May 16, 2026 •

edited

Loading

sudhanshu-raj May 22, 2026 •

edited

Loading