Changing isServerEncrypted parameter to Boolean to avoid NPE #32312
Conversation
ibrahimrabab
requested review from
jaschrep-msft and
alzimmermsft
as code owners
ghost
added
the
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
| @@ -31,7 +31,7 @@ public class BlockBlobItem { | |||
| * @param encryptionKeySha256 The encryption key used to encrypt the block blob. | |||
| */ | |||
| public BlockBlobItem(final String eTag, final OffsetDateTime lastModified, final byte[] contentMd5, | |||
| final boolean isServerEncrypted, final String encryptionKeySha256) { | |||
| final Boolean isServerEncrypted, final String encryptionKeySha256) { | |||
There was a problem hiding this comment.
This breaks the public API, instead my recommendation would be changing the code calling into this constructor to convert null to false (or true if that is the default for the service). Or, make all constructors taking boolean deprecated and add a new overload with Boolean that should be used.
There was a problem hiding this comment.
I recommend new constructor with Boolean and deprecating old constructors. Since the value describes the state of the data as the FE tells us, I would not want to make assumptions as to the state of the data if the FE has failed to tell us for whatever reason. Since the getter returns Boolean already, this should be safe that it sometimes is null instead of an explicit value, as we are meant to communicate the server response.
|
If we're fixing a bug we should be adding tests to confirm the fix. |
There was a problem hiding this comment.
You should not be re-overloading every single constructor. These are additive constructors where we've been forced to add a new parameter. You should be adding only one new constructor to the bottom of this list, which has all the current parameters and uses Boolean instead of boolean. Then mark all the others as deprecated, as the use of boolean is potentially unsafe.
|
/azp run java - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| * @param encryptionScope The encryption scope used to encrypt the block blob. | ||
| * @param versionId The version identifier of the block blob. | ||
| */ | ||
| public BlockBlobItem(final String eTag, final OffsetDateTime lastModified, final byte[] contentMd5, |
There was a problem hiding this comment.
adding this below the constructors instead of above will maintain the chronological progression of these constructors, making it easier to decipher later.
Description
resolves #32255
NullPointerException occurred due to null value passed in for isXMsRequestServerEncrypted. Changed to Boolean to handle nulls.