-
Notifications
You must be signed in to change notification settings - Fork 484
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
fix: rearrange, cleanup, deprecation and fixes in ServerSideEncryption #778
fix: rearrange, cleanup, deprecation and fixes in ServerSideEncryption #778
Conversation
public ServerSideEncryptionWithCustomerKey(SecretKey key, MessageDigest md5) { | ||
this.secretKey = key; | ||
this.md5 = md5; | ||
} | ||
|
||
@Override | ||
public final Type getType() { | ||
public final Type type() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java uses getter / setter for properties. Renaming is not idiomatic here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is old style and minio-java follows this style inspired from okhttp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, can/have we document that? Because a lot of Java code follows the "style" I mentioned...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no coding standard document we have. Modern style is followed from the beginning of minio-java. There is little different with getter style where getters may return reference of an class member but this style returns copy of a value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
@Override | ||
public void marshal(Map<String, String> headers) { | ||
public Map<String, String> headers() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we return a new HashMap instead of adding the headers to an existing one? The typical use case is to add the SSE-C headers to the HTTP header map. What do we gain here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning Map
gives full control to users how they will be manipulated, marshal(Map)
mutates passed values which we need to avoid for future failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand what you mean. How is
Map<String, String> headers = new Map<>();
sse.marshal(headers);
different from:
Map<String, String> headers = sse.headers();
apart from the situation that the the former case avoids a new map allocation when we serialize the SSE object into the HTTP headers - which is the entire purpose of this function. In the later case you crate a new map and "force" an explicit copy. So, in general both options are equally powerful just that we usually want to serialize to the HTTP header map of the request and not create a new map which we have to copy explicitly - or am I missing something.
Anyway minor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a little difference. marshal()
mutates header map passed, there could be a possible value loss if header map contains any such header. Whereas headers()
returns a new copy of SSE headers every time and caller has a control when/how to use returned headers. As this is public class/method, it is not a good idea to mutate a passed header map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a little difference. marshal() mutates header map passed, there could be a possible value loss if header map contains any such header.
That I got. I still don't see how this is different from headers
. Sure, you make a copy but callers can do that explicitly by passing a new map when they want to avoid overwriting with marshal
. So getting the same behavior as headers
like shown above. Anyway, the only purpose of this function is to serialize the object to the HTTP request. So you have a S3 API call:
putObject(...) {
req = new Request();
req.getHeaders().add("Content-MD5", ...);
//...
sse.marshal(req.getHeaders()); // With new method: req.getHeaders().addAll(sse.headers)
}
As this is public class/method, it is not a good idea to mutate a passed header map.
} | ||
|
||
static final class ServerSideEncryptionS3 extends ServerSideEncryption { | ||
@Override | ||
public final Type getType() { | ||
return Type.SSE_S3; | ||
public final Type type() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
} | ||
|
||
@Override | ||
public final void marshal(Map<String, String> headers) { | ||
headers.put("X-Amz-Server-Side-Encryption", "AES256"); | ||
public final Map<String, String> headers() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
87ce7af
to
f19ef50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, approved
1. Cleanup formatting issues. 2. Arrange code usage together. 3. Deprecate `getType()` and `marshal()`, `ServerSideEncryptionCopyWithCustomerKey` and `copyWithCustomerKey()`. Use `type()`, `headers()`, `copySourceHeaders()` and `withCustomerKey()`.
f19ef50
to
af2880e
Compare
ping @aead @sinhaashish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
getType()
andmarshal()
,ServerSideEncryptionCopyWithCustomerKey
andcopyWithCustomerKey()
. Usetype()
,headers()
,copySourceHeaders()
andwithCustomerKey()
.