ResourceBundleMessageSource should allow for custom PropertyResourceBundle subclass [SPR-12666]

[spring-projects-issues]

Dave Jarvis opened SPR-12666 and commented

Property files, resource bundles, and therefore property resources bundles incur a lot of duplication (in practice) because there is no way for one entry to refer to other entries.

Lines 431 to 433 of org.springframework.context.support.ResourceBundleMessageSource hard-code a specific instance of PropertyResourceBundle:

return (defaultEncoding != null ?
  new PropertyResourceBundle(new InputStreamReader(stream, defaultEncoding)) :
  new PropertyResourceBundle(stream));

These lines should be:

return (defaultEncoding != null ?
  createPropertyResourceBundle(new InputStreamReader(stream, defaultEncoding)) :
  createPropertyResourceBundle(stream));

Then two corresponding protected methods "createPropertyResourceBundle" should be created (with one calling the other). For example:

protected PropertyResourceBundle createPropertyResourceBundle( InputStream stream ) {
  return createPropertyResourceBundle( stream, Locale.getDefaultEncoding() );
}

This would allow for systems to override the specific type of PropertyResourceBundle. Once in place, it is then possible to write properties that refer to other properties, such as:

app.title=Spring Framework
app.login=Please log in to the ${app.title} to continue.
app.contact.thanks=Thank you for your suggestions to improve the ${app.title}.

Currently, this is not possible due to lines 431 - 433.

---

Affects: 4.1.4

Issue Links:

• ResourceBundleMessageSource should avoid ResourceBundle.Control on Jigsaw [SPR-16776] #21316 ResourceBundleMessageSource should avoid ResourceBundle.Control on Jigsaw
• Method getMergedProperties in ReloadableResourceBundleMessageSource does not set fileTimestamp [SPR-14583] #19152 Method getMergedProperties in ReloadableResourceBundleMessageSource does not set fileTimestamp

[spring-projects-issues]

Juergen Hoeller commented

Semantically, this isn't quite as trivial as introducing such template methods... since they are only being called from MessageSourceControl which we only use in case of specific settings.

For those template methods to cleanly apply, we'd either have to always use our custom MessageSourceControl, or introduce such template methods on MessageSourceControl itself in combination with a ResourceBundleMessageSource-level template method which allows for using a custom MessageSourceControl subclass.

We'll see what we can do for 4.2 here.

Alternatively, have you tried extending Spring's own ReloadableResourceBundleMessageSource for your purposes? It's not based on JDK ResourceBundle classes but does allow for customized Properties loading etc, based on the standard resource bundle format. That class is much more extensible than ResourceBundleMessageSource by design.

Juergen

[spring-projects-issues]

Dave Jarvis commented

A few items of note.

This architecture seems over-engineered. The ReloadableResourceBundleMessageSource class has a few lines where a java.util.Properties class is instantiated (e.g., lines 348 and 568), which violates the DRY principle; additionally, several lines contain "new PropertiesHolder()", which also violate DRY.

These lines should delegate creation to a protected method, which would permit subclasses to override a single method to provide customization.

Next, using a PropertiesPersister seems to offer no advantages over polymorphism. Further, the PropertiesPersister doesn't allow for extending the implementation of the Properties instance used to load the resource bundle. This is due to the following interface definition:

load(Properties props, InputStream is)

The method takes a pre-instantiated Properties instance, which precludes any possibility of changing the behaviour for getProperty(). The only option that this particular design allows is load-time changes, such as:

public class ResolvedResourceBundleMessageSource extends
		ReloadableResourceBundleMessageSource {
	public ResolvedResourceBundleMessageSource() {
		super();
		setPropertiesPersister( new ResolvedPropertiesPersister() );
	}
	
	private class ResolvedPropertiesPersister extends DefaultPropertiesPersister {
		public void load( Properties p, InputStream input ) throws IOException {
			ResolvedProperties rp = createResolvedProperties();
			rp.load( input );
			p.putAll( rp );
                        // Won't work because the Properties class cannot delegate
		}

                public void load( Properties p, Reader r ) throws IOException // ...etc
		
		protected ResolvedProperties createResolvedProperties() {
			return new ResolvedProperties();
		}
	}
}

This, however, is not an extensible design because the ResolvedProperties class, which overrides getProperty() to perform the substitution, cannot be injected. Even if the customized ResolvedProperties class could be used with this design (by changing the code to perform load-time substitutions instead of query-time), it is still a lot more code to write compared with a polymorphic solution:

public class ResolvedResourceBundleMessageSource extends ResourceBundleMessageSource {
  protected Properties createProperties() {
    return new ResolvedProperties();
  }
}

[spring-projects-issues]

Juergen Hoeller commented

Alright, so for 4.2, I've introduced a protected ResourceBundle loadBundle(Reader reader) throws IOException template method in ResourceBundleMessageSource, called from our internal MessageSourceControl. In order for that method to be called consistently, we are always using MessageSourceControl now, even for default settings. The default encoding is explicitly declared as ISO-8859-1, allowing for always using a Reader (since for a specified InputStream, properties loading would simply be defaulting to ISO-8859-1 internally).

And in ReloadableResourceBundleMessageSource, I've introduced a protected Properties newProperties() template method, allowing for specifying a custom Properties subclass both for loaded properties and merged property holders (which will be used depending on the cache mode). Subclasses are only meant to override that method with instantiation of a custom Properties subclass; no extra initialization or population to be performed at that point.

FWIW, I do not agree that repeated PropertiesHolder instantiation and the general structure of RRBMS is a violation of DRY or even "over-engineering". It's simply designed for extension in specific places and conceals internals in others. The use of Properties polymorphism is a fine new use case, which is why the above has been introduced now. Whereas use of custom PropertiesHolder classes would interfere with RRBMS-internal assumptions and is therefore still not opened up.

Hope that helps for your purposes,

Juergen