Fix filtering sub-queries on property-ref #3685
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| // For property-ref associations, we also need this unless finding a way in the IdentNode for the other hand of the comparison | ||
| // to detect it should yield the property-ref columns instead of the primary key columns. | ||
| bool comparisonWithNullableEntityOrThroughPropertyRef = Walker.IsComparativeExpressionClause | ||
| && (entityType.IsNullable || entityType.IsUniqueKeyReference); |
There was a problem hiding this comment.
The original test case does not need the IsUniqueKeyReference test, because the not-found="ignore" mapping causes the association to be considered nullable. But the trouble occurs without this attribute too.
| } | ||
|
|
||
| if ( joinIsNeeded ) | ||
| { | ||
| DereferenceEntityJoin(classAlias, entityType, implicitJoin, parent, comparisonWithNullableEntity); | ||
| if (comparisonWithNullableEntity) | ||
| var forceLeftJoin = comparisonWithNullableEntityOrThroughPropertyRef && !IsCorrelatedSubselect; |
fredericDelaporte
force-pushed
the
GH3609
branch
2 times, most recently
from
aabef6e to
315363f
Compare
| { | ||
| _columns = FromElement.GetIdentityColumns(); | ||
| DataType = FromElement.EntityPersister.EntityMetamodel.EntityType; |
There was a problem hiding this comment.
This fixes NH-892 regression.
fredericDelaporte
force-pushed
the
GH3609
branch
from
9ae0b28 to
30d8da9
Compare
|
This is currently based on master. As a v5.3, it should target v5.3.x, but that is so old now, I do not think it is worth it, since that would mean a new version with the recent CVE not fixed in it. I will see to target v5.4 instead. (Now it is done.) |
fredericDelaporte
force-pushed
the
GH3609
branch
from
30d8da9 to
d678b7b
Compare
And fix NH-892 partly broken by the way.
fredericDelaporte
force-pushed
the
GH3609
branch
from
d678b7b to
294d207
Compare
fredericDelaporte
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #3609.
The trouble for #3609 is, when comparing an entity from a direct select to a property-ref association on the entity, the primary key of the entity ends up compared to the property-ref of the association.
This fix forces a join when comparing entities through a property-ref association, replacing the property-ref in the comparison by the primary key.
Ideally, it would be better to instead replace the primary key in the comparison by the referenced property of the compared entity without adding a join. But I cannot find how to do that. And in case of comparisons between two association, one with a property-ref, the other with the primary key, the join is mandatory.
The fix had to take into account NH-892.
In the case of NH-892, a property-ref association is compared to an entity parameter. In this case, the parameter ends up fetched with the referenced property, and so, should be directly compared to the property-ref of the association. So, this gets broken by this fix. The fix had to further change the
DotNodetype to no more be the property-ref association type but the entity-type, in order for the parameter value to be fetched with the primary key value instead of the property-ref value.(Actually NH-892 was already half-broken: changing the test case association to use
not-found="ignore"triggers the same failure, because this forces a join replacing the property-ref in the comparison, too. That is likely a regression from #2081.)Overall, this fix adds a join which in many cases could theoretically be optimized out for comparing directly references property values, but that is tedious to do and bug prone. So, I settle for a simpler, less optimal solution, but more robust. For me, the first performance metric is correctness. Speed comes second.