Skip to content

Upgrade to ReLinq 2.2.0 #696

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

Merged
merged 5 commits into from
Apr 11, 2018
Merged

Conversation

ngbrown
Copy link
Contributor

@ngbrown ngbrown commented Sep 20, 2017

This should finish up NH-3990, sorry to continue to linger on that issue...

The issue was resolved on the Remotion.Linq project side by RMLNQ-115.

@ngbrown
Copy link
Contributor Author

ngbrown commented Sep 20, 2017

For more background and links to various discussions about the binding redirect issues in VS 2017 SDK style projects, see dotnet/sdk#1595 (comment)

@hazzik hazzik changed the title [WIP] NH-3990 - Reverts workaround in CompareGenerator for VB. [WIP] NH-3990 - Reverts workaround in CompareGenerator for VB Sep 21, 2017
@hazzik hazzik modified the milestones: 5.0, 5.1 Oct 5, 2017
@hazzik hazzik removed this from the 5.1 milestone Dec 23, 2017
@fredericDelaporte
Copy link
Member

Removing Blocked label since it does no more relies on a beta Remotion.Linq.

@hazzik
Copy link
Member

hazzik commented Mar 14, 2018

Something else broken.

@ngbrown
Copy link
Contributor Author

ngbrown commented Mar 23, 2018

Remotion.Linq 2.2.0 now has support for Queryable.AsQueryable extension method. See RMLNQ-28.

Without implementing the AsQueryableResultOperator.ISupportedByQueryModelVisitor marker interface it seems like there should have been no change needed because the visitor shouldn't be called. However, the tree has query IASTNode that NHibernate.Hql.Ast.ANTLR.PolymorphicQuerySourceDetector.GetClassName can't handle:

(( query ( select_from ( from ( range ( . category Products ) <generated>_1 ) ) ( select <generated>_1 ) ) ))

@ngbrown ngbrown changed the title [WIP] NH-3990 - Reverts workaround in CompareGenerator for VB NH-3990 - Reverts workaround in CompareGenerator for VB Mar 23, 2018
@ngbrown
Copy link
Contributor Author

ngbrown commented Mar 24, 2018

This is ready for review now. Both Remotion.Linq and Remotion.Linq.EagerFetching are updated to 2.2.0.

NHibernate was directly accessing the QueryModel generated by Remotion Linq and not relying solely on visitors, so the ResultOperatorRewriter needed to support the new AsQueryableResultOperator.

@hazzik hazzik added this to the 5.2 milestone Mar 28, 2018
@hazzik
Copy link
Member

hazzik commented Mar 28, 2018

Assigned to 5.2 because upgrading reference required to release a minor version.

@hazzik hazzik changed the title NH-3990 - Reverts workaround in CompareGenerator for VB Upgrade to ReLinq 2.2.0 Mar 28, 2018
@hazzik hazzik merged commit cf7247a into nhibernate:master Apr 11, 2018
@ngbrown ngbrown deleted the NH-3990-re-linq branch April 11, 2018 04:05
@cremor
Copy link
Contributor

cremor commented Jul 17, 2018

@ngbrown @hazzik Is it normal that a minor version update of ReLinq needs code changes in NHibernate? If yes, shouldn't the version contraint of the NuGet package be changed to "< 2.3.0"?

With 5.0.x and 5.1.x we now already have the situation that users of NHibernate that use the "old" style packages.config (so nearly everyone that doesn't target .NET Standard or .NET Core) are offered updates to ReLinq 2.2.0. So maybe it would also make sense to update the version constraint for the next patch releases of that versions to "< 2.2.0"?

@fredericDelaporte
Copy link
Member

Sometimes a minor version introduces new features which do not play well with old code emulating it for previous versions. Maybe it is this is the case with the AsQueryable support added in 2.2. (I have not actually analyzed why there are issues with NH5.1 used with ReLinq 2.2.)

Ideally it should not happen. We cannot predict whether this will happen again or not, be it with ReLinq or another dependency. So better stick to SemVer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants