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

Added UTF-8 encoding for blob names in methods that are used to build… #5943

Merged
merged 14 commits into from
Oct 23, 2019
Merged

Added UTF-8 encoding for blob names in methods that are used to build… #5943

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9230cc1
Added UTF-8 encoding for blob names in methods that are used to build…
vcolin7 Oct 18, 2019
6b94186
Merge branch 'master' of github.com:Azure/azure-sdk-for-java into iss…
vcolin7 Oct 20, 2019
a665c1a
Added missing comments about encryption to the Javadoc of some classe…
vcolin7 Oct 21, 2019
1227465
Changed a few couple Groovy tests that were failing because the expec…
vcolin7 Oct 21, 2019
55ac267
Removed a call to Utility.urlEncode() in BLobContainerAsyncClient tha…
vcolin7 Oct 21, 2019
b18fbed
Broke a couple lines in BlobBatch into two parts to comply with the c…
vcolin7 Oct 21, 2019
009d5ab
Added missing space to a line in BlobBatch. It was flagged by the CI …
vcolin7 Oct 21, 2019
995e411
Fixed an issue that made some test cases fail: blobName needs to be e…
vcolin7 Oct 21, 2019
aa04de2
Merge branch 'master' of github.com:Azure/azure-sdk-for-java into iss…
vcolin7 Oct 21, 2019
4b527de
Merge branch 'master' of github.com:Azure/azure-sdk-for-java into iss…
vcolin7 Oct 23, 2019
9bcd0a0
Made sure that blob names are decoded from UTF-8 before being returne…
vcolin7 Oct 23, 2019
2172d80
Because we are doing encoding blob names implicitly, there is no poin…
vcolin7 Oct 23, 2019
6489555
Made sure that the blobName used to generate a SAS signature is decod…
vcolin7 Oct 23, 2019
dfb9416
Merge branch 'master' into issue-4324
vcolin7 Oct 23, 2019
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
23 changes: 17 additions & 6 deletions ...torage/azure-storage-blob-batch/src/main/java/com/azure/storage/blob/batch/BlobBatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.azure.storage.blob.models.BlobAccessConditions;
import com.azure.storage.blob.models.DeleteSnapshotsOptionType;
import com.azure.storage.blob.models.LeaseAccessConditions;
import com.azure.storage.common.Utility;
import com.azure.storage.common.policy.StorageSharedKeyCredentialPolicy;
import reactor.core.Disposable;
import reactor.core.Exceptions;
Expand Down Expand Up @@ -126,6 +127,8 @@ public final class BlobBatch {
/**
* Adds a delete blob operation to the batch.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code sample</strong></p>
*
* {@codesnippet com.azure.storage.blob.batch.BlobBatch.deleteBlob#String-String}
Expand All @@ -137,12 +140,15 @@ public final class BlobBatch {
* @throws UnsupportedOperationException If this batch has already added an operation of another type.
*/
public Response<Void> deleteBlob(String containerName, String blobName) {
return deleteBlobHelper(String.format(PATH_TEMPLATE, containerName, blobName), null, null);
return deleteBlobHelper(String.format(PATH_TEMPLATE, containerName,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just call into the next overload so we don't have to worry about missing formatting/encoding logic in one of them

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, let's make it call the next overload.

Utility.urlEncode(Utility.urlDecode(blobName))), null, null);
}

/**
* Adds a delete blob operation to the batch.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code sample</strong></p>
*
* {@codesnippet com.azure.storage.blob.batch.BlobBatch.deleteBlob#String-String-DeleteSnapshotsOptionType-BlobAccessConditions}
Expand All @@ -157,8 +163,8 @@ public Response<Void> deleteBlob(String containerName, String blobName) {
*/
public Response<Void> deleteBlob(String containerName, String blobName,
DeleteSnapshotsOptionType deleteOptions, BlobAccessConditions blobAccessConditions) {
return deleteBlobHelper(String.format(PATH_TEMPLATE, containerName, blobName),
deleteOptions, blobAccessConditions);
return deleteBlobHelper(String.format(PATH_TEMPLATE, containerName,
Utility.urlEncode(Utility.urlDecode(blobName))), deleteOptions, blobAccessConditions);
}

/**
Expand Down Expand Up @@ -206,6 +212,8 @@ private Response<Void> deleteBlobHelper(String urlPath, DeleteSnapshotsOptionTyp
/**
* Adds a set tier operation to the batch.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code sample</strong></p>
*
* {@codesnippet com.azure.storage.blob.batch.BlobBatch.setBlobAccessTier#String-String-AccessTier}
Expand All @@ -218,12 +226,15 @@ private Response<Void> deleteBlobHelper(String urlPath, DeleteSnapshotsOptionTyp
* @throws UnsupportedOperationException If this batch has already added an operation of another type.
*/
public Response<Void> setBlobAccessTier(String containerName, String blobName, AccessTier accessTier) {
return setBlobAccessTierHelper(String.format(PATH_TEMPLATE, containerName, blobName), accessTier, null);
return setBlobAccessTierHelper(String.format(PATH_TEMPLATE, containerName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about calling the next overload.

Utility.urlEncode(Utility.urlDecode(blobName))), accessTier, null);
}

/**
* Adds a set tier operation to the batch.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code sample</strong></p>
*
* {@codesnippet com.azure.storage.blob.batch.BlobBatch.setBlobAccessTier#String-String-AccessTier-LeaseAccessConditions}
Expand All @@ -238,8 +249,8 @@ public Response<Void> setBlobAccessTier(String containerName, String blobName, A
*/
public Response<Void> setBlobAccessTier(String containerName, String blobName, AccessTier accessTier,
LeaseAccessConditions leaseAccessConditions) {
return setBlobAccessTierHelper(String.format(PATH_TEMPLATE, containerName, blobName), accessTier,
leaseAccessConditions);
return setBlobAccessTierHelper(String.format(PATH_TEMPLATE, containerName,
Utility.urlEncode(Utility.urlDecode(blobName))), accessTier, leaseAccessConditions);
}

/**
Expand Down
2 changes: 2 additions & 0 deletions ...c/main/java/com/azure/storage/blob/specialized/cryptography/EncryptedBlobAsyncClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ public class EncryptedBlobAsyncClient extends BlobAsyncClient {
/**
* Package-private constructor for use by {@link EncryptedBlobClientBuilder}.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @param pipeline The pipeline used to send and receive service requests.
* @param url The endpoint where to send service requests.
* @param serviceVersion The version of the service to receive requests.
Expand Down
12 changes: 11 additions & 1 deletion ...main/java/com/azure/storage/blob/specialized/cryptography/EncryptedBlobClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.azure.storage.blob.BlobContainerAsyncClient;
import com.azure.storage.blob.BlobServiceVersion;
import com.azure.storage.blob.BlobUrlParts;
import com.azure.storage.common.Utility;
import com.azure.storage.common.implementation.Constants;
import com.azure.storage.common.StorageSharedKeyCredential;
import com.azure.storage.common.implementation.connectionstring.StorageAuthenticationSettings;
Expand Down Expand Up @@ -104,6 +105,8 @@ public EncryptedBlobClientBuilder() {
/**
* Creates a {@link EncryptedBlobClient} based on options set in the Builder.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code Samples</strong></p>
*
* {@codesnippet com.azure.storage.blob.specialized.cryptography.EncryptedBlobClientBuilder.buildEncryptedBlobAsyncClient}
Expand All @@ -118,6 +121,8 @@ public EncryptedBlobClient buildEncryptedBlobClient() {
/**
* Creates a {@link EncryptedBlobAsyncClient} based on options set in the Builder.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code Samples</strong></p>
*
* {@codesnippet com.azure.storage.blob.specialized.cryptography.EncryptedBlobClientBuilder.buildEncryptedBlobClient}
Expand Down Expand Up @@ -324,6 +329,8 @@ public EncryptedBlobClientBuilder connectionString(String connectionString) {
* with blobs in the root container, it is best to set the endpoint to the account url and specify the blob name
* separately using the {@link EncryptedBlobClientBuilder#blobName(String) blobName} method.</p>
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it's setting us up for double encoding since they're already handing us an endpoint. I thought we agreed to have an overload that accepts a doNotEncodeBlobName flag or whatever it should be called.

Copy link
Member Author

Choose a reason for hiding this comment

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

BlobUrlParts.parse() takes care of parsing the endpoint and decoding its blob name from UTF-8 in case it is already encoded, to then finally encode it. The reason behind this is that since it's part of the public API, it is entirely possible that people might pass a non-encoded URL as an argument too.

I think there was also a communication mishap on my behalf when I told you about the last approach I discussed with Alan. There are several reasons why I think it's best to just implicitly do encoding for the user instead of letting the responsibility rest with them:

  • It makes the API simple to use if they never have to worry about passing encoded or non-encoded blob names, since with the changes from my last commit we will decode first to avoid double encoding. From running some tests I noticed this added little to no overhead.
  • It can be argued that having overloaded methods that add a flag is unnecessary work and would make the API look less simple and straightforward, since we could just modify the already existing methods to add an encodeBlobName flag as an argument.
  • If we added a flag to either an overloaded method or the already existing ones, I don't think it would be much different from letting the users know we need the blobName to be encoded and that they can manually use Utility.urlEncode() to do so, as you and Alan first discussed in the original issue.

The bottomline is that I believe implicit encoding is the most user-friendly way as long as we make sure we don't double encode anywhere in the flow.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see that you were doing a urlDecode before urlEncode in the setters. In that case, I think I'm fine with the PR as long as you add the update to return the unencoded name since we're assuming that will be the most common case

And can you actually also add to either the existing test or a new test that passes in an already encoded name and ensures it doesn't get double encoded. And also getting the name off of one client and passing it to another and again checking it doesn't get double encoded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add said getter and test changes.

* @param endpoint URL of the service
* @return the updated EncryptedBlobClientBuilder object
* @throws IllegalArgumentException If {@code endpoint} is {@code null} or is a malformed URL.
Expand Down Expand Up @@ -365,12 +372,15 @@ public EncryptedBlobClientBuilder containerName(String containerName) {
/**
* Sets the name of the blob.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @param blobName Name of the blob.
* @return the updated EncryptedBlobClientBuilder object
* @throws NullPointerException If {@code blobName} is {@code null}
*/
public EncryptedBlobClientBuilder blobName(String blobName) {
this.blobName = Objects.requireNonNull(blobName, "'blobName' cannot be null.");
this.blobName = Utility.urlEncode(Utility.urlDecode(Objects.requireNonNull(blobName,
"'blobName' cannot be null.")));
return this;
}

Expand Down
4 changes: 4 additions & 0 deletions sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobAsyncClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ public class BlobAsyncClient extends BlobAsyncClientBase {
/**
* Package-private constructor for use by {@link BlobClientBuilder}.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these comments on the constructors are accurate because the encoding happens in the builder. It seems like it's more accurate to say these constructors expect that the blobName is already properly encoded.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought of adding said comments wherever a blobName or URL/endpoint is passed as an argument to generate a blob client so that the user knew what would happen to it. In hindsight, I think we could omit these comments wherever the blobName is not directly altered. What do you think?

As a side note, there are some cases where the blobName is set in the constructor, such as BlobAsyncClientBase, which is used to create our specialized blob clients.

*
* @param pipeline The pipeline used to send and receive service requests.
* @param url The endpoint where to send service requests.
* @param serviceVersion The version of the service to receive requests.
Expand All @@ -94,6 +96,8 @@ protected BlobAsyncClient(HttpPipeline pipeline, String url, BlobServiceVersion
/**
* Creates a new {@link BlobAsyncClient} linked to the {@code snapshot} of this blob resource.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @param snapshot the identifier for a specific snapshot of this blob
* @return a {@link BlobAsyncClient} used to interact with the specific snapshot.
*/
Expand Down
12 changes: 11 additions & 1 deletion sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.azure.storage.blob.implementation.util.BuilderHelper;
import com.azure.storage.blob.models.CpkInfo;
import com.azure.storage.blob.models.CustomerProvidedKey;
import com.azure.storage.common.Utility;
import com.azure.storage.common.StorageSharedKeyCredential;
import com.azure.storage.common.implementation.connectionstring.StorageAuthenticationSettings;
import com.azure.storage.common.implementation.connectionstring.StorageConnectionString;
Expand Down Expand Up @@ -83,6 +84,8 @@ public BlobClientBuilder() {
* methods such as {@link BlobClient#download(OutputStream) download} and {@link BlobClient#getProperties() get
* properties}, use this when the blob type is unknown.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code Samples</strong></p>
*
* {@codesnippet com.azure.storage.blob.BlobClientBuilder.buildClient}
Expand All @@ -99,6 +102,8 @@ public BlobClient buildClient() {
* generic blob methods such as {@link BlobAsyncClient#download() download} and {@link
* BlobAsyncClient#getProperties()}, use this when the blob type is unknown.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code Samples</strong></p>
*
* {@codesnippet com.azure.storage.blob.BlobClientBuilder.buildAsyncClient}
Expand Down Expand Up @@ -251,6 +256,8 @@ public BlobClientBuilder connectionString(String connectionString) {
* with blobs in the root container, it is best to set the endpoint to the account url and specify the blob name
* separately using the {@link BlobClientBuilder#blobName(String) blobName} method.</p>
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @param endpoint URL of the service
* @return the updated BlobClientBuilder object
* @throws IllegalArgumentException If {@code endpoint} is {@code null} or is a malformed URL.
Expand Down Expand Up @@ -292,12 +299,15 @@ public BlobClientBuilder containerName(String containerName) {
/**
* Sets the name of the blob.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @param blobName Name of the blob.
* @return the updated BlobClientBuilder object
* @throws NullPointerException If {@code blobName} is {@code null}
*/
public BlobClientBuilder blobName(String blobName) {
this.blobName = Objects.requireNonNull(blobName, "'blobName' cannot be null.");
this.blobName = Utility.urlEncode(Utility.urlDecode(Objects.requireNonNull(blobName,
"'blobName' cannot be null.")));
return this;
}

Expand Down
11 changes: 8 additions & 3 deletions ...age/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobContainerAsyncClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.azure.storage.blob.models.ModifiedAccessConditions;
import com.azure.storage.blob.models.PublicAccessType;
import com.azure.storage.blob.models.StorageAccountInfo;
import com.azure.storage.common.Utility;
import com.azure.storage.common.implementation.StorageImplUtils;
import reactor.core.publisher.Mono;

Expand Down Expand Up @@ -112,6 +113,8 @@ public final class BlobContainerAsyncClient {
* the BlobAsyncClient and then call its WithPipeline method passing in the desired pipeline object. Or, call this
* package's getBlobAsyncClient instead of calling this object's getBlobAsyncClient method.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code Samples</strong></p>
*
* {@codesnippet com.azure.storage.blob.BlobContainerAsyncClient.getBlobAsyncClient#String}
Expand All @@ -129,6 +132,8 @@ public BlobAsyncClient getBlobAsyncClient(String blobName) {
* the BlobAsyncClient and then call its WithPipeline method passing in the desired pipeline object. Or, call this
* package's getBlobAsyncClient instead of calling this object's getBlobAsyncClient method.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code Samples</strong></p>
*
* {@codesnippet com.azure.storage.blob.BlobContainerAsyncClient.getBlobAsyncClient#String-String}
Expand All @@ -138,9 +143,9 @@ public BlobAsyncClient getBlobAsyncClient(String blobName) {
* @return A new {@link BlobAsyncClient} object which references the blob with the specified name in this container.
*/
public BlobAsyncClient getBlobAsyncClient(String blobName, String snapshot) {
return new BlobAsyncClient(getHttpPipeline(),
StorageImplUtils.appendToUrlPath(getBlobContainerUrl(), blobName).toString(), getServiceVersion(),
getAccountName(), getBlobContainerName(), blobName, snapshot, getCustomerProvidedKey());
return new BlobAsyncClient(getHttpPipeline(), StorageImplUtils.appendToUrlPath(getBlobContainerUrl(),
Utility.urlEncode(Utility.urlDecode(blobName))).toString(), getServiceVersion(), getAccountName(),
getBlobContainerName(), blobName, snapshot, getCustomerProvidedKey());
}

/**
Expand Down
4 changes: 4 additions & 0 deletions sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobContainerClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ public final class BlobContainerClient {
* BlobClient and then call its WithPipeline method passing in the desired pipeline object. Or, call this package's
* getBlobAsyncClient instead of calling this object's getBlobAsyncClient method.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @param blobName A {@code String} representing the name of the blob.
*
* <p><strong>Code Samples</strong></p>
Expand All @@ -81,6 +83,8 @@ public BlobClient getBlobClient(String blobName) {
* BlobClient and then call its WithPipeline method passing in the desired pipeline object. Or, call this package's
* getBlobAsyncClient instead of calling this object's getBlobAsyncClient method.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code Samples</strong></p>
*
* {@codesnippet com.azure.storage.blob.BlobContainerClient.getBlobClient#String-String}
Expand Down
12 changes: 11 additions & 1 deletion sdk/storage/azure-storage-blob/src/main/java/com/azure/storage/blob/BlobUrlParts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ public BlobUrlParts setContainerName(String containerName) {
/**
* Gets the blob name that will be used as part of the URL path.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @return the blob name.
*/
public String getBlobName() {
Expand All @@ -138,11 +140,13 @@ public String getBlobName() {
/**
* Sets the blob name that will be used as part of the URL path.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @param blobName The blob name.
* @return the updated BlobUrlParts object.
*/
public BlobUrlParts setBlobName(String blobName) {
this.blobName = blobName;
this.blobName = Utility.urlEncode(Utility.urlDecode(blobName));
Copy link
Contributor

Choose a reason for hiding this comment

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

Overload to skip encoding?

Copy link
Member Author

@vcolin7 vcolin7 Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

See answer about overloads and double encoding above.

return this;
}

Expand Down Expand Up @@ -211,6 +215,8 @@ public BlobUrlParts setUnparsedParameters(Map<String, String[]> unparsedParamete
/**
* Converts the blob URL parts to a {@link URL}.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @return A {@code URL} to the blob resource composed of all the elements in this object.
* @throws IllegalStateException The fields present on the BlobUrlParts object were insufficient to construct a
* valid URL or were ill-formatted.
Expand Down Expand Up @@ -267,6 +273,8 @@ public URL toUrl() {
* is no path element for the container, the name of this blob in the root container will be set as the
* containerName field in the resulting {@code BlobURLParts}.</p>
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Another place that feels likely to give us double encoding since we are taking in a URL

Copy link
Member Author

Choose a reason for hiding this comment

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

See answer about overloads and double encoding above.

*
* @param url The {@code URL} to be parsed.
* @return A {@link BlobUrlParts} object containing all the components of a BlobURL.
* @throws IllegalArgumentException If {@code url} is a malformed {@link URL}.
Expand All @@ -290,6 +298,8 @@ public static BlobUrlParts parse(String url) {
* is no path element for the container, the name of this blob in the root container will be set as the
* containerName field in the resulting {@code BlobURLParts}.</p>
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @param url The {@code URL} to be parsed.
* @return A {@link BlobUrlParts} object containing all the components of a BlobURL.
*/
Expand Down
2 changes: 2 additions & 0 deletions ...-storage-blob/src/main/java/com/azure/storage/blob/specialized/AppendBlobAsyncClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ public final class AppendBlobAsyncClient extends BlobAsyncClientBase {
/**
* Package-private constructor for use by {@link SpecializedBlobClientBuilder}.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @param pipeline The pipeline used to send and receive service requests.
* @param url The endpoint where to send service requests.
* @param serviceVersion The version of the service to receive requests.
Expand Down
7 changes: 6 additions & 1 deletion ...re-storage-blob/src/main/java/com/azure/storage/blob/specialized/BlobAsyncClientBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import com.azure.storage.blob.models.ReliableDownloadOptions;
import com.azure.storage.blob.models.SourceModifiedAccessConditions;
import com.azure.storage.blob.models.StorageAccountInfo;
import com.azure.storage.common.Utility;
import com.azure.storage.common.implementation.Constants;
import com.azure.storage.common.implementation.StorageImplUtils;
import reactor.core.publisher.Flux;
Expand Down Expand Up @@ -91,6 +92,8 @@ public class BlobAsyncClientBase {
/**
* Package-private constructor for use by {@link SpecializedBlobClientBuilder}.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* @param pipeline The pipeline used to send and receive service requests.
* @param url The endpoint where to send service requests.
* @param serviceVersion The version of the service to receive requests.
Expand All @@ -112,7 +115,7 @@ protected BlobAsyncClientBase(HttpPipeline pipeline, String url, BlobServiceVers

this.accountName = accountName;
this.containerName = containerName;
this.blobName = blobName;
this.blobName = Utility.urlEncode(Utility.urlDecode(blobName));
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we encode in the builder, so encoding in the constructor again would be double encoding?

Copy link
Member Author

Choose a reason for hiding this comment

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

See answer about overloads and double encoding above.

this.snapshot = snapshot;
this.customerProvidedKey = customerProvidedKey;
}
Expand Down Expand Up @@ -161,6 +164,8 @@ public final String getContainerName() {
/**
* Get the blob name.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't know that we should specify what method is doing the encoding since Utilities are supposed to be implementation details

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also probably be decoding on gets if we are encoding on sets. For one, we want it to round trip. For another, what if someone gets the name of one blob so they can set it as the name of a blob in a different container. Now we encode the name on setting it for the first blob, don't decode it in the getter, then encode it again in the second builder.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree we can leave out which method is used for encoding as long as we specify is UTF-8. As for the getters, even though we won't incur in any double encoding (see comment about that and overloads above), it sounds good to me so that users will always get to see and manipulate the original name of the blob.

*
* <p><strong>Code Samples</strong></p>
*
* {@codesnippet com.azure.storage.blob.specialized.BlobAsyncClientBase.getBlobName}
Expand Down
2 changes: 2 additions & 0 deletions ...e/azure-storage-blob/src/main/java/com/azure/storage/blob/specialized/BlobClientBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ public final String getContainerName() {
/**
* Get the blob name.
*
* <p>Blob name is encoded to UTF-8 using the {@link com.azure.storage.common.Utility#urlEncode(String)} method.</p>
*
* <p><strong>Code Samples</strong></p>
*
* {@codesnippet com.azure.storage.blob.specialized.BlobClientBase.getBlobName}
Expand Down
Loading