fix(resident): implement env-var fallbacks for secrets and auto-discard logic for pending packets (MOSIP-40514) - #1599
Open
Piyush0000 wants to merge 4 commits into
Conversation
Signed-off-by: Chandra Keshav Mishra <chandrakeshavmishra@gmail.com>
[DSD-10166] Delete .github/ISSUE_TEMPLATE directory
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughTest retry logic in OTP generation and update-data POST requests is enhanced to handle backpressure errors: RES-SER-524 now discards cancellable drafts via a new utility method, RES-SER-525 logs and waits, and both flows use centralized delay logic. Configuration loading gains environment variable fallback for blank properties. ChangesOTP Generation & Update Data Retry Flow
Gitignore Maintenance
Sequence DiagramsequenceDiagram
participant TestScript as OTP Test Script
participant ResidentUtil as ResidentUtil
participant API as Resident API
participant Sleep as Thread Sleep
TestScript->>API: POST send-OTP request
API-->>TestScript: RES-SER-524 (pending packet)
TestScript->>ResidentUtil: discardCancellablePendingDrafts()
ResidentUtil->>API: GET pending drafts (auth)
API-->>ResidentUtil: drafts list
alt Cancellable drafts found
ResidentUtil->>API: POST discard (per draft)
API-->>ResidentUtil: DISCARDED response
ResidentUtil-->>TestScript: count > 0
TestScript->>Sleep: skip conditional sleep
else No cancellable drafts
ResidentUtil-->>TestScript: count = 0
TestScript->>Sleep: sleep uinGenDelayTime
end
Sleep->>TestScript: resume
TestScript->>API: retry send-OTP request
API-->>TestScript: success / RES-SER-525
alt RES-SER-525 received
TestScript->>Sleep: log & sleep uinGenDelayTime
Sleep->>TestScript: resume
TestScript->>API: retry send-OTP request
end
API-->>TestScript: OTP sent (success)
TestScript->>API: POST update-data request
API-->>TestScript: response (with potential RES-SER-524/525)
alt Backpressure error
TestScript->>ResidentUtil: discardCancellablePendingDrafts()
ResidentUtil-->>TestScript: count
TestScript->>Sleep: conditional/log sleep
TestScript->>API: retry update-data
else Success
TestScript->>TestScript: validate output
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 6
🤖 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 @.claude/settings.local.json:
- Around line 1-8: Remove the committed local Claude permission overrides in
.claude/settings.local.json that expose shell commands (the "permissions" ->
"allow" entries), revert the file from the repository (delete it in a follow-up
commit or restore the tracked file to a safe default), and ensure it is ignored
for future commits by adding .claude/settings.local.json to repository ignore
(or use .git/info/exclude) so each developer keeps their own local copy; if
needed, provide a sanitized example or template (without real permissions) in
the repo instead.
In
`@api-test/src/main/java/io/mosip/testrig/apirig/resident/testscripts/PostWithBodyWithOtpGenerate.java`:
- Around line 127-141: The retry loops that inspect otpResponse and response can
fall through with a null value and later call otpResponse.asString() /
response.asString(), causing NPEs; in PostWithBodyWithOtpGenerate.java (search
for variables otpResponse and response and the retry blocks that call
ResidentUtil.discardCancellablePendingDrafts and sleepForUinGenDelay), add
explicit null checks inside the retry loops and immediately after the loops: if
otpResponse or response is null, either continue the retry (do not fall through
to the else { break; }) or fail fast with a clear exception/log message before
any asString() is called; ensure every call to otpResponse.asString() or
response.asString() is guarded by a null check and include contextual logging
(role/testCaseName) when aborting so downstream validation never receives a
null.
- Around line 220-226: In sleepForUinGenDelay(), separate exception handling so
NumberFormatException and InterruptedException are caught in distinct catch
blocks: handle NumberFormatException by logging the parse/config error (from
properties.getProperty("uinGenDelayTime")) without altering thread state, and
handle InterruptedException by logging the interruption and calling
Thread.currentThread().interrupt(); keep the Thread.sleep(Long.parseLong(...))
call as the try block target and ensure only the InterruptedException handler
invokes interrupt().
In
`@api-test/src/main/java/io/mosip/testrig/apirig/resident/testscripts/SimplePost.java`:
- Around line 184-190: In sleepForUinGenDelay(), split the combined catch
(NumberFormatException | InterruptedException) into two separate catches: catch
NumberFormatException to log a clear parsing/configuration error (include which
property: "uinGenDelayTime") and return/skip sleeping, and catch
InterruptedException to log and then call Thread.currentThread().interrupt() to
restore the interrupt flag; apply the same change to the three other test
scripts that use the same pattern to avoid poisoning test threads on config
parse failures.
- Around line 137-152: The retry loop incorrectly treats response == null as a
normal "break" path which later leads to NullPointerException when response is
dereferenced; update the loop in SimplePost.java to explicitly handle response
== null (e.g., log an error and either retry/wait via sleepForUinGenDelay() or
throw/return) before the existing string-contains checks, so that when
ResidentUtil.discardCancellablePendingDrafts(...) and sleepForUinGenDelay() run
you never proceed to the code that accesses response.asString() on a null
reference; ensure the chosen behavior (retry vs fail) is consistent with the
surrounding logic and testCaseDTO handling.
In
`@api-test/src/main/java/io/mosip/testrig/apirig/resident/utils/ResidentConfigManager.java`:
- Around line 55-63: In resolveFromEnv, the code treats whitespace-only
environment values as valid; update the checks for both the original key lookup
(value) and the upperKey lookup (value) to reject strings that are blank by
using trim().isEmpty() (i.e., accept only when value != null &&
!value.trim().isEmpty()), and keep using key, upperKey and System.getenv(...) to
locate env vars.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 231855aa-5c37-40fd-af49-8a5620695359
📒 Files selected for processing (5)
.claude/settings.local.jsonapi-test/src/main/java/io/mosip/testrig/apirig/resident/testscripts/PostWithBodyWithOtpGenerate.javaapi-test/src/main/java/io/mosip/testrig/apirig/resident/testscripts/SimplePost.javaapi-test/src/main/java/io/mosip/testrig/apirig/resident/utils/ResidentConfigManager.javaapi-test/src/main/java/io/mosip/testrig/apirig/resident/utils/ResidentUtil.java
Comment on lines
+1
to
+8
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(dir D:c4gt_mosip_resudent_Servicesresident-services *)", | ||
| "Bash(mvn -q -DskipTests compile)" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove committed local Claude permission overrides from source control.
This file grants shell execution permissions and is machine/local-workflow specific. Keeping it in the repo weakens security posture and creates environment-dependent behavior for other contributors and CI.
Proposed fix
-{
- "permissions": {
- "allow": [
- "Bash(dir D:c4gt_mosip_resudent_Servicesresident-services *)",
- "Bash(mvn -q -DskipTests compile)"
- ]
- }
-}If this is needed locally, keep it untracked (for example via .git/info/exclude or a repo ignore rule for .claude/settings.local.json).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| "permissions": { | |
| "allow": [ | |
| "Bash(dir D:c4gt_mosip_resudent_Servicesresident-services *)", | |
| "Bash(mvn -q -DskipTests compile)" | |
| ] | |
| } | |
| } |
🤖 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 @.claude/settings.local.json around lines 1 - 8, Remove the committed local
Claude permission overrides in .claude/settings.local.json that expose shell
commands (the "permissions" -> "allow" entries), revert the file from the
repository (delete it in a follow-up commit or restore the tracked file to a
safe default), and ensure it is ignored for future commits by adding
.claude/settings.local.json to repository ignore (or use .git/info/exclude) so
each developer keeps their own local copy; if needed, provide a sanitized
example or template (without real permissions) in the repo instead.
Piyush0000
force-pushed
the
fix/MOSIP-40514-resident-test-failures
branch
from
May 6, 2026 08:14
12427ef to
f03e54f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api-test/src/main/java/io/mosip/testrig/apirig/resident/utils/ResidentUtil.java (1)
227-229: ⚡ Quick winUse structured success checks instead of substring matching.
contains("DISCARDED")can produce false positives (e.g., error text containing the same token). Parse the discard response status/code and combine it with HTTP status for deterministic behavior.Suggested patch
- if (postResp != null && postResp.asString() != null - && postResp.asString().contains("DISCARDED")) { + boolean isDiscarded = false; + if (postResp != null && postResp.getStatusCode() / 100 == 2 && postResp.asString() != null) { + JSONObject discardBody = new JSONObject(postResp.asString()); + JSONObject discardResponse = discardBody.optJSONObject("response"); + isDiscarded = discardResponse != null + && "DISCARDED".equalsIgnoreCase(discardResponse.optString("status")); + } + if (isDiscarded) { discarded++; logger.info("Discarded cancellable pending draft eid=" + eid); } else {🤖 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-test/src/main/java/io/mosip/testrig/apirig/resident/utils/ResidentUtil.java` around lines 227 - 229, The current check in ResidentUtil that increments discarded by testing postResp.asString().contains("DISCARDED") is brittle; instead use postResp's HTTP status (e.g., postResp.getStatus()) combined with a parsed response status field to deterministically detect a discard. Update the logic around the postResp variable in ResidentUtil to first verify a successful HTTP code (or the specific expected status code for discard) and then parse the response body as JSON to read the canonical status field (e.g., "status" or "result") and compare it for equality to "DISCARDED" before incrementing discarded.
🤖 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-test/src/main/java/io/mosip/testrig/apirig/resident/utils/ResidentConfigManager.java`:
- Around line 30-38: The map is now receiving nulls when a property is blank and
resolveFromEnv(key) returns null; update the logic in ResidentConfigManager so
that when the original property is blank and resolveFromEnv returns null you
assign value = "" (empty string) before calling
moduleSpecificPropertiesMap.put(key, value), preserving the previous behavior
callers of getproperty(...) expect (avoid nulls that cause NPEs).
In
`@api-test/src/main/java/io/mosip/testrig/apirig/resident/utils/ResidentUtil.java`:
- Around line 230-233: The logs in ResidentUtil currently print raw identifiers
and full response bodies (logger.warn with eid and postResp.asString()), which
can leak sensitive data; update the logger calls in the discard branch to mask
the eid (e.g., show only a short suffix or hashed/obscured form of the eid
variable) and avoid logging the full response body—log only minimal metadata
such as the HTTP status code or an error code from postResp (use the response
status accessor on postResp instead of asString()) and a short sanitized
message; modify both the info/warn paths that reference eid and postResp to use
the masked identifier and status-only logging.
---
Nitpick comments:
In
`@api-test/src/main/java/io/mosip/testrig/apirig/resident/utils/ResidentUtil.java`:
- Around line 227-229: The current check in ResidentUtil that increments
discarded by testing postResp.asString().contains("DISCARDED") is brittle;
instead use postResp's HTTP status (e.g., postResp.getStatus()) combined with a
parsed response status field to deterministically detect a discard. Update the
logic around the postResp variable in ResidentUtil to first verify a successful
HTTP code (or the specific expected status code for discard) and then parse the
response body as JSON to read the canonical status field (e.g., "status" or
"result") and compare it for equality to "DISCARDED" before incrementing
discarded.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba9ccbaa-c28f-4102-bc28-a7b960a0aed1
📒 Files selected for processing (5)
.gitignoreapi-test/src/main/java/io/mosip/testrig/apirig/resident/testscripts/PostWithBodyWithOtpGenerate.javaapi-test/src/main/java/io/mosip/testrig/apirig/resident/testscripts/SimplePost.javaapi-test/src/main/java/io/mosip/testrig/apirig/resident/utils/ResidentConfigManager.javaapi-test/src/main/java/io/mosip/testrig/apirig/resident/utils/ResidentUtil.java
✅ Files skipped from review due to trivial changes (1)
- .gitignore
…rd logic for pending packets (MOSIP-40514) Signed-off-by: Piyush <rathorepiyush0000@gmail.com>
Piyush0000
force-pushed
the
fix/MOSIP-40514-resident-test-failures
branch
from
May 6, 2026 08:25
f03e54f to
0352659
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses MOSIP-40514, where the Resident API Test Rig was failing for ValidateWithUserIdOtp, sendOTPToUserId, and update my data due to authentication blockers and unhandled pending packet states.
Root Causes Identified:
Authentication Failure: The test rig relied on hardcoded secrets in resident.properties, which are often blank in public environments, leading to 401 Unauthorized errors.
Pending Packet Timeout: When a resident has a "cancellable" pending packet (Error RES-SER-524), the test rig would simply sleep and retry. If the packet wasn't manually cleared, the test would eventually time out after 200 seconds.
Changes Made:
Enhanced Configuration Management: Updated ResidentConfigManager.java to support environment variable fallbacks. If a secret is blank in resident.properties, the system now looks for an environment variable (supporting both literal and UPPER_SNAKE_CASE names).
Automated Draft Discarding:
Added ResidentUtil.discardCancellablePendingDrafts to handle the RES-SER-524 state.
Updated SimplePost.java and PostWithBodyWithOtpGenerate.java to automatically call the discard-draft endpoint when a cancellable pending packet is detected.
This removes the need for fragile long-duration sleeps and ensures the test rig can self-heal and continue execution.
Defensive Logic: Retained the existing wait-and-retry logic for non-cancellable packets (RES-SER-525) to ensure backward compatibility.
How to Test:
Set the following environment variables: KEYCLOAK_PASSWORD, MOSIP_RESIDENT_CLIENT_SECRET, MOSIP_IDREPO_CLIENT_SECRET, MOSIP_REGPROC_CLIENT_SECRET, and MOSIP_TESTRIG_CLIENT_SECRET.
Run the Resident API Test Rig smoke tests.
Observe that the system now correctly authenticates and bypasses "pending packet" errors by discarding stale drafts.
Fixes:
Fixes #1592 (MOSIP-40514)
Summary by CodeRabbit
New Features
Bug Fixes