Fix bugs found during code audit across api, adminapi, functional, examples#1711
Draft
balamurugana wants to merge 2 commits into
Conversation
Signed-off-by: Bala.FA <bala@minio.io>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR updates build and wrapper tooling, adds repository guidance, changes admin and API parsing and validation behavior, adjusts async client download and upload control flow, and refreshes examples and functional tests. ChangesRepository build and tooling
Admin API updates
Core API contracts and helpers
Async client, examples, and functional tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
balamurugana
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
api/src/main/java/io/minio/PromptObjectArgs.java (1)
59-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
offset(String)as a delegating alias Renaming this public builder method breaks existingPromptObjectArgsconsumers; preserve the old name and forward it toprompt(String)instead.🤖 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/src/main/java/io/minio/PromptObjectArgs.java` around lines 59 - 63, The public builder API in PromptObjectArgs.Builder should preserve backward compatibility by keeping offset(String) as a delegating alias instead of replacing it with prompt(String). Update the Builder methods so offset(String) forwards to prompt(String), and keep prompt(String) as the shared implementation that validates and sets the prompt field, ensuring existing consumers of offset(String) continue to work.functional/TestMinioClient.java (1)
2651-2658: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the notification filter too.
The expected config includes
new NotificationConfiguration.Filter("images", "pg"), but the mismatch check only validates queue ARN and event. A broken filter round-trip still passes this test.Proposed full-config comparison
NotificationConfiguration config = client.getBucketNotification( GetBucketNotificationArgs.builder().bucket(bucketName).build()); - if (config.queueConfigurations().size() != 1 - || !sqsArn.equals(config.queueConfigurations().get(0).queue()) - || config.queueConfigurations().get(0).events().size() != 1 - || !EventType.OBJECT_CREATED_PUT - .toString() - .equals(config.queueConfigurations().get(0).events().get(0))) { + String expectedXml = Xml.marshal(expectedConfig); + String actualXml = Xml.marshal(config); + if (!expectedXml.equals(actualXml)) { throw new Exception( - "config: expected: " + Xml.marshal(expectedConfig) + ", got: " + Xml.marshal(config)); + "config: expected: " + expectedXml + ", got: " + actualXml); }🤖 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 `@functional/TestMinioClient.java` around lines 2651 - 2658, The notification configuration check only validates the queue ARN and event in the relevant test, so a broken filter round-trip can still pass. Update the assertion logic around the config queue validation to also compare the filter on the queue configuration against the expected NotificationConfiguration.Filter("images", "pg"), using the existing config.queueConfigurations().get(0) and expectedConfig values to verify the full notification payload.adminapi/src/main/java/io/minio/admin/MinioAdminClient.java (1)
719-745: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the primitive
updateServiceAccountoverload. Changing this public parameter frombooleantoBooleanbreaks binary compatibility for existing callers and can triggerNoSuchMethodError. Add abooleanoverload that delegates to the nullable version.🤖 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 `@adminapi/src/main/java/io/minio/admin/MinioAdminClient.java` around lines 719 - 745, The public updateServiceAccount API has lost its primitive boolean signature, which breaks existing callers and can cause NoSuchMethodError. In MinioAdminClient, keep the nullable Boolean-based implementation but add a boolean overload for updateServiceAccount that delegates to the existing method, preserving binary compatibility while keeping the current logic centralized.gradlew.bat (1)
54-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTerminate the batch script on failure.
"%COMSPEC%" /c exit 1only exits the childcmd, so the error paths can continue into:findJavaFromJavaHome/:execute. Replace them withexit /b 1, and useexit /b %ERRORLEVEL%in:exitWithErrorLevel.🤖 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 `@gradlew.bat` around lines 54 - 82, The batch error handling in gradlew.bat does not stop the parent script, so failure paths can fall through into :findJavaFromJavaHome and :execute. Update the early failure exits and the invalid JAVA_HOME branch to terminate the script with exit /b 1, and change :exitWithErrorLevel to return the current error code with exit /b %ERRORLEVEL%. Use the existing labels :findJavaFromJavaHome, :execute, and :exitWithErrorLevel to locate the affected paths.Source: Linters/SAST tools
🤖 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 `@adminapi/src/main/java/io/minio/admin/MinioAdminClient.java`:
- Around line 473-480: The quota parsing in MinioAdminClient should reject
non-numeric JSON instead of coercing it to 0. In the response-handling code that
streams the parsed map and extracts the "quota" entry, check that the JsonNode
is actually numeric before converting it, and throw an error if it is missing or
not a number rather than relying on asLong(). Keep the existing quota lookup
flow, but update the parsing logic to treat malformed quota values as failures.
In `@functional/TestMinioClient.java`:
- Around line 965-967: In TestMinioClient, the cleanup in the test teardown
currently deletes the local file before calling client.removeObject, so a local
deletion failure can prevent remote object cleanup. Update the teardown logic
around args.filename(), args.bucket(), and args.object() to ensure
client.removeObject(RemoveObjectArgs.builder()...) runs first or is guaranteed
via a nested finally, then perform Files.deleteIfExists afterward so both
cleanup steps are always attempted.
In `@gradle/wrapper/gradle-wrapper.properties`:
- Around line 5-6: The Gradle wrapper configuration currently disables download
retries by setting retries to zero, which makes bootstrap fail on any transient
network issue. Update the wrapper properties to use a small non-zero retry count
so the retryBackOffMs setting in gradle-wrapper.properties actually takes effect
and can absorb flaky distribution downloads.
---
Outside diff comments:
In `@adminapi/src/main/java/io/minio/admin/MinioAdminClient.java`:
- Around line 719-745: The public updateServiceAccount API has lost its
primitive boolean signature, which breaks existing callers and can cause
NoSuchMethodError. In MinioAdminClient, keep the nullable Boolean-based
implementation but add a boolean overload for updateServiceAccount that
delegates to the existing method, preserving binary compatibility while keeping
the current logic centralized.
In `@api/src/main/java/io/minio/PromptObjectArgs.java`:
- Around line 59-63: The public builder API in PromptObjectArgs.Builder should
preserve backward compatibility by keeping offset(String) as a delegating alias
instead of replacing it with prompt(String). Update the Builder methods so
offset(String) forwards to prompt(String), and keep prompt(String) as the shared
implementation that validates and sets the prompt field, ensuring existing
consumers of offset(String) continue to work.
In `@functional/TestMinioClient.java`:
- Around line 2651-2658: The notification configuration check only validates the
queue ARN and event in the relevant test, so a broken filter round-trip can
still pass. Update the assertion logic around the config queue validation to
also compare the filter on the queue configuration against the expected
NotificationConfiguration.Filter("images", "pg"), using the existing
config.queueConfigurations().get(0) and expectedConfig values to verify the full
notification payload.
In `@gradlew.bat`:
- Around line 54-82: The batch error handling in gradlew.bat does not stop the
parent script, so failure paths can fall through into :findJavaFromJavaHome and
:execute. Update the early failure exits and the invalid JAVA_HOME branch to
terminate the script with exit /b 1, and change :exitWithErrorLevel to return
the current error code with exit /b %ERRORLEVEL%. Use the existing labels
:findJavaFromJavaHome, :execute, and :exitWithErrorLevel to locate the affected
paths.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: caa38ccf-7cc5-4b2e-adbd-e7c3255befa3
⛔ Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (58)
.gitattributes.github/workflows/gradle.ymlCLAUDE.mdadminapi/src/main/java/io/minio/admin/Crypto.javaadminapi/src/main/java/io/minio/admin/GetDataUsageInfoResponse.javaadminapi/src/main/java/io/minio/admin/GetServerInfoResponse.javaadminapi/src/main/java/io/minio/admin/MinioAdminClient.javaadminapi/src/main/java/io/minio/admin/Status.javaapi/src/main/java/io/minio/AppendObjectArgs.javaapi/src/main/java/io/minio/BaseS3Client.javaapi/src/main/java/io/minio/Checksum.javaapi/src/main/java/io/minio/CompleteMultipartUploadArgs.javaapi/src/main/java/io/minio/GetObjectAttributesArgs.javaapi/src/main/java/io/minio/GetPresignedObjectUrlArgs.javaapi/src/main/java/io/minio/Http.javaapi/src/main/java/io/minio/ListObjectVersionsArgs.javaapi/src/main/java/io/minio/ListObjectsV1Args.javaapi/src/main/java/io/minio/ListObjectsV2Args.javaapi/src/main/java/io/minio/ListPartsArgs.javaapi/src/main/java/io/minio/ListenBucketNotificationArgs.javaapi/src/main/java/io/minio/MinioAsyncClient.javaapi/src/main/java/io/minio/PartReader.javaapi/src/main/java/io/minio/PromptObjectArgs.javaapi/src/main/java/io/minio/PutObjectAPIBaseArgs.javaapi/src/main/java/io/minio/PutObjectArgs.javaapi/src/main/java/io/minio/UploadSnowballObjectsArgs.javaapi/src/main/java/io/minio/Utils.javaapi/src/main/java/io/minio/credentials/AwsConfigProvider.javaapi/src/main/java/io/minio/credentials/MinioClientConfigProvider.javaapi/src/main/java/io/minio/credentials/MinioEnvironmentProvider.javaapi/src/main/java/io/minio/errors/MinioException.javaapi/src/main/java/io/minio/messages/AccessControlPolicy.javaapi/src/main/java/io/minio/messages/Checksum.javaapi/src/main/java/io/minio/messages/Filter.javaapi/src/main/java/io/minio/messages/LifecycleConfiguration.javaapi/src/main/java/io/minio/messages/ObjectLockConfiguration.javaapi/src/main/java/io/minio/messages/ReplicationConfiguration.javaapi/src/main/java/io/minio/messages/VersioningConfiguration.javabuild.gradleexamples/GetObject.javaexamples/GetObjectProgressBar.javaexamples/GetPartialObject.javaexamples/GetPresignedPostFormData.javaexamples/MinioClientWithAssumeRoleProvider.javaexamples/MinioClientWithClientGrantsProvider.javaexamples/MinioClientWithWebIdentityProvider.javaexamples/ProgressStream.javaexamples/PutObjectProgressBar.javaexamples/PutObjectUiProgressBar.javaexamples/SelectObjectContent.javafunctional/MintLogger.javafunctional/PutObjectRunnable.javafunctional/TestMinioClient.javafunctional/TestUserAgent.javagradle/wrapper/gradle-wrapper.propertiesgradlewgradlew.batspotbugs-filter.xml
Comment on lines
+473
to
+480
| return OBJECT_MAPPER | ||
| .<Map<String, JsonNode>>readValue(response.body().bytes(), mapType) | ||
| .entrySet() | ||
| .stream() | ||
| .filter(entry -> "quota".equals(entry.getKey())) | ||
| .findFirst() | ||
| .map(entry -> Long.valueOf(entry.getValue().toString())) | ||
| .orElseThrow(() -> new IllegalArgumentException("found not quota")); | ||
| .map(entry -> entry.getValue().asLong()) | ||
| .orElseThrow(() -> new IllegalArgumentException("quota not found in response")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding method and relevant imports in the target file.
file="adminapi/src/main/java/io/minio/admin/MinioAdminClient.java"
wc -l "$file"
sed -n '430,520p' "$file"
# Find any other quota parsing or validation logic in the repository.
rg -n "quota not found in response|asLong\(\)|quota" adminapi/src/main/java/io/minio/admin -SRepository: minio/minio-java
Length of output: 4603
Parse quota as a number before returning it.
asLong() coerces non-numeric JSON to 0, so a malformed response is indistinguishable from a cleared quota. Treat a non-numeric quota as an error instead.
🤖 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 `@adminapi/src/main/java/io/minio/admin/MinioAdminClient.java` around lines 473
- 480, The quota parsing in MinioAdminClient should reject non-numeric JSON
instead of coercing it to 0. In the response-handling code that streams the
parsed map and extracts the "quota" entry, check that the JsonNode is actually
numeric before converting it, and throw an error if it is missing or not a
number rather than relying on asLong(). Keep the existing quota lookup flow, but
update the parsing logic to treat malformed quota values as failures.
Comment on lines
+965
to
967
| Files.deleteIfExists(Paths.get(args.filename())); | ||
| client.removeObject( | ||
| RemoveObjectArgs.builder().bucket(args.bucket()).object(args.object()).build()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Ensure remote cleanup still runs if local file deletion fails.
Files.deleteIfExists(...) can throw before client.removeObject(...), leaving the uploaded object behind and potentially breaking later bucket cleanup. Move remote cleanup before local deletion with a nested finally so both cleanup actions are attempted.
As per coding guidelines, functional/**/*.java tests hit a real S3/MinIO server, so cleanup should not be skipped.
Proposed cleanup ordering fix
} catch (Exception e) {
handleException(methodName, testTags, startTime, e);
} finally {
- Files.deleteIfExists(Paths.get(args.filename()));
- client.removeObject(
- RemoveObjectArgs.builder().bucket(args.bucket()).object(args.object()).build());
+ try {
+ client.removeObject(
+ RemoveObjectArgs.builder().bucket(args.bucket()).object(args.object()).build());
+ } finally {
+ Files.deleteIfExists(Paths.get(args.filename()));
+ }
}📝 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
| Files.deleteIfExists(Paths.get(args.filename())); | |
| client.removeObject( | |
| RemoveObjectArgs.builder().bucket(args.bucket()).object(args.object()).build()); | |
| try { | |
| client.removeObject( | |
| RemoveObjectArgs.builder().bucket(args.bucket()).object(args.object()).build()); | |
| } finally { | |
| Files.deleteIfExists(Paths.get(args.filename())); | |
| } |
🤖 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 `@functional/TestMinioClient.java` around lines 965 - 967, In TestMinioClient,
the cleanup in the test teardown currently deletes the local file before calling
client.removeObject, so a local deletion failure can prevent remote object
cleanup. Update the teardown logic around args.filename(), args.bucket(), and
args.object() to ensure client.removeObject(RemoveObjectArgs.builder()...) runs
first or is guaranteed via a nested finally, then perform Files.deleteIfExists
afterward so both cleanup steps are always attempted.
Source: Coding guidelines
Comment on lines
+5
to
+6
| retries=0 | ||
| retryBackOffMs=500 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
retries=0 disables Gradle wrapper download retries.
With retries=0, a single transient network failure while fetching the distribution will fail the wrapper bootstrap (and CI) with no retry, despite retryBackOffMs=500 being set (it has no effect at 0 retries). Consider a small non-zero value to absorb flaky downloads.
🔧 Suggested change
-retries=0
+retries=3
retryBackOffMs=500📝 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
| retries=0 | |
| retryBackOffMs=500 | |
| retries=3 | |
| retryBackOffMs=500 |
🤖 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 `@gradle/wrapper/gradle-wrapper.properties` around lines 5 - 6, The Gradle
wrapper configuration currently disables download retries by setting retries to
zero, which makes bootstrap fail on any transient network issue. Update the
wrapper properties to use a small non-zero retry count so the retryBackOffMs
setting in gradle-wrapper.properties actually takes effect and can absorb flaky
distribution downloads.
bbdbf8e to
c61bcbb
Compare
…amples api: - Compose/copy >5GiB: send per-part x-amz-copy-source-range (was loop-invariant) - Checksum.CRC64NVME.update: bound the slicing loop by len, not p.length - messages/Filter: keep And(String,Map) tags; "exactly one" via new Utils.xor - Http BaseUrl: ELB endpoints derive the real region (was "com") - messages/Checksum.headers(): emit x-amz-checksum-<algo> - PromptObjectArgs prompt setter; ListPartsArgs.Builder extends ObjectArgs.Builder - maxKeys() null-safe; Arrays.hashCode for array-backed args - downloadObject error propagation + temp cleanup; snowball/appendObject RAF close - uploadPartsParallelly buffer return/abort; executeAsync 304 return - credential providers: AwsConfig/MinioClientConfig/MinioEnvironment NPE -> clean - GetPresignedObjectUrlArgs expiry overflow; ObjectLockConfiguration duration(); AccessControlPolicy null owner; ListenBucketNotificationArgs requires bucket; GetObjectAttributesArgs validation; LifecycleConfiguration via Utils.xor; VersioningConfiguration.excludeFolders() primitive; ReplicationConfiguration text adminapi: - updateServiceAccount newStatus nullable (no silent disable) - Status.fromString null-guard; thread signing Credentials through execute() - getBucketQuota asLong(); Crypto.encrypt exact-multiple chunk; map getters return empty map when absent functional: - PutObjectRunnable surfaces thread failures; notification tests assert; legal-hold tests fixed; stream/temp-file cleanup; TestUserAgent guards; MintLogger throws UncheckedIOException instead of blank log examples: - close getObject/Response/progress streams (try-with-resources); fix GetObjectProgressBar/SelectObjectContent object names; real file size as object size; provider isSuccessful() check; placeholder credentials Also add CLAUDE.md with build commands and architecture overview. Signed-off-by: Bala.FA <bala@minio.io>
balamurugana
force-pushed
the
Fix-bugs-found-during-code-audit-across-api-adminapi-functional-examples
branch
from
c61bcbb to
f29cd3e
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
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.
api:
AccessControlPolicy null owner; ListenBucketNotificationArgs requires bucket;
GetObjectAttributesArgs validation; LifecycleConfiguration via Utils.xor;
VersioningConfiguration.excludeFolders() primitive; ReplicationConfiguration text
adminapi:
return empty map when absent
functional:
legal-hold tests fixed; stream/temp-file cleanup; TestUserAgent guards;
MintLogger throws UncheckedIOException instead of blank log
examples:
GetObjectProgressBar/SelectObjectContent object names; real file size as object
size; provider isSuccessful() check; placeholder credentials
Also add CLAUDE.md with build commands and architecture overview.
Signed-off-by: Bala.FA bala@minio.io
Summary by CodeRabbit
Bug Fixes
Chores
Depends on #1709