Breaking Changes - Permission models

[rakshith91]

Resolves #7508
Resolves #7510

Following changes are made to AccountPermissions, ContainerPermissions and BlobPermissions

• Rename models named ***Permissions to ***SasPermissions
• Remove enum-like list parameters
• Remove __add__ and or` methods
• Add a class method from_string

[adxsdk6]

Can one of the admins verify this patch?

[rakshith91]

Recordings that are failing are related to Rehydrate, not related to this PR
https://dev.azure.com/azure-sdk/public/_build/results?buildId=121111&view=ms.vss-test-web.build-test-results-tab&runId=4786488&resultId=100278&paneView=debug

[annatisch]

This is an enum of service values and should not be changed.

[annatisch]

We should construct self._str in here, and keep it around (or use directly the input from from_string. Then we can return it in __str__. This should keep the "non-lossy" behaviour discussed in the review

[annatisch]

You don't need to do this because strings are iterable in Python:

for letter in permission_str:
    read = (letter == 'r')

or you could just use the same logic that's in the constructor:

read = ('r' in permission)
write = ('w' in permission)
...

[annatisch]

We don't want to raise here, as we want to be able to support strings from future API versions that may have additional letters. Instead, we parse what we can, but keep the original string around and use that for the actual query.

def from_string(cls, permissions):
    # parse string
    parsed = cls(read=True, write=False, ....)
    parsed._str = permissions  # keeping original value
    return parsed

[annatisch]

list is a reserved word that we should avoid redefining.
Maybe use names like: p_read, p_write, p_list...

[annatisch]

Same comments re AccountSasPermissions

[annatisch]

Should be read=True, delete=True

[annatisch]

There's quite a few of these - I wont comment all of them

[rakshith91]

Cons of carelessly mass refactoring 😬

[annatisch]

Should be read=True, delete=True

[annatisch]

Should be read=True, delete=True

[annatisch]

We should update these so that we confirm that parsing does not fail for invalid characters, and that the invalid characters are still used in the query string.

[rakshith91]

@zezha-msft @xiafu-msft @annatisch
Do you think there should be a limit on the length of the string that should be passed?
Something like

if len(permission) > 8:
   raise ValueError("")

This would prevent someone to pass a really long string and slow down things. But on the other hand, adding the check might make it less forward-compatible, since if there are any new accepted letters that are added later, the length check might potentially break things.
One option as a middle ground is to limit the length to a larger number (like say, 100). Do you have any thoughts/ opinions?

[xiafu-msft]

I feel maybe it's not necessary. We only check if an letter is in the passed str using if 'r' in permission, so the returned permission will never be longer than 8(for now)?

[xiafu-msft]

those Permissions are under _shared folder, so we are supporsed to rename all in azure-storage-queue and azure-storage-file. Do we plan to rename all Permissions in another pr or do we have any other plan?

[xiafu-msft]

A little bit confused about this.
If permission is 'abcd' I guess parsed._str should be 'dc' instead of 'abcd'?

[rakshith91]

no, the parsed._str will be 'abcd' for non-lossy behavior. (We want to keep the original string.)

[xiafu-msft]

same question as the one in AccountSasPermission

[rakshith91]

no, the parsed._str will be 'abcd' for non-lossy behavior. (We want to keep the original string.)

[xiafu-msft]

I know we want to remove the enum like parameters. While some existing customers may feel more comfortable using that, can we just not write that in the api doc and leave these in the class. Those add or will give users more flexibility to use the api I feel like. And I feel like removing everything to make the code cleaner could cause a regression in user experience.

[rakshith91]

i believe it was decided in the api review to remove enum like params. also, having both enum-params and string is creating a very un-readable object

[xiafu-msft]