Skip to content

Update azure blob SDK version #1515

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.

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

Merged
merged 8 commits into from
Feb 17, 2021
Merged

Conversation

mdrichardson
Copy link
Contributor

@mdrichardson mdrichardson commented Feb 10, 2021

Partial fix for #1343

Please Read

#1343 is for porting over Azure Blobs and Queues from .NET and JS. In those SDK, Azure Blobs was plural-ified from Bot.Builder.Azure.BlobStorage to ...Blob*s*Storage, adding a new Blob Storage library. This was done because of a dependency namespace conflict that would break binary compatibility.

As far as I can tell, we can't do this in the Python SDK because that would require the SDK to depend on two different versions of azure.storage.blob. The current version in our SDK is 2.1, but the most recent version is 12.7.1. This PR updates to 12.7.0 because there's an msrest dependency conflict at 122.7.1.

So, this PR updates our current blob_storage to just use the new SDK. If approved, I'll then add an additional queue_storage.

There's some pretty big changes in the the internal code for this PR due to:

  1. Forced changes when upgrading azure.storage.blob 10 major versions, and
  2. Trying to make the code more or less match .NET's BlobsStorage and Node's BlobsStorage

Testing

All tests pass when running Storage Emulator and setting this to True so the tests run.

image

The warning are all of this nature:

image
image

...but per @axelsrz, that's expected for our SDK and Python 3.8.

Backwards Compatibility Testing

I also tested by:

  1. Downloading State Management sample and converting to use original blob_storage
  2. Conversed with bot
  3. Removed botbuilder.azure from sample, then added new blob_storage.py and ensured it used that
  4. Started a conversation with the bot as the same user
  5. Bot was able to retrieve previous conversation data from older blob_storage

image

Conversation and UserState data still look the same:

image

I'm not sure why e_tag gets quotes placed around it. I don't do any encoding, so I think it's something done by azure.storage.blob. I sort of mitigate it here

Note

This library will eventually benefit from investigating some of the Blob Storage options, but I won't have the bandwidth to land this in R12.

@mdrichardson mdrichardson marked this pull request as ready for review February 11, 2021 00:26
@mdrichardson mdrichardson requested review from tracyboehrer and axelsrz and removed request for tracyboehrer February 11, 2021 00:26
if changes is None:
raise Exception("Changes are required when writing")
if not changes:
return

self.client.create_container(self.settings.container_name)
self.client.set_container_acl(
self.settings.container_name, public_access=PublicAccess.Container
Copy link
Contributor Author

@mdrichardson mdrichardson Feb 11, 2021

Choose a reason for hiding this comment

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

I didn't see anything regarding public access referenced in .NET or JS and tests all passed fine without it. I can add it back, if necessary.

Comment on lines +159 to +164
if e_tag:
await blob_reference.upload_blob(
item_str, match_condition=MatchConditions.IfNotModified, etag=e_tag
)
except Exception as error:
raise error
else:
await blob_reference.upload_blob(item_str, overwrite=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Match conditions and eTags changed quite a bit and I'm not 100% confident that I implemented this correctly. I tried some other approaches and this was the one that made all of our tests pass. Please review this part closely.

@mdrichardson mdrichardson marked this pull request as draft February 11, 2021 19:15
@mdrichardson
Copy link
Contributor Author

mdrichardson commented Feb 11, 2021

Converting back to draft. Looks like the azure-storage-blob package adds some quotes to some of the blob references, which breaks backwards-compat. I partially handled this here, but am fixing elsewhere.

image

Edit: Fixed. I was trying to encode the blob names ('emulator/users/d4c5e82a-16f9-4709-89d6-b0b4cc0255e2') like the other SDKs do, but calling jsonpickler.encode() on it results in extra quotes being added, like: ("'emulator/users/d4c5e82a-16f9-4709-89d6-b0b4cc0255e2'").

@mdrichardson mdrichardson marked this pull request as ready for review February 11, 2021 19:38
@axelsrz axelsrz merged commit 51a6bde into microsoft:main Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants