JEP: Global implementation of replacement of environment properties

[alcarraz]

Summary

Enable the possibility to replace environment properties anywhere in XML deployment files.

Goals

Enable environment property replacement for any jPOS's XMLConfigurable.

Motivation

Sometimes you want to pass an environment property to a component that don't call Environment.getProperty, currently you are in one of two possibilities:

1. That component is under your control, and you can make the implementation.
2. The component is not under your control, e.g., a built-in jPOS or jPOS-EE one, and you have to ask for that to be implemented, or build your own version of jPOS.

With this, you could use environment properties replacement anywhere you want.

Description

Add a boolean attribute to XML elements that are passed to configurable components that indicates that any attribute or content in it or its children are modified by replacing any environment property there.

A first idea is to modify QFactory.setConfiguration to call a function that replaces all environment properties, if the element contains a given attribute with true value.

You can see a simple proof of concept implementation here.

Basically, the following method was added to QFactory

    public static void replaceEnvProperties(Element e) {
        Environment env =  Environment.getEnvironment();
       for (org.jdom2.Attribute attr : e.getAttributes()) {
           String value = attr.getValue();
           attr.setValue(env.getProperty(value, value));
       }
       for (Content child : e.getContent()) {
           if (child instanceof Element) {
               replaceEnvProperties((Element) child);
           } else if (child instanceof Text) {
               Text text = (Text) child;
               String textValue = text.getText();
               text.setText(env.getProperty(textValue, textValue));
           }
       }
    }

And it's called in instantiate if the attribute value is present:

    public void setConfiguration (Object obj, Element e)
        throws ConfigurationException 
    {
        try {
            if (Boolean.parseBoolean(e.getAttributeValue("replace-env-properties"))) replaceEnvProperties(e);
            Configuration cfg = getConfiguration (e);

With that, you could for example configure a logger like this:

<logger name="Q2" class="org.jpos.q2.qbean.LoggerAdaptor">
  <log-listener class="org.jpos.util.RealmLogFilter" enabled="${log.disable-realms.enabled}" replace-env-properties="true">
    <property name="dump-interval" value="60000"/>
    <disabled>${log.disable-realms:Q2.system}</disabled>
  </log-listener>
  <log-listener class="org.jpos.util.SimpleLogListener"/>
</logger>

And get the ${log.disable-realms:Q2.system} token replaced, even though RealmLogFilter does not call Environment.getProperty for enable or disable elements.

Alternatives

• Leverage the ConfigDecorationProvider mechanism, where a system could have many registered decorators, and type of decorators that acts over the parsed Element tree. And provide a mechanism to activate those decorators on a per-file basis. The ability to use more than one in a correctly defined order could also be supported.
• Implement a jPOS-EE module that provides a ConfigDecoratorProvider, that expand all properties in all deploy files.

Risks and Assumptions

If the attribute to modify this behavior is chosen with careful, the risk that this could impact some existing installation is near to 0. However, it could not be backward compatible with some installation that uses that attribute name with a true value for another thing, and it has a token of the form ${...} in it, and there exists an environment property for that, or the property includes a default value.

I could also minimally affect performance, but the recursive call would only be performed when the attribute value is true.

These risks could be mitigated with a global java property (or some other mechanism) that disables the feature.

[ar]

Thank you for sharing your idea in the JEP/Discussion format, which I believe should be the standard for future contributions.

Regarding the replace-env-properties attribute, it's not something I'm particularly fond of at first glance. My preference would be to skip this attribute entirely, and default to it being true. However, for the sake of maintaining backward compatibility, we could consider honoring a verbatim='true' attribute (default to false). This could be useful in situations where we wish to opt-out of this feature.

I propose testing this functionality in the next branch first (jPOS 3.x). If successful, we can consider back-porting it to the 2.x series.

jPOS 3.x is a version were we'll rock the boat a lot with a lot of changes, and it's still not in production, so it's safe to play with this there.

With this feature, we probably can remove all custom code that calls Environment.get here and there. We need to carefully remove it all at once, though.

This way, we can ensure stability in the current version while testing this new feature.

[alcarraz]

Excellent, thank you for your feedback, just want to check you don't have any concerns about those recursive calls, for every component, even when it is not needed?

I must admit, as much as I like the idea, something inside me is fighting against doing that by default. Specially, given that there may be some components out there that create and configure others on the fly by calling QFactory,

I guess we could overload QFactory.setConfiguration with a boolean parameter for that. That parameter could be taken from the verbatim attribute, in the method without the parameter.

[ar]

I don't think speed is a problem here, specially since the configuration is just read at QBean initialization time.

[ar]

Great related PR #546

[alcarraz]

I did something a little different from what I proposed here, as mentioned in this comment, instead of performing the replacement in the QFactory.setConfiguration method, I performed it in the instantiate method, for three reasons:

1. To be able to replace the class attribute, this is not needed because, the getAttributeValue for the class already does that, but we may want to remove all those extra calls to Environment.getProperty. I don't know if we should, though.
2. To respect the verbatim option for children, if we pass an inner element to setConfiguration to configure it, setConfiguration wouldn't know if some “ancestor” has it enabled, ad that behavior could not be intuitive and even feel quirky.
3. To avoid visiting the children many times, when setConfiguration is called on them.

[alcarraz]

In the latest commit to the PR, this was implemented a slightly different version of this. The responsability of calling the method that expands the environment properties, now called expandEnvProperties is for the caller, Q2 in this case. So if some other component needs to instantiate and configure an XML fragment, it just has to call the static expandEnvProperties on the element before calling QFactory.setConfiguration.

[alcarraz]

This was implemented in #554