Hierarchical Contexts broken by change to DefaultListableObjectFactory

[cherrydev]

It seems that as of M2, if you use VariablePlaceholderConfigurer in combination with any child context, your app is now broken. There is no work-around other than removing all child contexts or removing all VariablePlaceholderConfigurers.

[This][https://github.com/SpringSource/spring-net/blob/d8ff77d99941a2cf46e300c2f8dd080d58f7d18d/src/Spring/Spring.Core/Objects/Factory/Support/DefaultListableObjectFactory.cs] change to DefaultListableObjectFactory broke VariablePlaceholderConfigurer.

in VariablePlaceholderConfigurer.cs:

IList<string> objectDefinitionNames = factory.GetObjectDefinitionNames();
            for (int i = 0; i < objectDefinitionNames.Count; ++i)
            {
                string name = objectDefinitionNames[i];
                IObjectDefinition definition = factory.GetObjectDefinition( name );
                try
                {
                    visitor.VisitObjectDefinition( definition );

You can see that it uses GetObjectDefinitionNames() to list all of the definition names and then GetObjectDefinition() to get the definitions. GetObjectDefinitionNames() USED to only return definitions in the current context, but now it returns definitions in parent contexts as well. At a glance, I can't see why this was done, except it simplified (and I suppose obsoleted) ObjectFactoryUtils.ObjectNamesForTypeIncludingAncestors.

The problem now, though, is that there is no obvious way to retrieve the names of the definitions in the objectfactory without including the parents. A further problem seems to be that the related GetObjectDefinitionNames(Type type) method does NOT respect hierarchy, so now the two identically-named methods are very inconsistent in behaviour. And, of course, any code that behaves like the VariablePlaceholderConfigurer is now broken when using ancestor contexts.

My suggestions, in order of preference would be:

1. Roll back the change to the behaviour of DefaultListableObjectFactory.GetObjectDefinitionNames() to the old one and add an overloaded method that takes a bool argument, just like GetObjectDefinition() does. Modify ObjectFactoryUtils.ObjectNamesForTypeIncludingAncestors to call the overloaded version. I do not yet know what other new code relies on the new behaviour of GetObjectDefinitionNames(), if any.
2. Modify VariablePlaceholderConfigurer to ignore null values returned from GetObjectDefinition(). (Obviously this is probably a terrible idea).

I'm going to assume that the first option is the way to go, examine any calls to GetObjectDefinitionNames and make sure that none of them rely on the new behaviour. I would write a test that demonstrates the broken behaviour, but I'm not sure if it's even possible to construct a parent/child context with the integration test support, which is what's required in order to demonstrate the problem.

EDIT:

I've found the following additional code that will break if using the >=M2 version of DefaultListableObjectFactory:

ConfigurationClassPostProcessors.EnhanceConfigurationClasses: NRE on line 133
PropertyPlaceholderConfigurer.ProcessProperties: NRE within call on line 243
VariablePlaceholderConfigurer.ProcessProperties: NRE within call on line 256

Another question now presents itself:
Is it necessary to change the IConfigurableListableObjectFactory interface to provide the option of including ancestors? Is it necessary to modify GetObjectNamesForType and related methods to include those options? All of those methods call GetObjectDefinitionNames() and therefore now all behave differently than they used to, though it's reasonable to assume that the old behaviour was incorrect in some cases, especially since DoGetObjectNamesForType explicitly requests object definitions from ancestors.

Considering all of this, it MIGHT actually be better at this point to modify the three above broken classes to ignore null returns from GetObjectDefinition() since otherwise the effects of fixing this are wider and harder to predict and test.

Incidentally, this does appear to demonstrate a weakness in the testing that can be done when working in child contexts.

[thomast74]

I looked at all parts of the code that is using this method and there are more areas then the one you mentioned that are affected by a potential NPE exception because GetObjectDefinitionNames is returning from all parents but when try to get ObjectDefinitions is only looking in the current context.

Something is clearly a bit out of sync.

Will check what the Java version is doing and we see form there.

[cherrydev]

It would seem that we have a fairly clear distinction between components that want to access the whole hierarchy and ones that only want to access their local part:

When doing post-processing, each post-processor in any local or higher context runs separately in every sub-context. Therefore, it will only want to look in its local part each time it's run so that it's not processing the same objects multiple times, and not processing objects in higher contexts when it is defined only in a lower context.

When RESOLVING an object, on the other hand, you always want to look in the entire hierarchy.

The problem is that while we have a way of specifying whether or not to check ancestors while fetching object definitions, we're not provided with that option when fetching the names of the definitions, making it impossible to enumerate the definitions within your local context unless you try to fetch the definitions from your local context by name and check if they are null. While there's obviously a larger architectural problem that will need to be solved here, I think the most pragmatic solution this far into a major release cycle would be to modify the affected classes that call GetObjectDefinitionNames() to check for nulls when they only want local definitions, and modify the XML docs for that interface to make it clear that if they want only local definitions they will have to check to see if they are null or not.

My temporary solution for my own project has just been to make my own implementation of the broken VariablePlaceholderConfigurator and use that instead.

[thomast74]

The Java version is only looking in the current context. The Spring.NET version differs here.

I would recommend the following:
1.) I will add the methods that allows to tell if you want with ancestors or not. The default will be false.
2.) In all sub components that uses GetObjectDefinitionNames() will also check for null values.

But I'm not sure if that will be accepted, we have to see. At the moment the way it is done is not consistent.
On the other side that change will allow us later to change the sub components to check with ancestors quite easily.

[lahma]

This was fixed with pull #42