NH-3670 - Dynamic component should allow generic dictionary #305

rjperes · 2014-08-20T10:39:57Z

https://nhibernate.jira.com/browse/NH-3670 (#755)

Created additional tests with generic properties for NH-1796, NH-1039, NH-3571 and NH-2664

oskarb · 2014-08-20T20:40:18Z

Can you check if the reference documentation section on this would benefit from an update regarding this?

Should we deprecate the use of non-generic dictionary in this context and log an issue for NH5.0 to remove support for it?

hazzik · 2014-08-26T22:43:25Z

@rjperes could you investigate the failure?

rjperes · 2014-08-27T10:55:35Z

Will do

hazzik · 2014-08-27T21:32:30Z

Also, please rebase on top of master as @oskarb pushed line endings changes.

rjperes · 2014-08-31T21:29:52Z

Fixed problem. Need some help with the rebasing part
https://nhibernate.jira.com/browse/NH-3670

oskarb · 2014-09-02T20:00:04Z

In git command line:
git checkout NH-3670
git rebase master

You will get conflicts.

I use TortoiseGit to resolve:

Open TortoiseGit commit (DON'T commit).
Double-click on each conflicted file. I find that the merge gui tool is at this point actually clever enough to automatically resolve conflicts that in fact were only line ending changes. So you may just need to click on Mark as resolved in the toolbar, or there may be residual conflicts to adjust manually.
When you've resolved all conflicts cancel the TortoiseGit commit dialog.
Use git status and git add, to make sure all modified files are scheduled for commit.
Type git rebase --continue

If all is well you can now force push the new branch, which I believe should just replace the commits in this PR with your new rebased commits.

oskarb · 2014-09-02T20:01:28Z

Don't we have any tests outside of the NHSpecificTest directory as basic feature tests for dynamic components? There should be generic versions added to them.

rjperes · 2014-09-02T21:38:08Z

Created unit tests for classes MappingByCode\MappersTests\DynamicComponentMapperTests, MappingByCode\MappersTests and MappingByCode\ExplicitMappingTests

rjperes · 2014-09-02T21:38:54Z

BTW, git rebase told me that current branch is up to date... ?

hazzik · 2014-09-21T12:48:54Z

BTW, git rebase told me that current branch is up to date... ?

Probably you did not do git fetch --all

PS: I rebased everything

fredericDelaporte

It looks to me as good modernization. Now there are some points to tackle/investigate.

src/NHibernate/Linq/Functions/DictionaryGenerator.cs

src/NHibernate/Linq/NhRelinqQueryParser.cs

fredericDelaporte · 2017-04-04T16:23:59Z

src/NHibernate/Tuple/Component/DynamicMapComponentTuplizer.cs

@@ -9,21 +13,48 @@ namespace NHibernate.Tuple.Component
 	[Serializable]
 	public class DynamicMapComponentTuplizer : AbstractComponentTuplizer
 	{
+		private readonly Mapping.Component component;


Many non underscore prefixed fields in NHibernate seems to have their root in the Java port having kept Java naming convention. Shouldn't we adopt .Net convention for our own variables at least? (I personally tend to add the _ on code I port from Java.)

fredericDelaporte · 2017-04-04T16:29:52Z

src/NHibernate/Tuple/Component/DynamicMapComponentTuplizer.cs

+				{
+					return typeof(IDictionary);
+				}
+			}


This does not look lightweight for a property getter. Maybe should this be initialized in constructor instead, or implemented with a private Lazy<System.Type>.

It looks overly complicated when we look at implementation of below DynamicMapInstantiator: typeof(IDictionary<string, object>).IsAssignableFrom(this.component.Type.ReturnedClass). What causes this simpler implementation not to be used here?

Since 5.0 is about modernization too, I think we should default to IDictionary<string, object> instead of defaulting to IDictionary. Of course this would require some changes on later files below.

What causes this simpler implementation not to be used here?

Because the this.component.Type.ReturnedClass property calls this property:

nhibernate-core/src/NHibernate/Type/ComponentType.cs

Lines 99 to 103 in 2345bb4

/// <summary></summary>

public override System.Type ReturnedClass

{

get { return ComponentTuplizer.MappedClass; }

}

So, you cannot fetch the value of this property out of the void.

fredericDelaporte · 2017-04-04T16:44:33Z

src/NHibernate/Tuple/DynamicMapInstantiator.cs

@@ -48,7 +50,14 @@ public object Instantiate()

 		protected virtual IDictionary GenerateMap()
 		{
-			return new Hashtable();
+			if ((this.component != null) && (typeof(IDictionary<string, object>).IsAssignableFrom(this.component.Type.ReturnedClass) == true))


As stated in another review, I do understand neither the added value of comparing to true, nor the unneeded parenthesis.

if (this.component != null && typeof(IDictionary<string, object>).IsAssignableFrom(this.component.Type.ReturnedClass)

Gets the job done and is more readable, isn't it?

It's just his code style, however this needs to be changed to align with our code style.

fredericDelaporte · 2017-04-04T17:05:57Z

src/NHibernate/Tuple/DynamicMapInstantiator.cs

+			else
+			{
+				return new Hashtable();
+			}


If we want to default to strong typed dictionary, we should invert the null logic here:

if (this.component == null || typeof(IDictionary<string, object>).IsAssignableFrom(this.component.Type.ReturnedClass) { return new Dictionary<string, object>(); } else { return new Hashtable(); }

We cannot - MapInstantiator is also used for EntityMode.Map and that was not migrated to generic dictionary.

fredericDelaporte · 2017-04-04T17:09:47Z

src/NHibernate/Tuple/DynamicMapInstantiator.cs

@@ -65,7 +74,7 @@ public bool IsInstance(object obj)
 			}
 			else
 			{
-				return false;
+				return obj is IDictionary<string, object>;
 			}


If defaulting to strong typed dictionary, we would have to change the logic here too:

var that = obj as IDictionary<string, object>; if (that != null) { if (entityName == null) { return true; } string type = (string)that[KEY]; return type == null || isInstanceEntityNames.Contains(type); } else { return obj is IDictionary; }

This piece code here only for EntityMode.Map instantiator.

hazzik · 2018-06-06T01:56:01Z

src/NHibernate/Tuple/Component/DynamicComponentInstantiator.cs

+	[Serializable]
+	internal class DynamicComponentInstantiator : IInstantiator
+	{
+		private readonly bool _isGeneric;


This is just for compatibility in case someone has declared a property holding the component as Hashtable instead of IDictionary. Otherwise Dictionary<string, object> when casted to IDictionary behaves the same as a Hashtable.

If we do not require/want this level of compatibility we can simplify DynamicComponentInstantiator to this:

[Serializable] internal class DynamicComponentInstantiator : IInstantiator { public object Instantiate(object id) => Instantiate(); public object Instantiate() => new Dictionary<string, object>(); public bool IsInstance(object obj) => obj is IDictionary; }

@rjperes, @fredericDelaporte your thoughts? I think this can be easily declare as possible breaking change.

A possible breaking change, yes it is, but still a grey area one. The documentation clearly states to map it as an IDictionary.

The <dynamic-component> element allows an IDictionary to be mapped as a component, where the property names refer to keys of the dictionary.

The fact it was a HashTable is an implementation detail on which the user should not rely:

Users may also plug in their own tuplizers. Perhaps you require that a System.Collections.IDictionary implementation other than System.Collections.Hashtable be used while in the dynamic-map entity-mode;

I think we can accept this possible breaking change in a minor release. (With a small update to above example in documentation.)

Users may also plug in their own tuplizers.

<dynamic-component> does not support custom tuplizers.

I have removed the _isGeneric check as it did not work in all the cases. It seems that some code depends on ComponentClass being null for <dynamic-component>

<dynamic-component> does not support custom tuplizers.

Ok, I did not realize this PR was leaving dynamic entity mode "as-is", using HashTable, and was just changing the dynamic component. This creates some kind of inconsistency, although that still remains internal implementation details.

Yes, this PR's only about dynamic component.

hazzik · 2018-06-06T01:58:05Z

src/NHibernate/Cfg/XmlHbmBinding/ClassBinder.cs

 			else if (reflectedClass != null)
 			{
 				model.ComponentClass = reflectedClass;
 				model.IsEmbedded = false;
+				model.IsDynamic = componentMapping is HbmDynamicComponent;
 			}


Change to this file can be reverted if we do not care about "invalid" property declarations (see my comment).

Old review with a bunch of false assumptions

…nent - Created additional tests with generic properties for NH-1796, NH-1039, NH-3571 and NH-2664

hazzik · 2018-06-06T12:45:27Z

This is now ready for review.

fredericDelaporte

So now we can map a dynamic component as an IDictionary<string, object>, but we can not yet query a dynamic entity as an IDictionary<string, object>. An additional PR for this would be preferable, for avoiding what could be seen as a discrepancy.

fredericDelaporte · 2018-06-06T18:32:24Z

src/NHibernate/Mapping/ByCode/Impl/CustomizersImpl/JoinCustomizer.cs

@@ -140,6 +140,14 @@ protected override void RegisterComponentMapping<TComponent>(Expression<Func<TEn
 			base.RegisterDynamicComponentMapping(property, mapping);
 		}

+		protected override void RegisterDynamicComponentMapping<TComponent>(Expression<Func<TEntity, IDictionary<string, object>>> property
+			, Action<IDynamicComponentMapper<TComponent>> mapping)


Formatting style oddness, NHibernate code normally keeps comma at end of lines rather than at start.

hazzik · 2018-06-07T04:30:38Z

@fredericDelaporte I want to make dynamic component to support dynamic property, but this is for another PR as it requires much more work.

hazzik · 2018-06-07T08:29:06Z

I’ll merge it myself

hazzik · 2018-06-07T22:38:52Z

I just realised that some of the interfaces has changed. I'll make a follow-up PR to remove the breaking changes.

oskarb changed the title ~~NH-3670~~ NH-3670 Dynamic Component Should Allow Generic Dictionary Sep 2, 2014

hazzik added this to the 4.1.0 milestone Sep 21, 2014

hazzik force-pushed the master branch from eb44257 to 4b88cc1 Compare September 21, 2014 13:52

hazzik added the to review label Feb 22, 2015

hazzik changed the title ~~NH-3670 Dynamic Component Should Allow Generic Dictionary~~ NH-3670 - Dynamic Component Should Allow Generic Dictionary Apr 22, 2015

hazzik modified the milestones: 4.2.0, 4.1.0 Aug 12, 2016

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

hazzik modified the milestones: 4.2.0, 5.0.0 Dec 1, 2016

fredericDelaporte previously requested changes Apr 4, 2017

View reviewed changes

hazzik assigned hazzik and unassigned hazzik Jul 25, 2017

hazzik added needs work and removed to review labels Sep 1, 2017

hazzik modified the milestones: 5.0, 5.1 Sep 1, 2017

hazzik reviewed Jun 6, 2018

View reviewed changes

hazzik added t: Fix and removed needs work labels Jun 6, 2018

hazzik modified the milestones: 6.0, 5.2 Jun 6, 2018

hazzik changed the title ~~NH-3670 - Dynamic Component Should Allow Generic Dictionary~~ NH-3670 - Dynamic component should allow generic dictionary Jun 6, 2018

hazzik requested a review from fredericDelaporte June 6, 2018 12:32

rjperes and others added 5 commits June 7, 2018 00:44

NH-3670 - Add ability to specify generic dictionary for dynamic compo…

3a9f260

…nent - Created additional tests with generic properties for NH-1796, NH-1039, NH-3571 and NH-2664

Generate Async tests

b248c10

Fix mapping dynamic component inside join

04983f8

Simplify DynamicComponentInstantiator

ebca62a

Update documentation

7f74a96

fredericDelaporte previously approved these changes Jun 6, 2018

View reviewed changes

Fix code style

dd1081d

hazzik dismissed fredericDelaporte’s stale review via dd1081d June 7, 2018 01:44

Merge branch 'master' into NH-3670

ad57c37

hazzik approved these changes Jun 7, 2018

View reviewed changes

fredericDelaporte approved these changes Jun 7, 2018

View reviewed changes

hazzik merged commit dc71c7d into nhibernate:master Jun 7, 2018

hazzik added the r: Fixed label Jun 7, 2018

hazzik added a commit to hazzik/nhibernate-core that referenced this pull request Jun 7, 2018

Remove a binary breaking change introduced in nhibernate#305

4ada99d

hazzik added a commit to hazzik/nhibernate-core that referenced this pull request Jun 7, 2018

Remove a binary breaking change introduced in nhibernate#305

a9c0261

hazzik added a commit to hazzik/nhibernate-core that referenced this pull request Jun 8, 2018

Remove a binary breaking change introduced in nhibernate#305

5e81ece

fredericDelaporte mentioned this pull request Mar 12, 2019

Can not create a criteria query with return type of Hashtable #2051

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NH-3670 - Dynamic component should allow generic dictionary #305

NH-3670 - Dynamic component should allow generic dictionary #305

rjperes commented Aug 20, 2014 •

edited by fredericDelaporte

Loading

oskarb commented Aug 20, 2014

hazzik commented Aug 26, 2014

rjperes commented Aug 27, 2014

hazzik commented Aug 27, 2014

rjperes commented Aug 31, 2014

oskarb commented Sep 2, 2014

oskarb commented Sep 2, 2014

rjperes commented Sep 2, 2014

rjperes commented Sep 2, 2014

hazzik commented Sep 21, 2014

fredericDelaporte left a comment

fredericDelaporte Apr 4, 2017

fredericDelaporte Apr 4, 2017

hazzik Jun 6, 2018 •

edited

Loading

fredericDelaporte Apr 4, 2017

hazzik Jun 6, 2018

fredericDelaporte Apr 4, 2017

hazzik Jun 6, 2018 •

edited

Loading

fredericDelaporte Apr 4, 2017

hazzik Jun 6, 2018

hazzik Jun 6, 2018

hazzik Jun 6, 2018 •

edited

Loading

fredericDelaporte Jun 6, 2018

hazzik Jun 6, 2018

hazzik Jun 6, 2018

fredericDelaporte Jun 6, 2018

hazzik Jun 6, 2018 •

edited

Loading

hazzik Jun 6, 2018

hazzik commented Jun 6, 2018

fredericDelaporte left a comment •

edited

Loading

fredericDelaporte Jun 6, 2018

hazzik commented Jun 7, 2018

hazzik commented Jun 7, 2018 via email •

edited

Loading

hazzik commented Jun 7, 2018

	/// <summary></summary>
	public override System.Type ReturnedClass
	{
	get { return ComponentTuplizer.MappedClass; }
	}

NH-3670 - Dynamic component should allow generic dictionary #305

NH-3670 - Dynamic component should allow generic dictionary #305

Conversation

rjperes commented Aug 20, 2014 • edited by fredericDelaporte Loading

oskarb commented Aug 20, 2014

hazzik commented Aug 26, 2014

rjperes commented Aug 27, 2014

hazzik commented Aug 27, 2014

rjperes commented Aug 31, 2014

oskarb commented Sep 2, 2014

oskarb commented Sep 2, 2014

rjperes commented Sep 2, 2014

rjperes commented Sep 2, 2014

hazzik commented Sep 21, 2014

fredericDelaporte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hazzik Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hazzik Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hazzik Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hazzik Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hazzik commented Jun 6, 2018

fredericDelaporte left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hazzik commented Jun 7, 2018

hazzik commented Jun 7, 2018 via email • edited Loading

hazzik commented Jun 7, 2018

rjperes commented Aug 20, 2014 •

edited by fredericDelaporte

Loading

hazzik Jun 6, 2018 •

edited

Loading

hazzik Jun 6, 2018 •

edited

Loading

hazzik Jun 6, 2018 •

edited

Loading

hazzik Jun 6, 2018 •

edited

Loading

fredericDelaporte left a comment •

edited

Loading

hazzik commented Jun 7, 2018 via email •

edited

Loading