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] Add JoinMany and JoinOne methods for joining Models to Builder #46603

Closed
wants to merge 6 commits into from

Conversation

joelharkes
Copy link
Contributor

How it works

With this Merge a Laravel developer can easily join models without needing to think every time how to join the columns names, as long as you use default laravel naming conventions. You can easily Join models on each other.

class Blog extends Model {
    public function comments(){
        return $this->hasMany(Comment::class);
    }
}

class Comment extends Model {
}

Blog::query()->joinMany(Comment::class)->toSql();
// select * from "blogs"
// inner join "comments" on "comments"."blog_id" = "blogs"."id"

It also works with any scopes or where statements.
These will be added to the "on" part of the join to avoid having weird left or right joins. (otherwise if you left joins softdeletable models it would also remove also results that have a related soft deleted model and no other models connected. (in this case a blog with 1 deleted comment would not show up in the results). But that is not the case in this solution :)

class Blog extends Model {
    public function comments(){
        return $this->hasMany(Comment::class);
    }
}

class Comment extends Model {
    use SoftDeletes
}

Blog::query()->joinMany(Comment::class)->toSql();
// select * from "blogs"
// inner join "comments" on "comments"."blog_id" = "blogs"."id" and "comments"."deleted_at" is null

Example with where statement

Blog::query()->joinMany(Comment::query()->where('comments.created_at', '>', '2022-01-01')->toSql();
// select * from "blogs"
// inner join "comments" on "comments"."blog_id" = "blogs"."id" and "comments"."deleted_at" is null and "comments"."created_at" > ?

It also works when a relation is the base query:

(new Blog)->comments()->joinOne(User::query()->toSql();
// select * from "comments" inner join "users" on "users"."id" = "comments"."user_id" where "comments"."blog_id" = ? and "comments"."blog_id" is not null

Edge cases

  • It currently doesn't work ideally with relations query builder (see below). Although you can join on one you cannot use one in the join as it would add additional constraints.
  • Not supported are database/Builder of this moment, i could support it using the "from" table name and singularize it. It's something i would like to add in a followup PR.

Considerations & future work

Manually joining Relations

Joining relationships is much harder as it add would add the constraints automatically:

(new Blog())->newQuery()->joinMany($blog->comments())->toSql();
/// select * from "blogs" inner join "comments" on "comments"."blog_id" = "blogs"."id" and ("comments"."blog_id" = ? and "comments"."blog_id" is not null)

You might expect to just join all comments instead of only joining the comments for the Blog model that you just created.
But the problem is the "constraints" are added when making the Relationship, and this class has no control at this point.

To make this work Each relationship should either:

  • Not add constraints directly but somehow afterwards, but i expect this would break to many other things
  • Keep track of a second query builder that does not have the added contraints
  • Or add an extra method "withoutConstraints()" that would remove the "wheres" added on the builder for this relationship. I didn't do enough research to see if this would work.
  • Or not add the constraints if the "key" is null which happens when you instantiate the model yourself as i did in the example above. But i wonder if this might break anything.

I would love to solve this with some help

Join relationships (future work)

In future work i would like to add a joinRelation("name")

class Blog extends Model {
    public function comments(){
        return $this->hasMany(Comment::class);
    }
}

class Comment extends Model {
    public function user(){
        return $this->belongsToOne(User::class);
    }
}

Blog::query()->joinRelations('comments.user')->toSql();
// select * from "blogs"
// inner join "comments" on "comments"."blog_id" = "blogs"."id"
// inner join "users" on "users"."id" = "comments"."user_id"

But there are many considerations here, perhaps ideally we would like to use a table aliases. to be able to join models of the same class. for example:

class User extends Model {
    public function manager(){
        return $this->belongsToOne(User::class);
    }
}
User::query()->joinRelations('manager')->toSql();
// select * from "users"
// inner join "users" as "manager" on "manager"."user_id" = "users"."id"

This would need some extra work and some more research on my side.

Database Builder (future work)

Technically joining a database

Qualify all column names when joining:

When joining a query Builder it could be the query builder has where statements without qualified table names. This could lead to ambiguous column names.

Consider example below:

Blog::query()->joinMany(Comment::query()->where('created_at', '>', '2022-01-01')->toSql();
// select * from "blogs"
// inner join "comments" on "comments"."blog_id" = "blogs"."id" and "comments"."deleted_at" is null and "created_at" > ?

it's unclear if "created_at" is from comments or Blog Table.

This problem also exists when you manually build your join query so i think this can be merged/used with keeping this limitation in mind. Ideas:

  • Ideally the query builder qualifies the name always but this would be a backwards in-compatible change.
  • Alternatively all where statements could by qualified by the Model the moment it is joined into the base query.

@joelharkes joelharkes changed the title [10.x] Add JoinMany and JoinOne methods for directly adding models [10.x] Add JoinMany and JoinOne methods for joining Models to Builder Mar 27, 2023

return '('.substr($this->compileWheres($where['query']), $offset).')';
return '('.substr($whereSql, $offset).')';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was actually a bug, nested inner where's would be cut off on 'whe' and still leave 're ' in the query

is_string($model) => (new $model())->newQuery(),
$model instanceof Builder => $model,
$model instanceof Model => $model->newQuery(),
$model instanceof Relation => $model->getQuery(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relations actually don't work very well yet (see PR description), i could remove this for now so it would just throw an exception instead?

@cosmastech
Copy link
Contributor

cosmastech commented Mar 27, 2023

This would be a huge win!

I didn't look at the code too closely, but does this also hydrate the model & set the relation too? That would be 😗👌

edit: saw in your notes (Future Work) that this would require the joinRelations() functionality first.

@joelharkes
Copy link
Contributor Author

This would be a huge win!

I didn't look at the code too closely, but does this also hydrate the model & set the relation too? That would be 😗👌

edit: saw in your notes (Future Work) that this would require the joinRelations() functionality first.

hydration can better be done with the already existing ->with() method for eager loading. i think this is better than using joins for this

@cosmastech
Copy link
Contributor

cosmastech commented Mar 27, 2023

hydration can better be done with the already existing ->with() method for eager loading. i think this is better than using joins for this

Yes, it's possible currently using with(), however, it's performing a second query when the columns have already been retrieved. It's a minor issue and I'm grateful there is a nice, working solution for it, but it's one of those minor grievances I often wish I could solve for.

But regardless, this was just a thought I had. 👍

@joelharkes
Copy link
Contributor Author

joelharkes commented Apr 1, 2023

In future PR i would like to make this possible:

class User extends Model {
    public function manager(){
        return $this->belongsToOne(User::class)->where('some_filter', true);
    }
}
User::query()->joinRelations('manager')->toSql();
// select * from "users"
// inner join "users" as "manager" on "manager"."user_id" = "users"."id" and "manager"."some_filter" = 1

it includes added filters but not include 'default constraints' (based on model id).

but adding this in this PR i fear it might be to big a refactor and might get a lot of push-back, and might need to wait when it brings breaking changes.

@joelharkes
Copy link
Contributor Author

see here an example of joinRelation() method. but it's still deep work in progress:
https://github.com/laravel/framework/compare/10.x...joelharkes:framework_old:10/joins_models_and_relations?expand=1

@taylorotwell
Copy link
Member

Not sure I want to maintain this code at this time. Thanks.

@joelharkes
Copy link
Contributor Author

Not sure I want to maintain this code at this time. Thanks.

@taylorotwell Can you please elaborate? This fits completely within the Relationship structure and default naming schemes Laravel already handles. It's a minimum addition of code with a huge increase to developer experience for those who follow the naming conventions Laravel follows.

@Rizky92
Copy link

Rizky92 commented Apr 6, 2023

I suggest you take a look at kirschbaum-development/eloquent-power-joins to see what would be like if this was ever implemented.

@joelharkes
Copy link
Contributor Author

I suggest you take a look at kirschbaum-development/eloquent-power-joins to see what would be like if this was ever implemented.

Ah I didn't know this existed. It's very similar indeed.

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