fix: s3 200 errors handling #2968
Merged
Conversation
This file contains 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
yenfryherrerafeliz
force-pushed
the
fix_s3200_handling
branch
2 times, most recently
from
b91a09c
to
7cc95b1
Compare
yenfryherrerafeliz
force-pushed
the
fix_s3200_handling
branch
2 times, most recently
from
b44f432
to
9b1e18c
Compare
- This change includes the following: -- Refactor the s3 200 response error handling logic so it can be opened for all s3 operations. -- Migrate some S3 parsers to a result mutator implementation so the code can be a bit cleaner.
yenfryherrerafeliz
force-pushed
the
fix_s3200_handling
branch
from
9b1e18c
to
f921b56
Compare
Resolve merge conflicts found in S3ClientTest.php
GetObject operation has a streaming member which will be ignored and hence not treated as a 200 s3 error.
Make sure that operations where any of its member has an eventstream or streaming trait are ignored.
yenfryherrerafeliz
force-pushed
the
fix_s3200_handling
branch
from
acd2b1d
to
80a195c
Compare
stobrien89
requested changes
Aug 8, 2024
| * @param $checksumPriority | ||
| * @param ResponseInterface $response | ||
| */ | ||
| public function chooseChecksumHeaderToValidate( |
There was a problem hiding this comment.
I think we should make this private, as well as the one above it, so long as they aren't called directly anywhere else in our source code. We should be able to indirectly test selection/validation via the result returned when invoking this class. These are helper methods which shouldn't have been made public in the original implementation
yenfryherrerafeliz
force-pushed
the
fix_s3200_handling
branch
from
e91c098
to
bf7ae12
Compare
stobrien89
requested changes
Aug 9, 2024
| * @param $checksumPriority | ||
| * @param ResponseInterface $response | ||
| */ | ||
| public function chooseChecksumHeaderToValidate( |
yenfryherrerafeliz
force-pushed
the
fix_s3200_handling
branch
3 times, most recently
from
11ebd63
to
954c9d1
Compare
stobrien89
approved these changes
Aug 12, 2024
yenfryherrerafeliz
force-pushed
the
fix_s3200_handling
branch
from
954c9d1
to
41bbed7
Compare
- Split statements into multiple lines to improve readability. - Make pattern variable for finding the Error element name to be static. - Add a test in the GetBucketLocationResultMutatorTest to make sure the mutator just applies to GetBucketLocation. - Move some values into static variables in the GetBucketLocationMutator implementation. - Change the visibility of some methods in the ValidateResponseCheckResultMutator from public to private. - Refactor the tests on ValidateResponseCheckResultMutatorTest to make them be executed from the mutator invokation instead of testing those methods independently.
yenfryherrerafeliz
force-pushed
the
fix_s3200_handling
branch
from
41bbed7
to
bb1c0d4
Compare
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.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.