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: Injecting a custom Context field implementing __eq__ does not work #1806

Open
antoinehumbert opened this issue Sep 23, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@antoinehumbert
Copy link

antoinehumbert commented Sep 23, 2024

Describe the bug
Since https://github.com/airtai/faststream/pull/1667/files, when trying to use a custom Context field that implements __eq__ returning NotImplemented in some cases, the dependency is not injected in subscriber.

How to reproduce
Having following User class:

class User:
    def __init__(self, id):
        self.id = id

    def __eq__(self, other):
        if not isinstance(other, User):
            return NotImplemented
        return self.id == other.id

Having a middleware injecting a User object in the context:

from faststream import BaseMiddleware, context


class UserMiddleware(BaseMiddleware):
    async def on_consume(self, msg):
        context.set_local("user", User(1))
        return await super().on_consume(msg)

Having a subscriber with user from context as a dependency:

@broker.subscriber("authenticated-topic")
def handler(msg, user: User = Context("user")):
    ...

The user parameter is not injected leading to a pydantic validation error because field is missing.

The issue arises because this (

) if statement always evaluates to False.

Here is what happens when doing the comparison with EMPTY:

  • User.__eq__ is called, returning NotImplementError
  • So, the comparison User(1) != EMPTY evaluates to not(NotImplementedError) which is False, leading to not injecting user in kwargs.

I think that _EmptyPlaceHolder.__eq__ should return False instead of NotImplementedError when compared with an object which is not an _EmptyPlaceHolder object, because this kind of comparison is what would most commonly happen at

, and the fact is that any object that is not an _EmptyPlaceHolder instance should pass the test.

Additionaly, I think this comparison should be made with EMPTY as the lhs operand (i.e. EMPTY != ...), as this will ensure that _EmptyPlaceHolder.__eq__ is called first and will always return a result (True if the rhs is an _EmptyPlaceHolder object, False otherwise), avoiding potential side effects depending on implementation of __eq__ in the compared object

@antoinehumbert antoinehumbert added the bug Something isn't working label Sep 23, 2024
@Lancetnik
Copy link
Member

@antoinehumbert thank you for such detail Issue! Do you want to make a PR (I see you almost have a fix) or let me do it by myself?

@antoinehumbert
Copy link
Author

I'll provide a PR soon

@Lancetnik
Copy link
Member

Thank you a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants