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

Support HasManyThrough relationships #8

Merged
merged 18 commits into from
Dec 7, 2022

Conversation

jclarkkoala
Copy link
Collaborator

@jclarkkoala jclarkkoala commented Nov 30, 2022

What

Adds full support for Repositories that use models that contain a HasManyThrough relationship.

Why

EloquentRepository already supports filtering through fields that live in any Eloquent relationship. But, it lacks in these ways:

  • It is unable to apply sorts and aggregations using a field that is found through a HasManyThrough relationship
  • It cannot accept data to create a new model that is connected through a HasManyThrough relationship.

How

Full support for sort and aggregation with fields defined through a HasManyThrough relationship

EloquentQueryModifier::applyNestedJoins - Added a case join HasManyThrough related data - the "through" model and the "far" model - so the fields are accessible when used as a sort or aggregation.

Preliminary support for the creation of models related through HasManyThrough

EloquentRepository::fill()

The ability to create related child models lives here.

Related models are filled and associated according to how the input data is nested.

An example with directly nested model data:
A User "HasMany" Posts. And, a Post "HasMany" Reactions.
The posts are directly nested under under the user, and the reactions are nested directly under posts, so the relationship is easily known when it is time to create models and link them.

(new EloquentRepository(User::class, [ //Root-level fields will be assigned to the `User` model
    'username' => 'joe',
        'posts'    => [                 //Matches the name of the relationship defined in `User`
                [
                    'label' => 'First Post!', 
                    'reactions' => [     //Matches the name of the relationship defined in 'Post'
                        ['name' => 'John Doe', 'icon' => 'thumbs-up'
                    ]
                ]
            ]
        ]
    )->save();

And, an example with a HasManyThrough relationship:
HasManyThrough relationships make the relationship more difficult to parse. In this example, a User has many Reactions through their Posts. The Post still has the HasMany relationship with Reactions. But the "through" relationship is not apparent.

There is an assumption that a Post with post_id already exists.
TODO: Come up with a language in this fill body to relate a not-yet-created reactions model to a not-yet-created posts model. Create models in the fill action in an order where a new "through" model (in this case posts) is created first, and the reactions model is created last and contains the posts id.

(new EloquentRepository(User::class, [
    'username' => 'joe',
        'posts'    => [
                [
                    'label' => 'First Post'
                ]
            ]
        ],
        'reactions` => [      //Matches the relationship name defined in `User`
            ['post_id' => 1, 'name' => "John Doe", 'icon' => 'thumbs-up']
        ]
    )->save();

EloquentRepository::cascadeRelations

This is where the related models created in EloquentRepository::fill() are saved along with their relationships with other models. The persistence of Models that were created because of a HasManyThrough relationship are delayed until all other related models are saved. There is the same assumption that the "through" model already exists.
TODO: Handle cases where the "through" model is part of the same fill operation, and the id is not yet known until the "through" model is saved.
TODO: Properly handle chained HasManyThrough relationships, and save the models in the correct order.

CC 🐨

@jclarkkoala jclarkkoala changed the title WIP: Support HasManyThrough relationships Support HasManyThrough relationships Nov 30, 2022
@@ -54,6 +52,6 @@ public function resolveModelClass(Route $route): string
*/
final public function namespaceModel($model_class)
{
return sprintf('%s%s', $this->getAppNamespace(), $model_class);
return sprintf('%s%s', App::getNamespace(), $model_class);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AppNamespaceDetectorTrait was deprecated after Laravel 5.8 and rolled into the Application.

@@ -18,8 +18,6 @@
*/
class RouteGuessingModelResolver implements ModelResolver
Copy link
Collaborator Author

@jclarkkoala jclarkkoala Dec 7, 2022

Choose a reason for hiding this comment

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

This resolver is not used in our projects and it had no test coverage. It also contained a trait that was deprecated after Laravel 5.8.

I assumed that it was meant to work with named routes with a format of version.resource since the comments refer to a version and resource.

Copy link
Member

@walterbm walterbm left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few comments & questions but the only critical note is adding some documentation about this new HasManyThrough functionality to the README

//Use the relationship to build a new query without the model constraint and name the subtable the same as the requested relationship name
//Alias the "first" key to prevent collisions with other columns
$subQuery = $instance->$relation()->select([
$related->getQualifiedFirstKeyName().' AS '.($uniqeFirstKey = str_replace('.', '_', uniqId().$related->getQualifiedFirstKeyName())), //Id to left join on
Copy link
Member

Choose a reason for hiding this comment

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

Minor spelling suggestion

Suggested change
$related->getQualifiedFirstKeyName().' AS '.($uniqeFirstKey = str_replace('.', '_', uniqId().$related->getQualifiedFirstKeyName())), //Id to left join on
$related->getQualifiedFirstKeyName().' AS '.($uniqueFirstKey = str_replace('.', '_', uniqueId().$related->getQualifiedFirstKeyName())), //Id to left join on

@@ -652,6 +653,18 @@ protected function applyNestedJoins(array $relations, Model $instance, $field, $
// Join related's table on the base table's foreign key
$query->join($table, $instance->getQualifiedKeyName(), '=', $related->getQualifiedForeignKeyName());
break;
case HasManyThrough::class:
Relation::noConstraints(function () use ($related, $relation, $instance, $query, $field) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to use Relation::noConstraints here?

Copy link
Collaborator Author

@jclarkkoala jclarkkoala Dec 7, 2022

Choose a reason for hiding this comment

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

The relation query is constrained to the model that has the HasManyThrough relationship by default.
Going with the same example in the Pouch tests where a User has many Reaction through Post:
$user->reactions query looks like this SELECT FROM users INNER JOIN (SELECT * FROM posts INNER JOIN reactions ON reactions.post_id = posts.id WHERE posts.user_id = ?). The inner select query is the relation with constraints.

That is not useful for aggregations on a field that comes from a HasManyThrough relationship. The unconstrained query will have an inner query that looks like SELECT * FROM posts INNER JOIN reactions ON reactions.post_id = posts.id.

@@ -394,6 +394,19 @@ protected function fill(Model $instance): bool
'value' => $value,
];
break;
case HasManyThrough::class:
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a note to the README documentation to explain this functionality and the limitations

Copy link
Member

@ericjohnf ericjohnf left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

@jclarkkoala jclarkkoala merged commit 4e35f68 into main Dec 7, 2022
@delete-merged-branch delete-merged-branch bot deleted the chore/Accept-has-many-through-for-nested-join branch December 7, 2022 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants