fix: use the configured async client to get the instruction file #366
Conversation
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import static org.junit.jupiter.api.Assertions.fail; | ||
| import static org.junit.jupiter.api.Assertions.*; |
There was a problem hiding this comment.
Do we have style guidelines around not using * imports?
I like * imports for ease of development. But previous styleguides have suggested that my opinion is wrong. And we haven't used * imports in this file so far, so I'm wondering if there's some implicit styleguide here.
There was a problem hiding this comment.
we are not entirely consistent,
particularly between repos,
but most of this project prefers explicit imports,
so I'll apply that here.
| .build(); | ||
|
|
||
| ContentMetadata contentMetadata = ContentMetadataStrategy.decode(getObjectRequest, getObjectResponse); | ||
| ContentMetadata contentMetadata = new ContentMetadataDecodingStrategy(null).decode(getObjectRequest, getObjectResponse); |
There was a problem hiding this comment.
(nit) Seems this should be a mock S3 client instead of null; if this test were to attempt to call S3 it would result in NPE.
But this test doesn't call S3, so this is a nit.
There was a problem hiding this comment.
Do we need null checking somewhere?
If I pass in a null S3 client due to being (silly, adversarial, etc.), is this checked/guarded against in the non-internal class level?
There was a problem hiding this comment.
The top-level client builders will create a client when there is none provided,
so it is always the case that the client is not null.
That said,
I'll add a null check / mock in the name of robustness.
There was a problem hiding this comment.
Cool -- I think creating a default top-level client is sufficient but more checks never hurt anyone 👍
| } | ||
|
|
||
| @Test | ||
| public void s3EncryptionClientMixedCredentialsInstructionFile() { |
There was a problem hiding this comment.
I'm not sure what the scope of this test is; it looks like both a failure and success test case?
Could you clarify what exactly is being tested in comments here?
There was a problem hiding this comment.
sure, I'll break it out into two separate tests
kessplas
changed the title
| S3AsyncClient s3AsyncClient = S3AsyncClient.create(); | ||
| ContentMetadata contentMetadata = new ContentMetadataDecodingStrategy(s3AsyncClient).decode(getObjectRequest, getObjectResponse); |
There was a problem hiding this comment.
This is fine too, I originally meant something like
S3AsyncClient s3AsyncClient = mock(S3AsyncClient.class);
2 participants
Issue #, if available: n/a
Description of changes: When getting an instruction file, the S3EC creates a default client. This client cannot be configured, so unless the environment has the correct creds, the instruction file GET will fail. This change uses the same async client that GetObject uses to avoid issues with configuration.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: