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

Always coerce id fields when performing mongotize even if query_objectid_as_string is on. #1347

Merged
merged 8 commits into from
Jan 26, 2020

Conversation

ehiggs
Copy link
Contributor

@ehiggs ehiggs commented Jan 16, 2020

Addresses #1345

The Test Widget manufacturer now uses Mongo, so the SKUs resemble ObjectIds. Added a failing test to show why this is a problem. Fix the problem.

@ehiggs
Copy link
Contributor Author

ehiggs commented Jan 16, 2020

I understand performance in this area is important, so here are some microbenchmarks to try this out.

Given the following setup:

DOMAIN = <copy pasted from debug session when running unit tests>
import eve.io.mongo.mongo as eimm
eimm.config.DOMAIN = DOMAIN
eimm.versioned_id_field = lambda x: 'id_field_versioned'
resource = 'contacts'                                                                                                                                
source = {'_id': '5e20d925927678c6959b3d36', 'rows.sku': {'$in': ['2baf524e08e3c9daa7caab5f']}}                                                      

Old code, default coercion:

In [10]: %timeit mongo._mongotize(source, resource)                                                                                                          
9.12 µs ± 18.7 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Old code, no coercion:

In [13]: eimm.config.DOMAIN["contacts"]["query_objectid_as_string"] = True            
                                                                       
In [14]: %timeit mongo._mongotize(source, resource)                                                                                                          
7.74 µs ± 32.5 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

New code no coercion (restarted ipython):

In [9]: mongo._mongotize(source, resource)                                                                                                                   
Out[9]: 
{'_id': ObjectId('5e20d925927678c6959b3d36'),
 'rows.sku': {'$in': [ObjectId('2baf524e08e3c9daa7caab5f')]}}

In [10]: %timeit mongo._mongotize(source, resource)                                                                                                          
8.93 µs ± 21.3 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

New code with less coercion

In [12]: eimm.config.DOMAIN["contacts"]["query_objectid_as_string"] = True                                                                                   

In [13]: %timeit mongo._mongotize(source, resource)                                                                                                          
7.53 µs ± 79.2 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

@nicolaiarocci nicolaiarocci added this to the 1.0.1 milestone Jan 17, 2020
@nicolaiarocci
Copy link
Member

Very nice, thank you.

us to use non id_field fields as ObjectId if they are defined that way
in the schema.
@ehiggs
Copy link
Contributor Author

ehiggs commented Jan 17, 2020

Oops, I wasn't actually calling get_schema_type 😞
Added a commit that fixed this and also fixed a test that no longer demonstrated what it was supposed to (test_get_lookup_field_as_string, and I think it wasn't correct anyway since it didn't set the config variable it was claiming to be testing).

@ehiggs
Copy link
Contributor Author

ehiggs commented Jan 17, 2020

Trying to reproduce the travis error:

➜  eve git:(always-coerce-id-fields) ✗ .tox/py27/bin/py.test -k TestGet
==================================================================== test session starts ====================================================================
platform darwin -- Python 2.7.16, pytest-4.6.9, py-1.8.1, pluggy-0.13.1
rootdir: /Users/ehiggs/src/eve, inifile: pytest.ini, testpaths: eve/tests
collected 803 items / 702 deselected / 101 selected                                                                                                         

eve/tests/methods/get.py .....................................................................................................

======================================================== 101 passed, 702 deselected in 32.48 seconds ========================================================
➜  eve git:(always-coerce-id-fields) ✗ git push ehiggs
Everything up-to-date
➜  eve git:(always-coerce-id-fields) ✗ .tox/py27/bin/py.test -k TestGet
==================================================================== test session starts ====================================================================
platform darwin -- Python 2.7.16, pytest-4.6.9, py-1.8.1, pluggy-0.13.1
rootdir: /Users/ehiggs/src/eve, inifile: pytest.ini, testpaths: eve/tests
collected 803 items / 702 deselected / 101 selected                                                                                                         

eve/tests/methods/get.py .....................................................................................................

======================================================== 101 passed, 702 deselected in 32.56 seconds ========================================================
➜  eve git:(always-coerce-id-fields) ✗ .tox/py27/bin/py.test -k TestGet
==================================================================== test session starts ====================================================================
platform darwin -- Python 2.7.16, pytest-4.6.9, py-1.8.1, pluggy-0.13.1
rootdir: /Users/ehiggs/src/eve, inifile: pytest.ini, testpaths: eve/tests
collected 803 items / 702 deselected / 101 selected                                                                                                         

eve/tests/methods/get.py ........................................................................F............................

========================================================================= FAILURES ==========================================================================
______________________________________________ TestGet.test_get_where_mongo_objectid_as_string_but_field_is_id ______________________________________________

self = <eve.tests.methods.get.TestGet testMethod=test_get_where_mongo_objectid_as_string_but_field_is_id>

    def test_get_where_mongo_objectid_as_string_but_field_is_id(self):
        skus = self.to_list_string([item["sku"] for item in self.item_rows])
        where_in = '{"tid": "%s", "rows.sku": { "$in": %s} }' % (self.item_tid, skus)
        response, status = self.get(self.known_resource, "?where=%s" % where_in)
        self.assert200(status)
        resource = response["_items"]
        self.assertEqual(len(resource), 0)
    
        self.app.config["DOMAIN"]["contacts"]["query_objectid_as_string"] = True
        response, status = self.get(self.known_resource, "?where=%s" % where_in)
        self.assert200(status)
        resource = response["_items"]
>       self.assertEqual(len(resource), 1)
E       AssertionError: 0 != 1

eve/tests/methods/get.py:255: AssertionError
================================================================== short test summary info ==================================================================
FAILED eve/tests/methods/get.py::TestGet::test_get_where_mongo_objectid_as_string_but_field_is_id - AssertionError: 0 != 1
=================================================== 1 failed, 100 passed, 702 deselected in 32.01 seconds ===================================================

feelsbadman.png

@ehiggs
Copy link
Contributor Author

ehiggs commented Jan 20, 2020

I found that the tests were inconsistent because there were 0 - 5 rows generated. The new test requires 1 row with an sku to be generated so I updated the row generation to always make at least one.

nested document and needs the fact that tid is an object id so the state
should be pushed down into the recursion.
@ehiggs
Copy link
Contributor Author

ehiggs commented Jan 21, 2020

Fixed a bug where a get request like where?{"tid": { "$in": ["abc..."]}} would fail because in the recursion "$in" isn't in the schema and would prevent the [...] from being converted to ObjectIds.

nicolaiarocci added a commit that referenced this pull request Jan 26, 2020
@nicolaiarocci nicolaiarocci merged commit 534371a into pyeve:master Jan 26, 2020
@nicolaiarocci
Copy link
Member

thanks!

@micaelmalta
Copy link

@ehiggs this breaks #1468

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.

3 participants