tortoise / tortoise-orm

Familiar asyncio ORM for python, built with relations in mind
https://tortoise.github.io
Apache License 2.0
4.38k stars 356 forks source link

Doubts about the correctness of the result of the count() method #1607

Closed PieceOfGood closed 1 month ago

PieceOfGood commented 1 month ago

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior, preferably a small code snippet.

class BaseModel(Model):
    class Meta:
        abstract = True

    id = fields.IntField(pk=True, index=True)

class User(BaseModel):

    class Meta:
        table = "users"

    username: str = fields.CharField(max_length=50, null=True, unique=True)

    offers: fields.ReverseRelation["Offer"]

    orders: fields.ReverseRelation["Order"]

class Offer(BaseModel):

    class Meta:
        table = "offers"

    name: str = fields.CharField(max_length=50, null=False)

    user: fields.ForeignKeyRelation[User] = fields.ForeignKeyField(
        "models.User", related_name="offers"
    )

class Order(BaseModel):

    class Meta:
        table = "orders"

    created: datetime = fields.DatetimeField(null=False)

    user: fields.ForeignKeyRelation["User"] = fields.ForeignKeyField(
        "models.User", related_name="orders"
    )
    """ Client """

    offer: fields.ForeignKeyRelation["Offer"] = fields.ForeignKeyField(
        "models.Offer", related_name="orders"
    )

async def init() -> None:
    businessman = await User.create(username="businessman")

    offer1 = await Offer.create(name="offer1", user=businessman)
    offer2 = await Offer.create(name="offer2", user=businessman)
    offer3 = await Offer.create(name="offer3", user=businessman)

    await User.create(username="client_one")
    await User.create(username="client_two")
    await User.create(username="client_three")

    # ? Make orders
    moment = datetime(2024, 1, 1, 0, 0, 0)

    orders = []
    for i, offer in enumerate((offer1, offer2, offer3)):
        for j in range(3):
            orders.append(Order(created=moment, user_id=(j + 2), offer=offer))
            moment += timedelta(days=1)

    await Order.bulk_create(orders)

async def sql_count(query: QuerySet[Model]) -> int:
    _, result = await connections.get("default").execute_query(
        f"SELECT count(*) AS total FROM ({query.sql()})"
    )
    return result[0]["total"]

async def main() -> None:
    await Tortoise.init(config=TORTOISE_CONFIG)
    await Tortoise.generate_schemas()

    await init()

    # ? Get all clients who placed orders for offers
    # ? created by a businessman with ID = 1
    query = (
        User.filter(
            orders__offer__user_id=1
        ).distinct()
    )

    result = await query
    print(result)
    print(await query.count())
    print(await sql_count(query))

if __name__ == "__main__":
    run_async(main())

Output:

> [<User: 2>, <User: 3>, <User: 4>]
> 9
> 3

Expected behavior A clear and concise description of what you expected to happen. Output:

> [<User: 2>, <User: 3>, <User: 4>]
> 3
> 3

Additional context Add any other context about the problem here. Windows 10 Python 3.11.5 tortoise-orm 0.20.1

abondar commented 1 month ago

Yeah, there are some shenanigans going on with count, because we calculate it without nesting query inside another select, and because of that we capture some bugs like this on when working with joins

Probably should just wrap all count requests inside outer select count(*)

You can try to make PR about it, or may be I will do it later

PieceOfGood commented 1 month ago

I looked at the definition of the queryset.CountQuery class and did not understand how to do this in accordance with the existing rules. My subjective experience tells me to simply replace the body of the queryset.QuerySet.count() method with the body of the example function sql_count(). Since both approaches query the database with almost identical SQL, it does not look like an overhead, but returning the correct result.

With your permission, I leave this decision within your competence. Thank you for your time.

abondar commented 1 month ago

Should be fixed on 0.21.0

alistairmaclean commented 3 weeks ago

This is still broken from my testing:

Complete script to reproduce:

from datetime import datetime, timedelta

from tortoise import Model, run_async, Tortoise, connections, fields
from tortoise.queryset import QuerySet

class User(Model):

    id: int = fields.IntField(pk=True)
    username: str = fields.CharField(max_length=50, null=True, unique=True)

    offers: fields.ReverseRelation["Offer"]
    orders: fields.ReverseRelation["Order"]

class Offer(Model):

    id: int = fields.IntField(pk=True)
    name: str = fields.CharField(max_length=50, null=False)

    user: fields.ForeignKeyRelation[User] = fields.ForeignKeyField(
        "models.User", related_name="offers"
    )
    orders: fields.ReverseRelation["Order"]

class Order(Model):

    created: datetime = fields.DatetimeField(null=False)

    user: fields.ForeignKeyRelation["User"] = fields.ForeignKeyField(
        "models.User", related_name="orders"
    )
    offer: fields.ForeignKeyRelation["Offer"] = fields.ForeignKeyField(
        "models.Offer", related_name="orders"
    )

async def sql_count(query: QuerySet[Model]) -> int:
    _, result = await connections.get("default").execute_query(
        f"SELECT count(*) AS total FROM ({query.sql()})"
    )
    return result[0]["total"]

async def main() -> None:
    await Tortoise.init(db_url="sqlite://:memory:", modules={"models": ["__main__"]})
    await Tortoise.generate_schemas()

    businessman = await User.create(username="businessman")

    offer1 = await Offer.create(name="offer1", user=businessman)
    offer2 = await Offer.create(name="offer2", user=businessman)
    offer3 = await Offer.create(name="offer3", user=businessman)

    await User.create(username="client_one")
    await User.create(username="client_two")
    await User.create(username="client_three")

    moment = datetime(2024, 1, 1, 0, 0, 0)

    orders = []
    for i, offer in enumerate((offer1, offer2, offer3)):
        for j in range(3):
            orders.append(Order(created=moment, user_id=(j + 2), offer=offer))
            moment += timedelta(days=1)

    await Order.bulk_create(orders)

    query = (
        User.filter(
            orders__offer__user_id=1
        ).distinct()
    )

    result = await query
    print(result)
    print(await query.count())
    print(await sql_count(query))

if __name__ == "__main__":
    run_async(main())

Output:

[<User: 2>, <User: 3>, <User: 4>]
9
3

Versions:

pypika-tortoise==0.1.6
tortoise-orm==0.21.3