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

Bug: TestRabbitBroker's response type should be consistent between mock broker and real RabbitMQ broker #1437

Closed
maxalbert opened this issue May 9, 2024 · 2 comments · Fixed by #1452
Labels
bug Something isn't working Core Issues related to core FastStream functionality and affects to all brokers

Comments

@maxalbert
Copy link

Describe the bug

When performing an RPC call with a TestRabbitBroker where the subscriber returns a pydantic model, the response type is different depending on whether the test runs with a mock broker or a real RabbitMQ broker.

How to reproduce

Save the code below as example.py, then run pytest example.py. The test will pass.

import pytest
from faststream import FastStream
from faststream.rabbit import RabbitBroker, TestRabbitBroker
from pydantic import BaseModel

broker = RabbitBroker("amqp://guest:guest@localhost:5672/")
app = FastStream(broker)

class Message(BaseModel):
    content: str

@broker.subscriber("in-queue")
async def handle_msg(msg: Message) -> Message:
    return Message(content=f"Received message: {msg.content!r}")

@pytest.mark.asyncio
async def test_rpc():
    async with TestRabbitBroker(broker, with_real=False) as br:
        response = await br.publish(
            Message(content="Hello there!"),
            "in-queue",
            rpc=True,
        )

    assert response == Message(content="Received message: 'Hello there!'")

Now change the value of with_real to True and run it again. This time it raises an error because the response type of the RPC call is a plain dict:

E       assert {'content': "...ello there!'"} == Message(conte...ello there!'")
E
E         Full diff:
E         + {
E         - Message(content="Received message: 'Hello there!'")
E         ? ^^^^^^^^       ^                                  ^
E         +     'content': "Received message: 'Hello there!'",
E         ? ^^^^^       ^^^                                  ^
E         + }

Expected behavior

I would expect the return type of the RPC call to be the same for both with_real=False and with_real=True. Ideally, I'd like to be able to specify the pydantic model as the return type even if I'm using a real RabbitMQ broker (see case 2 in #1228). In the absence of this feature, it would be good if the TestRabbitBroker could also return a plain dict so that I don't have to add case distinctions in all my tests.

Observed behavior

For with_real=False, the return type is a proper pydantic model, whereas for with_real=True it is a plain dict.

Environment

$ faststream -v
Running FastStream 0.5.5 with CPython 3.12.3 on Darwin
@maxalbert maxalbert added the bug Something isn't working label May 9, 2024
@maxalbert
Copy link
Author

Quick addendum:

I just realised that the impact of this bug is worse than just requiring case distinctions in tests.

I had implemented some features in a test-driven way using TestRabbitBroker. All the tests pass, but the app itself (not just the tests) broke when I switched to the real RabbitMQ broker because in one of my message handlers I'm accessing an attribute response.status. This works with the mock broker because response is a pydantic model that has a status attribute. However, when using the real broker, response is suddenly a plain dictionary so I would need to access the field via response["status"] instead.

I would like to catch this issue early during development with the mock broker, not just after the fact when switching to the real one.

@Lancetnik Lancetnik added the Core Issues related to core FastStream functionality and affects to all brokers label May 16, 2024
@Lancetnik
Copy link
Collaborator

Well, it is a bug indeed, thank you for finding it. I suppose, we should return serializaed message from the TestBroker rpc instead of python objects the same way with the mock behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Core Issues related to core FastStream functionality and affects to all brokers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants