NH-3722 - Remove entity mode switching capability #579

hazzik · 2017-03-23T11:18:10Z

Ported from HHH-6330 hibernate/hibernate-orm@4a4f636
ISession.GetSession is renamed to ISession.GetChildSession

hazzik · 2017-03-24T03:03:14Z

src/NHibernate/Cfg/SettingsFactory.cs

@@ -268,11 +268,6 @@ public Settings BuildSettings(IDictionary<string, string> properties)
 			log.Info("Default flush mode: " + defaultFlushMode);
 			settings.DefaultFlushMode = defaultFlushMode;

-			EntityMode defaultEntityMode =
-				EntityModeHelper.Parse(PropertiesHelper.GetString(Environment.DefaultEntityMode, properties, "poco"));
-			log.Info("Default entity-mode: " + defaultEntityMode);


Here, it's better to warn that default entity mode is deprecated rather then silently remove it.

hazzik · 2017-03-24T03:04:37Z

src/NHibernate/ISession.cs

 		/// <returns>The new session</returns>
-		ISession GetSession(EntityMode entityMode);
+		ISession GetChildSession();


Need to add this method to 4.1.x and deprecate old one.

fredericDelaporte

Are we removing the dynamic-map mode altogether? I do not see how we are supposed to activate it since default_entity_mode parameter is removed. On Hibernate side, it looks like they are still supporting it.

Reference documentation on dynamic models needs to be either entirely removed, or amended for explaining how to use dynamic-map mode now. (And for removing reference to default_entity_mode configuration parameter, which is removed.)

It looks to me it is still there but handled at the mapping level, depending on class name presence.

nhibernate-mapping.xsd: still references xml entity mode (line 1435). And there is a number of embed-xml attributes which should be removed if they were for the removed embedded xml. (I do not find any reference to those attributes, even in master.)

src/UpgradeLog2.htm: that file is probably an unexpected add.

fredericDelaporte · 2017-03-24T02:01:36Z

src/NHibernate/Criterion/Example.cs

@@ -429,7 +417,7 @@ protected void AddPropertyTypedValue(object value, IType type, IList list)
 					}
 					value = stringValue;
 				}
-				list.Add(new TypedValue(type, value, EntityMode.Poco));
+				list.Add(new TypedValue(type, value));
 					// TODO NH Different behavior: In H3.2 EntityMode is nullable


Obsolete code comment which should be removed.

fredericDelaporte · 2017-03-24T02:22:10Z

src/NHibernate/Impl/SessionImpl.cs

@@ -2237,26 +2226,18 @@ public ISession GetSession(EntityMode entityMode)

 				if (rootSession != null)
 				{
-					return rootSession.GetSession(entityMode);
+					return rootSession.GetChildSession();
 				}

 				CheckAndUpdateSessionStatus();

 				ISession rtn = null;


rtn should be removed, see below.

fredericDelaporte · 2017-03-24T02:24:11Z

src/NHibernate/Impl/SessionImpl.cs

-					childSessionsByEntityMode.Add(entityMode, rtn);
+					log.Debug("Creating child session.");
+					rtn = new SessionImpl(this);
+					_childSession = rtn;
 				}

 				return rtn;


Should return _childSession instead of rtn. Otherwise, calling GetChildSession again would yield null instead of yielding previously created child session.

fredericDelaporte · 2017-03-24T02:34:54Z

src/NHibernate/Type/IType.cs

@@ -103,7 +100,7 @@ public interface IType : ICacheAssembler
 		/// <include file='IType.cs.xmldoc' 
 		///		path='//members[@type="IType"]/member[@name="M:IType.DeepCopy"]/*'
 		/// /> 
-		object DeepCopy(object val, EntityMode entityMode, ISessionFactoryImplementor factory);
+		object DeepCopy(object val, ISessionFactoryImplementor factory);


IType.cs.xmldoc file still refers to entityMode parameter.

fredericDelaporte · 2017-03-24T02:39:47Z

src/NHibernate/Type/IType.cs

 		/// <returns> boolean </returns>
-		bool IsEqual(object x, object y, EntityMode entityMode);
+		bool IsEqual(object x, object y);


Conflicts with IVersionType.IsEqual. If their are supposed to have the same semantic (which I think is the case), IVersionType.IsEqual should be removed since it would then inherit it from IType.

fredericDelaporte · 2017-03-24T03:05:17Z

src/NHibernate/Persister/PersisterFactory.cs

-				if (pc == null)
-				{
-					use4Parameters = true;
-					pc = persisterClass.GetConstructor(CollectionPersisterConstructor2Args);


Could this removal be a breaking change? (Is it possible for NHibernate users to supply custom persister to NHibernate?)
If yes, they may not discover it only by compiling since this works by reflection.

It is, it needs to be deprecated through additional release of 4.x version

fredericDelaporte · 2017-03-24T03:26:33Z

src/NHibernate/Type/MetaType.cs

-		{
-			return FromXMLString(xml.Value, factory);
-		}
-
 		public object FromXMLString(string xml, IMapping factory)


FromXMLString looks obsolete, no references found to it. A bit same for ToXMLString, only serves for ToLoggableString which could directly take ToXmlString implementation instead.

fredericDelaporte · 2017-03-24T03:31:05Z

src/NHibernate/Type/NullableType.cs

-		{
-			xml.InnerText = ToXMLString(value, factory);
-		}
-
 		public string ToXMLString(object value, ISessionFactoryImplementor pc)


Same as corresponding methods of MetaType: looks obsolete.

hazzik · 2017-03-24T03:44:57Z

Are we removing the dynamic-map mode altogether? I do not see how we are supposed to activate it since default_entity_mode parameter is removed. On Hibernate side, it looks like they are still supporting it.

The entity mode is supported only on a entity/component level, eg. entity/component has to be defined as dynamic-map, to be able to use it. Please see DynamicClassFixture.

Default entity mode was passed only to a SessionImpl constructor, so it's not needed and do not have any effect now.

The drivers for the changes are ComponentMetamodel and EntityMetamodel: replace EntityModeToTuplizerMapping with static ITuplizer and remove all unused EntityModes and you'll be in the current state.

src/UpgradeLog2.htm: that file is probably an unexpected add.

Sure.

hazzik · 2017-03-24T03:57:37Z

nhibernate-mapping.xsd: still references xml entity mode (line 1435).

I'm not sure how shall we handle xsd schema changes.

hazzik · 2017-03-24T04:37:33Z

And there is a number of embed-xml attributes which should be removed if they were for the removed embedded xml. (I do not find any reference to those attributes, even in master.)

Because the EntityMode.Xml was never actually implemented

hazzik · 2017-03-24T04:38:52Z

Code issues addressed. I'll update documentation later

fredericDelaporte · 2017-03-24T15:23:28Z

I'm not sure how shall we handle xsd schema changes.

Me too, but it looks to me your 2 commits have done it right. I guess changing the xsd namespace version would be a hassle for migration, requiring everyone to change it in their existing mappings.

Great work by the way, code base simplification is always welcome.

* Ported from HHH-6330 hibernate/hibernate-orm@4a4f636 * ISession.GetSession is renamed to ISession.GetChildSession

…ntation

…rsister * Ported HHH-7736 from hibernate/hibernate-orm@1a5bdd9

* Ported from HHH-10073 hibernate/hibernate-orm@1376b12

* Ported from HHH-10073 hibernate/hibernate-orm@47b8ed5

fredericDelaporte · 2017-03-26T20:59:47Z

On the subject of methods to obsolete, I wonder if this is not a bit late to obsolete them in 4.x for suppressing them completely in 5.
Many users may step over last 4.x versions for upgrading directly to 5.
Maybe should we put them back in 5 but marked obsolete, and remove them only in 6. They may already no more be able of mimicking their old behavior, but at least the user will have a warning telling what to do instead of having a compilation error.

hazzik · 2017-03-26T22:32:22Z

I think we can do it for ISession.GetSession(EntityMode). For other methods this does not really make a lot of sense. The thing is, that we are now (in 5.0) in an unique position to reshuffle the API as we need to be able to add new features in the future.

hazzik force-pushed the NH-3722 branch 2 times, most recently from 953ad31 to 6932f76 Compare March 23, 2017 22:46

hazzik added the needs work label Mar 23, 2017

hazzik force-pushed the NH-3722 branch from 6932f76 to 563f228 Compare March 23, 2017 23:29

hazzik requested a review from fredericDelaporte March 23, 2017 23:42

hazzik removed the needs work label Mar 24, 2017

hazzik commented Mar 24, 2017

View reviewed changes

fredericDelaporte requested changes Mar 24, 2017

View reviewed changes

hazzik added the needs work label Mar 24, 2017

fredericDelaporte approved these changes Mar 24, 2017

View reviewed changes

hazzik force-pushed the NH-3722 branch 3 times, most recently from 7ed466c to 3c89e7e Compare March 25, 2017 06:21

hazzik removed the needs work label Mar 25, 2017

hazzik added 8 commits March 27, 2017 09:14

NH-3722 - Remove entity mode switching capability

13081b2

* Ported from HHH-6330 hibernate/hibernate-orm@4a4f636 * ISession.GetSession is renamed to ISession.GetChildSession

NH-3722 - Remove per-session EntityMode switch capability from docume…

50bafb3

…ntation

NH-3722 - Remove EntityMode.Xml related properties from ICollectionPe…

fba9c79

…rsister * Ported HHH-7736 from hibernate/hibernate-orm@1a5bdd9

NH-3722 - Not propagating XML node names configured in mappings

fd1b81a

* Ported from HHH-10073 hibernate/hibernate-orm@1376b12

NH-3722 - Remove IsEmbedded

0dbacba

* Ported from HHH-10073 hibernate/hibernate-orm@47b8ed5

Fix schema for NH-2053

cd02df4

NH-3722 - Remove xml entity mode and embed-xml from the schema

acd305d

NH-3722 - Remove mentions of default_entity_mode from documentation

ba6576b

hazzik force-pushed the NH-3722 branch from 3c89e7e to ba6576b Compare March 26, 2017 20:14

hazzik merged commit 7ba52d2 into nhibernate:master Mar 26, 2017

fredericDelaporte added this to the 5.0.0 milestone Mar 26, 2017

hazzik deleted the NH-3722 branch April 6, 2017 22:57

hazzik added the r: Fixed label Aug 3, 2017

fredericDelaporte added the t: Improvement label Sep 7, 2017

fredericDelaporte mentioned this pull request Jun 24, 2018

Obsolete unused "xml" type methods #1771

Merged

NH-3722 - Remove entity mode switching capability #579

NH-3722 - Remove entity mode switching capability #579

Uh oh!

Conversation

hazzik commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 24, 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!

fredericDelaporte Mar 24, 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 Mar 24, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

hazzik commented Mar 24, 2017

Uh oh!

hazzik commented Mar 24, 2017

Uh oh!

hazzik commented Mar 24, 2017

Uh oh!

fredericDelaporte commented Mar 24, 2017

Uh oh!

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

Uh oh!

Uh oh!

hazzik commented Mar 26, 2017

Uh oh!

Uh oh!

fredericDelaporte Mar 24, 2017 •

edited

Loading

fredericDelaporte Mar 24, 2017 •

edited

Loading

fredericDelaporte Mar 24, 2017 •

edited

Loading

hazzik commented Mar 24, 2017 •

edited

Loading

fredericDelaporte commented Mar 26, 2017 •

edited

Loading