Skip to content

Fix HQL and LINQ query by a type on <any/> with meta-types other than int #1803

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 18, 2018
Merged

Fix HQL and LINQ query by a type on <any/> with meta-types other than int #1803

merged 4 commits into from
Jul 18, 2018

Conversation

hazzik
Copy link
Member

@hazzik hazzik commented Jul 18, 2018

Fixes #1774

@hazzik hazzik added this to the 5.2 milestone Jul 18, 2018
fredericDelaporte
fredericDelaporte reviewed Jul 18, 2018
View reviewed changes
src/NHibernate/Hql/Ast/ANTLR/Tree/BinaryLogicOperatorNode.cs Outdated
@@ -277,25 +278,29 @@ private void ProcessMetaTypeDiscriminatorIfNecessary(IASTNode lhs, IASTNode rhs)
return;
}

var lhsNodeMetaType = lhsNode.DataType as MetaType;
if (lhsNodeMetaType != null)
if (lhsNode.DataType is MetaType lhsNodeMetaType)
Copy link
Member

Choose a reason for hiding this comment

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

Having a quick look at it, it seems the same could be done with ClassMetaType, which should allow supporting the Class mapping case. Or am I missing something?
(Maybe ClassMetaType should even extend MetaType, with a hard-coded in base constructor call StringType base type.)

Copy link
Member

Choose a reason for hiding this comment

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

Well, the HQL test is invalid for that case, because class meta-type asks for the full class name.

By the way there is a workaround for all HQL cases: using parameters. It seems this was considered as a known limitation of the any mapping at some point, see this Jira comment.

Copy link
Member Author

@hazzik hazzik Jul 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Adjusting the tests for HQL is really easy. But I was struggling with making a generic Linq change. So I just gave up. Anyway, IMO, ClassMetaType is weird creature of a frankenstein: it was created as a compatibility layer. I really think it shall finally RIP.

@hazzik
Copy link
Member Author

hazzik commented Jul 18, 2018

@fredericDelaporte I think here we shall fix only what's been asked for (meta-type). Rest can be fixed later at some point, if we'll desire to do so.

@hazzik
Copy link
Member Author

hazzik commented Jul 18, 2018

Gave it another try and I was able to make it work with for meta-type="class"

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Jul 18, 2018 •
edited
Loading

Great. I was still looking in all this.

Currently, "implicit" is in fact equivalent to meta-type="string" without any meta-value specified. Not having meta-values with a basic type is normally an error. (At least that is the way I have documented it it is documented, and it now appears to me I have to review that part of the doc again. (Update: in fact I have just moved that section without rewriting it.))
But it was previously resolving to TypeType, which was equivalent to class. So indeed, things are a bit messy currently about the any type.

@hazzik
Copy link
Member Author

hazzik commented Jul 18, 2018

Ok, this broke an old test.

fredericDelaporte
fredericDelaporte approved these changes Jul 18, 2018
View reviewed changes
Copy link
Member

@fredericDelaporte fredericDelaporte left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The "implicit" case will require another, maybe more impact-full, fix.
(I think the line 43 here:

nhibernate-core/src/NHibernate/Mapping/Any.cs

Lines 40 to 44 in c85d038

type =
new AnyType(
metaValues == null
? ("class".Equals(metaTypeName) ? new ClassMetaType(): TypeFactory.HeuristicType(metaTypeName))
: new MetaType(metaValues, TypeFactory.HeuristicType(metaTypeName)),

Needs to always yield an IMetaType.)

@fredericDelaporte fredericDelaporte mentioned this pull request Jul 18, 2018
Merged
@hazzik hazzik changed the title Fix HQL and LINQ query by the type on <any/> with meta-type Fix HQL and LINQ query by the type on <any/> with meta-types other than int Jul 18, 2018
@hazzik hazzik changed the title Fix HQL and LINQ query by the type on <any/> with meta-types other than int Fix HQL and LINQ query by a type on <any/> with meta-types other than int Jul 18, 2018
@hazzik hazzik merged commit 78abbbe into nhibernate:master Jul 18, 2018
hazzik added a commit that referenced this pull request Jul 19, 2018
@hazzik
Regenerate async, see #1803
8a92082
@hazzik hazzik deleted the GH1774 branch July 27, 2018 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredericDelaporte fredericDelaporte fredericDelaporte approved these changes

Assignees
No one assigned
Labels
r: Fixed t: Fix
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
2 participants
@hazzik @fredericDelaporte