Clean-up IObjectsFactory usages #1781

fredericDelaporte · 2018-07-03T13:43:55Z

IObjectsFactory is meant for instantiating NHibernate dependencies, but it was used in some cases for instantiating value types. Some of its methods are also hard or impossible to implement with many dependency injection frameworks.

Follow up to #1758

Possible breaking change:

Users providing some custom logic for instantiating value types will now need to supply their own result transformer if they were using AliasToBeanResultTransformer with value types, or their own entity tuplizer if they were using value types as entities.
Users providing some custom logic for instantiating their custom session contexts will have to implement ICurrentSessionContextWithFactory and add a parameterless public constructor to their custom context, and move their custom instantiation logic from IObjectsFactory.CreateInstance(Type, object[]) to IObjectsFactory.CreateInstance(Type).

fredericDelaporte · 2018-07-03T13:51:35Z

src/NHibernate/Tool/hbm2ddl/SchemaExport.cs

-				ScriptSplitter splitter = (ScriptSplitter)objFactory.CreateInstance(typeof(ScriptSplitter), sql);
-
-				foreach (string stmt in splitter)
+				foreach (var stmt in new ScriptSplitter(sql))


As discussed on #1758, the ScriptSplitter does not expose anything overridable, and so it cannot benefit from being instantiated by the object factory: it cannot supply a custom implementation.

fredericDelaporte · 2018-07-03T13:53:46Z

src/NHibernate/Transform/AliasToBeanResultTransformer.cs

@@ -85,7 +85,7 @@ public override object TransformTuple(object[] tuple, String[] aliases)
 			{
 				result = _resultClass.IsClass
 							? _beanConstructor.Invoke(null)
-							: Cfg.Environment.ObjectsFactory.CreateInstance(_resultClass, true);
+							: Activator.CreateInstance(_resultClass, true);


As discussed in #1758, this usage is not a dependency but a DTO instantiation. If the user wants some custom behavior for instantiating value types, he now would have to provide its own transformer.

fredericDelaporte · 2018-07-03T13:54:16Z

src/NHibernate/Tuple/PocoInstantiator.cs

@@ -103,7 +103,7 @@ private object GetInstance()
 			}
 			if (mappedClass.IsValueType)
 			{
-				return Cfg.Environment.ObjectsFactory.CreateInstance(mappedClass, true);
+				return Activator.CreateInstance(mappedClass, true);


As discussed in #1758, this usage is not a dependency but an entity instantiation. If the user wants some custom behavior for instantiating value types, he now would have to provide its own tuplizer.

I don't think that the true here is required.

Probably in some partial trust or similar environments.

I have just preferred to be quite conservative, inlining the default previous implementation "as is".

hazzik · 2018-07-04T10:55:24Z

src/NHibernate/Impl/SessionFactoryImpl.cs

@@ -1261,35 +1261,65 @@ private void Init()

 		private ICurrentSessionContext BuildCurrentSessionContext()
 		{


I would not change this method and would not change the standard contexts' constructors. Also I would make the ICurrentSessionContextWithFactory interface optional.

If this method is not changed then ICurrentSessionContextWithFactory would not have any usage and should not be kept.

So, do you mean you would not change just the direct instantiation cases?

About making ICurrentSessionContextWithFactory optional, then the obsoleted line:

(ICurrentSessionContext) Environment.ObjectsFactory.CreateInstance(implClass, this);

Would have to be replaced by:

(ICurrentSessionContext) Activator.CreateInstance(implClass, this);

In order to avoid leaving a non-obsoleted feature depending on an obsoleted one.

src/NHibernate/Context/ICurrentSessionContext.cs

hazzik · 2018-07-10T11:57:44Z

Shall we go further (in other PR) and replace IObjectsFactory with IServiceProvider?

fredericDelaporte · 2018-07-10T12:49:02Z

Shall we go further (in other PR) and replace IObjectsFactory with IServiceProvider?

Yes, we should.

hazzik · 2018-07-20T01:26:28Z

src/NHibernate/Impl/SessionFactoryImpl.cs

+
+				var context = (ISessionFactoryAwareCurrentSessionContext) Environment.ObjectsFactory.CreateInstance(implClass);
+				context.SetFactory(this);
+				return context;


I was thinking a lot about this chunk of code, but could not come up with a great solution, unfortunately.

So, I came up with this:

var implClass = ReflectHelper.ClassForName(impl); var constructor = implClass.GetConstructor(new [] { typeof(ISessionFactoryImplementor) }); ICurrentSessionContext context; if (constructor != null) { context = (ICurrentSessionContext) constructor.Invoke(new object[] { this }); } else { context = (ICurrentSessionContext) Environment.ObjectsFactory.CreateInstance(implClass); } if (context is ISessionFactoryAwareCurrentSessionContext sessionFactoryAwareContext) { sessionFactoryAwareContext.SetFactory(this); }

Ok, changed for this. (And rebased.)

IObjectsFactory is meant for instantiating NHibernate dependencies, but it was used in some cases for instantiating value types. Some of its methods are also hard or impossible to implement with many dependency injection frameworks. Follow up to nhibernate#1758

Do an alternate cleanup for session context

Rename the added interface

@hazzik

Use session context creation logic from @hazzik Co-authored-by: Alexander Zaytsev <hazzik@gmail.com>

src/NHibernate/Impl/SessionFactoryImpl.cs

Ooops

fredericDelaporte added t: Improvement c: Core p: Minor labels Jul 3, 2018

fredericDelaporte added this to the 5.2 milestone Jul 3, 2018

fredericDelaporte commented Jul 3, 2018

View reviewed changes

fredericDelaporte added the possible breaking change label Jul 3, 2018

hazzik reviewed Jul 4, 2018

View reviewed changes

fredericDelaporte force-pushed the CleanUpObjectsFactoryUsages branch from 38c4923 to 0c2f139 Compare July 4, 2018 16:46

This comment has been minimized.

Sign in to view

hazzik reviewed Jul 10, 2018

View reviewed changes

src/NHibernate/Context/ICurrentSessionContext.cs Outdated Show resolved Hide resolved

maca88 mentioned this pull request Jul 12, 2018

WIP - Replace IObjectsFactory with IServiceProvider interface #1793

Open

hazzik reviewed Jul 20, 2018

View reviewed changes

fredericDelaporte and others added 4 commits July 20, 2018 11:23

fixup! Clean-up IObjectsFactory usages

4adf041

Do an alternate cleanup for session context

fixup! Clean-up IObjectsFactory usages

37c9abb

Rename the added interface

fixup! Clean-up IObjectsFactory usages

009648f

Use session context creation logic from @hazzik Co-authored-by: Alexander Zaytsev <hazzik@gmail.com>

fredericDelaporte force-pushed the CleanUpObjectsFactoryUsages branch from 09d9902 to 009648f Compare July 20, 2018 09:24

hazzik reviewed Jul 20, 2018

View reviewed changes

src/NHibernate/Impl/SessionFactoryImpl.cs Show resolved Hide resolved

fixup! Clean-up IObjectsFactory usages

647c39e

Ooops

hazzik approved these changes Jul 20, 2018

View reviewed changes

fredericDelaporte merged commit eec2ee6 into nhibernate:master Jul 21, 2018

fredericDelaporte deleted the CleanUpObjectsFactoryUsages branch July 21, 2018 10:56

fredericDelaporte added the r: Fixed label Jul 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean-up IObjectsFactory usages #1781

Clean-up IObjectsFactory usages #1781

fredericDelaporte commented Jul 3, 2018 •

edited

fredericDelaporte Jul 3, 2018

fredericDelaporte Jul 3, 2018

fredericDelaporte Jul 3, 2018

hazzik Jul 4, 2018

hazzik Jul 4, 2018

fredericDelaporte Jul 6, 2018

hazzik Jul 4, 2018

fredericDelaporte Jul 4, 2018 •

edited

This comment has been minimized.

hazzik commented Jul 10, 2018

fredericDelaporte commented Jul 10, 2018

hazzik Jul 20, 2018

fredericDelaporte Jul 20, 2018

		@@ -1261,35 +1261,65 @@ private void Init()

		private ICurrentSessionContext BuildCurrentSessionContext()
		{

Clean-up IObjectsFactory usages #1781

Clean-up IObjectsFactory usages #1781

Conversation

fredericDelaporte commented Jul 3, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte Jul 4, 2018 • edited

Choose a reason for hiding this comment

This comment has been minimized.

hazzik commented Jul 10, 2018

fredericDelaporte commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte commented Jul 3, 2018 •

edited

fredericDelaporte Jul 4, 2018 •

edited