[BEAM-3813] Support (de)serialization of S3 encryption options via JSON #5491
Conversation
| * A Jackson {@link Module} that registers a {@link JsonDeserializer} for {@link SSECustomerKey} and | ||
| * {@link SSEAwsKeyManagementParams}. The serialized form is based on a Bean serialization. | ||
| */ | ||
| @AutoService(Module.class) |
There was a problem hiding this comment.
Any reason now to put the extra serializers/deserializes into AwsModule.java?
There was a problem hiding this comment.
No, I just thought since they corresponded to a different object S3Options they would be good in their own module but I can put all in AwsModule if you think it is better.
| final String algorithm = | ||
| node.get("algorithm") != null ? node.get("algorithm").textValue() : null; | ||
| final String md5 = node.get("md5") != null ? node.get("md5").textValue() : null; | ||
| return new SSECustomerKey(key).withAlgorithm(algorithm).withMd5(md5); |
There was a problem hiding this comment.
Annoyingly, passing in null doesn't have the SSECustomerKey set the defaults: https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-s3/src/main/java/com/amazonaws/services/s3/model/SSECustomerKey.java#L162
We should only call withYYY if the user specified YYY in the JSON.
| @Override | ||
| public SSECustomerKey deserialize(JsonParser parser, DeserializationContext context) | ||
| throws IOException { | ||
| JsonNode node = parser.getCodec().readTree(parser); |
There was a problem hiding this comment.
Might be easier to read if you convert the value into a Map<String, String> using:
Map<String, String> asMap =
jsonParser.readValueAs(new TypeReference<Map<String, String>>() {
});
ditto on below.
| } | ||
|
|
||
| @Test | ||
| public void testSSECustomerKeySerializationDeserialization() throws Exception { |
There was a problem hiding this comment.
Try a test where you set the minimal set of properties.
iemejia
force-pushed
the
fix-s3-json-options
branch
from
838b258
to
bbf98fb
Compare
|
Fixes done. PTAL again @lukecwik |
|
Thanks for the review @lukecwik , it really improved it by making it simpler. |
Following your remarks, I reused the same JIRA ticket since this is related. PTAL
R: @lukecwik