Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

New limit behavior breaking database queries #50602

Closed
punyflash opened this issue Mar 16, 2024 · 5 comments
Closed

New limit behavior breaking database queries #50602

punyflash opened this issue Mar 16, 2024 · 5 comments

Comments

@punyflash
Copy link
Contributor

punyflash commented Mar 16, 2024

Laravel Version

11.0.7

PHP Version

8.3.3

Database Driver & Version

PgSQL 15.6

Description

I've used this query to display total amount of transactions on a day basis:

// Controller
$model
    ->load(['transactions' => fn ($q) => Transaction::scopeReferralGroups($q, 3)])

// Transaction
public static function scopeReferralGroups(Builder $query, int $days): Builder
{
    return $query->select(
        DB::raw('DATE(completed_at) as date'),
        DB::raw('COUNT(*) as count'),
        DB::raw('SUM(credits_amount) as credits_total'),
    )
        ->where('is_referral', true)
        ->orderBy('date', 'desc')
        ->groupBy('date')
        ->limit($days);
}

This works correctly on Laravel 10.x or without limit statement, but now fails with error:

SQLSTATE[42703]: Undefined column: 7 ERROR: column "date" does not exist LINE 1: ...r (partition by "transactions"."user_id" order by "date" des... ^

Right now, adding a limit clause produces a database query wrapped in window function:

SELECT *
FROM
  (SELECT DATE(completed_at) AS date,
          COUNT(*) AS COUNT,
          SUM(credits_amount) AS credits_total,
          row_number() OVER (PARTITION BY "transactions"."user_id"
                             ORDER BY "date" DESC) AS "laravel_row"
   FROM "transactions"
   WHERE "transactions"."user_id" in (1)
     AND "is_referral" = ?
   GROUP BY "date") AS "laravel_table"
WHERE "laravel_row" <= 3
ORDER BY "laravel_row"

However expected query is:

SELECT DATE(completed_at) AS date,
       COUNT(*) AS COUNT,
       SUM(credits_amount) AS credits_total
FROM "transactions"
WHERE "transactions"."user_id" in (1)
  AND "is_referral" = ?
GROUP BY "date"
ORDER BY "date" DESC
LIMIT 3

I believe this is something to do with #49695. I think this should not be implemented as default behavior, but rather added with condition, for example like limit(3, window: true)

Steps To Reproduce

Add limit clause to any ordered group by query on HasMany relationship instance

@punyflash
Copy link
Contributor Author

punyflash commented Mar 16, 2024

As a solution right now, it's possible to load data as plain query:

$model
    ->load(['transactions' => fn ($q) => Transaction::scopeReferralGroups($q->getQuery(), 3)])

@staudenmeir
Copy link
Contributor

Hi @punyflash,
IMO, you're misusing load() here. You aren't loading the transactions relationship but running a custom aggregate query instead. Not sure how this even worked for you in Laravel 10 – it shouldn't work and doesn't for me.

You can use setRelation() instead:

$model->setRelation(
    'transactions',
    Transaction::scopeReferralGroups($model->transactions()->getQuery(), 3)->get()
);

@punyflash
Copy link
Contributor Author

punyflash commented Mar 17, 2024

@staudenmeir, yeah, I agree, wasn't paying attention that result is not set, your example seems more reasonable.

Though, the issue is still there: adding the limit statement to the relationship query adds a window function to SQL query where it should not be. I can simplify my code example if Transaction::scopeReferralGroups seems confusing:

$model->load([
    'transactions' => fn ($q) => $q->select(
        DB::raw('DATE(completed_at) as date'),
        DB::raw('COUNT(*) as count'),
        DB::raw('SUM(credits_amount) as credits_total'),
    )
        ->where('is_referral', true)
        ->orderBy('date', 'desc')
        ->groupBy('date')
        ->limit(3) // breaks with limit
]);

@MrPunyapal
Copy link
Contributor

As a solution right now, it's possible to load data as plain query:

$model
    ->load(['transactions' => fn ($q) => Transaction::scopeReferralGroups($q->getQuery(), 3)])

why not?

$model->load(['transactions' => fn ($q) => $q->referralGroups(3)]);

@staudenmeir
Copy link
Contributor

result is not set

The result is not set because your query is not a valid relationship query. It doesn't select actual transactions but custom aggregate rows. The result can only be set if you select the user_id column, but that's not possibly here.

Laravel 11's limit implementation "breaks" your query, but the query itself is already invalid in combination with load().

@laravel laravel locked and limited conversation to collaborators Mar 18, 2024
@crynobone crynobone converted this issue into discussion #50622 Mar 18, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants