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.

Sign up for GitHub

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

Changing isServerEncrypted parameter to Boolean to avoid NPE #32312

Merged
merged 7 commits into from
Dec 1, 2022
Merged

Changing isServerEncrypted parameter to Boolean to avoid NPE #32312

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
24e86f0
changing isServerEncrypted parameter to Boolean to avoid NPE
ibrahimrabab Nov 24, 2022
7488687
deprecating previous constructors that used boolean instead of Boolean
ibrahimrabab Nov 29, 2022
82dea2e
fixing indentation
ibrahimrabab Nov 29, 2022
f4e3858
adding test to handle x-ms-request-server-encrypted returned as null
ibrahimrabab Nov 30, 2022
caef629
using LiveOnly to see if test passes
ibrahimrabab Nov 30, 2022
4953b35
adding basic test to verify if BlockBlobItem handles null values
ibrahimrabab Nov 30, 2022
e1dd61a
moving newly added constructor to the bottom
ibrahimrabab Dec 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 56 additions & 4 deletions ...storage/azure-storage-blob/src/main/java/com/azure/storage/blob/models/BlockBlobItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

@alzimmermsft alzimmermsft Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this(eTag, lastModified, contentMd5, isServerEncrypted, encryptionKeySha256, null);
}

Expand All @@ -46,7 +46,7 @@ public BlockBlobItem(final String eTag, final OffsetDateTime lastModified, final
* @param encryptionScope The encryption scope used to encrypt the block blob.
*/
public BlockBlobItem(final String eTag, final OffsetDateTime lastModified, final byte[] contentMd5,
final boolean isServerEncrypted, final String encryptionKeySha256, final String encryptionScope) {
final Boolean isServerEncrypted, final String encryptionKeySha256, final String encryptionScope) {
this(eTag, lastModified, contentMd5, isServerEncrypted, encryptionKeySha256, encryptionScope, null);
}

Expand All @@ -62,8 +62,8 @@ public BlockBlobItem(final String eTag, final OffsetDateTime lastModified, final
* @param versionId The version identifier of the block blob.
*/
public BlockBlobItem(final String eTag, final OffsetDateTime lastModified, final byte[] contentMd5,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding this below the constructors instead of above will maintain the chronological progression of these constructors, making it easier to decipher later.

final boolean isServerEncrypted, final String encryptionKeySha256,
final String encryptionScope, final String versionId) {
final Boolean isServerEncrypted, final String encryptionKeySha256, final String encryptionScope,
final String versionId) {
this.eTag = eTag;
this.lastModified = lastModified;
this.contentMd5 = CoreUtils.clone(contentMd5);
Expand All @@ -73,6 +73,58 @@ public BlockBlobItem(final String eTag, final OffsetDateTime lastModified, final
this.versionId = versionId;
}

/**
* Constructs a {@link BlockBlobItem}.
*
* @param eTag ETag of the block blob.
* @param lastModified Last modified time of the block blob.
* @param contentMd5 Content MD5 of the block blob.
* @param isServerEncrypted Flag indicating if the block blob is encrypted on the server.
* @param encryptionKeySha256 The encryption key used to encrypt the block blob.
* @deprecated Use {@link #BlockBlobItem(String, OffsetDateTime, byte[], Boolean, String)} instead.
*/
@Deprecated
public BlockBlobItem(final String eTag, final OffsetDateTime lastModified, final byte[] contentMd5,
final boolean isServerEncrypted, final String encryptionKeySha256) {
this(eTag, lastModified, contentMd5, (Boolean) isServerEncrypted, encryptionKeySha256);
}

/**
* Constructs a {@link BlockBlobItem}.
*
* @param eTag ETag of the block blob.
* @param lastModified Last modified time of the block blob.
* @param contentMd5 Content MD5 of the block blob.
* @param isServerEncrypted Flag indicating if the block blob is encrypted on the server.
* @param encryptionKeySha256 The encryption key used to encrypt the block blob.
* @param encryptionScope The encryption scope used to encrypt the block blob.
* @deprecated Use {@link #BlockBlobItem(String, OffsetDateTime, byte[], Boolean, String, String)} instead.
*/
@Deprecated
public BlockBlobItem(final String eTag, final OffsetDateTime lastModified, final byte[] contentMd5,
final boolean isServerEncrypted, final String encryptionKeySha256, final String encryptionScope) {
this(eTag, lastModified, contentMd5, (Boolean) isServerEncrypted, encryptionKeySha256, encryptionScope);
}

/**
* Constructs a {@link BlockBlobItem}.
*
* @param eTag ETag of the block blob.
* @param lastModified Last modified time of the block blob.
* @param contentMd5 Content MD5 of the block blob.
* @param isServerEncrypted Flag indicating if the block blob is encrypted on the server.
* @param encryptionKeySha256 The encryption key used to encrypt the block blob.
* @param encryptionScope The encryption scope used to encrypt the block blob.
* @param versionId The version identifier of the block blob.
* @deprecated Use {@link #BlockBlobItem(String, OffsetDateTime, byte[], Boolean, String, String, String)} instead.
*/
@Deprecated
public BlockBlobItem(final String eTag, final OffsetDateTime lastModified, final byte[] contentMd5,
final boolean isServerEncrypted, final String encryptionKeySha256, final String encryptionScope,
final String versionId) {
this(eTag, lastModified, contentMd5, (Boolean) isServerEncrypted, encryptionKeySha256, encryptionScope, versionId);
}

/**
* @return the eTag of the block blob
*/
Expand Down