castor

Preliminary version of refactored configuration class, at the moment named NewConfiguration. That class does not offer the whole functionality yet but I like to discuss a few things bassed on that code:

• Shell we keep the loading strategy of properties files from current Configuration. I refactored loading strategy of current Configuration for better readability into NewConfiguration.loadDefaultProperties().

In my opinion we should change to:
loadFromClassPath(FILEPATH + FILENAME);
loadFromClassPath("/" + FILENAME)
and omit:
loadFromJavaHome(FILENAME);
loadFromWorkingDirectory(FILENAME);

• I'd like to treat ClassLoader's for application and domain classes as well as LogFactory special with separate get methods instead of putting them in configuration map.

• The methods to access properties of different types are allmost finished. I'd like to hear opinons on how I handle default values or throw exceptions. In addition, are there any other accessors you like to be included?

Hello Ralf,

my thoughts and opinions about the patch:

>>> loading of properties and loadDefaultProperties
Property loading styles look fine to me. The loadDefaultProperties method is a bit confusing for me. The comments (for me) express something different then the code does... I would understand that the different configuration files are all loaded on top of each other, each time replacing values that are in both files - the code stops to load properties after the first load

>>> LogFactory
Why having a logFactory handling inside here? The feature to have a exchangeable logger is implemented by commons-logging - I wouldn't do something similar again in our configuration.

>>> ClassLoader
I think it would be a good idea to have a single source of truth when it comes to class loaders.... but this requires a lot of refactoring work in other places.

>>> access method: Object get(String)
I would like to have this method private (or protected)

>>> access methods
And introduce getObject, getClass and on demand getXy accessors which are public

>>> exceptions
I'm convinced that unchecked exceptions are a good thing to use in frameworks... but throwing directly RuntimeException is a crime. We need to have a Castor exception hierarchy, derived from RuntimeException) and throw these exceptions...

>>> how should the configuration be instantiated?
I'm missing how this class should be instantiated... should we have an own factory class? or a getInstance() method?

Have fun

Joachim

Thanks for your comments Joachim

>>> loading of properties and loadDefaultProperties
It seams I made a mistake when refactoring code. The comments express the right behaviour. I'll fix that.

>>> LogFactory
Some users reported problems with our current logging. As far as I recall the problems where related to the construction of log instances using:

private static final Log LOG = LogFactory.getLog(FooBar.class);

I guess their problems are related to classloading or the time when the Log instance is created. That was the reason I thought of passing around a LogFactory instace with the configuration. Maybe I got a bit to far with that approach. Any suggestions are welcome.

>>> ClassLoader
I'm aware of the required refactoring work but IMHO we need to start at some point the gain the advantages of this.

>>> access method: Object get(String)
Will make it protected.

>>> access methods
Will add getObject and getClass.

>>> exceptions
I only wanted to highlight that I intend to throw unchecked exceptions and planed to introduce a exception hierarchy as follows:
ConfigurationException -> CastorRunTimeException -> RuntimeException

>>> how should the configuration be instantiated?
I do not have a final opinon how configurations should be instantiated but will propose something with my next preview.

Ralf, I don't know whether you remember, but we have had some complains about class-loading related to the use of commons-logging when we still used the 1.0.x releases of commons-logging. Since we upgraded to 1.1, this issue has been fully resolved (as that's what the 1.1 release was meant to improve tremendously). In other words, given that we have arrived at 1.1 and later, I don't think that there will be an issue again.

One thing to note: there's some users (especially on Linux/Unix that rely on e.g. castor.properties to be available in the user directory. Frequently, production applications are fitted with a production user id. In other words, I am not sure whether we should really drop support for $HOME or not.

I'll remove the support for passing around a LogFactory instance with the configuration for now. If the issue with logging arises once again we can add that later.

I also changed my mind to keep the current loading strategy.

Hello Ralf,

I would change the existing CastorException extends Exception to CastorException extends RuntimeException and then introduce a CastorConfigurationException extends CastorException.

I wouldn't introduce a new exception hierarchy. Changing the existing hierarchy from Exception to RuntimeException shouldn't break existing code...

Actually, you are right that it should not break existing code, but right now CastorException is a checked exception, and making CastorException derive from RuntimeException sort of breaks that contract, doesn't it ?

I agree that we should refactor our exception handling but I don't think it is a good idea to simply make CastorException extend RuntimeException instead of Exception. Having said that I am not sure if that change wouldn't break things at JDO. We should work on that at a separate issue some time. For this issue I'll move available and unused org.exolab.castor.exceptions.CastorRuntimeException to org.castor.core.exceptions.CastorRuntimeException. ConfigurationException will extend this CastorRuntimeException.

Hope to be able to attach an improved patch soon.

Allmost finished patch introducing new configuration for review.

What's missing is splitting of the castor.properties file into the new castor.<modul>.properties files.

I need to note that it will not be possible to define individual getters for properties like .e.g. getSaxParser() at XMLConfiguration and it is also not intended to add such method to the abstract Configuration class.

The package names I have chosen is something we may need to discuss also.

I'm think of adding a possibility for the user to specify the name of his configuration file with additional newInstance() methods.

Having said that I think the changes to other Castor classes to use this new configuration should be worked on at separate issues.

I have forgotten to mention that I still need to think about if and how this could be tested.

Why will it not be possible to specify methods like e.g. getSaxParser() in classes deriving from this new configuration class. ? And yes, changes to other classes required to switch to this new class should be as separate (follow<up) issues.

The modul configurations (CoreConfiguration, XMLConfiguration, CPAConfiguration) are responsible for the modul specific predefined default values defined in castor.|modul|.properties files in one of the Castor jars. The CastorConfiguration class on the other side loads the user specific configuration from classpath root or current working directory. This design allows to maintain the cofigurations of the modules independend of each other. As you don't want to use a separate configuration for each modul the different configurations are linked with the parent property to be searched hierarchically. This is modelled with the same pattern used at java.lang.Properties.

If you look at the newInstance() methods of CPAConfiguration (XMLConfiguration) you will see how the hierarchy is created. From top to bottom:

CastorConfiguration (as user should be able to overwrite everything)
CPAConfiguration (as some XML properties may need to be overwritten to allow loading of JDO mapping or config)
XMLConfiguration
CoreConfiguration

As you see the concrete configuration returned will always be a CastorConfiguration holding the user properties. You won't have access to an additional getSAXParser() method defined at XMLConfiguration. The only chance to add other getters would be the abstract Configuration class where all the default getters are defined.

Apart of the limitation of the design I personally think that a configuration class should not be responsible to handle such specific tasks like construction of a SAX parser. A better approach in my opinon would be to create the SAX parser somewhere else and put it in the configuration programatically for later retrieval by the getObject() method. Having said that I am not sure if things like a SAX parser instance should be put into a configuration at all.

Hello Ralf,

I just had a look onto patch-C1927-20070627.txt and I had a lot of problems applying it against HEAD...
So I looked onto the patch file instead - the changes I requested are in (no direct RuntimeException, getObject, getObjects...) so I'm fine with it.

Final patch.

For the sake of reference, I have been through this patch twice over the last few days, and I really dig both the effort and the quality delivered (especially in terms of separation of concerns). I'll make some small changes to Javadocs over the next few days to make a few things more explicit, hoping that this will help us (everybody who touches code to integrate this new code) in our efforts.

Here's one final question: is the HOME directory still included in the search for a user-defined castor.properties file ?

The Castor JAR and Java HOME directory is search for:

• castor.core.properties
• castor.cpa.properties
• castor.xml.properties
  but not for castor.properties. This is only searched for under:
• classpath root
• current working directory
  If one placed his castor.properties under:
• classpath org.exolab.castor
• Java HOME
  it will not be recognized any more. Having said that we can change that. If you have objections according to packages or class hierarchy im also open for ideas how to further improve this.