T1 (#808): softwareUpdateHandler — resumable downloads to .artifacts/#814
T1 (#808): softwareUpdateHandler — resumable downloads to .artifacts/#814GOKULRAJ136 wants to merge 5 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds resumable download staging for software updates, routes update handling through staged artifact downloads, expands downloader tests, and updates registration UI automation, scenario flows, and fixtures for preview, acknowledgement, and EOD approval validation. ChangesResumable download infrastructure
Registration UI automation and scenario flow
Upgrade guidance documents
Sequence Diagram(s)Not included. Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@registration/registration-services/src/main/java/io/mosip/registration/update/ResumableDownloader.java`:
- Around line 345-351: Add validation in the Artifact constructor to sanitize
the name parameter before using it to construct File paths. At the beginning of
the Artifact constructor, before assigning this.name or creating any File
objects, validate that the name parameter does not contain path separators
(forward slash or backslash) or the parent directory reference (..) sequence. If
validation fails, throw an IllegalArgumentException with a descriptive error
message. This ensures that malicious filenames with path traversal sequences
cannot be used to write files outside the intended targetDir directory.
In
`@registration/registration-services/src/main/java/io/mosip/registration/update/SoftwareUpdateUtil.java`:
- Around line 130-133: Update the throw statement in the IOException catch block
where RegBaseCheckedException with error code "REG-BUILD-005" is constructed to
pass the caught exception (variable e) as the third argument to the
RegBaseCheckedException constructor. The constructor accepts three parameters:
error code, error message, and a Throwable cause. This will preserve the
original IOException in the exception chain for better debugging. Apply the same
fix to the other location (around line 149) where RegBaseCheckedException is
similarly thrown without the cause parameter.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f064773-d274-4a7e-bc11-f465d653a42f
📒 Files selected for processing (4)
registration/registration-services/src/main/java/io/mosip/registration/update/ResumableDownloader.javaregistration/registration-services/src/main/java/io/mosip/registration/update/SoftwareUpdateHandler.javaregistration/registration-services/src/main/java/io/mosip/registration/update/SoftwareUpdateUtil.javaregistration/registration-services/src/test/java/io/mosip/registration/test/update/SoftwareUpdateUtilTest.java
…acts/ Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
| * @return {@code true} if the file was completed and finalized; {@code false} if the partial was | ||
| * found stale/unusable and discarded, so the caller should restart from scratch. | ||
| */ | ||
| private static boolean attemptDownload(String url, Artifact artifact, boolean allowResume, |
There was a problem hiding this comment.
@GOKULRAJ136 Instead of writing our implementation for resumable file download, why not use OkHttp3 library which already supports resumable file downloads.
There was a problem hiding this comment.
@anushasunkada I did evaluate it.
OkHttp3 doesn't provide resumable downloads as a built-in feature. It simply allows you to set the Range header, which HttpURLConnection already supports. The actual resumable download logic, including persisting the .part file, validating with ETag/If-Range to prevent mixing different versions, handling 200/206/416 responses, verifying Content-Length, and performing an atomic finalization, must be implemented manually regardless of which HTTP client is used.
Additionally, OkHttp is not currently a runtime dependency (it is only used by mockwebserver for tests), so adopting it would introduce a new runtime dependency. ResumableDownloader was intentionally designed to depend only on the JDK and slf4j so that T2 can move it into registration-launcher-common unchanged and share it with _launcher.jar.
2 participants
Refer Story #807
Fixes #808
Summary by CodeRabbit
Release Notes
Summary by CodeRabbit
New Features
Bug Fixes