michaelkryukov / mongomock_motor

Library for mocking AsyncIOMotorClient built on top of mongomock.
MIT License
102 stars 24 forks source link

`unique=True` being ignored #9

Closed pahrohfit closed 2 years ago

pahrohfit commented 2 years ago

The unique constraint on a field/index appears to be getting ignored

michaelkryukov commented 2 years ago

I run some tests, everything seems ok. Can you add concrete examples to the issue?

import pytest
from pymongo.errors import DuplicateKeyError
from mongomock_motor import AsyncMongoMockClient

@pytest.mark.anyio
async def test_unique_index():
    collection = AsyncMongoMockClient()['tests']['test']

    await collection.create_index("a", unique=True)

    await collection.insert_one({'a': 1})

    with pytest.raises(DuplicateKeyError):
        await collection.insert_one({'a': 1})

    assert len(await collection.find({}).to_list(None)) == 1

@pytest.mark.anyio
async def test_unique_index_with_duplicates():
    collection = AsyncMongoMockClient()['tests']['test']

    await collection.insert_one({'a': 1})
    await collection.insert_one({'a': 1})

    with pytest.raises(DuplicateKeyError):
        await collection.create_index("a", unique=True)

    assert len(await collection.find({}).to_list(None)) == 2
pahrohfit commented 2 years ago

When running API tests against a Sanic app under pytest, its getting ignored, and duplicate DB entries are created. When hitting the API directly curing curl, it behaves as is expected. Using umongo, motorasync, and mongomock-motor in pytest.

@instance.register
class User(Document):
    _id                 = fields.ObjectIdField(dump_only=True)
    email               = fields.EmailField(required=True, unique=True, null=False)
    password            = fields.StringField(required=True, validate=[validate.Length(max=512)], null=False)
    created_at          = fields.DateTimeField(null=False, load_default=lambda: dt.datetime.utcnow())
    roles               = fields.ListField(fields.ReferenceField('Role'))

    class Meta:
        strict = False

    def __repr__(self):
        return f'<User({self.email})>'

Sanic route:

class UserRegister(HTTPMethodView):

    async def post(self, request):
        try:
            req = request.json
        except Exception as e:
            return json({'status': "failure", 'message': 'No valid JSON in request'}, status=422)

        if not req:
            return json({'status': "failure", 'message': 'No registration details provided in request'}, status=400)

        try:
            new_user = await User(**req).commit()
            return json({"status": "success", "message": str(new_user.inserted_id)}, status=200)

        except ValidationError as err:
            if "'email': 'Field value must be unique.'" in repr(err):
                # We have an already registered user
                return json({'status': "failure", 'message': 'That email is already registered'}, status=400)
            else:
                return json({'status': "failure",
                             'message': f'Registration Validation Failure: {err.messages}'},
                             status=400)
        except Exception as err:
            return json({'status': "failure",
                         'message': f'Generic error encountered registering user: {repr(err)}'},
                         status=400)

Pytest:

    def test_sanic_auth_create_duplicate_user_api(self, logger, app: Sanic, test_user):
        _client = ReusableClient(app, host='127.0.0.1', port='8000')
        with _client:
            request, response = _client.post("/auth/register/", json=test_user)
            assert request.method.lower() == "post"
            assert response.status == 200
            assert response.json['status'] == 'success'

            request, response = _client.post("/auth/register/", json=test_user)
            assert request.method.lower() == "post"
            assert response.status == 400
            assert response.json['status'] == 'failure'
            assert response.json['message'] == 'That email is already registered'

I get a 200 response on the second every time, and can validate the DB is populated twice. If I run this w/o the mock on mongo, I get a 400 on the second call, as expected.

FAILED tests/test_auth_api.py::TestAuthAPI::test_sanic_auth_create_duplicate_user_api - assert 200 == 400

Now, if I modify the API endpoint, and add a await User.ensure_indexes() To the before the call to commit the user, I get an uncaught exception failure (summarized here):

>           assert response.json['message'] == 'That email is already registered'
E           assert 'Generic erro...bscriptable")' == 'That email i...dy registered'
E             - That email is already registered
E             + Generic error encountered registering user: TypeError("'NoneType' object is not subscriptable")

When I run this with more debugging, its coming back up to an unknown 'keyPattern' in umongo/frameworks/motor_asyncio.py, which only happens under mocking ... so something isn't getting raised correctly:

190         except DuplicateKeyError as exc:
191             # Sort value to make testing easier for compound indexes
192             keys = sorted(exc.details['keyPattern'].keys())
michaelkryukov commented 2 years ago

First of all, you have to call ensure_indexes in order to create indexes for document. No indexes will be created and no exceptions will be thrown otherwise. The issue with keyPattern here is that mongomock raises DuplicateKeyError without details, so umongo can't extract data about fields that caused duplication error.

I made a small patch to support umongo's usage of DuplicateKeyError in merge #10. Can you run your tests with patched version? If everything's ok, I can merge solution into the main and wait for https://github.com/mongomock/mongomock/issues/773 to close (then patch won't be needed).

pahrohfit commented 2 years ago

Yeap, that fixed the issue!