Uploaded image for project: 'Sakai'
  1. Sakai
  2. SAK-39622

Resolve placeholder properties in values returned from ServerConfigurationService.getXxx()

    Details

    • Type: Task
    • Status: CLOSED
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6.x
    • Fix Version/s: 2.6.x, 2.7.x
    • Component/s: Kernel
    • Labels:
      None

      Description

      The executive summary:

      1) sakai.properties sets:
      property1=foo
      property2=${property1}
      property@org.sakaiproject.MyBean=${property1}
      2) I expect:
      ServerConfigurationService.getString("property1").equals(ServerConfigurationService.getString("property2")) &&
      ServerConfigurationService.getString("property2").equals(myBean.getProperty())
      3) Instead:
      ServerConfigurationService.getString("property1").equals("foo") &&
      ServerConfigurationService.getString("property2").equals("${property1}") &&
      myBean.getProperty().equals("foo")

      The attached patch (property-placeholder-dereferencing.diff) fixes this asymmetry.

      Now in more detail...

      We've run into a few surprises where we've attempted to consolidate configuration by making use of property placeholders in sakai.properties. For example, we have a single site type identifier which needs to be set as the value of several different properties. It would be convenient in these cases to centralize the definition of that identifier using property placeholder syntax, e.g.:

      special.site.type.identifier=foo
      use.special.behavior.for.site.type=${special.site.type.identifier}
      use.other.special.behavior.for.site.type=${special.site.type.identifier}

      Interestingly, this works exactly as expected if ${special.site.type.identifier} appears in a Spring bean definition. It also works as expected when overriding Spring bean properties using the special Sakai bean addressing syntax, e.g.:

      siteType@org.sakaiproject.SiteTypeSensitiveBean=${special.site.type.identifier}

      It does not work as expected, though, for callers of ServerConfigurationService.getString() (nor its brethren accessors). For example, ServerConfigurationService.getString("use.other.special.behavior.for.site.type") would return the literal "${special.site.type.identifier}". (In case it's not 100% clear, "expected" behavior is for ${special.site.type.identifier} to expand to "foo".)

      I can conceive of situations where access to raw property definitions might be or is valuable, e.g. ConfigViewer/ConfigEditor. However, I am guessing that this asymmetry between bean property overrides and config service getters is surprising. It certainly was for me.

      If we exclude backward compatibility with ConfigViewer/ConfigEditor, though, is there a good reason why ServerConfigurationService.getXxx() should not dereference property placeholders?

      Assuming there is no such good reason, I'm going ahead and attaching a patch (property-placeholder-dereferencing.diff). A few notes:

      1) Of the new test methods in ConfigurationLoadingTest, the following fail without the patch. All others are just regression tests:

      testGetStringDereferencesPlaceholderPropertiesForSimpleProperties()
      testGetStringDereferencesPlaceholderPropertiesForBeanAddressingProperties()
      testGetStringsDereferencesPlaceholderProperties()
      testPropertyPromotionDereferencesPlaceholderProperties()

      2) The long test method names don't really match existing kernel testing conventions, but the approach in general is not just a personal whim (http://googletesting.blogspot.com/2007/02/tott-naming-unit-tests-responsibly.html). I'll change the names, though, if chatty names are unpalatable.

      3) If necessary, I can also collapse tests to match the more coarse-grained testSakaiProperties(). Since the component manager is only started once for the entire test class, though, I didn't really see any reason to avoid what I perceive as the advantages of tightly-scoped test methods.

      4) If the patch is unacceptable without also offering an API to callers needing access to raw property definitions, I can definitely work up something along those lines. The main issue there, I think, is that any change to a referenced property requires recalculation of referencing properties unless we change ServerConfigurationService getters to dereference placeholders on the fly. In general, though, my sense is that it is still not altogether clear what all should happen when a ServerConfigurationService-owned property is changed at runtime.

      5) SakaiProperties.dereferenceProperties() uses reflection to get at the exact same property placeholder resolution logic used by ReversiblePropertyOverrideConfigurer and PropertyPlaceholderConfigurer. Unfortunately, Spring does not offer a public API for property placeholder resolution, but I wanted to be sure I was in fact delivering the symmetry I felt had been missing. The issue of burying placeholder resolution code in bean factory processing code doesn't seem to be causing Spring to lose any sleep (http://jira.springframework.org/browse/SPR-2321), but if people object to the use of reflection, I can try to work up a patch for Spring. I don't know that that would actually solve the problem for us, though, since (among other reasons) our Spring upgrades are so infrequent. And in any event, at least in this case we have integration tests to guard against regressions.

        Gliffy Diagrams

          Zeplin

            Attachments

              Activity

                People

                Assignee:
                dmccallum Daniel McCallum
                Reporter:
                dmccallum Daniel McCallum
                Votes:
                1 Vote for this issue
                Watchers:
                0 Start watching this issue

                  Dates

                  Created:
                  Updated:
                  Resolved:

                    Git Integration