Let S3Mock accept * and "*" for conditional requests#2382
Merged
Conversation
S3 API accepts * for conditional requests on all APIs. All APIs but PutObject succeed with "*" as well. Fixes #2371
68b8718 to
384ddda
Compare
230389e to
9f506ba
Compare
9f506ba to
9a1e155
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances S3Mock’s conditional request handling by allowing both "" and """" as valid wildcard etags. In addition, tests have been refactored to use centralized file constants and bucket naming has been updated for consistency.
- Updates conditional matching logic in ObjectService to consider both wildcards.
- Replaces local File instantiation in tests with UPLOAD_FILE constants and improves resource usage.
- Refines bucket name generation in S3TestBase to replace "=" with "-" for consistency.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/src/main/java/com/adobe/testing/s3mock/service/ObjectService.java | Adjusted conditional etag matching and reordered date-based checks to support "*" wildcard requests. |
| integration-tests/src/test/kotlin/com/adobe/testing/s3mock/its/* | Updated test files to use centralized UPLOAD_FILE constants and ensure proper resource management via .use. |
| integration-tests/src/test/kotlin/com/adobe/testing/s3mock/its/S3TestBase.kt | Modified bucket naming logic by replacing "=" with "-" and added a helper method for directory buckets. |
Comments suppressed due to low confidence (2)
server/src/main/java/com/adobe/testing/s3mock/service/ObjectService.java:374
- [nitpick] Verify that moving the unmodified-since check to after the match condition does not introduce any unintended side effects in conditional request evaluation.
var setUnmodifiedSince = ifUnmodifiedSince != null && !ifUnmodifiedSince.isEmpty();
integration-tests/src/test/kotlin/com/adobe/testing/s3mock/its/S3TestBase.kt:248
- [nitpick] Confirm that replacing '=' with '-' in bucket names aligns with the expected S3 naming conventions and does not impact any external integrations.
.replace('=', '-')
| } | ||
|
|
||
| var setMatch = match != null && !match.isEmpty(); | ||
| if (setMatch) { |
There was a problem hiding this comment.
[nitpick] Consider adding an inline comment to explain why the condition now checks for both '' and '""', which will aid future maintainers in understanding the rationale behind expanding the wildcard support.
Suggested change
| if (setMatch) { | |
| if (setMatch) { | |
| // Check for both quoted ("*") and unquoted (*) wildcards to ensure compatibility | |
| // with different client implementations or specifications. |
f9e04c0 to
6e91c2d
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
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.
Description
S3 API accepts * for conditional requests on all APIs. All APIs but PutObject succeed with "*" as well.
Related Issue
Fixes #2371
Tasks