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

[10.x] Fix CanBeOneOfMany giving erroneous results #47427

Merged
merged 15 commits into from
Sep 21, 2023
Merged

[10.x] Fix CanBeOneOfMany giving erroneous results #47427

merged 15 commits into from
Sep 21, 2023

Conversation

Guilhem-DELAITRE
Copy link
Contributor

@Guilhem-DELAITRE Guilhem-DELAITRE commented Jun 13, 2023

This PR topic and content is identical to #46309 except I added a fix for strict RDBMS (like PostgreSQL) which doesn't support approximate usage of aggregate / group by.

@Guilhem-DELAITRE Guilhem-DELAITRE marked this pull request as ready for review July 4, 2023 05:57
@GrahamCampbell GrahamCampbell changed the title FIX on CanBeOneOfMany trait giving erroneous results (v2) [10.x] Fix CanBeOneOfMany giving erroneous results Jul 8, 2023
@Guilhem-DELAITRE
Copy link
Contributor Author

Guilhem-DELAITRE commented Jul 27, 2023

Is there anything wrong with my PR preventing it to be merged ?

@driesvints
Copy link
Member

@Guilhem-DELAITRE just think Taylor hasn't gotten to this one yet with all the pre and post Laracon US stuff.

@Guilhem-DELAITRE
Copy link
Contributor Author

@driesvints
Ok, not trying being pushy, I just wanted to know if the PR was lacking anything, thx for the feedback

if ($key === 0) {
$aggregatedColumn = "{$aggregate}({$aggregatedColumn})";
} else {
$aggregatedColumn = "min({$aggregatedColumn})";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why min here? Can you explain the entire logic behind this PR and how it fixes the issue?

Copy link
Contributor Author

@Guilhem-DELAITRE Guilhem-DELAITRE Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The min purpose is to use an aggregate to select any value on a column which we already know all value are the same because it's the sub-level aggregate. We still need an aggregate because there's no group by clause on that column. The most correct solution would probably be to add a group by on that column, but I didn't manage to do it in the time I had.

The more global purpose of the PR is fully stated and detailed (and is demonstrated by the modification of the generated query) in the original PR : #46309

The last two commits purpose is to fix the problem mentionned in
#46394 which was due to my modification not working with DBRMS which are strict with aggregates / group by like PostgreSQL.

@crynobone
Copy link
Member

crynobone commented Sep 13, 2023

This PR shouldn't be called "Fix" as it introduced new usage from

Current

return $this->hasOne(Profle::class)->ofMany('created_at', 'max');

After

return $this->hasOne(Profle::class)->ofMany([
    'created_at', 'max',
    'updated_at', 'max',
]);

Also I believe a similar tests should be added using Integrations tests to confirm the code works on SQLite, MySQL, Postgres and MSSQL.

@Guilhem-DELAITRE
Copy link
Contributor Author

Guilhem-DELAITRE commented Sep 13, 2023

@crynobone
It doesn't introduce new usage, this usage already exists as can be seen in function price of file tests/Database/DatabaseEloquentHasOneOfManyTest.php

BUT this usage (using multiple aggregate in ofMany method) actually produces erroneous results in cases and for reasons detailed in the following PR : #46309

You are right about the part Also I believe a similar tests should be added using Integrations tests to confirm the code works on SQLite, MySQL, Postgres and MSSQL., and the fact that the pre-existing multi-aggregate ofMany wasn't integration tested with multi databases is the reason the origin PR that I previously mentionned produced errors on PostgreSQL when it was merged.

@crynobone
Copy link
Member

Regardless, this need Integration tests

@crynobone crynobone marked this pull request as draft September 13, 2023 08:03
@Guilhem-DELAITRE
Copy link
Contributor Author

Guilhem-DELAITRE commented Sep 13, 2023

@crynobone
I basically duplicated the test I added in tests/Database into tests/Integration/Database with some tweaks.

Seems that they passed successfully.

@crynobone crynobone marked this pull request as ready for review September 14, 2023 08:02
@taylorotwell taylorotwell merged commit 05c7572 into laravel:10.x Sep 21, 2023
18 checks passed
timacdonald pushed a commit to timacdonald/framework that referenced this pull request Sep 22, 2023
* Add test to pinpoint the dysfunction

* FIX CanBeOneOfMany.php

* FIX test on raw sql

* FIX missing aggregate for strict RDBMS (like postgresql)

* FIX

* Add integration test to ensure multi-databases coverage

* Update EloquentHasOneOfManyTest.php

* Update EloquentHasOneOfManyTest.php

* wip

* wip

* wip

* wip

* wip

---------

Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants