Add support for OData group by queries #2322

maca88 · 2020-03-06T18:00:42Z

Fixes #2221
Fixes part of #2334
Fixes #2135

maca88 · 2020-03-06T18:04:35Z

src/NHibernate/Linq/GroupBy/GroupBySelectClauseRewriter.cs

@@ -67,8 +75,8 @@ protected override Expression VisitMember(MemberExpression expression)
 				return base.VisitMember(expression);
 			}

-			if ((elementSelector is NewExpression || elementSelector.NodeType == ExpressionType.Convert)


Instead of checking whether it is a ExpressionType.Convert expression, its operand is checked in order to verify whether it is actually supported. The ExpressionType.Convert was used for ExpressionType.ArrayIndex expression which was added by #433.

maca88 · 2020-03-30T15:52:14Z

The last commit resolves the issue with group by reported in #2334.

maca88 · 2020-03-30T15:52:42Z

src/NHibernate/Linq/Visitors/TransparentIdentifierRemovingExpressionVisitor.cs

+
+			// Added logic: In some cases (e.g OData), the member can be from a different derived class, in such case
+			// we need to check the member DeclaringType instead of ReflectedType
+			if (matchingAssignment == null && memberExpression.Expression.NodeType == ExpressionType.MemberInit)


The only way to test this was to include and use Microsoft.AspNetCore.OData package as this cannot be reproduced by creating classes in code.

src/NHibernate/Linq/Visitors/TransparentIdentifierRemovingExpressionVisitor.cs

hazzik · 2020-03-31T21:06:42Z

src/NHibernate/Linq/Visitors/TransparentIdentifierRemovingExpressionVisitor.cs

+			                         .LastOrDefault();
+
+			// Added logic: In some cases (e.g OData), the member can be from a different derived class, in such case
+			// we need to check the member DeclaringType instead of ReflectedType


It's all good for now, but probably one of 2 things should happen:

This fixed in OData provider so it generates the correct expression.

This fixed in ReLinq so we do not copy code from there and just use fixed version.

but probably one of 2 things should happen:

I do agree and created an issue for ODAta, which I thnik is where the issue should be fixed.

hazzik · 2020-03-31T21:17:46Z

@maca88 what about #2135?

hazzik · 2020-03-31T21:18:53Z

src/NHibernate/Linq/GroupResultOperatorExtensions.cs

-			if (expr is NewArrayExpression)
-				return (expr as NewArrayExpression).Expressions.SelectMany(ExtractKeyExpressions);
+			// --> new List<string> { x.A, x.B }
+			// --> new CustomType(x.A, x.B) { C = x.C }


Not sure if this is correct. There are examples when this assumption is not working.

public class CustomType1 { public int A => 0; public CustomType(int a) { } }

public class CustomType2 { public int A { get; } public int B { get; } public CustomType(int a, int b) { A = b; B = a; } }

It would be incorrect for Linq to Objects, but in our scenario the group by is done on the database, so the constructor of CustomType2 will never be called. In our case the following expressions will produce the same result:

GroupBy(x => new List<string> { x.A, x.B, x.C }) GroupBy(x => new CustomType(x.A, x.B) { C = x.C }) GroupBy(x => new { x.A x.B, x.C })

The idea of this method is to collect all mapped properties inside the expression and use them as keys for grouping.

I think this might be restricted to some known types (potentially a configurable option) and some heuristics (like looking for [CompilerGeneratedAttribute]).

so the constructor of CustomType2 will never be called.

What about case when group by key is selected?

GroupBy(x => new CustomType(x.A, x.B) { C = x.C }).Select(x => x.Key);

In any case I think it's better keep the code simpler and allow it without any added heuristics (and I've seen multiple requests specifically to support ctors for user DTOs for whatever legacy reasons).

There are examples when this assumption is not working.

Well it applies for all cases with user types - user can override equality methods or getters with some strange logic and that's it.

There are examples when this assumption is not working.

This concern already holds true for current master state. I do not think we should consider this as added or worsen by this PR. So I think it should be addressed in another PR, if it actually needs to be addressed.

I think this might be restricted to some known types (potentially a configurable option) and some heuristics (like looking for [CompilerGeneratedAttribute])

What would happen when the restriction is not met? For example the following query:

_session.Query<X>().GroupBy(x => new CustomType(x.A, x.B) { C = x.C }).ToList();

if the restriction is not met we could either throw an exception or execute _session.Query<X>() and do the rest on the client, which I don't thnik we should do.

like looking for [CompilerGeneratedAttribute]

In OData, the wrappers that they use have only JsonConverter attribute which I don't think we should check. The only restriction that we may add, is to check whether the type overriders Equals and GetHashCode methods and throw when it does. But even that restriction does not make much sense as grouping in Linq To Objects by a type that does not override those methods won't group any items together, where in our case it may group from zero to all items together. In my opinion we should by default allow all types and optionally having a configurable option that would throw when the given restriction is not met.

What about case when group by key is selected?

In that case the constructor will be called for each group. The following query produces the same result:

GroupBy(x => new {x.A, x.B, x.C }).Select(x => new CustomType(x.Key.A, x.Key.B) { C = x.Key.C });

When the user wants to return all groups as a list of CustomType then IMO your query is better as it is shorter to write.

to check whether the type overrides Equals and GetHashCode methods and throw when it does

I don't get it. Why? Wouldn't it throw for all anonymous types? Did you mean and throw when it **doesn't"**? If I got it right the main concern was about difference between Linq to Objects and NHibernate results. And from this context user type must override equality members to have the same results.
But there is so many ways that user can break the difference in results so I wouldn't implement any additional restrictions at all.

I don't get it. Why?

Whether we would want to prevent the user from thinking that the Equals and GetHashCode would be used for grouping.

Did you mean and throw when it doesn't"?

No, I ment when it does override those methods it would throw.

But there is so many ways that user can break the difference in results so I wouldn't implement any additional restrictions at all.

True, that is why I also agree that we should not add any restriction by default.

Also, I have to correct myself as in the following case the constructor will also be called:

_session.Query<X>().GroupBy(x => new CustomType(x.A, x.B) { C = x.C }).Select(o => new { o.Key.A, o.Key.B, o.Key.C }).ToList();

beacuse there is no explicit link in the expression between o.Key.A\o.Key.B and x.A\x.B, due to the fact that x.A and x.B are passed to the constructor.
Which means that it will work correctly even when x.A is set in the constructor to property CustomType.B.

maca88 · 2020-04-01T16:43:39Z

@maca88 what about #2135?

This PR solves it as well, added it in the description.

src/NHibernate/Linq/Visitors/TransparentIdentifierRemovingExpressionVisitor.cs

fredericDelaporte · 2020-04-13T12:04:20Z

Either we squash it all for merging, or the last three commits should be squashed together for then merging the two remaining commits. (No need to keep me as a co-author for just a todo comment, by the way.)

bahusoid · 2020-04-13T12:24:38Z

Either we squash it all for merging, or the last three commits should be squashed together for then merging the two remaining commits.

I think changes that would be no longer necessary after OData/WebApi#2108 is fixed should go as separate commit. And I think we should also add TODO in code with link to opened ODATA issue and suggestion to drop this code when it's fixed (if it's really only a hack for ODATA)

…hibernate#2221)

Co-Authored-By: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com>

bahusoid · 2020-04-13T14:26:00Z

Oh.. Somehow missed your commit. So yes I'm for your suggestion with separate commits.

maca88 · 2020-04-13T18:15:14Z

Squashed into two commits and preserved @fredericDelaporte as a co-author. I didn't tag #2334 in the commit as the issue is also about selection which is not fixed by this PR.

fredericDelaporte · 2020-04-13T18:49:02Z

So I have updated the description for avoiding closing #2334 when merging this PR.

maca88 commented Mar 6, 2020

View reviewed changes

maca88 force-pushed the GH2221 branch from 5e3278a to 104ab93 Compare March 30, 2020 15:51

maca88 commented Mar 30, 2020

View reviewed changes

maca88 changed the title ~~Add support for MemberInit expression in GroupBySelectClauseRewriter~~ Add support for OData group by queries Mar 30, 2020

maca88 mentioned this pull request Mar 30, 2020

NHibernate with AutoMapper and OData #2334

Open

hazzik reviewed Mar 30, 2020

View reviewed changes

src/NHibernate/Linq/Visitors/TransparentIdentifierRemovingExpressionVisitor.cs Show resolved Hide resolved

hazzik reviewed Mar 31, 2020

View reviewed changes

fredericDelaporte reviewed Apr 12, 2020

View reviewed changes

src/NHibernate/Linq/Visitors/TransparentIdentifierRemovingExpressionVisitor.cs Show resolved Hide resolved

fredericDelaporte added the t: Fix label Apr 13, 2020

fredericDelaporte previously approved these changes Apr 13, 2020

View reviewed changes

fredericDelaporte added this to the 5.3 milestone Apr 13, 2020

maca88 and others added 2 commits April 13, 2020 14:37

Add support for MemberInit expression in GroupBySelectClauseRewriter (n…

f47b06d

…hibernate#2221)

Add support for OData group by queries (nhibernate#2135)

66b9143

Co-Authored-By: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com>

This comment has been minimized.

Sign in to view

maca88 dismissed fredericDelaporte’s stale review via 66b9143 April 13, 2020 18:14

maca88 force-pushed the GH2221 branch from 90b3316 to 66b9143 Compare April 13, 2020 18:14

fredericDelaporte approved these changes Apr 13, 2020

View reviewed changes

fredericDelaporte merged commit aadc6ce into nhibernate:master Apr 14, 2020

fredericDelaporte added the r: Fixed label Apr 14, 2020

maca88 mentioned this pull request May 17, 2020

OData NotSupportedException MemberInit on base class member #2380

Closed

Add support for OData group by queries #2322

Add support for OData group by queries #2322

Uh oh!

Conversation

maca88 commented Mar 6, 2020 • edited by fredericDelaporte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maca88 commented Mar 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Mar 31, 2020

Uh oh!

hazzik Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maca88 commented Apr 1, 2020

Uh oh!

Uh oh!

fredericDelaporte commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bahusoid commented Apr 13, 2020

Uh oh!

This comment has been minimized.

bahusoid commented Apr 13, 2020

Uh oh!

maca88 commented Apr 13, 2020

Uh oh!

fredericDelaporte commented Apr 13, 2020

Uh oh!

Uh oh!

maca88 commented Mar 6, 2020 •

edited by fredericDelaporte

Loading

hazzik Mar 31, 2020 •

edited

Loading

bahusoid Apr 10, 2020 •

edited

Loading

bahusoid Apr 13, 2020 •

edited

Loading

fredericDelaporte commented Apr 13, 2020 •

edited

Loading