Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify SDK to not fail on invalid Expires header #5056

Merged
merged 27 commits into from Apr 30, 2024

Conversation

anirudh9391
Copy link
Contributor

@anirudh9391 anirudh9391 commented Mar 29, 2024

Implementing a SEP to modify behavior while parsing S3 Expires header.

Motivation and Context

  • SEP to implement an alternate way of unmarshalling exception handling for the "Expires" header.

Modifications

  • SDKs will not throw an exception if an invalid expires header is present.
  • "Expires" header will be defined as a timestamp regardless of any changes to its model definition.

Testing

Defined a custom unit test and validated by running it once. Source code

    @Test
    void test() throws IOException {

        String bucketName = "anirudkr-bucket-2";
        String key = "key1.txt";

        //File file = new RandomTempFile(10_000);
        S3Client client = S3Client.builder().build();

        //PutObjectRequest put = PutObjectRequest.builder().bucket(bucketName).key(key).expires(Instant.now()).build();
        //client.putObject(put, file.toPath());

        LogCaptor logCaptor = LogCaptor.create();

        HeadObjectRequest request = HeadObjectRequest.builder().bucket(bucketName).key(key).build();
        Assertions.assertThatCode(() -> client.headObject(request))
                  .doesNotThrowAnyException();
        assertTrue(client.headObject(request).expires() == null);
        assertTrue(client.headObject(request).expiresString() != null);

        List<LogEvent> events = logCaptor.loggedEvents();
        String expected = "Invalid datetime format provided in the Expires field 2034-01-01T00:00:00Z";
    }

Runs successfully.

The Expires header on this bucket is an invalid format
2034-01-01T00:00:00Z

Test validates

  1. exception is not logged
  2. warning is thrown

Screenshots (if appropriate)

Types of changes

  • [x ] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

@anirudh9391 anirudh9391 requested a review from a team as a code owner March 29, 2024 17:27
@anirudh9391 anirudh9391 changed the title Anirudkr/s3 sep expires Modify SDK to not fail on invalid Expires header Mar 29, 2024
Crissttianreyes

This comment was marked as off-topic.

@L-Applin
Copy link
Contributor

L-Applin commented Apr 25, 2024

I don't see the mentioned unit test in the added code? Also, should we add unit-test the HeaderUnmarshaller.INSTANT un-marshaller?

@L-Applin
Copy link
Contributor

I think we should include a codegen test for the added trait

@anirudh9391
Copy link
Contributor Author

I don't see the mentioned unit test in the added code? Also, should we add unit-test the HeaderUnmarshaller.INSTANT un-marshaller?

Im working on the wiremock tests. I think the wiremock should cover the code path

I don't see the mentioned unit test in the added code? Also, should we add unit-test the HeaderUnmarshaller.INSTANT un-marshaller?

Im working on the wiremock tests, should publish that along with the next revision.

Copy link

sonarcloud bot commented Apr 29, 2024

@anirudh9391 anirudh9391 merged commit 31ec0e7 into master Apr 30, 2024
17 checks passed
@anirudh9391 anirudh9391 deleted the anirudkr/s3-sep-expires branch April 30, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants