RCF-1302 implemented logic for packet storage location #672
Conversation
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
Signed-off-by: Madhuravas reddy <madhu@mosip.io>
WalkthroughThis PR centralizes packet storage resolution via a new StorageUtils, adds a PACKET_STORE_LOCATION constant and accessor, refactors disk-space validation to use the resolved packet storage directory (raising PAK_DISK_SPACE_LOW when insufficient), updates packet base path defaults, and adds localized PAK_DISK_SPACE_LOW messages. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java`:
- Around line 679-689: The current disk-space check uses a potentially relative
or invalid packetStoreLocation and silently falls back to context.getFilesDir(),
which can mask low-space on the real packet storage; update the logic around
globalParamRepository.getCachedStringPacketStoreLocation(), actualDiskSpace and
usableSpace to: normalize the configured path (use new
File(packetStoreLocation).getCanonicalFile()), ensure it is absolute and points
to the intended storage (create parent dirs with mkdirs() if needed), if the
configured path is invalid/unwritable treat that as an error/low-space condition
(do not silently switch to context.getFilesDir()), and only use
StorageUtils.getPacketStorageDir(context) when there is no configured value;
keep all checks against actualDiskSpace.getUsableSpace() after validation so
low-disk conditions on the configured storage are correctly detected and
reported.
In
`@android/packetmanager/src/main/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImpl.java`:
- Around line 73-77: The null-check mentioned is unreachable because
StorageUtils.getPacketStorageDir(context) always returns a File; update
initPosixAdapterService to validate the returned File instead of checking for
null: after calling StorageUtils.getPacketStorageDir(context) (in
initPosixAdapterService) verify that the File exists, is a directory and is
writable (e.g., file.exists(), file.isDirectory(), file.canWrite()) before
assigning BASE_LOCATION, and handle the error path (log/error/throw) if those
checks fail so BASE_LOCATION is only set when the directory is usable.
- Around line 207-218: The validation block in PosixAdapterServiceImpl (symbols:
BASE_LOCATION, baseDir, appContext, putObject) currently silently returns and
contains an unreachable null-check; update it to (1) remove the unnecessary
BASE_LOCATION == null early return (StorageUtils.getPacketStorageDir() is
non-null), (2) replace the startsWith logic with a robust path comparison using
canonical paths (e.g., compare baseDir.getCanonicalPath() vs
appContext.getFilesDir().getCanonicalPath() or use a utility to check whether
baseDir is a child of app files) so the external-storage check only runs when
truly outside app files, and (3) replace silent returns with logged error/warn
messages (use the existing logger in PosixAdapterServiceImpl) that include the
reason for aborting so callers like putObject can fail with actionable logs.
In
`@android/packetmanager/src/main/java/io/mosip/registration/packetmanager/util/StorageUtils.java`:
- Around line 49-53: The current StorageUtils method that creates baseDir (uses
Environment.getExternalStoragePublicDirectory and calls baseDir.mkdirs()) logs
failures but still returns a non-existent/unwritable path; change it to handle
failures and scoped-storage constraints by: attempt creation as now, then if
mkdirs() fails or the directory is not writable, fall back to app-private
storage via context.getExternalFilesDir(location) (or context.getFilesDir() if
getExternalFilesDir is null); do not use
Environment.getExternalStoragePublicDirectory for apps targeting API 30+; before
returning ensure the chosen directory exists and is writable (or return null /
throw an IOException) so callers of StorageUtils (the method that builds
baseDir) receive a valid, usable path or an explicit error.
In `@assets/l10n/app_hi.arb`:
- Line 305: The errors message for the "errors" key uses the wrong punctuation
for PAK_DISK_SPACE_LOW: replace the Japanese/Chinese ideographic full stop (。
U+3002) with the correct Hindi punctuation (देवनागरी danda '।') or a standard
ASCII period '.' as appropriate; update the PAK_DISK_SPACE_LOW segment inside
the "errors" select mapping so the string ends with the chosen correct
punctuation instead of U+3002.
In `@assets/l10n/app_kn.arb`:
- Line 305: Replace the English message for the select-case key
PAK_DISK_SPACE_LOW inside the "errors" select mapping with a Kannada
translation; locate the "errors" select string (contains keys like
REG_TRY_AGAIN, PAK_APPRVL_MAX_TIME, etc.) and change PAK_DISK_SPACE_LOW{Minimum
required space is not available to create the packet.} to a proper Kannada
sentence conveying the same meaning (e.g., PAK_DISK_SPACE_LOW{ಪಟ್ಟೆ ಸೃಷ್ಟಿಸಲು
ಅಗತ್ಯವಿರುವ ಕನಿಷ್ಠ ಖಾಲಿ ಜಾಗ ಲಭ್ಯವಿಲ್ಲ.}).
🧹 Nitpick comments (2)
android/packetmanager/src/main/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImpl.java (2)
220-222: Missing error handling for directory creation.
containerFolder.mkdirs()result is not checked. If directory creation fails (e.g., permissions issue on the newly validated external storage), subsequent file operations will fail silently.Suggested improvement
File containerFolder = new File(BASE_LOCATION, account); - if(!containerFolder.exists()) - containerFolder.mkdirs(); + if (!containerFolder.exists() && !containerFolder.mkdirs()) { + Log.e(TAG, "Failed to create container folder: " + containerFolder.getAbsolutePath()); + return; + }
232-233: Resource leak:FileInputStreamnot closed.The
FileInputStreamat line 232 is passed togetAllExistingEntries()which closes the stream, but if an exception occurs between creation and that call, the stream leaks. Consider using try-with-resources.Suggested fix
} else { - InputStream ios = new FileInputStream(containerZip); - Map<ZipEntry, ByteArrayOutputStream> entries = getAllExistingEntries(ios); - try (ZipOutputStream packetZip = new ZipOutputStream(out)) { + Map<ZipEntry, ByteArrayOutputStream> entries; + try (InputStream ios = new FileInputStream(containerZip)) { + entries = getAllExistingEntries(ios); + } + try (ZipOutputStream packetZip = new ZipOutputStream(out)) {Note: This requires refactoring
getAllExistingEntries()to not close the stream, or adjusting the logic accordingly.
...nager/src/main/java/io/mosip/registration/clientmanager/service/RegistrationServiceImpl.java
Outdated
Show resolved
Hide resolved
...nager/src/main/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImpl.java
Show resolved
Hide resolved
...nager/src/main/java/io/mosip/registration/packetmanager/service/PosixAdapterServiceImpl.java
Show resolved
Hide resolved
android/packetmanager/src/main/java/io/mosip/registration/packetmanager/util/StorageUtils.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Madhuravas reddy <madhu@mosip.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
mosip.registration.registration_packet_store_location
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.