[8.x] Fix HasOneOfMany with callback issue

[bastien-phi]

I found some kind of unexpected behaviour when using hasOne()->ofMany() with a callback.

I did not find a solution yet but having a failing test is helpful if someone wants to fix this.

Having something like

class User extends Model {

    public function last_updated_foo_state()
    {
        return $this->hasOne(HasOneOfManyTestState::class, 'user_id')->ofMany([
            'updated_at' => 'max',
            'id' => 'max',
        ], function ($q) {
            $q->where('type', 'foo');
        });
    }
}

and some States

$previousFoo = $user->states()->create([
    'type' => 'foo',
    'state' => 'bar',
    'updated_at' => '2020-01-01 00:00:00'
]);
$newFoo = $user->states()->create([
    'type' => 'foo',
    'state' => 'active',
    'updated_at' => '2021-01-01 12:00:00'
]);
$newBar = $user->states()->create([
    'type' => 'bar',
    'state' => 'active',
    'updated_at' => '2021-01-01 12:00:00'
]);

we expect $user->last_updated_foo_state to be $newFoo. Actually it is $newBar.

The problem comes from the query. Actually, the generated sql looks like

select *
from "states"
inner join (
    select max("id") as "id", "states"."user_id"
    from "states"
    inner join (
        select max("updated_at") as "updated_at", "states"."user_id"
        from "states"
        where "type" = ?
        and "states"."user_id" = ?
        and "states"."user_id" is not null
        group by "states"."user_id"
    ) as "last_updated_foo_state"
    on "last_updated_foo_state"."updated_at" = "states"."updated_at"
    and "last_updated_foo_state"."user_id" = "states"."user_id"
    group by "states"."user_id"
) as "last_updated_foo_state"
on "last_updated_foo_state"."id" = "states"."id" 
and "last_updated_foo_state"."user_id" = "states"."user_id"
where "states"."user_id" = ?
and "states"."user_id" is not null

My guess is that the callback is only applied once in the deeper inner join but should be also applied in the other one.

select *
from "states"
inner join (
    select max("id") as "id", "states"."user_id"
    from "states"
    inner join (
        select max("updated_at") as "updated_at", "states"."user_id"
        from "states"
        where "type" = ?
        and "states"."user_id" = ?
        and "states"."user_id" is not null
        group by "states"."user_id"
    ) as "last_updated_foo_state"
    on "last_updated_foo_state"."updated_at" = "states"."updated_at"
    and "last_updated_foo_state"."user_id" = "states"."user_id"
+   where "type" = ?    
    group by "states"."user_id"
) as "last_updated_foo_state"
on "last_updated_foo_state"."id" = "states"."id" 
and "last_updated_foo_state"."user_id" = "states"."user_id"
where "states"."user_id" = ?
and "states"."user_id" is not null

This could be solved by changing CanBeOneOfMany::ofMany with

if (isset($previous)) {
                $this->addOneOfManyJoinSubQuery($subQuery, $previous['subQuery'], $previous['column']);
-} elseif(isset($closure)) {
+}
+
+if(isset($closure))
    $closure($subQuery);
}

but it comes with other problems. Some tests are failing (testMultipleAggregates, testEagerLoadingWithMultipleAggregates) with General error: 1 ambiguous column name: published_at.

It can be solved in userland with

public function price()
{
    return $this->hasOne(HasOneOfManyTestPrice::class, 'user_id')->ofMany([
        'published_at' => 'max',
        'id' => 'max',
    ], function ($q) {
-        $q->where('published_at', '<', now());
+        $q->where('prices.published_at', '<', now());
    });
}

but I'm not sure it is the right move.

If someone has any idea how to fix this, I left everything here !

[JayBizzle]

/ping @cbl

[cbl]

@bastien-phi I found a fix (see bastien-phi#1, feel free to merge), however this would introduce a breaking change. I have to wrap my head around it for a bit. Maybe there is another fix for this.

Edit: I just noticed the fix is exactly what you described above...

[bastien-phi]

@cbl it is indeed what I meant. I really don't have any idea what to do here ^^

[bastien-phi]

I think I found a solution here.

The problem, when just applying this fix

if (isset($previous)) {
                $this->addOneOfManyJoinSubQuery($subQuery, $previous['subQuery'], $previous['column']);
-} elseif(isset($closure)) {
+}
+
+if(isset($closure))
    $closure($subQuery);
}

is that it produces sql queries with ambiguous column names.

For example

select *
from "prices"
inner join (
    select max("id") as "id", "prices"."user_id"
    from "prices"
    inner join (
        select max("published_at") as "published_at", "prices"."user_id"
        from "prices"
        where "published_at" < ?
        and "prices"."user_id" = ?
        and "prices"."user_id" is not null
        group by "prices"."user_id"
    ) as "price"    
    on "price"."published_at" = "prices"."published_at"
    and "price"."user_id" = "prices"."user_id"
    where "published_at" < ? -- here is the ambiguous column. does it references price.published_at or prices.published_at ?
    group by "prices"."user_id"
) as "price"
on "price"."id" = "prices"."id" 
and "price"."user_id" = "prices"."user_id"
where "prices"."user_id" = ?
and "prices"."user_id" is not null

I suggest to modify the sql in order to remove the ambiguity

select *
from "prices"
inner join (
    select max("id") as "id", "prices"."user_id"
    from "prices"
    inner join (
-        select max("published_at") as "published_at", "prices"."user_id"
+        select max("published_at") as "published_at_aggregate", "prices"."user_id"
        from "prices"
        where "published_at" < ?
        and "prices"."user_id" = ?
        and "prices"."user_id" is not null
        group by "prices"."user_id"
    ) as "price"    
-    on "price"."published_at" = "prices"."published_at"
+    on "price"."published_at_aggregate" = "prices"."published_at"
    and "price"."user_id" = "prices"."user_id"
    where "published_at" < ? -- No more ambiguity, it references prices.published_at
    group by "prices"."user_id"
) as "price"
on "price"."id" = "prices"."id" 
and "price"."user_id" = "prices"."user_id"
where "prices"."user_id" = ?
and "prices"."user_id" is not null

The implementation could be a little bit cleaner but I think it solves the problem without adding a breaking change.

[taylorotwell]

Any additional thoughts @cbl?

[cbl]

@bastien-phi Great, good idea! This should solve the problem without introducing breaking changes.

[driesvints]

I'm sending in a PR to revert this. See #39266