NH-3944 - Upgrade Remotion.Linq to v2.1 #568

hazzik · 2017-03-16T02:31:14Z

Fix package build (release).

fredericDelaporte · 2017-03-16T15:22:11Z

Great job, looks like it was a bit a heavy task. Probably better to merge it soon for allowing pending Linq related PR to get re-based and adapted to those incoming changes.

fredericDelaporte · 2017-04-05T15:56:07Z

Is there any clean up you have thought of I may do?

hazzik · 2017-04-05T22:45:53Z

I actually do not like all these new INh... stuff (INhQueryModelVisitor, NhHavingClause, etc). Now it looks like we are using relinq improperly.

fredericDelaporte · 2017-04-09T16:36:59Z

I have tried to understand what is going on.

Currently, I think Relinq provides a Linq model meant to represent the Linq query, and a visitor logic to transform this model to the target (HQL tree for NHibernate).

The model supports being modified during visits. But NHibernate not only modifies it but also extends its semantic with clauses unknown from Linq (having, with, the join accepting with instead of where).

This does not look expected by Relinq designers. And doing that is harder with v2 since it has sealed a bunch of classes NHibernate was deriving from for adding and handling those additional clauses.

If we want to cease tinkering the model with additional non Linq semantic, I think we need to remove rewriters like AggregatingGroupByRewriter and handle what they were doing during the final model visit.

Maybe it is not even feasible. But it looks to me we would have easier trying to do that with current master implementation, where those additional semantics were not needing a lot of supporting code, rather than with this current migration to Relinq v2.

(And maybe there are some other issues I have not spotted currently.)

hazzik · 2017-04-09T23:38:56Z

Agreed

hazzik · 2017-05-07T11:51:30Z

@fredericDelaporte I'm going to update this PR with Expression Trees v2 Spec. At this stage, I would prefer to leave the code as close to the original as possible. It will make #594, #599 obsolete. Later if we'd think that the work in #594 is needed we can re-apply it.

There is DbOnly attribute introduced in #599. I would like it to be raised as a separate Jira. But, also, I think that it's a purpose of LinqExtensionMethodAttribute.

fredericDelaporte · 2017-05-07T12:25:58Z

Ok, closing #594 and #599. I will add a Jira for DbOnly (NH-4009). Making it a purpose of LinqExtensionMethod requires to have an additional case for specifically handling MappedAs, hence my choice to have it separated, but why not.

fredericDelaporte · 2017-05-07T12:44:38Z

DbOnly is usable on other user extensions not being a simple sql function call too. Providing it only through LinqExtensionMethod would be more limiting than having it as standalone.

We may keep it while adding an option on LinqExtensionMethod for enabling it to control that itself.

fredericDelaporte · 2017-05-07T15:27:29Z

src/NHibernate/Linq/EagerFetchingExtensionMethods.cs

@@ -15,7 +15,7 @@ public static class EagerFetchingExtensionMethods
            if (query == null) throw new ArgumentNullException(nameof(query));
            if (relatedObjectSelector == null) throw new ArgumentNullException(nameof(relatedObjectSelector));

-            var methodInfo = ((MethodInfo)MethodBase.GetCurrentMethod()).MakeGenericMethod(typeof(TOriginating), typeof(TRelated));
+	        var methodInfo = ((MethodInfo)MethodBase.GetCurrentMethod()).MakeGenericMethod(typeof(TOriginating), typeof(TRelated));


Series of four partial conversion from space to tabs. Better convert all undue spaces of the line (or even of the file).

Probably rebase glitch.

fredericDelaporte · 2017-05-07T17:16:48Z

src/NHibernate/Linq/Clauses/NhHavingClause.cs

 		{
+			if (visitor == null) throw new ArgumentNullException(nameof(visitor));
+			if (queryModel == null) throw new ArgumentNullException(nameof(queryModel));
+			((INhQueryModelVisitor) visitor).VisitNhHavingClause(this, queryModel, index);


Smell. I think we should open an issue on ReLinq side about their choice of restricting extensibility. Maybe will they point us toward a better of handling that, or reconsider and enable it again.

Check this out: https://www.re-motion.org/jira/browse/RMLNQ-86. In short they added IBodyClause and IFromClause because of us.

fredericDelaporte · 2017-05-07T17:19:21Z

src/NHibernate/Linq/Clauses/NhJoinClause.cs

+			if (queryModel == null) throw new ArgumentNullException(nameof(queryModel));
+			var nhVisitor = visitor as INhQueryModelVisitor;
+			if (nhVisitor == null)
+				throw new ArgumentException("Expect visitor to implement INhQueryModelVisitor", nameof(visitor));


Better than previous but still not much satisfactory. A ReLinq owner point of view on the subject would be great.

I'll align all these with the same code style

fredericDelaporte · 2017-05-07T17:21:24Z

src/NHibernate/Linq/Clauses/NhWithClause.cs

+		{
+			if (visitor == null) throw new ArgumentNullException(nameof(visitor));
+			if (queryModel == null) throw new ArgumentNullException(nameof(queryModel));
+			((INhQueryModelVisitor)visitor).VisitNhWithClause(this, queryModel, index);


Same as previous.

fredericDelaporte · 2017-05-07T17:24:50Z

src/NHibernate/Linq/GroupBy/GroupBySelectClauseRewriter.cs

-				NhCountExpression countExpression;
-				if (where != null && (countExpression = expression.QueryModel.SelectClause.Selector as NhCountExpression) !=
-				null && countExpression.Expression.NodeType == (ExpressionType)NhExpressionType.Star)
+				if (@where != null &&


I tend to remove @ "keyword escaping" prefix when it works without it.

fredericDelaporte · 2017-05-07T17:48:41Z

src/NHibernate/Linq/Visitors/QueryModelVisitor.cs

 			{
 				// It's a join
 				_hqlTree.AddFromClause(
 					_hqlTree.TreeBuilder.Join(
-						HqlGeneratorExpressionTreeVisitor.Visit(fromClause.FromExpression, VisitorParameters).AsExpression(),
+						HqlGeneratorExpressionVisitor.Visit(fromClause.FromExpression, VisitorParameters).AsExpression(),
 						_hqlTree.TreeBuilder.Alias(querySourceName)));
 			}
 			else
 			{
 				// TODO - exact same code as in MainFromClause; refactor this out


Was done here, with minimal changes. Maybe should be redone in this branch too.

fredericDelaporte · 2017-05-07T17:59:00Z

src/NHibernate/Linq/Visitors/SelectClauseNominator.cs

+			         expression is NhCountExpression ||
+			         expression is NhAverageExpression ||
+			         expression is NhMaxExpression ||
+			         expression is NhMinExpression)


I was keeping the NhNodeType for avoiding these kind of cast series, which I believe to be slower than a single cast followed by enum comparison. (And same for the new C#7 casting switch, but maybe I am wrong for him.) But I have not done any bench-marking for checking this was having a measurable impact.

I tend to not space align anything since this forbid auto-formatting (ctrl+k, ctrl+d) if we want to preserve such space alignements. Fallback on manual re-formatting instead is not a way to go for me. Tab align is not good for GitHub, so I just do not align, I simple indent (eventually putting the start on its own line if this really help reading to align).

Cast should be as cheap as the equal.

The alignment was discussed with @oskarb. The consensus was that we use tabs for indents and spaces (after tabs) to align. This way it looks the same everywhere despite the size of a tab.

The default VS tooling does not support this well. As soon as we have written some code (or specifically, pasted some java code) and want to auto-fix the formatting, it switches those spaces alignements to tabs in the whole file.

Maybe I have mis-configured the tooling, but then I am unable to find the right option for preserving such formatting. (And I have tried with R# too.)

hazzik · 2017-05-15T01:30:48Z

Cleaned up, rebased, and squashed.

fredericDelaporte

LGTM

ngbrown · 2017-06-11T03:16:13Z

Anything holding this back from being merged?

hazzik · 2017-06-11T03:47:18Z

I would like to wait until the release of "fetching"

ngbrown · 2017-06-12T20:58:23Z

@hazzik Remotion.Linq.EagerFetching v2.0.1 should be fine, because there's not actually any code changes to v2.1/2.2 just compiling for .NETStandard.

hazzik · 2017-06-12T21:02:12Z

I don't like binding redirects

…

On Tue, 13 Jun 2017 at 8:58 AM, Nathan Brown ***@***.***> wrote: @hazzik <https://github.com/hazzik> Remotion.Linq.EagerFetching v2.0.1 <https://www.nuget.org/packages/Remotion.Linq.EagerFetching/2.0.1> should be fine, because there's not actually any code changes to v2.1/2.2 just compiling for .NETStandard. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#568 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI1lyS_N45zF_t1YbULm7Fd6Pv2ZDDKks5sDaZwgaJpZM4MexTJ> .

fredericDelaporte · 2017-06-13T10:55:58Z

Even with the beta 2.1 those binding redirects stays required, although it directly depends on the ReLinq 2.1 we use.

hazzik · 2017-06-18T22:37:20Z

Even with the beta 2.1 those binding redirects stays required

Why is that?

fredericDelaporte · 2017-06-19T09:52:33Z

I do not know. I have just upgraded to the beta lib, seen the bindings were still there, removed them, try testing, failure trying to load Relinq 2.0 instead of 2.1. I have not taken yet the time to enable binding failure trace and check what was going on.

hazzik · 2017-06-19T11:49:12Z

Ok, there is a broken reference in the beta.

hazzik · 2017-06-19T12:28:35Z

https://www.re-motion.org/jira/browse/RMLNQFETCH-21

hazzik · 2017-06-26T21:52:19Z

Upgraded to re-linq 2.1.2, re-linq fetch 2.1.0. Rebased & squashed.

@fredericDelaporte please review.

fredericDelaporte

An unneeded dependency should be removed in main project and test project, see comment details.

Some xml comments are wrong, see comment details.

Some small additional refactoring could be done, see comment details.

C# 7 syntax could be used to reduce some new code size. But well, all that date back a bit and I would not mandate this change. What do you think about it?

fredericDelaporte · 2017-06-27T12:41:17Z

src/NHibernate/NHibernate.csproj

    </Reference>
    <Reference Include="System" />
+    <Reference Include="System.ComponentModel.Composition" />


This dependency looks unneeded now. Removing it, removing Remotion Nuget packages and re-adding those packages do not add it back. And tests still works without it.

fredericDelaporte · 2017-06-27T12:41:56Z

src/NHibernate.Test/NHibernate.Test.csproj

    <Reference Include="System" />
+    <Reference Include="System.ComponentModel.Composition" />


This dependency looks unneeded now. Removing it, removing Remotion Nuget packages and re-adding those packages do not add it back. And tests still works without it.

fredericDelaporte · 2017-06-27T12:54:31Z

src/NHibernate/Linq/Clauses/NhJoinClause.cs

+		/// <summary>
+		///     Accepts the specified visitor by calling its
+		///     <see
+		///         cref="M:Remotion.Linq.IQueryModelVisitor.VisitAdditionalFromClause(Remotion.Linq.Clauses.AdditionalFromClause,Remotion.Linq.QueryModel,System.Int32)" />


Irrelevant comment. Here we call VisitNhJoinClause.

fredericDelaporte · 2017-06-27T12:58:07Z

src/NHibernate/Linq/Clauses/NhJoinClause.cs

 		public NhJoinClause(string itemName, System.Type itemType, Expression fromExpression)
 			: this(itemName, itemType, fromExpression, new NhWithClause[0])
 		{
 		}

+		/// <summary>
+		///     Initializes a new instance of the <see cref="T:Remotion.Linq.Clauses.NhJoinClause" /> class.


Bad namespace in the cref. The T: prefix seems to disable IDE checks.

fredericDelaporte · 2017-06-27T13:00:55Z

src/NHibernate/Linq/Clauses/NhJoinClause.cs

+		///     uniquely identifying <see cref="T:Remotion.Linq.Clauses.IQuerySource" /> objects. To match an
+		///     <see cref="T:Remotion.Linq.Clauses.IQuerySource" /> with its references, use the
+		///     <see cref="P:Remotion.Linq.Clauses.Expressions.QuerySourceReferenceExpression.ReferencedQuerySource" /> property
+		///     rather than the <see cref="P:Remotion.Linq.Clauses.NhFromClauseBase.ItemName" />.


Invalid cref. But not even sure the whole remarks section is useful here.

fredericDelaporte · 2017-06-27T13:03:31Z

src/NHibernate/Linq/Clauses/NhJoinClause.cs

+		///     Gets or sets the type of the items generated by this from clause.
+		/// </summary>
+		/// <note type="warning">
+		///     Changing the <see cref="P:Remotion.Linq.Clauses.NhFromClauseBase.ItemType" /> of a


Invalid cref.

fredericDelaporte · 2017-06-27T13:18:34Z

src/NHibernate/Linq/LinqExtensionMethods.cs

@@ -112,7 +112,7 @@ public static IEnumerable<TSource> ToFuture<TSource>(this IQueryable<TSource> so
 		/// <exception cref="T:System.ArgumentNullException"><paramref name="source" /> is <see langword="null"/>.</exception>
 		/// <exception cref="T:System.NotSupportedException"><paramref name="source" /> <see cref="IQueryable.Provider"/> is not a <see cref="INhQueryProvider"/>.</exception>
 		public static IFutureValue<TSource> ToFutureValue<TSource>(this IQueryable<TSource> source)
-		{
+			{


Rebase glitch?

fredericDelaporte · 2017-06-27T13:31:19Z

src/NHibernate/Linq/Visitors/QuerySourceLocator.cs

+			if (_type.IsAssignableFrom(joinClause.ItemType))
+			{
+				if (_querySource == null)


Looks as an additional change, for which I do not get the rational. Is it fixing an untested case?

It was here, see L30

Ok, got confused by reading VisitMainFromClause instead of VisitAdditionalFromClause, sorry.

fredericDelaporte · 2017-06-27T13:36:41Z

src/NHibernate/Linq/Visitors/WhereJoinDetector.cs

+				{
+					_joiner.MakeInnerIfJoined(memberExpression);
+				}
+			}


From L100 till here, could be refactored in a method with code from L84 to L93.

I'll do it later

fredericDelaporte · 2017-07-11T11:45:17Z

I haven't seen you had made changes and so have not reviewed it again for having the merge "green". I you rather have an accepted review, feel free to "ping" me after changes or tell it is good enough to go without more changes.

hazzik force-pushed the NH-3944 branch from 87ebf1f to ee084dc Compare March 16, 2017 03:20

fredericDelaporte mentioned this pull request Mar 18, 2017

NH-3961 - Linq auto-parameterization failures #570

Merged

hazzik force-pushed the NH-3944 branch 2 times, most recently from 9df2fc4 to 5fd7550 Compare March 20, 2017 04:10

hazzik added the to clean up label Mar 23, 2017

hazzik added the needs work label Apr 9, 2017

This was referenced Apr 10, 2017

NH-3944 - preliminary work, Nh clauses eliminated. #594

Closed

NH-3944 - Relinq v2 migration #599

Closed

hazzik force-pushed the NH-3944 branch from 5fd7550 to bddc2df Compare May 7, 2017 12:52

hazzik requested a review from fredericDelaporte May 7, 2017 14:00

hazzik removed the needs work label May 7, 2017

fredericDelaporte reviewed May 7, 2017

View reviewed changes

hazzik force-pushed the NH-3944 branch 3 times, most recently from 9f18308 to 0909592 Compare May 13, 2017 12:36

fredericDelaporte previously approved these changes May 15, 2017

View reviewed changes

ngbrown mentioned this pull request May 28, 2017

NH-3807 - Support for .NET Core 2.0 #633

Closed

29 tasks

hazzik dismissed fredericDelaporte’s stale review via b6c2e28 June 26, 2017 00:03

hazzik force-pushed the NH-3944 branch from 9b4860a to 1e0d618 Compare June 26, 2017 21:50

hazzik removed the to clean up label Jun 26, 2017

fredericDelaporte reviewed Jun 27, 2017

View reviewed changes

Update Remotion.Linq to 2.1

5cb7b3f

hazzik force-pushed the NH-3944 branch from 47c986b to 5cb7b3f Compare July 11, 2017 00:55

hazzik merged commit 5cb7b3f into nhibernate:master Jul 11, 2017

hazzik added this to the 5.0 milestone Aug 4, 2017

hazzik added r: Fixed t: Improvement labels Aug 4, 2017

hazzik deleted the NH-3944 branch August 4, 2017 00:03

maca88 mentioned this pull request Feb 11, 2021

Fix Queryable.Contains method for Linq provider #2674

Merged

		<Reference Include="System" />
		<Reference Include="System.ComponentModel.Composition" />

NH-3944 - Upgrade Remotion.Linq to v2.1 #568

NH-3944 - Upgrade Remotion.Linq to v2.1 #568

Uh oh!

Conversation

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

Uh oh!

Uh oh!

fredericDelaporte commented Mar 16, 2017

Uh oh!

fredericDelaporte commented Apr 5, 2017

Uh oh!

hazzik commented Apr 5, 2017

Uh oh!

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

Uh oh!

Uh oh!

hazzik commented Apr 9, 2017

Uh oh!

hazzik commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented May 7, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik May 7, 2017 • 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!

fredericDelaporte May 7, 2017 • 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!

hazzik May 7, 2017 • 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!

hazzik commented May 15, 2017

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

ngbrown commented Jun 11, 2017

Uh oh!

hazzik commented Jun 11, 2017

Uh oh!

ngbrown commented Jun 12, 2017

Uh oh!

hazzik commented Jun 12, 2017 via email

Uh oh!

fredericDelaporte commented Jun 13, 2017

Uh oh!

hazzik commented Jun 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Jun 19, 2017

Uh oh!

hazzik commented Jun 19, 2017

Uh oh!

hazzik commented Jun 19, 2017

Uh oh!

hazzik commented Mar 16, 2017 •

edited

Loading

fredericDelaporte commented Apr 9, 2017 •

edited

Loading

hazzik commented May 7, 2017 •

edited

Loading

fredericDelaporte commented May 7, 2017 •

edited

Loading

hazzik May 7, 2017 •

edited

Loading

fredericDelaporte May 7, 2017 •

edited

Loading

hazzik May 7, 2017 •

edited

Loading

hazzik commented Jun 18, 2017 •

edited

Loading