Fix: String object comparison and forward-iteration deletion in medication stash, missing GCN_SEQNO for drug search flow, and drug name encoding on input#2344
Conversation
📝 WalkthroughWalkthroughThe changes fix three interconnected bugs in prescription/drug handling: activating GCN_SEQNO assignment in drug selection, replacing primitive equality checks with null-safe comparison for stash duplicate detection, fixing ArrayList deletion order to prevent index corruption, and updating JavaScript encoding for drug names in the JSP. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts medication stash duplicate detection and deletion logic, restores GCN_SEQNO assignment during drug selection, and fixes drug name encoding/escaping in the prescribing UI to prevent duplicates, crashes, and corrupted names. Sequence diagram for medication re-staging and stash deduplicationsequenceDiagram
actor User
participant prescribe_jsp as prescribe_jsp
participant RxChooseDrug2Action as RxChooseDrug2Action
participant RxWriteScript2Action as RxWriteScript2Action
participant RxSessionBean as RxSessionBean
participant RxPrescription as RxPrescriptionData_Prescription
User->>prescribe_jsp: Selects drug from search results
prescribe_jsp->>RxChooseDrug2Action: execute(drugId)
RxChooseDrug2Action->>RxPrescription: setGCN_SEQNO(drugId)
RxChooseDrug2Action-->>prescribe_jsp: Preconfigured RxPrescription
User->>prescribe_jsp: Save_and_Print or stage_medication
prescribe_jsp->>RxWriteScript2Action: createNewRx()
RxWriteScript2Action->>RxSessionBean: addStashItem(loggedInInfo, rx)
RxSessionBean->>RxSessionBean: loop over existing stash items
RxSessionBean->>RxSessionBean: compare brandName and Objects.equals(GCN_SEQNO)
alt matching_item_found
RxSessionBean-->>RxWriteScript2Action: return_existing_index
else new_item
RxSessionBean-->>RxWriteScript2Action: add_new_and_return_index
end
Sequence diagram for safe deletion of closed prescriptions from stashsequenceDiagram
participant RxWriteScript2Action as RxWriteScript2Action
participant allIndex as allIndex_List_Integer
participant deletedIndex as deletedIndex_ArrayList_Integer
participant Collections as java_util_Collections
participant RxSessionBean as RxSessionBean
RxWriteScript2Action->>allIndex: collect indices of closed prescriptions
RxWriteScript2Action->>deletedIndex: new ArrayList(allIndex)
RxWriteScript2Action->>Collections: sort(deletedIndex, reverseOrder())
Collections-->>RxWriteScript2Action: deletedIndex sorted_desc
loop for each index in deletedIndex
RxWriteScript2Action->>RxSessionBean: removeStashItem(index)
end
Class diagram for updated medication stash and drug selection logicclassDiagram
class RxSessionBean {
+int addStashItem(LoggedInInfo loggedInInfo, RxPrescriptionData_Prescription rx)
}
class RxChooseDrug2Action {
+String execute()
-RxPrescriptionData_Prescription rx
}
class RxWriteScript2Action {
+String createNewRx()
+String updateSaveAllDrugs()
}
class RxPrescriptionData_Prescription {
+String getBrandName()
+String getGCN_SEQNO()
+void setGCN_SEQNO(String gcnSeqno)
}
class LoggedInInfo
class java_util_Objects {
+static boolean equals(Object a, Object b)
}
class java_util_List_Integer {
+void add(Integer index)
}
class java_util_Collections {
+static void sort(java_util_List_Integer list, java_util_Comparator_Integer comparator)
+static java_util_Comparator_Integer reverseOrder()
}
class java_util_ArrayList_Integer {
+java_util_ArrayList_Integer(java_util_List_Integer source)
}
class java_util_Comparator_Integer
RxSessionBean --> RxPrescriptionData_Prescription : operates_on
RxSessionBean --> LoggedInInfo : uses
RxSessionBean ..> java_util_Objects : uses_equals
RxChooseDrug2Action --> RxPrescriptionData_Prescription : configures_rx
RxChooseDrug2Action ..> RxWriteScript2Action : part_of_prescribing_flow
RxWriteScript2Action ..> RxSessionBean : manages_stash
RxWriteScript2Action ..> java_util_List_Integer : uses_allIndex
RxWriteScript2Action ..> java_util_ArrayList_Integer : copies_to_deletedIndex
RxWriteScript2Action ..> java_util_Collections : sorts_deletedIndex
java_util_Collections ..> java_util_Comparator_Integer
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical bugs in the medication prescription stash, specifically preventing duplicate entries and resolving IndexOutOfBoundsException crashes during the re-staging of medications. The changes ensure robust handling of medication data by correcting string comparison logic and improving the safety of list item removal operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
|
@SourceryAI review |
|
@coderabbitai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
RxWriteScript2Action.updateSaveAllDrugs,List<Integer> deletedIndex = allIndex;followed by sorting mutatesallIndexas well; if you intend to preserve the original ordering ofallIndex, create a copy instead (e.g.,List<Integer> deletedIndex = new ArrayList<>(allIndex);).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RxWriteScript2Action.updateSaveAllDrugs`, `List<Integer> deletedIndex = allIndex;` followed by sorting mutates `allIndex` as well; if you intend to preserve the original ordering of `allIndex`, create a copy instead (e.g., `List<Integer> deletedIndex = new ArrayList<>(allIndex);`).
## Individual Comments
### Comment 1
<location path="src/main/java/ca/openosp/openo/prescript/pageUtil/RxWriteScript2Action.java" line_range="1250" />
<code_context>
}
}
List<Integer> deletedIndex = allIndex;
+ Collections.sort(deletedIndex, Collections.reverseOrder());
// remove closed Rx from stash
for (Integer n : deletedIndex) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Sorting the alias list may unintentionally mutate the original allIndex list.
Because `deletedIndex` references `allIndex`, sorting it will reorder `allIndex` in place. To avoid unintended side effects on `allIndex` (especially if it’s used elsewhere or expected to be unmodified), create a copy before sorting, e.g. `List<Integer> deletedIndex = new ArrayList<>(allIndex);`.
Suggested implementation:
```java
import java.util.ArrayList;
import java.util.Collections;
import java.util.Arrays;
import java.util.Calendar;
import java.util.Date;
import java.util.List;
```
```java
List<Integer> deletedIndex = new ArrayList<>(allIndex);
Collections.sort(deletedIndex, Collections.reverseOrder());
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces two key fixes: in RxSessionBean.java, it updates the comparison of GCN_SEQNO values from == to Objects.equals() to ensure correct Integer value comparison and handle potential nulls; and in RxWriteScript2Action.java, it adds a reverse sort to the deletedIndex list before removing items, which prevents index shifting issues and ensures accurate item removal from a collection.
There was a problem hiding this comment.
Pull request overview
Fixes two stash-handling defects in the Rx prescribing flow that could create duplicate “ghost” stash items and trigger incorrect deletions / IndexOutOfBoundsException during cleanup when re-staging previously saved medications.
Changes:
- Fix duplicate detection in
RxSessionBean.addStashItem()by switchingGCN_SEQNOcomparison from reference (==) to value-basedObjects.equals(...). - Prevent index-shift corruption during stash cleanup in
RxWriteScript2Action.updateSaveAllDrugs()by sorting deletion indices in descending order before removals.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/main/java/ca/openosp/openo/prescript/pageUtil/RxWriteScript2Action.java |
Sorts deletion indices in reverse order before removing stash entries to avoid index shifting. |
src/main/java/ca/openosp/openo/prescript/pageUtil/RxSessionBean.java |
Uses null-safe value equality for GCN_SEQNO to properly deduplicate stash items. |
|
@coderabbitai review |
… instead of a reference
|
I think I see another issue with duplication checking, now the duplication checking is working in terms of comparing String values, but it seems to still show a duplicate if following the "Save And Print" flow, what I expect is the issue is that GCN_SEQNO does not seem to be set in the success path of the action, I tried to prompt claude with this before in this PR: #2176 But it didn't seem to work out well, will probably investigate and fix it in this PR. |
|
There seems to be another matching issue, will be investigating it, see information below: In both additions, I searched for: ACETAMINOPHEN 1000MG/DEXTROMETHORPHAN HYDROBROMIDE 30MG SYRUP first addition after save and print debug logs: 2026-03-18 18:24:36,264 WARN pageUtil.RxSessionBean (RxSessionBean.java:216) - addStashItem: incoming item isCustom=false, brandName='JUNIPER BERRIES 100MG/ZEA MAYS 100MG/ALTHAEA OFFICINALIS 50MG/UVA URSI 100MG CAPSULE', second addition after edit, then save and print, debug logs (should be the same drug): 2026-03-18 18:25:23,937 WARN pageUtil.RxSessionBean (RxSessionBean.java:216) - addStashItem: incoming item isCustom=false, brandName='JUNIPER BERRIES 100MG/ZEA MAYS 100MG/ALTHAEA OFFICINALIS 50MG/UVA URSI 100MG CAPSULE', Querying for last 5 meds added to test DB, still set as the correct brand name but it failed to match on duplicate?: SELECT drugid, BN, GCN_SEQNO FROM drugs WHERE demographic_no = 1 ORDER BY drugid DESC LIMIT 5; |
|
Ok, I am pretty sure this is an Open-Drugref issue, I have tried with other medications and I see that some work, some don't: Tried with: ACETAMINOPHEN 500MG first addition after save and print debug logs: 2026-03-18 19:03:16,110 WARN pageUtil.RxSessionBean (RxSessionBean.java:216) - addStashItem: incoming item isCustom=false, brandName='ACETAMINOPHEN 500MG', GCN_SEQNO='27734', stashSize=0 second addition after edit, then save and print, debug logs (should be the same drug): 2026-03-18 19:03:25,822 WARN pageUtil.RxSessionBean (RxSessionBean.java:216) - addStashItem: incoming item isCustom=false, brandName='ACETAMINOPHEN 500MG', GCN_SEQNO='27734', stashSize=1 CHILDREN'S IBUPROFEN SUSPENSION 100MG first addition after save and print debug logs: 2026-03-18 19:04:13,843 WARN pageUtil.RxSessionBean (RxSessionBean.java:216) - addStashItem: incoming item isCustom=false, brandName='CHILDREN'S IBUPROFEN SUSPENSION 100MG', GCN_SEQNO='4052', stashSize=0 second addition after edit, then save and print, debug logs (should be the same drug): 2026-03-18 19:04:22,177 WARN pageUtil.RxSessionBean (RxSessionBean.java:216) - addStashItem: incoming item isCustom=false, brandName='CHILDREN'S IBUPROFEN SUSPENSION 100MG', GCN_SEQNO='4052', stashSize=1 AG-AMOXICILLIN 250MG first addition after save and print debug logs: 2026-03-18 19:04:54,958 WARN pageUtil.RxSessionBean (RxSessionBean.java:216) - addStashItem: incoming item isCustom=false, brandName='AG-AMOXICILLIN 250MG', GCN_SEQNO='10421', stashSize=0 second addition after edit, then save and print, debug logs (should be the same drug): 2026-03-18 19:05:01,800 WARN pageUtil.RxSessionBean (RxSessionBean.java:216) - addStashItem: incoming item isCustom=false, brandName='AG-AMOXICILLIN 250MG', GCN_SEQNO='10421', stashSize=1 As you can see, only some of the medications searched result in this issue of duplicate medications still being staged. With testing I can see that the medications that don't match the correct brand name are:
While some that do match and the blocking duplicate medications when staging are:
This shows that the duplication functionality works for some but not all, I assume there is something wrong with how Open-Drugref is mapping these medications to their ID's Since the IBUPROFEN issue seems more contained (\ beside the 's), it might be an issue on our end? will investigate. |
|
So at this point, the issue with CHILDREN'S IBUPROFEN SUSPENSION 100MG has been fixed, still unsure about ACETAMINOPHEN 1000MG/DEXTROMETHORPHAN HYDROBROMIDE 30MG SYRUP |
|
I will investigate the reported "Found Issues" after I have finished my other tasks, I think I have a good idea of both but I don't think either are high priority to fix. |
Review Summary by QodoFix medication stash bugs: duplicate detection, deletion loop, and encoding issues
WalkthroughsDescription• Fixed String reference comparison using == instead of Objects.equals() for GCN_SEQNO duplicate detection • Fixed forward-iteration deletion causing IndexOutOfBoundsException by sorting indices in reverse order • Restored GCN_SEQNO assignment from drugId in drug selection flow • Removed improper Encode.forJava() calls on input data and added proper output encoding • Fixed XSS vulnerabilities in JavaScript contexts with Encode.forJavaScript() Diagramflowchart LR
A["String == comparison bug"] -->|Fix with Objects.equals| B["Correct duplicate detection"]
C["Forward-iteration deletion"] -->|Sort indices reverse| D["Safe ArrayList removal"]
E["Missing GCN_SEQNO assignment"] -->|Restore drugId mapping| F["Enable duplicate detection"]
G["Improper input encoding"] -->|Remove forJava on input| H["Store raw data"]
H -->|Add forJavaScript on output| I["Prevent XSS and corruption"]
File Changes1. src/main/java/ca/openosp/openo/prescript/pageUtil/RxChooseDrug2Action.java
|
Code Review by Qodo
1. addFav() uses wrong encoder
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
src/main/webapp/oscarRx/prescribe.jsp (1)
242-242: Use the event-handler-specific encoder for inline onclick attributes.
drugNameis being inserted into JavaScript that lives inside an HTML onclick attribute.Encode.forJavaScriptAttribute(...)is the appropriate encoder for this context, notEncode.forJavaScript(...). This JSP already usesEncode.forJavaScriptAttribute()for similar inline event handlers elsewhere in the file, so standardizing on this encoder improves consistency and matches OWASP Java Encoder guidance for attribute-embedded JavaScript.Suggested change
- <a tabindex="-1" href="javascript:void(0);" style="float:right;;margin-left:5px;margin-top:0px;padding-top:0px;" title="Add to Favorites" onclick="addFav('<%=rand%>','<%=Encode.forJavaScript(drugName)%>')">F</a> + <a tabindex="-1" href="javascript:void(0);" style="float:right;;margin-left:5px;margin-top:0px;padding-top:0px;" title="Add to Favorites" onclick="addFav('<%=rand%>','<%=Encode.forJavaScriptAttribute(drugName)%>')">F</a>Also applies to: line 456
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/oscarRx/prescribe.jsp` at line 242, The onclick inline JavaScript uses Encode.forJavaScript(drugName) but should use the attribute-specific encoder; update the onclick call that invokes addFav('<%=rand%>','<%=Encode.forJavaScript(drugName)%>') to use Encode.forJavaScriptAttribute for drugName (and similarly fix the same pattern at the other occurrence around line 456) so the value is properly encoded for an HTML attribute-embedded event handler while keeping the rand parameter and addFav function call intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/ca/openosp/openo/prescript/pageUtil/RxSessionBean.java`:
- Around line 226-228: The current condition in RxSessionBean that requires both
matching brandName and GCN_SEQNO still blocks matches when GCN_SEQNO alone
should identify the drug; change the logic around rx.getBrandName(),
item.getBrandName(), and Objects.equals(rx.getGCN_SEQNO(), item.getGCN_SEQNO())
so that a matching GCN_SEQNO is sufficient (i.e., check
Objects.equals(rx.getGCN_SEQNO(), item.getGCN_SEQNO()) first and accept the
match regardless of brandName), and only fall back to brandName-based comparison
when GCN_SEQNO is null/empty; update the branch containing
getBrandName()/getGCN_SEQNO() accordingly in RxSessionBean to reflect this
priority.
In `@src/main/java/ca/openosp/openo/prescript/pageUtil/RxWriteScript2Action.java`:
- Around line 1242-1246: When removing closed Rx entries, also remove the
corresponding annotation keys so indexes stay aligned: after building
deletedIndex and inside the loop that calls bean.removeStashItem(n), also remove
the i-th element from the attributeNames/attrib_names list (or call the
attributeNames.remove(n) equivalent) using the same index n (deletedIndex
entries are in reverse order to avoid shifting). Ensure the same index set
(deletedIndex) is applied to both bean.removeStashItem(n) and
attributeNames.remove(n) so that saveDrug(HttpServletRequest request) pairing of
bean.getStashItem(i) with attrib_names.get(i) remains correct.
---
Nitpick comments:
In `@src/main/webapp/oscarRx/prescribe.jsp`:
- Line 242: The onclick inline JavaScript uses Encode.forJavaScript(drugName)
but should use the attribute-specific encoder; update the onclick call that
invokes addFav('<%=rand%>','<%=Encode.forJavaScript(drugName)%>') to use
Encode.forJavaScriptAttribute for drugName (and similarly fix the same pattern
at the other occurrence around line 456) so the value is properly encoded for an
HTML attribute-embedded event handler while keeping the rand parameter and
addFav function call intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3100438e-b447-4a85-b1e6-d612a3f001a3
📒 Files selected for processing (4)
src/main/java/ca/openosp/openo/prescript/pageUtil/RxChooseDrug2Action.javasrc/main/java/ca/openosp/openo/prescript/pageUtil/RxSessionBean.javasrc/main/java/ca/openosp/openo/prescript/pageUtil/RxWriteScript2Action.javasrc/main/webapp/oscarRx/prescribe.jsp
LiamStanziani
changed the title
…nd-forward-iteration-deletion-loop-medications Fix: String object comparison and forward-iteration deletion in medication stash, missing GCN_SEQNO for drug search flow, and drug name encoding on input
Summary
Fixes two interconnected bugs in the medication prescription stash that cause duplicate entries and IndexOutOfBoundsException crashes when re-staging previously saved medications.
Fixes #2340
Fixes #1879
Problem
Bug
#2(Regression): String == comparison (RxSessionBean.java:228): After GCN_SEQNO was refactored from int to String, the duplicate detection in addStashItem() uses == (reference comparison) instead of value comparison. This silently allows duplicate"ghost" entries into the stash when re-adding the same drug.
Bug
#3(Pre-existing): Forward-iteration deletion (RxWriteScript2Action.java:1250-1255): The stash cleanup loop iterates forward through indices while calling ArrayList.remove(), which shifts remaining elements down after each removal. This causes wrongitems to be deleted or throws IndexOutOfBoundsException: Index 3 out of bounds for length 3.
Bug
#8(Cascading): Together, Bugs#2and#3cascade into a 500 crash during Save & Print after re-staging medications via Edit Rx.Missing GCN_SEQNO in drug search flow (#1879): The GCN_SEQNO assignment in RxChooseDrug2Action was commented out during the Vigilance integration, leaving prescriptions with null GCN_SEQNO and breaking duplicate detection, custom name classification, and favorite lookups.
Drug name corruption (Pre-existing + Regression): Encode.forJava() was misapplied to input data in createNewRx(), encoding values at storage time instead of at output time. Combined with manual replace("'", "\'") calls in prescribe.jsp, this double-escaped apostrophes and quotes in brand names (e.g., CHILDREN'S → CHILDREN\'S), corrupting data stored in the database and breaking duplicate detection for affected drugs.
Solution
Bug
#2: Changed == to Objects.equals() for null-safe String value comparison on GCN_SEQNO.Bug
#3: Added Collections.sort(deletedIndex, Collections.reverseOrder()) before the removal loop so indices are removed highest-first, preventing index shifting corruption. Used a defensive copy (new ArrayList<>(allIndex)) to avoid mutating the original list.#1879: Restored rx.setGCN_SEQNO(drugId) in RxChooseDrug2Action, matching the pre-Vigilance behavior where the drug reference ID was used as GCN_SEQNO.
Drug name corruption: Removed Encode.forJava() calls on text and drugId in RxWriteScript2Action.createNewRx() — these encoded input data that should be stored raw and encoded only at output time. Removed manual drugName.replace("'", "\'") and drugName.replace(""","\"") in prescribe.jsp. Fixed three raw <%=drugName%> JavaScript usages with proper Encode.forJavaScript(drugName) for XSS prevention.
Found Issues (Will be separate investigations, and have been added under the additional concerns section in the google document)
Summary by Sourcery
Fix medication prescription stash handling to correctly deduplicate entries and safely remove closed prescriptions without index errors.
Bug Fixes:
Summary by cubic
Fixes duplicate medications and IndexOutOfBounds crashes in the prescription stash, and corrects drug name handling in the UI. Compares
GCN_SEQNOby value, sets it fromdrugId, deletes closed items safely, and uses JavaScript-safe encoding for names.GCN_SEQNOfromdrugIdto keep stash keys consistent.Objects.equalsforGCN_SEQNOString comparison to prevent duplicates.drugNamewithEncode.forJavaScriptinprescribe.jspand use JS-safe values in the UI.Written for commit 188c0d5. Summary will update on new commits.
Summary by Sourcery
Fix medication stash handling to correctly detect existing prescriptions and safely delete closed entries without index errors.
Bug Fixes:
Summary by Sourcery
Fix medication prescription stash duplicate detection and cleanup, and correct drug name handling and encoding in the prescribing UI.
Bug Fixes:
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor