1.9/dev - Relation condition use query's where instead of relation's on clause

[AdamSGit]

... And so return nothing because relation condition is not filled instead of the model with no relation record. Take this simple example of a Director model, having a has_many relation to the Movie model.

When using conditions on Model query, like this :

$directors = Model_Director::query()
    ->related( [
        'movie' => [
            'where' => [
                ['id', '=', 3],
            ],
        ],
    ] )
    ->get();

Given that there is no movie matching id = 3 in DB. The $directors should contain every director present in the database, with a null relation. Instead of what the variable is an empty array.

Query generated :

SELECT `t0`.`id` AS `t0_c0`, `t0`.`name` AS `t0_c1`, `t0`.`created_at` AS `t0_c2`, `t0`.`updated_at` AS `t0_c3`, `t1`.`id` AS `t1_c0`, `t1`.`director_id` AS `t1_c1`, `t1`.`title` AS `t1_c2`, `t1`.`created_at` AS `t1_c3`, `t1`.`updated_at` AS `t1_c4` 
FROM `directors` AS `t0` 
LEFT JOIN `movies` AS `t1` ON (`t0`.`id` = `t1`.`director_id`) 
WHERE `t1`.`id` = 3

Query should be :

SELECT `t0`.`id` AS `t0_c0`, `t0`.`name` AS `t0_c1`, `t0`.`created_at` AS `t0_c2`, `t0`.`updated_at` AS `t0_c3`, `t1`.`id` AS `t1_c0`, `t1`.`director_id` AS `t1_c1`, `t1`.`title` AS `t1_c2`, `t1`.`created_at` AS `t1_c3`, `t1`.`updated_at` AS `t1_c4` 
FROM `directors` AS `t0` 
LEFT JOIN `movies` AS `t1` ON (`t0`.`id` = `t1`.`director_id`) AND ON (`t1`.`id` = 1)

I already created an issue ( see #390 ) about this a long time ago. Do you want me to have a look at it ?

[WanWizard]

No, I disagree,

The question here is "give me all directors who have movies who's movie ID equals 3". To which the response correctly is "there aren't any".

[AdamSGit]

Alright. So, how would you implement the following question, then : "Give me all directors. If by any chance somebody did the movie 3, I want the relation as well." ?

To me, it seem that the question you expressed should be implemented this way :

$directors = Model_Director::query()
    ->related('movie')
    ->where('movie.id', 3)
    ->get();

[WanWizard]

That results in exactly the same query, your first example is just using array notation. This is reflected in the docs too, see https://fuelphp.com/dev-docs/packages/orm/relations/intro.html#/usage_nested_rels

If you make this breaking change, I'm pretty sure a lot of apps out there will fail.

Afaik there are no provisions in the ORM to ask this question, in the end it is not a query builder, there are a lot more questions you can't ask. Additional JOIN clauses are afaik only possible using conditions on the relation definition.

We have an app that has "businesses" and "people" tables, and use different relation definitions with conditions to filter specific types of businesses or people from those tables. But conditions aren't exposed to the ORM API so you can't define them in a single query.

[AdamSGit]

This is weird that you say that, you said in the forum post referenced in issue #390 :

« WHERE's defined on the relation should become part of the ON clause, and not generate a WHERE clause.
I know that works fine on conditions defined in the relation definition in the model, which is what I always use. I have to dive into the code to see if your syntax is supported, or why that creates a different result. »

And it does use a ON clause if defined in the relation definition on the model. The relation => condition in query should be the same as the conditions in relation definition in model, for consistent design.

But if it is written in the doc this way, you are right to say that it would be a breaking change and cannot be done, which is kind of a pitty. But then, what about adding some new on condition in query relation condition, that would extends the join ON clause ?

[WanWizard]

Sorry about that. Coding has become a lot harder after I suffered brain damage in 2014. :cry: I guess I wrote that because it does sound very logical. But clearly not how it was implemented.

If I look in the code, conditions can be defined in the model, on the relation, and on a lazy-load get(). There is also a condition() method to retrieve conditions defined on the model. So clearly started, but obviously not finished.

I can't immediately see if it is implemented in array notation, like so:

$directors = Model_Director::query()
    ->related( [
        'movie' => [
            'conditions' => [
                'where' => [
                    ['id', '=', 3],
                ],
            ],
        ],
    ] )
    ->get();

but if not, it would probably be the most consistent way of implementing it?

I haven't come up with a proper solution for chained notation yet, as you have both "order_by" and "where" conditions (to change the order of the related objects in the result, or to filter them).

[AdamSGit]

Please, don't apologie. I know what happened to you, and that suck. Hope your recovering is doing well.

The solution you suggested (not implemented) seem good. I know it for a fact because it's the first thing I did when trying to do this. Not having chained notation wouldn't be an issue, I think, as this is not a very common use case.

I can take care of this is it's ok for you.

[WanWizard]

If you want to, please. I'll think about the chained notation in the meantime, as the two notations need to stay in sync.

[AdamSGit]

Mmh... Since conditions are dependent of a given relation identified by its name, chained notation could be (using php's magic method __call()) :

$directors = Model_Director::query()
    ->related( 'movie' )
    ->where_movie('id', 'in', [3, 4, 5])
    ->order_by_movie('release_year', 'asc')
    ->get();

I guess another solution would be :

$directors = Model_Director::query()
    ->related( 'movie' )
    ->conditions_open('movie')
        ->where('id', 'in', [3, 4, 5])
        ->order_by('release_year', 'asc')
    ->conditions_close()
    ->get();

[WanWizard]

I have been able to sleep on it (sort of).

We're approaching this all wrong, all this isn't needed, the result you want is already supported. See https://fuelphp.com/dev-docs/packages/orm/relations/intro.html under "Join Types".

So array notation is already implemented, but the methods join_on() and join_type() are missing from the Query class. Most likely because of the discussion above: how to define on which relation they will apply.

For the moment I can live with passing them in part-array notation, so that should be something like

$directors = Model_Director::query()
    ->related('movie', array(
        'join_type' => 'left',
        'join_on' => array(
            array('id', 'in', [3, 4, 5]),
        ),
    ))
    ->order_by('movie.release_year', 'asc')
    ->get();

?

edit: I see in the docs (and your SQL above) that "LEFT" is the default for a JOIN.

[AdamSGit]

Nice, I confirm that this works as intended, and that the default join for Orm is left. Which is not the case for the DB classe, BTW, which will just output JOIN if jointure type is not provided, which will result as an INNER JOIN by default for Mysql.

[WanWizard]

The default for ORM is LEFT because if not, you'll never get all parent records you query for.

For DB, it is just a query builder without any "magic", you so need to specify what type you want.

I guess this issue can be closed now?

[AdamSGit]

Yup, indeed.