Skip to content

O3-5693: Resolve previousQueueEntry via a column to avoid N+1 #117

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
UjjawalPrabhat wants to merge 5 commits into
base: main
Choose a base branch
from
Open

O3-5693: Resolve previousQueueEntry via a column to avoid N+1 #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d1537a
O3-5693: Resolve previousQueueEntry via a column to avoid N+1
UjjawalPrabhat Jun 13, 2026
524d556
O3-5693: Populate previousQueueEntry on all writes; fix backfill self…
Jul 24, 2026
527fc8f
O3-5693: Fix changelog XML parsing; ignore voided previous entries
UjjawalPrabhat Jul 29, 2026
e758134
Merge branch 'main' into O3-5693-queue-entry-overfetch
UjjawalPrabhat Aug 31, 2026
817a603
O3-5693: Address review feedback on previousQueueEntry
UjjawalPrabhat Aug 31, 2026
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
12 changes: 7 additions & 5 deletions api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,16 @@ public interface QueueEntryService {
List<QueueEntry> getOverlappingQueueEntries(Patient patient, Queue queue, Date startedAt, Date endedAt);

/**
* Given a specified queue entry Q, return its previous queue entry P, where P has same patient and
* visit as Q, and P.endedAt time is same as Q.startedAt time, and P.queue is same as
* Given a specified queue entry Q, return the previous queue entry recorded against it. This is set
* when Q is created, either directly by a transition or, for a queue entry saved with a
* queueComingFrom, by looking up the entry P with the same patient and, when Q has a visit, the
* same visit as Q, whose endedAt time is the same as Q.startedAt time, and whose queue is
* Q.queueComingFrom
*
* @param queueEntry
* @return the previous queue entry, null otherwise.
* @throws IllegalStateException if multiple previous queue entries are identified
* @return the previous queue entry recorded against this entry. Returns null when no predecessor
* was recorded at creation (no match, or more than one match), or when the recorded
* predecessor is voided.
*/
@Authorized(PrivilegeConstants.GET_QUEUE_ENTRIES)
QueueEntry getPreviousQueueEntry(@NotNull QueueEntry queueEntry);
Expand Down Expand Up @@ -100,7 +103,6 @@ public interface QueueEntryService {
* @param queueEntry the queue entry to undo transition to. Must be active
* @return the previous queue entry, re-activated
* @throws IllegalArgumentException if the previous queue entry does not exist
* @throws IllegalStateException if multiple previous entries are identified
*/
@Authorized({ PrivilegeConstants.MANAGE_QUEUE_ENTRIES })
QueueEntry undoTransition(@NotNull QueueEntry queueEntry);
Expand Down
24 changes: 14 additions & 10 deletions api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ public List<QueueEntry> getOverlappingQueueEntries(Patient patient, Queue queue,
public QueueEntry saveQueueEntry(QueueEntry queueEntry) {
Double sortWeight = getSortWeightGenerator().generateSortWeight(queueEntry);
queueEntry.setSortWeight(sortWeight);
if (queueEntry.getId() == null && queueEntry.getPreviousQueueEntry() == null) {
queueEntry.setPreviousQueueEntry(resolvePreviousQueueEntry(queueEntry));
}
return dao.createOrUpdate(queueEntry);
}

Expand Down Expand Up @@ -183,6 +186,8 @@ public QueueEntry undoTransition(@NotNull QueueEntry queueEntry) {
throw new IllegalStateException("Previous queue entry was modified by another transaction");
}

// Cleared before voiding so that voidQueueEntry's own save persists it, rather than a second write
queueEntry.setPreviousQueueEntry(null);
getProxiedQueueEntryService().voidQueueEntry(queueEntry, "Transition undone");

// Reload the previous entry to return the updated state
Expand Down Expand Up @@ -295,6 +300,14 @@ private static Date roundToSecond(Date date) {
@Override
@Transactional(readOnly = true)
public QueueEntry getPreviousQueueEntry(@NotNull QueueEntry queueEntry) {
QueueEntry previousQueueEntry = queueEntry.getPreviousQueueEntry();
if (previousQueueEntry == null || previousQueueEntry.getVoided()) {
return null;
}
return previousQueueEntry;
}

private QueueEntry resolvePreviousQueueEntry(QueueEntry queueEntry) {
Queue queueComingFrom = queueEntry.getQueueComingFrom();
if (queueComingFrom == null) {
return null;
Expand All @@ -307,15 +320,6 @@ public QueueEntry getPreviousQueueEntry(@NotNull QueueEntry queueEntry) {
criteria.setQueues(Collections.singletonList(queueComingFrom));

List<QueueEntry> prevQueueEntries = dao.getQueueEntries(criteria);

if (prevQueueEntries.size() == 1) {
return prevQueueEntries.get(0);
} else if (prevQueueEntries.size() > 1) {
// TODO: Exceptions should be translatable and human readable on the frontend.
// See: https://openmrs.atlassian.net/browse/O3-2988
throw new IllegalStateException("Multiple previous queue entries found");
} else {
return null;
}
return prevQueueEntries.size() == 1 ? prevQueueEntries.get(0) : null;
}
}
8 changes: 8 additions & 0 deletions api/src/main/java/org/openmrs/module/queue/model/QueueEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import lombok.NoArgsConstructor;
import lombok.Setter;
import lombok.ToString;
import org.hibernate.annotations.BatchSize;
import org.openmrs.BaseChangeableOpenmrsData;
import org.openmrs.Concept;
import org.openmrs.Location;
Expand All @@ -38,6 +39,7 @@
@Getter
@ToString
@Entity
@BatchSize(size = 100)

@github-actions github-actions Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The batch size is load-bearing for the whole point of this change, and nothing in the suite would notice if it went away. Without it, reading previousQueueEntry across a page initializes each proxy on its own select, so the N+1 comes straight back in a different shape, with every test still green and the payload byte-identical. The column itself is well covered by the new tests; this one property isn't covered by anything.

A test alongside the new ones in the integration suite could pin it: turn statistics on (sessionFactory.getStatistics().setStatisticsEnabled(true)), transition two or three entries, flush and clear, then reload the successors and call getPreviousQueueEntry on each, asserting the prepared-statement count goes up once rather than once per entry. Your call whether that's worth the setup, and it doesn't block merging.

@Table(name = "queue_entry")
public class QueueEntry extends BaseChangeableOpenmrsData {

Expand Down Expand Up @@ -98,6 +100,12 @@ public class QueueEntry extends BaseChangeableOpenmrsData {
@JoinColumn(name = "queue_coming_from", referencedColumnName = "queue_id")
private Queue queueComingFrom;

//The queue entry the patient was transitioned from, if any.
@ToString.Exclude
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "previous_queue_entry", referencedColumnName = "queue_entry_id")
private QueueEntry previousQueueEntry;

@Column(name = "started_at", nullable = false)
private Date startedAt;

Expand Down
1 change: 1 addition & 0 deletions api/src/main/java/org/openmrs/module/queue/model/QueueEntryTransition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public QueueEntry constructNewQueueEntry() {
queueEntry.setLocationWaitingFor(queueEntryToTransition.getLocationWaitingFor());
queueEntry.setProviderWaitingFor(queueEntryToTransition.getProviderWaitingFor());
queueEntry.setQueueComingFrom(queueEntryToTransition.getQueue());
queueEntry.setPreviousQueueEntry(queueEntryToTransition);
queueEntry.setStartedAt(transitionDate);
return queueEntry;
}
Expand Down
73 changes: 73 additions & 0 deletions api/src/main/resources/liquibase.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,4 +798,77 @@
</createIndex>
</changeSet>

<changeSet id="add_previous_queue_entry_to_queue_entry_20260613" author="ujjawalprabhat">
<preConditions onError="WARN" onFail="MARK_RAN">
<tableExists tableName="queue_entry"/>
<not><columnExists tableName="queue_entry" columnName="previous_queue_entry"/></not>
</preConditions>
<comment>
Add column previous_queue_entry to queue entry table
</comment>
<addColumn tableName="queue_entry">
<column name="previous_queue_entry" type="int"/>
</addColumn>
</changeSet>

<changeSet id="add_previous_queue_entry_fk_20260613" author="ujjawalprabhat">
<preConditions onError="WARN" onFail="MARK_RAN">
<columnExists tableName="queue_entry" columnName="previous_queue_entry"/>
<not><foreignKeyConstraintExists foreignKeyTableName="queue_entry" foreignKeyName="queue_entry_previous_queue_entry_fk"/></not>
</preConditions>
<comment>
Add foreign key from queue_entry.previous_queue_entry to queue_entry.queue_entry_id
</comment>
<addForeignKeyConstraint baseColumnNames="previous_queue_entry" baseTableName="queue_entry" constraintName="queue_entry_previous_queue_entry_fk" onDelete="SET NULL" onUpdate="NO ACTION" referencedColumnNames="queue_entry_id" referencedTableName="queue_entry"/>
</changeSet>
Comment thread
UjjawalPrabhat marked this conversation as resolved.

<changeSet id="backfill_previous_queue_entry_20260613" author="ujjawalprabhat" dbms="mysql,mariadb">
<preConditions onFail="MARK_RAN">
<columnExists tableName="queue_entry" columnName="previous_queue_entry"/>
</preConditions>
<comment>Backfill previous_queue_entry for existing entries whose predecessor is unambiguous</comment>
<sql>
update queue_entry curr
inner join (
select curr2.queue_entry_id as curr_id, min(prev.queue_entry_id) as prev_id, count(*) as n
from queue_entry curr2
inner join queue_entry prev
Comment thread
UjjawalPrabhat marked this conversation as resolved.
on prev.queue_entry_id != curr2.queue_entry_id
and prev.patient_id = curr2.patient_id
and prev.queue_id = curr2.queue_coming_from
and prev.ended_at = curr2.started_at
and prev.voided = 0
and (prev.visit_id = curr2.visit_id or (prev.visit_id is null and curr2.visit_id is null))
where curr2.queue_coming_from is not null and curr2.voided = 0
group by curr2.queue_entry_id
) m on m.curr_id = curr.queue_entry_id and m.n = 1
set curr.previous_queue_entry = m.prev_id;
</sql>
</changeSet>

<changeSet id="backfill_previous_queue_entry_20260613_postgres" author="ujjawalprabhat" dbms="postgresql">
<preConditions onFail="MARK_RAN">
<columnExists tableName="queue_entry" columnName="previous_queue_entry"/>
</preConditions>
<comment>Backfill previous_queue_entry for existing entries whose predecessor is unambiguous</comment>
<sql>
UPDATE queue_entry curr
SET previous_queue_entry = m.prev_id
FROM (
SELECT curr2.queue_entry_id AS curr_id, MIN(prev.queue_entry_id) AS prev_id, COUNT(*) AS n
FROM queue_entry curr2
JOIN queue_entry prev
ON prev.queue_entry_id != curr2.queue_entry_id
AND prev.patient_id = curr2.patient_id
AND prev.queue_id = curr2.queue_coming_from
AND prev.ended_at = curr2.started_at
AND prev.voided = false
AND (prev.visit_id = curr2.visit_id OR (prev.visit_id IS NULL AND curr2.visit_id IS NULL))
WHERE curr2.queue_coming_from IS NOT NULL AND curr2.voided = false
GROUP BY curr2.queue_entry_id
) m
WHERE m.curr_id = curr.queue_entry_id AND m.n = 1;
</sql>
</changeSet>

</databaseChangeLog>
72 changes: 70 additions & 2 deletions api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.util.Arrays;
import java.util.Calendar;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -132,6 +133,70 @@ public void shouldCreateNewQueueEntryRecord() {
assertThat(result.getPriority(), is(conceptPriority));
}

@Test
public void saveQueueEntryShouldResolvePreviousEntryFromQueueComingFrom() {
QueueEntry prevEntry = new QueueEntry();
prevEntry.setQueueEntryId(1);

QueueEntry newEntry = new QueueEntry();
newEntry.setQueue(new Queue());
newEntry.setPatient(new Patient());
newEntry.setVisit(new Visit());
newEntry.setStatus(new Concept());
newEntry.setPriority(new Concept());
newEntry.setStartedAt(DateUtils.truncate(new Date(), Calendar.SECOND));
newEntry.setQueueComingFrom(new Queue());

when(dao.getQueueEntries(any())).thenReturn(Collections.singletonList(prevEntry));
when(dao.createOrUpdate(any())).thenAnswer(invocation -> invocation.getArgument(0));

QueueEntry result = queueEntryService.saveQueueEntry(newEntry);

assertThat(result.getPreviousQueueEntry(), equalTo(prevEntry));
Comment thread
UjjawalPrabhat marked this conversation as resolved.
verify(dao).getQueueEntries(queueEntrySearchCriteriaArgumentCaptor.capture());
QueueEntrySearchCriteria criteria = queueEntrySearchCriteriaArgumentCaptor.getValue();
assertThat(criteria.getPatient(), equalTo(newEntry.getPatient()));
assertThat(criteria.getVisit(), equalTo(newEntry.getVisit()));
assertThat(criteria.getEndedOn(), equalTo(newEntry.getStartedAt()));
assertThat(criteria.getQueues(), equalTo(Collections.singletonList(newEntry.getQueueComingFrom())));
}

@Test
public void saveQueueEntryShouldNotSetPreviousEntryWhenMatchIsAmbiguous() {
QueueEntry candidate1 = new QueueEntry();
candidate1.setQueueEntryId(1);
QueueEntry candidate2 = new QueueEntry();
candidate2.setQueueEntryId(2);

QueueEntry newEntry = new QueueEntry();
newEntry.setQueue(new Queue());
newEntry.setPatient(new Patient());
newEntry.setStatus(new Concept());
newEntry.setPriority(new Concept());
newEntry.setStartedAt(DateUtils.truncate(new Date(), Calendar.SECOND));
newEntry.setQueueComingFrom(new Queue());

when(dao.getQueueEntries(any())).thenReturn(Arrays.asList(candidate1, candidate2));
when(dao.createOrUpdate(any())).thenAnswer(invocation -> invocation.getArgument(0));

QueueEntry result = queueEntryService.saveQueueEntry(newEntry);

assertNull(result.getPreviousQueueEntry());
}

@Test
public void getPreviousQueueEntryShouldTreatAVoidedPredecessorAsAbsent() {
QueueEntry prevEntry = new QueueEntry();
prevEntry.setQueueEntryId(1);
prevEntry.setVoided(true);

QueueEntry queueEntry = new QueueEntry();
queueEntry.setQueueEntryId(2);
queueEntry.setPreviousQueueEntry(prevEntry);

assertNull(queueEntryService.getPreviousQueueEntry(queueEntry));
}

@Test
public void shouldVoidQueueEntry() {
User user = new User(1);
Expand Down Expand Up @@ -247,6 +312,7 @@ public void shouldTransitionQueueEntry() {
assertThat(queueEntry2.getQueueComingFrom(), equalTo(queue1));
assertThat(queueEntry2.getStartedAt(), equalTo(date2));
assertNull(queueEntry2.getEndedAt());
assertThat(queueEntry2.getPreviousQueueEntry(), equalTo(queueEntry1));

// Next transition test that appropriate fields can be changed
QueueEntryTransition transition2 = new QueueEntryTransition();
Expand All @@ -270,6 +336,7 @@ public void shouldTransitionQueueEntry() {
assertThat(queueEntry3.getQueueComingFrom(), equalTo(queue1));
assertThat(queueEntry3.getStartedAt(), equalTo(date3));
assertNull(queueEntry3.getEndedAt());
assertThat(queueEntry3.getPreviousQueueEntry(), equalTo(queueEntry2));
}

@Test
Expand Down Expand Up @@ -326,8 +393,8 @@ public void shouldUndoTransitionQueueEntry() {
transition1.setQueueEntryToTransition(queueEntry1);
transition1.setTransitionDate(date2);
QueueEntry queueEntry2 = queueEntryService.transitionQueueEntry(transition1);
assertThat(queueEntry2.getPreviousQueueEntry(), equalTo(queueEntry1));

when(dao.getQueueEntries(any())).thenReturn(Arrays.asList(queueEntry1));
User user = new User(1);
UserContext userContext = mock(UserContext.class);
when(userContext.getAuthenticatedUser()).thenReturn(user);
Expand All @@ -337,6 +404,7 @@ public void shouldUndoTransitionQueueEntry() {
queueEntryService.undoTransition(queueEntry2);

assertThat(queueEntry2.getVoided(), equalTo(true));
assertNull(queueEntry2.getPreviousQueueEntry());
assertNull(queueEntry1.getEndedAt());
}
finally {
Expand Down Expand Up @@ -453,9 +521,9 @@ public void shouldThrowWhenUndoingTransitionOnConcurrentlyModifiedPreviousEntry(
currentEntry.setPriority(concept1);
currentEntry.setStartedAt(date2);
currentEntry.setQueueComingFrom(queue1);
currentEntry.setPreviousQueueEntry(prevEntry);

when(dao.get(2)).thenReturn(Optional.of(currentEntry));
when(dao.getQueueEntries(any())).thenReturn(Arrays.asList(prevEntry));
when(dao.updateIfUnmodified(any(), any())).thenReturn(false);

queueEntryService.undoTransition(currentEntry);
Expand Down
Loading