Use only one Messages.java common for all the projects

[virchete]

I was thinking about how to improve the Messages.java and try to simplify the code.

Firstly I have seen that in both dtgov and sramp, all the messages.java extends from a AbstractMessages file, property of dtgov and sramp respectively. OverlordCommons Messages files are different from sramp and dtgov.

In my opinion we can include this AbstractMessages in the overlord-commons-i18n project and modify sramp/dtgov/overlord commons to extend from this class.

The second option I was thinking about: removing all the Messages.java files, and have only one Messages file. You can notice the number of Messages.java files....

Right now, the way we call the Messages is:

Messages.i18n.format  --> Messages.i18n is an static call and internally it knows where to find the messages.properties (in the same package than the Messages.java).

Then, if we have only one Messages.java file located  in overlord-commons we need to find out where is the messages.properties, because we ll have different messages.properties, located in different packages.

My solution:

class Messages extends AbstractMessages{

    public static final Messages i18n = new Messages();

    public Messages() {

        //With this instruction we get the caller class and then the package

        //In the super class we will find out where is located the messages.properties

        super(Reflection.getCallerClass(1));

    }

}

public class AbstractMessages{

    private static List<String> propertiesPaths;

    static{

         propertiesPaths=new ArrayList();

         Set<PropertiesPathProvider> providers = ServiceRegistryUtil.getServices(PropertiesPathProvider.class);

         //Here for every path provider, sramp, dtgov, overlord commons we ll load the paths defined by them

         for(PropertiesPathProvider providerroviders){

             propertiesPaths.addAll(provider.getPropertiesPaths());

         }

    }

    public AbstractMessages(String packagePath){

         //Here we try to find the properties path that matches better the packagePath

    }

}

This is only a pseudocode. Then we only need one PropertiesPathProvider class in every subproject, sramp, dtgov, overlord-commons and dtgov.

Every time we want to include for a project a messages.properties we should include a messages.properties in src/main/resource/{package} and include an entry in the PropertiesPathProvider.

[eric.wittmann]

We would need an alternative to Reflection.getCallerClass(1) as that has been removed from the JDK.

David - how do you propose to find the right messages.properties from the list of possible files discovered for all the provided property paths?

I'm also somewhat skeptical of adding an overlord-commons dependency on every other JAR we have.  But certainly having the i18n stuff in a common place seems to make sense.

What do others use for messages?  I think one of the things we do a bit differently is set the locale used by the Messages class depending on the current context.  If the code is executing on behalf of an HTTP request of some sort for example (e.g. a REST api call) then we'll set the locale based on the inbound client.  Otherwise we would use the system default locale.

[virchete]

About the Reclection.getCallerClass, I ll find a way. I think from the stacktrace it is possible to obtain:

http://stackoverflow.com/questions/8340785/is-there-any-way-to-know-the-caller-class-name

About how I think we can get the right messages:

We ll have a list<String> paths that will contains all the paths with a messages.properties.

Example:

paths={org.overlord.x,org.overlord.x.y}

The messages.i18n is called from these packages:

   org.overlord.x.y --> then it would read the messages.properties located in the  org.overlord.x.y package.

   org.overlord.x  --> Then it would read the messages.properties located in the org.overlord.x package.

   org.overlord.x.z --> Then it would read the messages.properties that better match this package: org.overlord.x

   org.overlord.other --> Then it would null or empty string.

About adding this overlord-commons-i18n jar in all the subprojects, I have a solution for this. Maybe we can add this jar to the commons project. For example in sramp, we have s-ramp-commons and in dtgov, dtgov-commons. Then normally this dependency should be common for all the subprojects. The same could apply for log4j.

[brmeyer]

David, first off, +1 for wanting to clean that all up -- very good idea.

At first glance, the concept sounds useful.  Each module would offer up a service identifying the i18n packages they contain.  Using the "callers" ClassLoader, iterate over the packages and see what resource is visible to that CL.  That would be fine for Java SE and probably EE.  However, OSGi really complicates things.  Assume the following setup:

Bundle A contains foo.package1 (both Java code and i18n properties)

Bundle A exports foo.package1 in its manifest

Bundle B imports foo.package1 in its manifest

Bundle B contains foo.package2 (both Java code and i18n properties)

In OSGi, a Bundle's ClassLoader has access to its own packages, as well as all packages it imports!  So, in the above case, Bundle B's CL would have access to all classes and resources in foo.package1 and foo.package2.  If every single i18n entry is unique, then that might not be a big deal.  But if there is duplication, we could run into issues on Fuse.  If we prefixed every entry with something unique, we might be ok.

The other concern is performance.  Rather than each module using its own Messages instance, we could potentially be iterating over all module ClassLoaders for every single log, stout, etc.  I'm worried that might become a bottleneck.

[eric.wittmann]

I think OSGi makes it even worse than this.  If the code that is loading the messages.properties files lives in overlord-commons-i18n (for example).  When running in OSGi that guy is a bundle with an isolated classloader.  So by default it won't be able to load anything from any other module because it doesn't import any packages. 

I suppose the current thread context classloader might have access to all the correct packages, in which case all of Brett's OSGi concerns come into play.

I also agree with the performance concern.  How can we ensure that strings can be quickly looked up and that we get the right one?  We really don't want i18n string lookup to be a bottleneck.

I think a lot projects don't do any code sharing at all for their Messages - just duplicate a simple Messages class in every JAR.  This makes it very fast and simple.  The only reason I'm not doing that is the Locale issue I mentioned above - I needed to at least have a common place to set the Locale Thread Local.

[brmeyer]

> I think OSGi makes it even worse than this.  If the code that is loading the messages.properties files lives in overlord-commons-i18n (for example).  When running in OSGi that guy is a bundle with an isolated classloader.  So by default it won't be able to load anything from any other module because it doesn't import any packages.

> I suppose the current thread context classloader might have access to all the correct packages, in which case all of Brett's OSGi concerns come into play.

Depending on how you implemented the "getCallerClass", that would typically carry the calling Bundle's CL.  Also, I could be wrong about this, but technically if you created an instance of the Messages class from within Bundle A, Messages' constructor could do "this.getClass().getClassLoader()" and that would return Bundle A.  I'd have to play with that -- can't remember exactly.

Of course, even if that's doable, you're still back to the original visibility problem...

[eric.wittmann]

Also, I could be wrong about this, but technically if you created an instance of the Messages class from within Bundle A, Messages' constructor could do "this.getClass().getClassLoader()" and that would return Bundle A.  I'd have to play with that -- can't remember exactly.

I believe this is incorrect, based on my trials and tribulations with Apache Santuario and Fuse.  If the class is being provided by a bundle then I'm pretty sure that a getClass().getClassLoader() on the instance of that class will return the bundle that provides it, not the bundle that instantiates it.  But I'd have to try that again as well! 

In any case, I think we have established that there are lots of potential OSGi concerns with this idea.  For that reason alone I think we should defer this to after the release.

Depending on how you implemented the "getCallerClass", that would typically carry the calling Bundle's CL.

I eagerly anticipate seeing this getCallerClass() implementation.

David - perhaps you could create a proof of concept of this idea in a few weeks when release fervor has died down?

[virchete]

Hi guys,

This new functionality would have big impact in all the projects, and is not going to be included in the release version.

After the release I ll investigate and make a proof of concept. I do not believe it would be a bottle neck in this messages.java class. This class could be like a factory that store Singletons Messages, one per  messages.properties in the system.

And then the bottleneck is going to be more or less the same than before, where we have a Messages.java per messages.properties. Would be something like this the code:

class Messages{

    static Map<String, Messages>  messages;

     static{

          loadMessages();

    }

   static Messages getInstance(){

        //Here we get the callerPackage

       String callerPackage=getCallerPackage();

       //Algorithm that would return the Messages object that better match the package param

       return getMessages(callerPackage);

}

static Messages getMessages(String package){

}

}

I do not see here possible bottleneck. At the end it is the same use as before, but only having one Messages.java.

The getCaller reflection method. I need to investigate and create a proof of concept. This is the only point that I am not 100% sure. Could be very quick to make the test in osgi, that I think is the pòssible conflict point.