Decouple configuration of IObjectsFactory from BytecodeProvider

[maca88]

Here is a simple solution to configure IObjectsFactory separately from IBytecodeProvider, related issue. I know that having it configured statically is not the best solution, but the problem is that today, IObjectsFactory is used by many static methods that are public. If we want this in 5.2, this is the only possible solution that I see.
In addition, the IObjectsFactory has an overload with the nonPublic parameter which is not possible to implement with most of the IoC containers. The overload is currently used in only two places (AliasToBeanResultTransformer and PocoInstantiator) and all tests passes when the second argument is removed. Wouldn't it be better to obsolete that overload and let the implementor decide whether to use a non public constructor?

Even the overload with the ctorArgs is not possible to implement with some IoC containers as it is considered as a code smell. This overload is also used only two times, one time in SessionFactoryImpl which is not that easy to change because of the circular dependency between ICurrentSessionContext and ISessionFactory and the second in SchemaExport which is quite strange as the ScriptSplitter does not have any virtual members.

Fixes #1671

[hazzik]

When I wrote #1671 I had in mind exact this changes, so I just mark it as fixing #1671.

.

[hazzik]

Actually, we need to add a configuration option to be able to configure the objects factory by xml.

[fredericDelaporte]

Actually, we need to add a configuration option to be able to configure the objects factory by xml.

So, need to add something working the same way than the bytecode.provider setting.

In addition, the IObjectsFactory has an overload with the nonPublic parameter which is not possible to implement with most of the IoC containers. The overload is currently used in only two places (AliasToBeanResultTransformer and PocoInstantiator) and all tests passes when the second argument is removed. Wouldn't it be better to obsolete that overload and let the implementor decide whether to use a non public constructor?

First, better handle changes for this subject (if any) in another PR I think.

Then, the AliasToBeanResultTransformer is a special case. It uses the objects factory only when the type to create is a value type. When it is a class, it manually resolves its parameter-less constructor (public or not). This was maybe done as an optimization, for avoiding resolving the class constructor at each instantiation of a bean. But of course this means the most common case of instantiation does not use the objects factory. Then why not do a small possible breaking change by ceasing using the objects factory for structs too, by inlining its default implementation there?
I think a bean is what is called a "newable" in your link you have provided on nhibernate/NHibernate-Caches#45. That is not a thing to consider for dependency injection. (A bean is just a container for a query result element data, when we want to project the data to something else than entities while still having strong typing.)
The PocoInstantiator is very similar: it uses the objects factory only when the type is a value type (instead of testing IsClass it tests IsValueType). And likewise, entities should be considered "newables". So there too, why not dropping the usage of the objects factory too?
(The poco instantiator is used to instantiate entities.)

So in fact, should the objects factory usage be restricted to things for which dependency injection could make sens? I think yes, since there is only the two above cases which use it for "newables", and only when these "newable" are structs.

Even the overload with the ctorArgs is not possible to implement with some IoC containers as it is considered as a code smell. This overload is also used only two times, one time in SessionFactoryImpl which is not that easy to change because of the circular dependency between ICurrentSessionContext and ISessionFactory and the second in SchemaExport which is quite strange as the ScriptSplitter does not have any virtual members.

Again, better address this in another PR. But I think we could obsolete this overload.

The SessionFactoryImpl uses it to instantiate ICurrentSessionContext while providing it with the session factory. We could obsolete that way of building the session context by adding a transitional interface for session context to implement, and allowing to set once the session factory. (Of course ensuring "once" would be implementors responsibility.)

For the ScriptSplitter, the coder has surely overlooked what the objects factory is meant for by coding this nonsense. I think we can replace that call by a direct new call here. (By the way if there was an interface instead for the script splitter (or at the very least overridable methods), allowing it to be actually injected with a custom implementation, I think that instantiating it with the script to split is a bad design. It should instead have a Split method taking the script to handle.)

[maca88]

So, need to add something working the same way than the bytecode.provider setting.

I've added it in the same way as the bytecode.provider setting with the exception of adding a property in the IHibernateConfiguration interface in order to avoid breaking changes.

I agree with what you said @fredericDelaporte, I have the same thinking about how and when the IObjectsFactory should be used.

[fredericDelaporte]

I've added it in the same way as the bytecode.provider setting with the exception of adding a property in the IHibernateConfiguration interface in order to avoid breaking changes.

Can you add a // 6.0 TODO add to IHibernateConfiguration somewhere for this to be done someday?

[fredericDelaporte]

I am going to add another commit for updating the documentation. And maybe another one to avoid a breaking change.

Thinking about it, this PR is maybe only a first step. The objects factory receives as argument concrete types, which are to be configured through NHibernate settings. A next step (in another PR) could be to add a new setting telling NHibernate to use the objects factory directly on interface types, allowing for a true dependency injection logic to be put in place.

[fredericDelaporte]

To avoid a possible breaking change, we should default to using BytecodeProvider.ObjectsFactory here. But for completeness, we should also use the objects factory to create the byte-code provider...

I may add a commit sorting this out.

[fredericDelaporte]

I have added maca88#2.

[hazzik]

I'm late to the party, but CreateInstance(typeof(/*struct*/), true) does not make any sense at all as structs always have a default public constructor to the extent that user cannot even define one.

UPD: But strangely enough, the typeof(/*struct*/).GetConstructor(Type.EmptyTypes) returns null.

[fredericDelaporte]

There are definitely some more work to do with current objects factory usages, but anyway I was thinking dedicated PR were better for these related subjects.