-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[8.x] Add one-of-many relationship (inner join) #37362
Conversation
So... which PR am I supposed to review? |
@taylorotwell Both prs solve the same problem in a different way, It is up to you which one you prefer. However this solution is much faster and less complex which makes it better for users and maintainers. I actually think that the other solution offers few advantages, so I think you should review this pr. |
src/Illuminate/Database/Eloquent/Relations/Concerns/ComparesRelatedModels.php
Outdated
Show resolved
Hide resolved
How would this be ported to support MorphOne as well? |
I've pushed up a handful of formatting fixes - removed methods that were never called at all (tests still pass when I remove them, so I assume not needed). |
The query would look like this: SELECT *
FROM `states`
INNER JOIN (
SELECT MAX(id) AS id
FROM states
GROUP BY states.stateful_id, states.stateful_type,
) AS state
ON state.id = states.id For examples that do not have the key as aggregate, the morph id and type need bo added to the ...
INNER JOIN (
SELECT MAX(foo) AS foo
...
GROUP BY states.stateful_id, states.stateful_type,
) AS state
ON state.foo = states.foo AND state.stateful_id = states.stateful_id AND state.stateful_type = states.stateful_type To the |
@Sergiobop seems like |
@Sergiobop |
@Sergiobop @dianfishekqi
|
@dianfishekqi Nope, im doing this in my Model.php (I'm not even using the id, im using other column, of type 'datetime')
And i get the same error. (I tried I had to write my own AGGREGATE to skip the error (maybe some docs should be added, novice pgsql users who use uuid as primary keys will have to deal with this) But now, i have this: Maybe this error? #37362 (comment) |
@Sergiobop #37436 this should fix the err |
|
||
if (! array_key_exists($keyName, $columns)) { | ||
$columns[$keyName] = 'MAX'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed for proper functionality? It doesn't allow this feature to be used with UUID columns.
@ibrasho A unique column must be added, otherwise duplicate values would lead to unwanted behaviour. |
The only way to get this to work with UUID in Postgres specifically is to cast the column type to text. I can't do this without overriding The line that force-add the key column of the model makes this impossible. I think a better approach than forcing the ID to be used could help solve this problem. |
@ibrasho Can you please open an issue, giving a detailed description including a sql-fiddle and the code that fixes the issue for you? Then I can provide a fix with a test. |
When trying to use this with a date
Is this an expected behaviour if |
@gthedev yes this is expected, you need to disable |
Here is the current failing query, To fix it you need to cast some values: select * from "product_sale_prices"
inner join (
- select MAX(id) as id , "product_sale_prices"."product_id"
+ select MAX(id::text) as id , "product_sale_prices"."product_id"
from "product_sale_prices"
group by "product_sale_prices"."product_id"
) as "salePrice"
- on "salePrice"."id" = "product_sale_prices"."id"
+ on "salePrice"."id" = "product_sale_prices"."id"::text
and "salePrice"."product_id" = "product_sale_prices"."product_id"
where "product_sale_prices"."product_id" = '8d90f3a6-1bb8-4576-b553-c146465974c0'
and "product_sale_prices"."product_id" is not null
limit 1 |
I found a dirty solution that might lead to some better ideas. The following changes in /**
* Get a new query for the related model, grouping the query by the given column, often the foreign key of the relationship.
*
* @param string|array $groupBy
* @param string|null $column
* @param string|null $aggregate
* @return \Illuminate\Database\Eloquent\Builder
*/
protected function newOneOfManySubQuery($groupBy, $column = null, $aggregate = null)
{
$subQuery = $this->query->getModel()
->newQuery();
foreach (Arr::wrap($groupBy) as $group) {
$subQuery->groupBy($this->qualifyRelatedColumn($group));
}
if (! is_null($column)) {
+ $alias = $column;
+ if ($this->getParent()->hasCast($column, ['string'])) {
+ $column = "$column::text";
+ }
+
+ $subQuery->selectRaw($aggregate.'('.$column.') as '.$alias);
- $subQuery->selectRaw($aggregate.'('.$column.') as '.$column);
}
$this->addOneOfManySubQueryConstraints($subQuery, $groupBy, $column, $aggregate);
return $subQuery;
}
/**
* Add the join subquery to the given query on the given column and the relationship's foreign key.
*
* @param \Illuminate\Database\Eloquent\Builder $parent
* @param \Illuminate\Database\Eloquent\Builder $subQuery
* @param string $on
* @return void
*/
protected function addOneOfManyJoinSubQuery(Builder $parent, Builder $subQuery, $on)
{
$parent->joinSub($subQuery, $this->relationName, function ($join) use ($on) {
+ $subselectColumn = $this->qualifySubSelectColumn($on);
+ $relatedColumn = $this->qualifyRelatedColumn($on);
+
+ if ($this->getParent()->hasCast($on, ['string'])) {
+ $relatedColumn = new Expression($this->newQuery()->getGrammar()->wrap($relatedColumn).'::text');
+ }
+
+ $join->on($subselectColumn, '=', $relatedColumn);
- $join->on($this->qualifySubSelectColumn($on), '=', $this->qualifyRelatedColumn($on));
$this->addOneOfManyJoinSubQueryConstraints($join, $on);
});
} |
@ibrasho this probably only works for certain versions of progresql 🤔 |
Hi @cbl Thanks for this feature, I've already been busy putting it to use! Using your price example, I was wondering how I would retrieve all products with their price on a given date using with rather than whereHas so that any products without a price on that day return null for the relationship. Thanks 😎 |
AFAIK this syntax has been supported since Postgres 9 (released in 2010). But again I'm trying to think of a way to make it DB-independent. |
This reverts commit b7a54de.
really awesome work and discussion 🔥 can we support the many to many and morph many relationship too? i have the following tables
every workflow has only one so i can convert the following relationship to be
|
Hey @cbl, i'm trying again after the patch #37436 I'm getting this now, what am i doing wrong? Illuminate\Database\QueryException: SQLSTATE[08P01]: <>: 7 ERROR: bind message supplies 0 parameters, but prepared statement "pdo_stmt_00000008" requires 1 Model 1: (Model)
Model 2: (OtherModel)
Code causing the error: |
Hey @Sergiobop this seems to be an error with your sql connection. What connection are you using? And what is the result if you run the following: Model::with(['current_other_model'])->dd(); |
@cbl doing that i get this: "select * from "models" where "id" = ? and "models"."deleted_at" is null" I'm using pgsql |
Hi again @cbl , i tried to follow up the problem, and i achieved to log these queries being executed; the problem is the last one, (the bindings are empty): SQL: SQL:
Bindings: [] |
¿Se podría llevar hasOne a |
Background
This pr provides a solution for creating one-to-one relations that are a partial relation of a one-to-many relation.
These are: hasOne ∈ hasMany, morphOne ∈ morphMany
(Especially useful for event sourcing approaches)
This pr solves the same problem as #37252, but using an
inner join
instead of asubselect
.On stackoverflow there is the tag
greatest-n-per-group
, which deals with this kind of queries.Problem
To explain the problem I use the latest_login example. A user has many
logins
but only onelatest_login
(latest_login ∈ logins). One might think that this can build as follows:However this does not work for several reasons.
1. This creates a
n+1
problem when eager loading the relationship.If we take a look at the query that is executed when eager loading the relationship we see that all logins for the given users are loaded where we only need one for each user:
Adding a
->limit(1)
obviously does not solve this since only one login is loaded where we need all latest logins for the given users:2. Querying works not as expected.
latest_login()->get()
gets all logins (Same problem withcount
, ...)latest_login()->is($firstLogin)
returnstrue
even if there are newer logins.The Solution
Filter the intersection of the relation joined with itself:
This way only the needed models are eager loaded and all relation related eloquent methods provide the wanted result:
Examples
This repository cbl/laravel-one-of-many contains all the examples described below, to try out locally
The following describes various use cases and how they are build:
latest_login/first_login
payment_state
This example is an event sourced state. All state changes are stored in the states table and the payment_state relation of the model is the latest created state with the type
payment_state
.The constraint for `type` needs to be added to the inner join subselect to not load an id for another type that is higher.
As a morphOne:
See SQL query...
price
This example contains an event sourced price for a product. The price for the product is the latest published price with the maximum id, so it depends on multiple columns:
The problem that comes with this use case is that the first column
published_at
is not unique, there may be duplicates, therefore we want to get the row that has the max id. To solve this, nested inner join clauses are added for each column and the associated aggregate.First inner join.
Second inner join.
The following query combines both inner joins and loads the latest published_at with the maximum id.
How It Works
Explained how it works under the hood ...
Building Inner Joins
This is where the nested joins are built:
framework/src/Illuminate/Database/Eloquent/Relations/Concerns/CanBeOneOfMany.php
Lines 54 to 73 in 571db72
The first inner join is grouped by the foreign key, the following inner joins are grouped by the previous aggregate:
framework/src/Illuminate/Database/Eloquent/Relations/Concerns/CanBeOneOfMany.php
Lines 54 to 57 in 571db72
The closure always receives the subselect for the first aggregate from the array:
framework/src/Illuminate/Database/Eloquent/Relations/Concerns/CanBeOneOfMany.php
Lines 59 to 63 in 571db72
Existence Queries
In order for existence queries to be working, the join must be added in
getRelationExistenceQuery
:framework/src/Illuminate/Database/Eloquent/Relations/HasOne.php
Lines 94 to 100 in 571db72
CompareRelatedModels
Since the relations are a partial relation of has-many, it is not enough to check if the keys match, it must be checked via an existence query if the given model matches that of the relation:
framework/src/Illuminate/Database/Eloquent/Relations/Concerns/ComparesRelatedModels.php
Lines 16 to 23 in 571db72
framework/src/Illuminate/Database/Eloquent/Relations/Concerns/ComparesRelatedModels.php
Lines 77 to 90 in 571db72
Tests
The integration test case tests, whether the n+1 problem is solved for eager loading and only required models are loaded:
framework/tests/Integration/Database/EloquentHasOneOfManyTest.php
Lines 35 to 56 in 571db72
Furthermore, the behavior of all use cases described above is tested here:
https://github.com/laravel/framework/blob/571db720abbb41c3631577037039c89d85b54be0/tests/Database/DatabaseEloquentHasOneOfManyTest.php