Skip to content

Commit

Permalink
[Storage] Fixed an issue of: StoragePipelineOptions is not passed con…
Browse files Browse the repository at this point in the history
…structor correctly (#25516)

Fixed an issue of: StoragePipelineOptions is not passed into
constructors for blob clients correctly when using undefined as
credential parameter.

Repro code is like:

```
const options: StoragePipelineOptions = { keepAliveOptions: { enable: false }, retryOptions: { maxTries: 1, maxRetryDelayInMs: 0, retryDelayInMs: 0 } }

const blockBlobClient = new BlockBlobClient(sasUrl.sasTokenUri, undefined, options);
```

The issue is because: 
BlobClient and BlockBlobClient supports constructor with parameters
like:

```
constructor(
    connectionString: string,
    containerName: string,
    blobName: string,
    // Legacy, no fix for eslint error without breaking. Disable it for this interface.
    /* eslint-disable-next-line @azure/azure-sdk/ts-naming-options*/
    options?: StoragePipelineOptions
  );

  constructor(
    url: string,
    credential?: StorageSharedKeyCredential | AnonymousCredential | TokenCredential,
    // Legacy, no fix for eslint error without breaking. Disable it for this interface.
    /* eslint-disable-next-line @azure/azure-sdk/ts-naming-options*/
    options?: StoragePipelineOptions
  );
```
StoragePipelineOptions can be the third or the forth parameter. It
chooses the forth parameter as StoragePipelineOptions while an undefined
credential is used, but the third one is the correct one.

Note:
1. No such issue in other storage SDKs, because in other storage SDKs
StoragePipelineOptions is always the third parameter.
2. PageBlobClient/AppendBlobClient doesn't have this issue, because they
don't support nullable credential object. For
BlobClient/BlockBlobClient, following code can work around the issue:

`const blockBlobClient = new BlockBlobClient(sasUrl.sasTokenUri, new
AnonymousCredential(), options);`

3. Currently we have no way to test whether the option has been passed
into the client with unit test. I have manually test it against an
injected service.
  • Loading branch information
EmmaZhu authored Apr 13, 2023
1 parent 38d6412 commit d620be0
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 3 deletions.
2 changes: 2 additions & 0 deletions sdk/storage/storage-blob/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

### Bugs Fixed

- Fixed an issue of: StoragePipelineOptions is not passed into constructors for blob clients correctly when using undefined as credential parameter.

### Other Changes

## 12.14.0-beta.1 (2023-03-29)
Expand Down
6 changes: 6 additions & 0 deletions sdk/storage/storage-blob/src/Clients.ts
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,9 @@ export class BlobClient extends StorageClient {
// (url: string, credential?: StorageSharedKeyCredential | AnonymousCredential | TokenCredential, options?: StoragePipelineOptions)
// The second parameter is undefined. Use anonymous credential.
url = urlOrConnectionString;
if (blobNameOrOptions && typeof blobNameOrOptions !== "string") {
options = blobNameOrOptions as StoragePipelineOptions;
}
pipeline = newPipeline(new AnonymousCredential(), options);
} else if (
credentialOrPipelineOrContainerName &&
Expand Down Expand Up @@ -3666,6 +3669,9 @@ export class BlockBlobClient extends BlobClient {
// (url: string, credential?: StorageSharedKeyCredential | AnonymousCredential | TokenCredential, options?: StoragePipelineOptions)
// The second parameter is undefined. Use anonymous credential.
url = urlOrConnectionString;
if (blobNameOrOptions && typeof blobNameOrOptions !== "string") {
options = blobNameOrOptions as StoragePipelineOptions;
}
pipeline = newPipeline(new AnonymousCredential(), options);
} else if (
credentialOrPipelineOrContainerName &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1490,7 +1490,7 @@ export type ShareAccessTier = "TransactionOptimized" | "Hot" | "Cool";
//
// @public
export class ShareClient extends StorageClient {
constructor(connectionString: string, name: string, options?: StoragePipelineOptions);
constructor(connectionString: string, name: string, options?: ShareClientOptions);
constructor(url: string, credential?: Credential_2 | TokenCredential, options?: ShareClientOptions);
constructor(url: string, pipeline: Pipeline, options?: ShareClientConfig);
create(options?: ShareCreateOptions): Promise<ShareCreateResponse>;
Expand Down
4 changes: 2 additions & 2 deletions sdk/storage/storage-file-share/src/Clients.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ import {
FileLastWrittenMode,
} from "./generatedModels";
import { Share, Directory, File } from "./generated/src/operations";
import { newPipeline, StoragePipelineOptions, Pipeline } from "./Pipeline";
import { newPipeline, Pipeline } from "./Pipeline";
import {
DEFAULT_MAX_DOWNLOAD_RETRY_REQUESTS,
DEFAULT_HIGH_LEVEL_CONCURRENCY,
Expand Down Expand Up @@ -566,7 +566,7 @@ export class ShareClient extends StorageClient {
*/
// Legacy, no way to fix the eslint error without breaking. Disable the rule for this line.
/* eslint-disable-next-line @azure/azure-sdk/ts-naming-options */
constructor(connectionString: string, name: string, options?: StoragePipelineOptions);
constructor(connectionString: string, name: string, options?: ShareClientOptions);
/**
* Creates an instance of ShareClient.
*
Expand Down

0 comments on commit d620be0

Please sign in to comment.