Bug 43619

Created attachment 20974 [details]
Proposed patch - 'skeletal' version

The attached file only adds the pluggable property resolver to the PropertyConfigurator.  It does not 
appear to affect the DOMConfigurator.  I'd be relunctant to add a new capability to PropertyConfigurator 
without a consistent equivalent for DOMConfigurator.

You may be able to address your use case with a couple of different approaches that would not require 
modifications to log4j.  Since older log4j's are widely deployed and hard to displace, it would probably 
be in your interest to use capabilities in those versions when at all possible.

If you would be willing to extend DOMConfigurator, you could override DOMConfigurator.subst(String) 
and accomplish the same type of goal by passing in a specialized implementation of Properties 
downstream.

You could also extend PropertyConfigurator.configure(Properties, LoggerRepository) and wrap the 
passed Properties object with a specialized implementation of Properties that will also pull properties 
from JMX.  System properties would still be checked in preference to properties defined in the config 
file or in JMX, but that might be sufficient for your needs.

Are you 100% positive?

I'm using the same patch right now with DOMConfigurator. In production
environment, no less.

The idea was that DOMConfigurator.subst(String) calls
DOMConfigurator.subst(String,Props), which calls
OptionConverter.substVars(String,Props), which in turn calls my
PropertyResolver.getInstance().resolveProperty(String,Props). As long as a
specific implementation of Configurator eventually calls
OptionConverter.substsVars, my
PropertyResolver.getInstance().resolveProperty(...) is called.

Yes, I know it can be accomplished easier by subclassing a XxxConfigurator, but
this way we can have any combination of XxxConfigurator and YyyPropertyResolver.

Quite a bit less than 100%.  I did a quick read through the patch and apparently came to a faulty 
conclusion.

There is an arbitrariness of where you stop using System.getProperty().  If you rework 
OptionConverter.getSystemProperty() (instead of OptionConverter.substVars) to use a user-specified 
source for properties, then you could have JMX specify the default configurator, etc.  The source for 
"system properties" would be the only property that would have to come from System.getProperty().

I'd like to avoid adding a new interface, but overriding Map seems like an inordinate amount of effort 
and likely to be short-cut.  Plus it is hard to explain what the effective value of the current behavior is.  
Perhaps a property ("log4j.systemPropertyClass" ?) could specify a class name whose getProperty(String) 
method would be used for system properties.  The effective default value would be "java.lang.System".

Wouldn't that be over-simplifing things?
Note the key method has (String key, Properties localProps, String def)
arguments - it allows any strategy, for example to use System, local (defined in
config file), JNDI props in any order. You can have System1stLocal2nd stratedy
which would effectively override local properties with system properties, or
Local1stSystem2nd which would do the opposite. Not so with a simple
getProperty(String) replacement.

Basically what I mean is, it would be better to have pluggable 'strategies'
instead of 'sources'.


> Plus it is hard to explain what the effective value of the current behavior is

Why "hard"?
Current code:
	  // first try in System properties
	  String replacement = getSystemProperty(key, null);
	  // then try props parameter
	  if(replacement == null && props != null) {
	    replacement =  props.getProperty(key);
	  }

Try system property first, then try local property. System overrides local. Simple.

Redirecting the calls that currently go to java.lang.System.getProperty(String) to another arbitrary 
class's static getProperty(String) method, should still allow you to use JNDI in preference to system 
properties or vice-versa.

What I meant as hard to explain is that in either case you can't say something like the default value of 
this new property is some existing class that the user will be familiar.  

In redirecting the getSystemProperty() call, you can explain that you are redirecting the source of all 
system properties with the exception of "log4j.systemPropertySource" and the user has some familiarity 
with System.getProperty(String).  

In redirecting OptionConverter.substVars(), you have to say that you are redirecting from some 
description of the current strategy or a some new class that encapsulate that strategy.

Overriding OptionConverter.getSystemProperty() would allow you, for example, to set the location of 
the configuration file as an environment variable (assuming a JNDI implementation of getProperty()) that 
overriding OptionConverter.substVars() wouldn't.  Like:

export log4j.configuration=~/mylogconfig.xml

I'm not sure I understand that completely.

> What I meant as hard to explain is that in either case you can't say something
like the default value of 
this new property is some existing class that the user will be familiar.

It's simply DefaultPropertyResolver.resolveProperty, which does exactly what
404-409 lines in OptionConverter did.

However, there's one major issue I see with your version - no access to 'local'
properties, and no ability to choose order. With setting a 'source', as opposed
to 'strategy', you have to hardcode the overriding order, either the custom
source overrides local properties, or local properties override the custom
source. Also, your version depends on reflection, does it not? And that is
hardly easier for Joe Newbie, no compile-time checks, etc.

With 'strategies' you can have things like system properties "x" and
"x.allowOverride=false", and a strategy which disallows overriding by local
properties if a property with ".allowOverride" suffix and "false" value exists.

Well, why not have both? Have "log4j.propertyResolver" for 'strategies' and
"log4j.propertySource" for 'sources', with the default being a
use-source-strategy with "java.lang.System.getProperty(String)"?