Temporarily change configuration parameter at runtime

Development
#1

Hi everyone,

I need to temporarily change the value of a configuration parameter at runtime. While discussing this on the #xwiki chat @tmortagne suggested introducing a configuration source that reads from the Execution Context. This practically means:

  • Adding ConfigurationSource#removeProperty(String key) (see below for the usage)
  • Implement ExecutionContextConfigurationSource with read & write support (the way the configuration is stored on the Execution Context is an implementation detail, the users of this class should use the ConfigurationSource interface to read and set configuration values)
  • modify DefaultConfigurationSource to check first the ExecutionContextConfigurationSource, i.e. look first in the Execution Context before checking the current document

This is how the new configuration source would be used:

@Inject
@Named("executionContext")
private ConfigurationSource ecConfigSource;

...

// Backup before overwritting.
boolean wasAlreadySet = this.ecConfigSource.containsKey(key);
Object previousValue = this.ecConfigSource.getProperty(key);
try {
  this.ecConfigSource.setProperty(key, tempValue);
  // Do stuff that relies on the modified configuration.
} finally {
  // Restore or cleanup.
  if (wasAlreadySet) {
    this.ecConfigSource.setProperty(key, previousValue);
  } else {
    this.ecConfigSource.removeProperty(key);
  }
}

WDYT?

Thanks,
Marius

#2

+1

Just a detail: not sure about the @Named in your example. There is no technical or codestyle reason to start it with a lower case and ExecutionContext might make it more obvious since that’s what it’s about.

1 Like
#3

@mflorea globally +1

now I’m wondering: the snippet you added seems generic enough to allow having a method defined for it no? e.g. a temporarySetProperty(String key, Object tempValue, Callable methodCall) that would allow to reuse the structure you exposed?

#4

+1, sounds good.

To me it sounds like it, +1 for having such a wrapper that avoids repeating this pattern again and again.

#5

We usually use lowercase and I’m +1 to continue that. See https://extensions.xwiki.org/xwiki/bin/view/Extension/Configuration%20Module#HDefaultConfigurationSources

We might even want to use executioncontext to be in sync with xwikiproperties and xwikicfg.

+1 globally, thanks.

#6

This would be a useful helper, yes. Now it does not make any sense in ConfigurationSource, so the question is where to put it.

#7

+1 thanks

#8

Hi everyone,

I created XCOMMONS-2921: Add support for overwritting the configuration from the execution context by mflorea · Pull Request #760 · xwiki/xwiki-commons · GitHub . The part that doesn’t feel right is the TemporaryConfigurationExecutor.

<V> V call(String sourceHint, Map<String, Object> temporaryConfiguration, Callable<V> callable) throws Exception;

I had a discussion with @tmortagne where he suggested that in the end it’s probably better to add the temporarySetProperty method to the ConfigurationSource role and implement an abstract configuration source that uses ThreadLocal to overwrite the configuration properties for the current thread (configuration sources are normally shared by all threads and a temporary configuration overwrite should affect only the current thread). One benefit of this is that you can overwrite the configuration at a specific level without affecting the entire chain, but I’m not sure how useful this is.

While trying to implement Thomas’ suggestion (based on Simon’s suggestion) I discovered that it poses some problems:

  • Even if we provide an abstract class, existing configuration sources can’t “receive” the new behavior without refactoring (i.e. the getProperty methods have to be modified to call the base class first, to check the thread local variable, and then fall-back on the standard behavior)
    if (hasTemporaryProperty(key)) {
  return getTemporaryProperty(key);
} else {
  // getPermanentProperty() is the abstract method that concrete implementations have to provide,.
  return getPermanentProperty(key);
}
  • The code executed with temporary configuration can call setProperty. What should be the behavior in this case? I think it makes sense for the setProperty to overwrite the thread local value (at least that’s what the code calling setProperty expects), but then we end up with two sets of permanent properties: those set before the temporary call and those set within the temporary call. The temporary properties would overwrite the first group but be overwritten by the second group. We can find ways to implement this I guess, but it feels too complex.
    configSource.temporarySetProperties(map, () -> {
  // Somewhere in a nested call:
  configSource.setProperty(key, value);
  // expecting getProperty to return the set value, not the previously set temporary value.
});
  • The code executed with temporary configuration can trigger another, nested, temporary configuration call, which forces us to keep a stack of the temporary thread-local properties, so that we can properly restore after a nested temporary call, without reverting the outer temporary call too soon. Again, this feels too complex.
    configSource.temporarySetProperties(map, () -> {
  // Somewhere in a nested call:
  configSource.temporarySetProperties(anotherMap, () -> {
    // expecting here anotherMap to __partially__ overwrite (merge with) the previous map.
  }
});

So WDYT? Is it worth implementing this complex temporary support directly inside a configuration source? I have doubts. I find it more simple to have a dedicated “execution context” (or “thread local”) configuration source, and to make the default configuration source check it first (in the chain), even if the TemporaryConfigurationExecutor feels a bit awkward.

Thanks,
Marius