BeanieODM / beanie

Asynchronous Python ODM for MongoDB
http://beanie-odm.dev/
Apache License 2.0
2k stars 211 forks source link

ElemMatch should support nested objects #498

Closed mikeckennedy closed 1 year ago

mikeckennedy commented 1 year ago

Hi @roman-right sorry if issue isn't the right format for this. Feel free to close it if you think it doesn't belong...

ElemMatch currently supports matching on a flat list, such as:

ElemMatch(Sample.results, [80, 85])
# becomes
# {"results": {"$elemMatch": [80, 85]}}

But for work I'm doing, we have a list of embedded documents which is one of the primary reasons to use elemMatch.

class Release(pydantic.BaseModel):
    major_ver: int
    minor_ver: int
    build_ver: int

class Package(beanie.Document):
    releases: list[Release] = []

Then I want all packages with a certain version, say 7.1.0. I can do that like this now:

count = await Package.find(
    ElemMatch(
        Package.releases,
        {"major_ver": 7, "minor_ver": 1, "build_ver": 0}
    )
).count()

But I think ElemMatch should support some way to do this without falling back to raw dictionaries (so things like "find usages" and refactoring work):

count = await Package.find(
    ElemMatch(
        Package.releases,
        (Release.major_ver==7, Release.minor_ver==1, Release.build_ver==0)
    )
).count()

# raw mongo query:
# {'releases': {'$elemMatch': {'major_ver': 7, 'minor_ver': 1, 'build_ver': 0}}}

You could even keep it with dictionaries such as:

count = await Package.find(
    ElemMatch(
        Package.releases,
        {Release.major_ver: 7, Release.minor_ver: 1, Release.build_ver: 0}
    )
).count()
roman-right commented 1 year ago

Hi Michael, Sorry for the delay. On it

roman-right commented 1 year ago

The first problem I think I'm not able to solve nicely is that Release is a BaseModel class, which is not patched to call arguments on the class (non-instance) level and the magic methods (like eq) for it are also not patched. I see two ways to solve this:

I think the first option is better, as the syntax is more consistent there.

@mikeckennedy What do you think?

mikeckennedy commented 1 year ago

Hey @roman-right Thanks for thinking about this one. Both are interesting solutions.

As for having an EmbeddedDoc base class, I think I’d be fine with that. MongoEngine has something similar. On one hand, I think it would lead to a more consistent API. But I don’t know how much trouble/work it’ll cause for the larger codebase.

The second way looks good to me too. Although, at that point, it’s not adding much value beyond the dictionary variation I put in the description of issue.

So I guess my thoughts are if you’re 100% behind beanie.Embedded, that’s cool. But otherwise, maybe just close and tell me to use the dictionary version I’m already using. ;)