Fix custom sql loader with composite id #452

hailtondecastro · 2015-12-22T18:19:02Z

Test case and fix for NH-3079 (#1117) on branch master

hailtondecastro · 2015-12-22T18:43:58Z

Pull request posted on
https://nhibernate.jira.com/browse/NH-3079?focusedCommentId=39554&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-39554

fredericDelaporte · 2018-12-18T13:30:06Z

src/AsyncGenerator.yml

      containingTypeName: IFutureEnumerable
+    - conversion: Ignore
+      name: ComputeFlattenedParameters
+      containingTypeName: SqlQueryImpl


Not ignoring it would cause an interface change by adding an async counterpart to a method belonging to an interface.

Moreover it generates an async counterpart due to a second level cache check, which would incurs IO only if a distributed cache is enabled.
There is also an IType.GetPropertyValue call which has an async counterpart, but it is called only on component types which currently have a fully synchronous implementation for it.

fredericDelaporte · 2018-12-18T13:33:16Z

src/NHibernate.Test/NHSpecificTest/NH3079/Fixture.cs

+					session
+						.GetNamedQuery("personNoComponentSql")
+						.SetParameter(0, new PersonNoComponent { IdA = 17, IdB = 18 })
+						.SetParameter(1, new PersonNoComponent { IdA = 19, IdB = 20 })


The tests implying custom sql loaders only have one parameter. This test allows to check the change works also when multiple parameters needs flattening.

src/NHibernate/Async/Impl/AbstractQueryImpl.cs

fredericDelaporte · 2018-12-18T13:37:04Z

src/NHibernate/Impl/AbstractQueryImpl.cs

-		/// if true, the first ? will not be verified since
-		/// its needed for e.g. callable statements returning a out parameter
+		/// If true, the first positional parameter will not be verified since
+		/// its needed for e.g. callable statements returning an out parameter.


Current code base always calls this method with false. The true path looks like dead code. And maybe it was always dead, if that can be useful only for callable statement, as I do not think callable statements have been ported.

fredericDelaporte · 2018-12-18T13:42:12Z

src/NHibernate/Impl/AbstractQueryImpl.cs

+			// Values and Types may be overriden to yield refined parameters, check them
+			// instead of the fields.
+			var values = Values;
+			var types = Types;


Trying to use here ValueArray() and TypeArrays() instead wrecks CollectionFilterImpl and ExpressionFilterImp, because they override these methods and cause them to yield values failing the validation. So I had to use the Values and Types properties instead.

fredericDelaporte · 2018-12-18T13:43:57Z

src/NHibernate/Impl/SqlQueryImpl.cs

+
+		public override object[] ValueArray()
+		{
+			return _flattenedValues?.ToArray() ??


It could also be Values.Cast<object>().ToArray(). In which case it could be moved in the base class instead.

Added a TODO for 6.0 for changing Values type to IList<object>.

fredericDelaporte · 2018-12-18T13:45:47Z

I have rebased and refactored this.

Disable it for the parameters flattening method, to avoid a binary breaking change: it would add an async counterpart to an interface method. Moreover it generates an async counterpart due to a second level cache check, which would incurs IO only if a distributed cache is enabled. There is also an `IType.GetPropertyValue` call which has an async counterpart, but it is called only on component types which currently have a fully synchronous implementation for it.

fredericDelaporte

I consider this PR good for merging.
But as I have refactored it heavily, I also consider it should be approved by someone else.

maca88

Simplified FlattenTypesAndValues to use a for statement and a single list for types/values.

bahusoid · 2020-03-30T19:32:39Z

Simplified FlattenTypesAndValues to use a for statement and a single list for types/values.

Hm.. I believe I've been pushing something very similar some time ago. It seems those changes were lost by force push.. Let me check tomorrow If I also refactored something else.

fredericDelaporte · 2020-03-30T19:38:23Z

Sorry if I have missed getting your own changes before resolving the conflicts this PR had.

bahusoid

Yeap. Pretty much the only change I've made. My changes are here just for reference.

Co-authored-by: Frédéric Delaporte <12201973+fredericdelaporte@users.noreply.github.com> Co-authored-by: maca88 <bostjan.markezic@siol.net>

hazzik added the to review label Dec 30, 2015

hazzik force-pushed the master branch from e20411b to 9b7a5a3 Compare October 26, 2016 01:26

hailtondecastro mentioned this pull request Apr 28, 2017

Test case and fix for NH-3079 on branch 3.4 #451

Closed

hazzik assigned hazzik and unassigned hazzik Jul 25, 2017

nhibernate-bot mentioned this pull request Oct 12, 2017

NH-3079 - Cannot use a sql custom loader with a composite ID #1117

Closed

hazzik force-pushed the master branch from 5f01b69 to 01c2af8 Compare November 14, 2017 09:34

fredericDelaporte force-pushed the master branch from 01c2af8 to 9cbb19b Compare December 18, 2018 13:27

fredericDelaporte reviewed Dec 18, 2018

View reviewed changes

fredericDelaporte force-pushed the master branch from 9cbb19b to d2c8863 Compare December 18, 2018 13:56

fredericDelaporte changed the title ~~Test case and fix for NH-3079 on branch master~~ Fix custom sql loader with composite id Dec 20, 2018

fredericDelaporte added the t: Fix label Dec 20, 2018

fredericDelaporte added this to the 5.3 milestone Dec 20, 2018

fredericDelaporte force-pushed the master branch from fc7730b to d99c968 Compare December 23, 2018 14:05

bahusoid force-pushed the master branch from 4b984db to ba77a89 Compare September 11, 2019 16:25

Test case and fix for NH-3079 on branch master

9f57e5a

fredericDelaporte force-pushed the master branch from 5be374a to 72996be Compare March 1, 2020 17:35

fredericDelaporte added 3 commits March 1, 2020 18:47

Clean-up the test case

162b079

Refactor the fix

ca2fcb9

fredericDelaporte force-pushed the master branch from 72996be to 979aa4d Compare March 1, 2020 17:48

fredericDelaporte reviewed Mar 29, 2020

View reviewed changes

Simplify FlattenTypesAndValues method

37d8795

maca88 approved these changes Mar 30, 2020

View reviewed changes

bahusoid approved these changes Mar 31, 2020

View reviewed changes

fredericDelaporte merged commit b81e422 into nhibernate:master Mar 31, 2020

fredericDelaporte added r: Fixed and removed to review labels Mar 31, 2020

Fix custom sql loader with composite id #452

Fix custom sql loader with composite id #452

Uh oh!

Conversation

hailtondecastro commented Dec 22, 2015 • edited by fredericDelaporte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hailtondecastro commented Dec 22, 2015

Uh oh!

fredericDelaporte Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fredericDelaporte Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

maca88 Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Dec 18, 2018

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

maca88 left a comment

Choose a reason for hiding this comment

Uh oh!

bahusoid commented Mar 30, 2020

Uh oh!

fredericDelaporte commented Mar 30, 2020

Uh oh!

bahusoid left a comment • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hailtondecastro commented Dec 22, 2015 •

edited by fredericDelaporte

Loading

bahusoid left a comment •

edited

Loading