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

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

Closed
PieceOfGood opened this issue May 9, 2024 · 3 comments · Fixed by #1624
Closed

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

PieceOfGood opened this issue May 9, 2024 · 3 comments · Fixed by #1624
Labels
bug Something isn't working

Comments

@PieceOfGood
Copy link

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
Copy link
Member

abondar commented May 9, 2024

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
Copy link
Author

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
Copy link
Member

abondar commented May 24, 2024

Should be fixed on 0.21.0

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

Successfully merging a pull request may close this issue.

2 participants