MOTOR-689: Add async wrapper for pymongo.encryption.ClientEncryption #103

guanlinzhou · 2021-03-25T22:45:48Z

Add AsyncIOMotorClientEncryption class as a wrapper for Explicit Client Encryption

oops branch was named wrong

motor/motor_asyncio.py

motor/core.py

motor/motor_asyncio.py

ShaneHarvey

Could you add some tests for the new class? It doesn't have to be as extensive as PyMongo's tests but we should at least see that create_data_key, encrypt, and decrypt work as expected.

motor/core.py

guanlinzhou · 2021-03-25T23:50:58Z

Could you add some tests for the new class? It doesn't have to be as extensive as PyMongo's tests but we should at least see that create_data_key, encrypt, and decrypt work as expected.

I think @prashantmital mentioned that we would discuss this tomorrow so I'm leaving this alone for today

prashantmital

Great work so far!

For testing the gold standard would be to port the tests from https://github.com/mongodb/mongo-python-driver/blob/0752280adaa69b3cd80fabdf96aac31a26af826f/test/test_encryption.py#L310 in Motor. There don't seem to be too many tests so I think we can really just port over all of them, but my mind isn't made up in this regard.

guanlinzhou · 2021-03-26T19:45:54Z

Ok I'll work on porting those tests over

prashantmital

We need to update the Evergreen matrix to be able to run CSFLE tests. We can discuss offline how to do this.

Edit: on second thought, I think we also need to add an optional extra for encryption to the motor setup.py like we do for pymongo: https://github.com/mongodb/mongo-python-driver/blob/master/setup.py#L285

motor/core.py

motor/docstrings.py

test/asyncio_tests/test_asyncio_encryption.py

prashantmital

Great job getting this far, but I think we might have over-engineered this. I recommend rolling back the changes you made to mimic PyMongo's CSFLE testing setup and using tox to do the same thing instead.

Tox supports installation of 'extras' - https://tox.readthedocs.io/en/latest/config.html#conf-extras
This means we can simply update the [testenv] section to include:

[testenv]
extras =
    encryption

and this will essentially install Motor as pip install -e .[encryption] which is exactly what we need. AFAICT there is no way to conditionally do this based on whether an environment variable is defined so it will be an all-or-nothing kind of deal (unless we do something funky like dynamically generating tox.ini which I don't think is a good idea).

setup.py

motor/docstrings.py

.evergreen/config.yml

.evergreen/run-tox.sh

guanlinzhou · 2021-03-30T18:51:10Z

Great job getting this far, but I think we might have over-engineered this. I recommend rolling back the changes you made to mimic PyMongo's CSFLE testing setup and using tox to do the same thing instead.

Tox supports installation of 'extras' - https://tox.readthedocs.io/en/latest/config.html#conf-extras
This means we can simply update the [testenv] section to include:
[testenv]
extras =
    encryption
and this will essentially install Motor as pip install -e .[encryption] which is exactly what we need. AFAICT there is no way to conditionally do this based on whether an environment variable is defined so it will be an all-or-nothing kind of deal (unless we do something funky like dynamically generating tox.ini which I don't think is a good idea).

Thanks! This is a good point - I was curious about how much I should be actually trying to granularly test this file instead of just running every setup with these tests.

prashantmital

LGTM other than the one minor change to .travis.yml

.travis.yml

prashantmital

Actually, can you also update the CSFLE doc to properly state how to get started with motor + encryption using the extra intall syntax -
https://github.com/mongodb/motor/blob/master/doc/examples/encryption.rst#dependencies

Something like

To get started using client-side field level encryption in your project, you will need to install the pymongocrypt library as well as the driver itself. Install both the driver and a compatible version of pymongocrypt like this:

$ python -m pip install 'motor[encryption]'

ShaneHarvey

Great work!

.travis.yml

motor/core.py

motor/docstrings.py

ShaneHarvey · 2021-03-30T20:53:44Z

setup.py

@@ -33,6 +33,10 @@

 tests_require = ['mockupdb>=1.4.0']

+extras_require = {
+    'encryption': ['pymongo[encryption]>=3.11,<4'],


Do we need to add the >=3.11,<4 here? I would hope that the version reqs are added transitively from pymongo in install_requires.

@prashantmital thoughts on this?

I think extras are applied on top of (i.e. after) install_requiries so we would probably need this. I'll run a quick test and get back to you.

With a setup.py that looks like:

setup( name='mypackage', version='0.1.0', packages=find_packages(), ext_modules=get_extension_modules(), install_requires=['pymongo==3.10'], extras_require={'foo': ['pymongo[srv]']}, )

installing the package has a rather strange output log:

➜ pip install -e '.[foo]' Obtaining file:///Users/pmital/Developer/playgrounds/python-packaging Collecting pymongo==3.10 Downloading pymongo-3.10.0-cp37-cp37m-macosx_10_9_x86_64.whl (350 kB) |████████████████████████████████| 350 kB 1.6 MB/s Collecting pymongo[srv] Using cached pymongo-3.11.3-cp37-cp37m-macosx_10_6_intel.whl (414 kB) Downloading pymongo-3.11.2-cp37-cp37m-macosx_10_6_intel.whl (414 kB) |████████████████████████████████| 414 kB 1.7 MB/s Downloading pymongo-3.11.1-cp37-cp37m-macosx_10_6_intel.whl (414 kB) |████████████████████████████████| 414 kB 2.6 MB/s Downloading pymongo-3.11.0-cp37-cp37m-macosx_10_9_x86_64.whl (378 kB) |████████████████████████████████| 378 kB 3.2 MB/s Downloading pymongo-3.10.1-cp37-cp37m-macosx_10_9_x86_64.whl (350 kB) |████████████████████████████████| 350 kB 3.8 MB/s Collecting dnspython<2.0.0,>=1.16.0 Using cached dnspython-1.16.0-py2.py3-none-any.whl (188 kB) Installing collected packages: pymongo, dnspython, mypackage Attempting uninstall: mypackage Found existing installation: mypackage 0.1.0 Uninstalling mypackage-0.1.0: Successfully uninstalled mypackage-0.1.0 Running setup.py develop for mypackage Successfully installed dnspython-1.16.0 mypackage pymongo-3.10.0 ➜ pip list Package Version Location ---------- ------- ---------------------------------------------------- Cython 0.29.22 dnspython 1.16.0 mypackage 0.1.0 /Users/pmital/Developer/playgrounds/python-packaging pip 21.0.1 pymongo 3.10.0 setuptools 47.1.0 wheel 0.36.2

for some reason, it downloads all the versions between 3.10 and latest but ends up only installing the one in install_requires. So @ShaneHarvey does seem to be correct in that pip honors the install_requires version. That being said, it might result in longer install times if there are a lot versions to download.

Specifying the version in extras_require eliminates the extra downloads:

➜ pip --no-cache-dir install -e '.[foo]' Obtaining file:///Users/pmital/Developer/playgrounds/python-packaging Collecting pymongo==3.10 Downloading pymongo-3.10.0-cp37-cp37m-macosx_10_9_x86_64.whl (350 kB) |████████████████████████████████| 350 kB 2.1 MB/s Requirement already satisfied: dnspython<2.0.0,>=1.16.0 in /Users/pmital/.pyenv/versions/3.7.10/envs/packaging/lib/python3.7/site-packages (from pymongo==3.10->mypackage==0.1.0) (1.16.0) Installing collected packages: pymongo, mypackage Running setup.py develop for mypackage Successfully installed mypackage pymongo-3.10.0

@ShaneHarvey how would you like to proceed?

Oh well. Let's keep >=3.11,<4. We'll just need to ensure those two reqs never drift apart.

Ah I see your concern. Let's single-source the PyMongo dependency. Maybe something like:

pymongo_ver = ">=3.11,<4" install_requires = ["pymongo" + pymongo_ver] extras_require = {'encryption': ["pymongo" + "[encryption]" + pymongo_ver]}

Thoughts @ShaneHarvey ?

Yes let's do your single-source option.

@guanlinzhou can you implement this?

test/asyncio_tests/test_asyncio_encryption.py

prashantmital · 2021-03-31T17:38:24Z

setup.py

@@ -33,6 +33,10 @@

 tests_require = ['mockupdb>=1.4.0']

+extras_require = {
+    'encryption': ['pymongo[encryption]>=3.11,<4'],


Ah I see your concern. Let's single-source the PyMongo dependency. Maybe something like:

pymongo_ver = ">=3.11,<4" install_requires = ["pymongo" + pymongo_ver] extras_require = {'encryption': ["pymongo" + "[encryption]" + pymongo_ver]}

Thoughts @ShaneHarvey ?

prashantmital

LGTM after you've address the last comment.

prashantmital · 2021-03-31T19:52:36Z

setup.py

+
+install_requires = ["pymongo" + pymongo_ver]
+
+extras_require = {'encryption': ["pymongo" + "[encryption]" + pymongo_ver]}


I suppose this can just be
["pymongo[encryption]" + pymongo_ver]

ShaneHarvey

Great work!

added wrapper for Asyncio explicit encryption

ee7b490

guanlinzhou requested review from ShaneHarvey and prashantmital March 25, 2021 22:45

ShaneHarvey reviewed Mar 25, 2021

View reviewed changes

motor/motor_asyncio.py Outdated Show resolved Hide resolved

motor/core.py Show resolved Hide resolved

motor/motor_asyncio.py Show resolved Hide resolved

guanlinzhou added 4 commits March 25, 2021 16:22

add docs

e252b38

syntax mistake

8fed6c1

add docstring

1ef5c2a

add tornado support

3e11eb7

ShaneHarvey reviewed Mar 25, 2021

View reviewed changes

motor/core.py Outdated Show resolved Hide resolved

motor/core.py Outdated Show resolved Hide resolved

shorten docstring

dea8025

prashantmital suggested changes Mar 26, 2021

View reviewed changes

guanlinzhou added 10 commits March 26, 2021 14:25

expose close api

9a60728

checkout

75d2fdc

checkout

9a58140

get tests working

a3575c0

remove .eggs

bf05409

newline

13f4cdb

eof

4254aae

eof

efddbe8

cleanup calls back

d3a73ab

cleanup calls back

0c0e70b

guanlinzhou requested review from ShaneHarvey and prashantmital March 29, 2021 21:31

updated travis config for installing pymongocrypt

c6c7edd

prashantmital suggested changes Mar 29, 2021

View reviewed changes

guanlinzhou added 3 commits March 29, 2021 15:14

add setup

7f9b84a

augment evergreen testing

aea3442

fix config

0b2ca3b

add testing lines

e0337ca

prashantmital suggested changes Mar 30, 2021

View reviewed changes

setup.py Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

motor/docstrings.py Outdated Show resolved Hide resolved

.evergreen/config.yml Outdated Show resolved Hide resolved

.evergreen/run-tox.sh Outdated Show resolved Hide resolved

guanlinzhou added 4 commits March 30, 2021 11:20

install through tox

d569bd0

restore dsi files

ef043bb

replace config.yml

e093948

remove extra newline

64f68d7

guanlinzhou requested review from ShaneHarvey and prashantmital March 30, 2021 18:51

prashantmital approved these changes Mar 30, 2021

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

prashantmital suggested changes Mar 30, 2021

View reviewed changes

ShaneHarvey requested changes Mar 30, 2021

View reviewed changes

nits

a9b976b

guanlinzhou requested review from ShaneHarvey and prashantmital March 30, 2021 21:17

guanlinzhou added 3 commits March 30, 2021 14:19

Merge branch 'master' of github.com:mongodb/motor into PYTHON-689

7011887

update installation info

c4e81a3

link to pymongocrypt

9a03574

prashantmital reviewed Mar 31, 2021

View reviewed changes

single source python dependency

171becc

guanlinzhou requested a review from prashantmital March 31, 2021 19:26

guanlinzhou added 2 commits March 31, 2021 12:42

extra

26cf82f

revert changes

81f87cd

prashantmital approved these changes Mar 31, 2021

View reviewed changes

guanlinzhou added 3 commits March 31, 2021 13:06

fix failure with setUp

3f5f880

fix failure with setUp

87e711f

update older pip version

8f6026e

ShaneHarvey approved these changes Mar 31, 2021

View reviewed changes

guanlinzhou merged commit 1b5a21a into mongodb:master Mar 31, 2021

guanlinzhou deleted the PYTHON-689 branch March 31, 2021 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MOTOR-689: Add async wrapper for pymongo.encryption.ClientEncryption #103

MOTOR-689: Add async wrapper for pymongo.encryption.ClientEncryption #103

guanlinzhou commented Mar 25, 2021

ShaneHarvey left a comment

guanlinzhou commented Mar 25, 2021

prashantmital left a comment

guanlinzhou commented Mar 26, 2021

prashantmital left a comment •

edited

prashantmital left a comment

guanlinzhou commented Mar 30, 2021

prashantmital left a comment

prashantmital left a comment

ShaneHarvey left a comment

ShaneHarvey Mar 30, 2021

guanlinzhou Mar 30, 2021

prashantmital Mar 30, 2021

prashantmital Mar 30, 2021 •

edited

ShaneHarvey Mar 30, 2021

prashantmital Mar 31, 2021

ShaneHarvey Mar 31, 2021

prashantmital Mar 31, 2021

prashantmital Mar 31, 2021

prashantmital left a comment

prashantmital Mar 31, 2021

ShaneHarvey left a comment


		install_requires = ["pymongo" + pymongo_ver]

		extras_require = {'encryption': ["pymongo" + "[encryption]" + pymongo_ver]}

MOTOR-689: Add async wrapper for pymongo.encryption.ClientEncryption #103

MOTOR-689: Add async wrapper for pymongo.encryption.ClientEncryption #103

Conversation

guanlinzhou commented Mar 25, 2021

ShaneHarvey left a comment

Choose a reason for hiding this comment

guanlinzhou commented Mar 25, 2021

prashantmital left a comment

Choose a reason for hiding this comment

guanlinzhou commented Mar 26, 2021

prashantmital left a comment • edited

Choose a reason for hiding this comment

prashantmital left a comment

Choose a reason for hiding this comment

guanlinzhou commented Mar 30, 2021

prashantmital left a comment

Choose a reason for hiding this comment

prashantmital left a comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashantmital Mar 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashantmital left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

prashantmital left a comment •

edited

prashantmital Mar 30, 2021 •

edited