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] id is unknown type-wise in find statements. #1098

Open
ItsDrike opened this issue Dec 27, 2024 · 4 comments
Open

[BUG] id is unknown type-wise in find statements. #1098

ItsDrike opened this issue Dec 27, 2024 · 4 comments

Comments

@ItsDrike
Copy link

ItsDrike commented Dec 27, 2024

Describe the bug
The docs suggest that a find statement like:

houses = await House.find(
    House.door.id == PydanticObjectId("DOOR_ID_HERE")
).to_list()

Should work regardless of whether fetch_links is True or False. And yes, on runtime, it does.

However, on typing-time the id attribute is not recognized.

To Reproduce
Using the pyright type checker:

from beanie import Document, Indexed, Link

# This is a fix for #820, but it wouldn't work even without it, when the type would be purely Link[T], 
# rather than a union of T | Link[T], it's just that there'd be other typing issues too then.

if TYPE_CHECKING:
    type BeanieLink[T] = T | Link[T]
else:
    BeanieLink = Link

class User(Document):
    username: Annotated[str, Indexed(unique=True)]

class Category(Document):
    user: Annotated[list[BeanieLink[User]], Indexed]
    name: str

async def main():
    ...  # code to init db connection
    
    user =  User(username="ItsDrike")
    user = await user.create()
    
    category = Category(user=user, name="A")
    category = await category.create()
    
    my_cat = await Category.find(Category.user.id == user.id).to_list() 
    # 1. pyright: Cannot access attribute "id" for class "Link[User]"
    #  Attribute "id" is unknown [reportAttributeAccessIssue]

Expected behavior
The id attribute should have a proper type definition.

@ItsDrike
Copy link
Author

ItsDrike commented Dec 27, 2024

Looking a bit more into this, the problem is that accessing the field on the class type itself (not it's instance) gives you back an ExpressionField, rather than the actual type.

This behavior is nice, but there should be some typing support for this to avoid issues like this one. You might be able to resolve this with descriptors, but I'm not entirely sure how complex would doing so be.

The workaround that I'm currently using is this:

def expr(ex: Any) -> ExpressionField:
    """Convert type to ExpressionField.

    When accessing Beanie documents, there is a difference between attempting to access
    an attribute of the document class type itself and an attribute of the document instance.

    Unfortunately, Beanie doesn't provide a way to distinguish between these two cases type-wise
    and misinterprets the former as an attempt to access an attribute of the document instance,
    which results in type checker using an incorrect type, often leading to type errors.

    This function is essentially just a cast to ExpressionField, with a runtime isinstance check
    as a safeguard against accidental misuse.
    """
    if not isinstance(ex, ExpressionField):
        raise TypeError(f"Expected ExpressionField, got {type(ex).__name__}")
    return ex
    
# Using it like:
events = await (Event.find(expr(Event.attendees).id == user_id, fetch_links=True)).to_list()

Obviously though, this isn't ideal and as far as workarounds go, this is a very annoying one.

Note that I didn't just use cast since some type checkers also provide an incorrect cast check when the cast type isn't a supertype nor a subtype of the original. E.g.: cast(ExpressionField, Event.attendees).id == user_id, leading to:

1. basedpyright: Conversion of type `list[BeanieLink[User]]` to type `ExpressionField` may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to `object` first. [reportInvalidCast]

Also, a cast would require specifying the type each time, whereas this little helper function is a bit shorter & nicer, which is important considering how many times it needs to be used in Beanie code.

@ItsDrike ItsDrike changed the title [BUG] Link.id is unknown type-wise [BUG] id is unknown type-wise in find statements. Dec 27, 2024
@staticxterm
Copy link
Contributor

Hi, thank you for opening an issue and for looking more into this.

There's multiple open issues on the same or similar subject. We are (or at least I'm) aware of it.
Unfortunately, this one issue is difficult to solve. The types workaround is basically just that, a workaround, or a 'hack' for another 'hack'.

This one 'hack' is a feature in Beanie (I'll use your example) which you rightly point out:

 my_cat = await Category.find(Category.user.id == user.id).to_list() 

(the arguments / syntax in .find())
Upon calling init_beanie() for a Document model, Beanie transforms all class attributes to instances of an ExpressionField() in order to enable the above search query syntax functionality in Document.find() and similar. Some report that this transform now also breaks Pydantic model validation use cases.

For your other example (and modifying it a little for simplicity/to achieve the original code):

events = await Event.find(Event.attendees.id == user_id, fetch_links=True).to_list()

(the Event.attendees.id, attendees and id types not being recognized/correct)
The Link/BackLink functionality was also not first developed with satisfying the type checkers in mind. One way to get around it would be to support using the Annotated syntax. My worry here is that (I guess) you would still need to explicitly tell the type checker which exact type it is when required (for instance, in the same way that we tell it that some value is not None when it can be).

With all that said, I simply don't believe there is an easy fix for all this, unless we drastically change the interfaces / functionality here.
We may get away with 'hacking the type system' / fooling the type checkers a little, but I have a bad feeling that this may just end piling up, one 'type hack' on top of another...

@ItsDrike
Copy link
Author

ItsDrike commented Dec 28, 2024

With all that said, I simply don't believe there is an easy fix for all this, unless we drastically change the interfaces / functionality here.
We may get away with 'hacking the type system' / fooling the type checkers a little, but I have a bad feeling that this may just end piling up, one 'type hack' on top of another...

Yeah, I do get that, introducing typing support for something that was never written with it in mind can be a challenge, especially for something as dynamic in nature as an ODM library. But it would be a shame to force users that do like typing to having to use alternative libs just to get proper support.

It's hard to say whether a drastic change like this would be something even worth considering without knowing how the Beanie development is currently structured and whether the people involved are ready to do that and have enough experience with typed python to even be able to do that. Not to mention that it would be at a cost of likely breaking a lot of backwards compatibility just to add typing support.

From my, admittedly short, use of Beanie, the typing support isn't terrible, but definitely lacks a lot things, which indeed probably wouldn't be an easy fix without a lot of rewrites.

@staticxterm
Copy link
Contributor

But it would be a shame to force users that do like typing to having to use alternative libs just to get proper support.

I am in the same boat as you, more or less, that's why I have opened #1050.
All we can do is drive an improvement and discuss proposal of an API for some future major Beanie version that would be 100% type correct. Or try our best to fix what we can fix with the types in v1.
Beanie has transitioned to a team-based approach so I would say that we are open to new suggestions/improvements. All contributions are welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants