Pluto
  1. Pluto
  2. PLUTO-609

PortletPreferencesImpl doesn't handle null preferences correctly

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.0.3, 2.1.0-M3, 2.1.0
    • Component/s: None
    • Labels:
      None

      Description

      PLT.17.1 states "Preference attributes are String array objects. Preferences attributes can be set to null." In Pluto if you call PortletPreference.setValue("name", null), PortletPreference.setValues("name", String[]

      {null}

      ), or PortletPreference.setValues("name", null) the correct data is passed to the underlying preference storage SPI.

      The problem is when calling getValue("name", "DEFAULT") or getValues("name", new String[]

      { "DEFAULT" }

      ) for any of the three previous cases "DEFAULT" is returned. From my reading of the spec this is not correct as in each case the preference has been set but with a single null value or a null values array.

        Activity

        Hide
        Ate Douma added a comment -

        Thanks, I'll apply the same change to trunk then and resolve this issue.

        Show
        Ate Douma added a comment - Thanks, I'll apply the same change to trunk then and resolve this issue.
        Hide
        Eric Dalquist added a comment -

        It appears to be working as expected in uPortal.

        Show
        Eric Dalquist added a comment - It appears to be working as expected in uPortal.
        Hide
        Ate Douma added a comment -

        Eric, any update this?

        Show
        Ate Douma added a comment - Eric, any update this?
        Hide
        Eric Dalquist added a comment -

        This looks like a good solution, I'll give the latest code a try in uPortal 4 tonight to verify our tests as well.

        Show
        Eric Dalquist added a comment - This looks like a good solution, I'll give the latest code a try in uPortal 4 tonight to verify our tests as well.
        Hide
        Ate Douma added a comment -

        I've committed the above proposed changes. Please review.

        Show
        Ate Douma added a comment - I've committed the above proposed changes. Please review.
        Hide
        Ate Douma added a comment -

        For the record, if no (persistent) default value has been defined for a preference, the reset method is specified to actually remove the preference, so that is supported by the spec.

        One thing the specification is not explicit (enough) about is: is it allowed to call setValues(key, null) or setValues(key, new String[0])?
        The javadoc for setValues only says: "null values in the values parameter are allowed". It doesn't say a null or zero length array is allowed/supported.

        Please note the difference between setValues(key, new String[0]) and setValues(key, new String[]

        {null}).
        This is particularly important with how setValue(key, null) should be interpreted: as setValues(key, null), as setValues(key, new String[]{null}

        ) or as setValues(key, new String[0])?

        My interpretation and proposal for all this now is:

        • setValues(key, new String[] {null}) should lead to returning [null] (array size 1) from getValues(key, new String[]{"DEFAULT"}) but "DEFAULT" from getValue(key, "DEFAULT")
          (this is your adjusted proposal above)
          - setValues(key, null) should be interpreted/translated into setValues(key, new String[0])
          - setValues(key, new String[0]) should lead to returning a [] (array size 0) from getValues(key, new String[]{"DEFAULT"}) and a null value from getValue(key, "DEFAULT")
          (this is currently implemented through the changes for this issue)

          Now the question remains: how should setValue(key, null) be treated?
          To keep semantical balance with getValue(key,<default>) which accesses the first element of the value array, it seems logical to translate it into setValues(key, new String[]{null}

          ).
          This is also the currently implementation!
          However, with the above semantics for handling setValues, this leads to the illogical result that setValue(key, null) != getValue(key, "DEFAULT").
          So it might actually be better to reimplement setValue(key, null) as setValues(key, null) -> setValues(key, new String[0]).

        If we then implement the above, I think there are actually only a few changes to make based on the original implementation (so after first reverting the current changes):
        a) in getValue: if getValues(...) returns a array length 0 ([]), return null, else return either the first array element or the default if either null
        b) in setValue: delegate to setValues(key, value == null ? null : new String[]

        {value}

        )
        c) in setValues: if value == null then value = new String[0]

        I've tested these changes against the TCK and Pluto now still passes all tests, as is Jetspeed using this version of Pluto.

        However, the Pluto testsuite now fails on the PreferenceCommonTest.checkSetPreferenceNull test, which still assumed the original behavior (setting a null value resulting in the default being returned from getValue). So, if to implement these logic changes, that test now has to be adjusted likewise (returning null instead of the provided default).

        Please note that it depends on the PortletPreferenceImpl backing model if String[0] actually gets "stored"...
        For Pluto which by default only implements a transient/in-memory model, setting a String[0] value will be retained.
        Jetspeed however, which uses database persistency, this currently will result in the existing value(s) being destroyed (and thus fallback to returning the "default" values).
        We probably should review if we can/should adapt Jetspeed for this new interpretation of the spec, but technically the above changes won't change its current behavior AFAIK.

        I'll preliminary implement these proposed changes so they can be properly reviewed and tested.
        If this still isn't according to the desired interpretation it is pretty easy to revert again or adjust otherwise.

        Show
        Ate Douma added a comment - For the record, if no (persistent) default value has been defined for a preference, the reset method is specified to actually remove the preference, so that is supported by the spec. One thing the specification is not explicit (enough) about is: is it allowed to call setValues(key, null) or setValues(key, new String [0] )? The javadoc for setValues only says: "null values in the values parameter are allowed". It doesn't say a null or zero length array is allowed/supported. Please note the difference between setValues(key, new String [0] ) and setValues(key, new String[] {null}). This is particularly important with how setValue(key, null) should be interpreted: as setValues(key, null), as setValues(key, new String[]{null} ) or as setValues(key, new String [0] )? My interpretation and proposal for all this now is: setValues(key, new String[] {null}) should lead to returning [null] (array size 1) from getValues(key, new String[]{"DEFAULT"}) but "DEFAULT" from getValue(key, "DEFAULT") (this is your adjusted proposal above) - setValues(key, null) should be interpreted/translated into setValues(key, new String [0] ) - setValues(key, new String [0] ) should lead to returning a [] (array size 0) from getValues(key, new String[]{"DEFAULT"}) and a null value from getValue(key, "DEFAULT") (this is currently implemented through the changes for this issue) Now the question remains: how should setValue(key, null) be treated? To keep semantical balance with getValue(key,<default>) which accesses the first element of the value array, it seems logical to translate it into setValues(key, new String[]{null} ). This is also the currently implementation! However, with the above semantics for handling setValues, this leads to the illogical result that setValue(key, null) != getValue(key, "DEFAULT"). So it might actually be better to reimplement setValue(key, null) as setValues(key, null) -> setValues(key, new String [0] ). If we then implement the above, I think there are actually only a few changes to make based on the original implementation (so after first reverting the current changes): a) in getValue: if getValues(...) returns a array length 0 ([]), return null, else return either the first array element or the default if either null b) in setValue: delegate to setValues(key, value == null ? null : new String[] {value} ) c) in setValues: if value == null then value = new String [0] I've tested these changes against the TCK and Pluto now still passes all tests, as is Jetspeed using this version of Pluto. However, the Pluto testsuite now fails on the PreferenceCommonTest.checkSetPreferenceNull test, which still assumed the original behavior (setting a null value resulting in the default being returned from getValue). So, if to implement these logic changes, that test now has to be adjusted likewise (returning null instead of the provided default). Please note that it depends on the PortletPreferenceImpl backing model if String [0] actually gets "stored"... For Pluto which by default only implements a transient/in-memory model, setting a String [0] value will be retained. Jetspeed however, which uses database persistency, this currently will result in the existing value(s) being destroyed (and thus fallback to returning the "default" values). We probably should review if we can/should adapt Jetspeed for this new interpretation of the spec, but technically the above changes won't change its current behavior AFAIK. I'll preliminary implement these proposed changes so they can be properly reviewed and tested. If this still isn't according to the desired interpretation it is pretty easy to revert again or adjust otherwise.
        Hide
        Eric Dalquist added a comment -

        I disagree, there is an explicit: PortletPreferences.reset(String) method which is used to reset a preference to its default value, the API does not give any way to remove a preference, only to reset it to its default value.

        Also setValue states that only null keys are NOT allowed and more importantly setValues states that "null values in the values parameter are allowed." which means that if I store "new String[]

        {null}" that should actually be stored for me by the portal and I should be able to retrieve that value. Further more since setValue(key, value) is simply short hand for setValues(key, new String[] { value }) calling setValue(key, null) should also store a String array length 1 that contains a single null value.

        This is further confirmed by PLT.17.1 which states "Preferences attributes can be set to null." and if a value is set to null it is NOT the same as "no value being associated", a null entry in an array is still a value that has been associated with the key.

        I can see reverting part of the change since getValue states that "A null value is treated as a non-existent value". While getValues has the same statement I'd assume that meant the array as a whole.

        Taking the javadoc of getValue into account I believe the following behavior is what should be happening:

        Calling either setValue(key, null) OR setValues(key, new String[] {null}

        ) should result in:
        getValue(key, "DEFAULT") == "DEFAULT"
        getValues(key, new String[]

        { "DEFAULT" }

        ) = [null]

        I believe this is the case since setValue(key, value) is simply short hand for setValues(key, new String[]

        { value }

        ) and since a String array of length 1 has been stored getValues must return that array even though its only value is null.

        Show
        Eric Dalquist added a comment - I disagree, there is an explicit: PortletPreferences.reset(String) method which is used to reset a preference to its default value, the API does not give any way to remove a preference, only to reset it to its default value. Also setValue states that only null keys are NOT allowed and more importantly setValues states that "null values in the values parameter are allowed." which means that if I store "new String[] {null}" that should actually be stored for me by the portal and I should be able to retrieve that value. Further more since setValue(key, value) is simply short hand for setValues(key, new String[] { value }) calling setValue(key, null) should also store a String array length 1 that contains a single null value. This is further confirmed by PLT.17.1 which states "Preferences attributes can be set to null." and if a value is set to null it is NOT the same as "no value being associated", a null entry in an array is still a value that has been associated with the key. I can see reverting part of the change since getValue states that "A null value is treated as a non-existent value". While getValues has the same statement I'd assume that meant the array as a whole. Taking the javadoc of getValue into account I believe the following behavior is what should be happening: Calling either setValue(key, null) OR setValues(key, new String[] {null} ) should result in: getValue(key, "DEFAULT") == "DEFAULT" getValues(key, new String[] { "DEFAULT" } ) = [null] I believe this is the case since setValue(key, value) is simply short hand for setValues(key, new String[] { value } ) and since a String array of length 1 has been stored getValues must return that array even though its only value is null.
        Hide
        Ate Douma added a comment -

        I locally reverted these changes to PortletPreferencesImpl and now get all TCK tests succeed again.

        Show
        Ate Douma added a comment - I locally reverted these changes to PortletPreferencesImpl and now get all TCK tests succeed again.
        Hide
        Ate Douma added a comment -

        Looking further into both the code for the test and the javadoc for the PortletPreferences#getValue and #getValues methods, I think the previous behavior, returning the provided defaultValue(s) when either no or null values were retrieved for the preference, actually was correct. That is: according to the specification.

        The javadoc says (for both methods):
        "If there are no preference values associated with the given key, or the backing preference database is unavailable, it returns the given default value. A null value is treated as a non-existent value."

        This can be read as: if existing preference values are overridden with a null value array, this should be treated as "removing" the current values for the preference (making it "non-existent").

        The current TCK tests use a "default" portletpreference configuration in the portlet.xml where the key (preferredLanguage or preferredLanguages) is defined, but without any value (e.g. non-existent or null, no way to distinguish these two), and expect the provided default value(s) on the #getValue and #getValues methods to be returned then instead.

        While this could be debated if it actually is the desired behavior, as this effectively makes it impossible to explicitly store a NULL value for a preference, not to be "overridden" by a provided default value,
        that use-case isn't supported by the specification. And, as we at least should abide to the specification, I think the change implemented by the issue should be reverted.

        Show
        Ate Douma added a comment - Looking further into both the code for the test and the javadoc for the PortletPreferences#getValue and #getValues methods, I think the previous behavior, returning the provided defaultValue(s) when either no or null values were retrieved for the preference, actually was correct. That is: according to the specification. The javadoc says (for both methods): "If there are no preference values associated with the given key, or the backing preference database is unavailable, it returns the given default value. A null value is treated as a non-existent value." This can be read as: if existing preference values are overridden with a null value array, this should be treated as "removing" the current values for the preference (making it "non-existent"). The current TCK tests use a "default" portletpreference configuration in the portlet.xml where the key (preferredLanguage or preferredLanguages) is defined, but without any value (e.g. non-existent or null, no way to distinguish these two), and expect the provided default value(s) on the #getValue and #getValues methods to be returned then instead. While this could be debated if it actually is the desired behavior, as this effectively makes it impossible to explicitly store a NULL value for a preference, not to be "overridden" by a provided default value, that use-case isn't supported by the specification. And, as we at least should abide to the specification, I think the change implemented by the issue should be reverted.
        Hide
        Ate Douma added a comment - - edited

        com/sun/ts/tests/portlet/api/javax_portlet/PortletPreferences/URLClient.java#GetValueDefaultTest:
        --> com.sun.ts.tests.portlet.api.javax_portlet.PortletPreferences.GetValueDefaultTestPortlet.java:
        Expected default preference value= Java
        Actual default preference value = null

        com/sun/ts/tests/portlet/api/javax_portlet/PortletPreferences/URLClient.java#GetValuesDefaultTest:
        --> --> com.sun.ts.tests.portlet.api.javax_portlet.PortletPreferences.GetValuesDefaultTestPortlet.java:
        PortletPreference.getValues() did not return expected results
        Actual list returned is null. Expected list is not

        Show
        Ate Douma added a comment - - edited com/sun/ts/tests/portlet/api/javax_portlet/PortletPreferences/URLClient.java#GetValueDefaultTest: --> com.sun.ts.tests.portlet.api.javax_portlet.PortletPreferences.GetValueDefaultTestPortlet.java: Expected default preference value= Java Actual default preference value = null com/sun/ts/tests/portlet/api/javax_portlet/PortletPreferences/URLClient.java#GetValuesDefaultTest: --> --> com.sun.ts.tests.portlet.api.javax_portlet.PortletPreferences.GetValuesDefaultTestPortlet.java: PortletPreference.getValues() did not return expected results Actual list returned is null. Expected list is not
        Hide
        Eric Dalquist added a comment -

        Can you state which TCK test fails? I can take a look to see if the TCK test is following the spec rules.

        Show
        Eric Dalquist added a comment - Can you state which TCK test fails? I can take a look to see if the TCK test is following the spec rules.
        Hide
        Ate Douma added a comment -

        I just ran the JSR-286 TCK against the pluto 2.0.3 branch as sanity check to validate if its still compliant before we can start the release process, and it failed on two PortletPreferences checks.
        It seems the changes introduced by this issue are not compliant with the specification, or the TCK tests are not, this first needs further review.

        Show
        Ate Douma added a comment - I just ran the JSR-286 TCK against the pluto 2.0.3 branch as sanity check to validate if its still compliant before we can start the release process, and it failed on two PortletPreferences checks. It seems the changes introduced by this issue are not compliant with the specification, or the TCK tests are not, this first needs further review.
        Hide
        Eric Dalquist added a comment -

        Fixed logic to only return the default value if the PortletPreference itself was not found. If a PortletPreference was found the value or values are always returned, even if null.

        Show
        Eric Dalquist added a comment - Fixed logic to only return the default value if the PortletPreference itself was not found. If a PortletPreference was found the value or values are always returned, even if null.

          People

          • Assignee:
            Ate Douma
            Reporter:
            Eric Dalquist
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development