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

fix: assertion of DynamoType failing #7073

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

verysamuel
Copy link
Contributor

This is a complete test which reproduces the issue.

The reload function is sourced from this Stack Overflow response. Our tests currently rely on this behaviour. The use of this function contributes to the occurrence of this bug.

import importlib
import sys

import boto3
import pytest
from boto3.dynamodb.types import TypeSerializer
from moto import mock_dynamodb

serializer = TypeSerializer()


@pytest.fixture(autouse=True)
def reload(monkeypatch):
    importlib.reload(sys.modules["moto"])
    to_reload = [m for m in sys.modules if m.startswith("moto.")]
    for m in to_reload:
        importlib.reload(sys.modules[m])


@pytest.fixture
def setup_dynamo():
    with mock_dynamodb():
        dynamodb = boto3.client("dynamodb", region_name="eu-west-1")
        dynamodb.create_table(
            TableName="SomeTable",
            AttributeDefinitions=[
                {"AttributeName": "SomeKey", "AttributeType": "S"},
            ],
            KeySchema=[
                {"AttributeName": "SomeKey", "KeyType": "HASH"},
            ],
            BillingMode="PAY_PER_REQUEST",
        )
        yield dynamodb


@pytest.mark.parametrize(
    "value",
    [
        serializer.serialize(value)
        for value in [
            {"This test case passes": "A"},
            {"This test case fails": "B"},
        ]
    ],
)
def test_reproduce_dynamo_type_assertion_error(setup_dynamo, value):
    config_key = "SomeKeyValue"

    setup_dynamo.put_item(
        TableName="SomeTable",
        Item={"SomeKey": {"S": config_key}, "ConfigValue": serializer.serialize({})},
    )

    setup_dynamo.update_item(
        TableName="SomeTable",
        Key={
            "SomeKey": {"S": config_key},
        },
        UpdateExpression="SET SomeValue = :some_value",
        ExpressionAttributeValues={":some_value": value},
    )

    dynamo_item = setup_dynamo.get_item(
        TableName="SomeTable",
        Key={"SomeKey": {"S": config_key}},
    )["Item"]["SomeValue"]

    assert dynamo_item == value

Running the above code results in this assertion failing:

assert isinstance(value, DynamoType), "DDBTypedValue must be of DynamoType"

It seems something along the lines of this is going on: the reload function combined with the inconsistent imports means two different instances of DynamoType coexist and are in use.

Because the reload function is called between test invocations, this error only occurs when more than one test runs in a single pytest execution. So with the example above, running each case individually they'll both pass.

If appropriate, I can add the example above as a test here.

Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

That's odd - and must have been fun to debug!

This is definitely something that would need a test case though, just to prevent us from ever re-introducing the bug by accident.

Just out of interest - can it be reproduced by using the manual mocks?

m = mock_dynamodb().
m.start().
..
m.stop()
# reloading magic
m.start()
m.stop()

If so, that would mean the fix could be verified in a single test.

@verysamuel verysamuel force-pushed the fix-dynamodb-type-assertion branch from 681e1d5 to d6b0e16 Compare November 28, 2023 15:45
@verysamuel
Copy link
Contributor Author

@bblommers "fun to debug!" is certainly one way to put it 😅

I have added a self-contained test which reproduces the issue using the style you've suggested.


mock_dynamo = mock_dynamodb()
mock_dynamo.start()
dynamo_client = boto3.client("dynamodb")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our CI does not have a region configured, so we have to specify it here explicitly

ExpressionAttributeValues={
":some_value": {"M": {"Some key": {"S": "Some new value"}}}
},
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should stop the mock at the end:
mock_dynamo.stop()

Not a big problem if you only run the one test, but if you run multiple tests and one mock is still active, it wreaks havoc.

@verysamuel verysamuel requested a review from bblommers November 28, 2023 20:04
@bblommers
Copy link
Collaborator

Hmm.. Our CI still doesn't seem to like that.

All the tests after the new tests are now failing:

2023-11-28T21:23:54.2345124Z tests/test_dynamodb/test_dynamodb.py::test_dynamo_type_assertion_exception_not_raised_when_reloading_modules PASSED
2023-11-28T21:23:54.3234042Z tests/test_dynamodb/test_dynamodb_batch_get_item.py::test_batch_items_returns_all PASSED
2023-11-28T21:23:54.7353450Z 
2023-11-28T21:23:54.7402019Z ##[error]test_batch_items_throws_exception_when_requesting_100_items_for_single_table

botocore.errorfactory.ResourceInUseException: An error occurred (ResourceInUseException) when calling the CreateTable operation: Table already exists: users
2023-11-28T21:23:54.7414803Z tests/test_dynamodb/test_dynamodb_batch_get_item.py::test_batch_items_throws_exception_when_requesting_100_items_for_single_table FAILED

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