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.

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

Cosmos partitioned - Parity with C# and Node | Also storage base tests #459

Merged
merged 15 commits into from
Dec 6, 2019

Conversation

mdrichardson
Copy link
Contributor

@mdrichardson mdrichardson commented Nov 27, 2019

This is kind of two PRs in one.

1

Fixes #327

Add cosmosdb_paritioned_storage for parity with both C# and Node

2

Parity with: microsoft/botbuilder-js#1371

The Dotnet repo has a StorageBaseTests class that all of the Storage classes borrow from to reduce duplicate code and ensure that the Storage classes all pass a base set of tests.

Python SDK doesn't have this. Instead, there's a ton of duplicate code...until now!

Specific Changes

This doesn't remove any tests from any of the storage providers. Instead, it replaces them with equivalent tests and in some cases, adds a few for tests that probably should have been conducted, but weren't.

  • Adds storage_base_tests class to botbuilder-testing for parity w/ Dotnet, which Memory, Blob, Cosmos, and CosmosPartitioned storage providers now implement in addition to their own tests (mostly constructor argument checks)
    • Note: In C#/Node, this falls under botbuilder-core, but I couldn't seem to make that work in Python without too many additional __init__s. Maybe somebody who isn't Python-dumb can help?
  • Structure/layout of Blob, Cosmos, CosmosPartitioned now matches more closely
  • Some NOOP/linting fixes and general code cleanup

Changes Worth a Closer Look

  • All tests have a testRan check. Is this necessary? I'd seen it used in some dotnet tests and thought it might be worthwhile since there aren't assert checks in most of the individual provider's tests.
  • MemoryStorage, BlobStorage, and CosmosDbStorage all had areas where they sort of assumed that objects being written were StoreItems. This shouldn't be the case, so I changed from item.e_tag to if hasattr(item, "e_tag") <use it> else item.get("e_tag") in a LOT of places.
  • I threw exceptions for some null/None checks to match other SDKs in several places
  • MemoryStorage now deepcopy()s on write, which I think is related to this PR
  • In BlobStorage, I removed this line, item["id"] = blob.name, in read(), because it would overwrite an item that had that id set and didn't seem to make any tests fail when changing it
  • This is just generally a fairly big change and has implications on testing validity of future PRs. Worth a pretty close look just for that reason.

Testing

CoreBot works w/ MemoryStorage:

image

CoreBot works w/ Blob:

image

CoreBot works w/ Cosmos:

image

CoreBot works w/ CosmosPartitioned:

image

@mdrichardson mdrichardson changed the title Cosmos partitioned - Parity with C# (merged) and Node (unmerged) Cosmos partitioned - Parity with C# and Node | Also storage base tests Nov 27, 2019
@mdrichardson mdrichardson requested a review from axelsrz November 27, 2019 22:37
@EricDahlvang
Copy link
Member

We should consider the use of CosmosDbKeyEscape.sanitize_key and whether or not 255 is an actual requirement by the libraries used for this.

@axelsrz axelsrz merged commit e189211 into master Dec 6, 2019
@axelsrz axelsrz deleted the cosmosPartitioned branch December 6, 2019 17:44
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.

Port new CosmosDB Partitioned Storage from C#
3 participants