Fix for issue #11505

[gmantri]

Fixes the issue #11505 (#11505)

[ghost]

Thank you for your contribution gmantri! We will review the pull request and get back to you soon.

[joheredi]

Thanks a lot for your contribution! The changes look good in general, I just have a question about the scenario where an ACL is set without an access policy as the REST API documentation doesn’t seem to explicitly say which elements are required.

[ljian3377]

The change itself looks good to me.

Two additional issues:

• accessPolicy in SignedIdentifier should have its fields as optional.

azure-sdk-for-js/sdk/storage/storage-file-share/src/ShareClient.ts

Line 233 in 7f97d0e

export interface SignedIdentifier {

export interface SignedIdentifier {
  /**
   * @member {string} id a unique id
   */
  id: string;
  /**
   * @member {AccessPolicy} accessPolicy
   */
  accessPolicy: {
    /**
     * @member {Date} startsOn the date-time the policy is active.
     */
    startsOn: Date;
    /**
     * @member {string} expiresOn the date-time the policy expires.
     */
    expiresOn: Date;
    /**
     * @member {string} permissions the permissions for the acl policy
     * @see https://docs.microsoft.com/en-us/rest/api/storageservices/set-share-acl
     */
    permissions: string;
  };
}

• ShareClient.setAccessPolicy() should change accordingly.

azure-sdk-for-js/sdk/storage/storage-file-share/src/ShareClient.ts

Line 1038 in 7f97d0e

for (const identifier of shareAcl || []) {

In File:

      for (const identifier of shareAcl || []) {
        acl.push({
          accessPolicy: {
            expiresOn: truncatedISO8061Date(identifier.accessPolicy.expiresOn),
            permissions: identifier.accessPolicy.permissions,
            startsOn: truncatedISO8061Date(identifier.accessPolicy.startsOn)
          },
          id: identifier.id
        });
      }

In Queue

      for (const identifier of queueAcl || []) {
        acl.push({
          accessPolicy: {
            expiresOn: identifier.accessPolicy.expiresOn
              ? truncatedISO8061Date(identifier.accessPolicy.expiresOn)
              : undefined,
            permissions: identifier.accessPolicy.permissions,
            startsOn: identifier.accessPolicy.startsOn
              ? truncatedISO8061Date(identifier.accessPolicy.startsOn)
              : undefined
          },
          id: identifier.id
        });
      }

[ljian3377]

Also want to hold the merge of this PR till after our October release as we changed the file structure and deleted this file in our feature branch. Merging this now would make it difficult to pick up the changes when merging.

[gmantri]

Also want to hold the merge of this PR till after our October release as we changed the file structure and deleted this file in our feature branch. Merging this now would make it difficult to pick up the changes when merging.

@ljian3377 ... What's the ETA for the October release? This is a blocking issue for us. One thing you could do is simply take the edits from my PR and make the changes in the feature branch accordingly.

[ljian3377]

@ljian3377 ... What's the ETA for the October release? This is a blocking issue for us. One thing you could do is simply take the edits from my PR and make the changes in the feature branch accordingly.

@gmantri
Around October 10th. You need us to release a pacakge, right? If that's the case, we should consider including this change for our next release. But it's gonna be a prerelease version (preview for new storage features). Is that OK with you?

If prerelease version on October 10th doesn't work for you, we may need to think about other workarounds.

[ljian3377]

Can't change the accessPolicy in SignedIdentifier's fields and itself to optional as it's a breaking change.
Returning undefined for required field will introduce inconsistency but will live with for now till we have a new major release.

[ljian3377]

The change is taken and shipped in @azure/[email protected] with another PR.