Pluto
  1. Pluto
  2. PLUTO-487

PortletPreferencesImpl should not store the preferences every time it is instantiated

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0-refactoring, 2.0.0
    • Fix Version/s: 2.0-refactoring, 1.1.6, 2.0.0
    • Component/s: portlet container
    • Labels:
      None
    • Environment:
      Tomcat 5.5.26

      Description

      In Pluto 1.1.5, the constructor of org.apache.pluto.internal.impl.PortletPreferencesImpl calls internalStore() every time. It would be better to only store the preferences when it is actually necessary – I believe this is the case only when the prefs from portlet.xml have not yet been stored:
      ...
      Set portletXmlPrefNames = getPreferenceNames(defaultPreferences);
      Set persistedPrefNames = getPreferenceNames(storedPreferences);
      if (!persistedPrefNames.containsAll(portletXmlPrefNames))

      { internalStore(); }

      ...

      private Set getPreferenceNames(InternalPortletPreference[] prefs) {
      Set prefNames = new HashSet();
      for (int i = 0; i < prefs.length; i++)

      { prefNames.add(prefs[i].getName()); }

      return prefNames;
      }

        Activity

        Hide
        Nikita Dubrovsky added a comment - - edited

        Attached my proposed fix.

        Show
        Nikita Dubrovsky added a comment - - edited Attached my proposed fix.
        Hide
        Eric Dalquist added a comment -

        I'll take a look at the patch either today or early next week and see about applying it. I'll also take a look and see if the trunk needs this fix as well.

        Show
        Eric Dalquist added a comment - I'll take a look at the patch either today or early next week and see about applying it. I'll also take a look and see if the trunk needs this fix as well.
        Hide
        Eric Dalquist added a comment -

        The more I read PLT.14.1 and look at the code I'm not sure why the PortletPreferencesImpl calls internalStore() at all in the constructor. PLT.14.1 seems to be more worded towards ensuring the PortletPreferencesImpl does not contain stale preferences, especially since the spec doesn't address how preferences are actually persisted or the layering is done when loading them.

        As far as I can tell from the code the only downside of not calling internalStore() in the constructor is that the PortletPreferencesService does not get a chance to store the default preferences post-merge. This seems like a minor down-side as the PortletPreferencesService impl would have access to the default portlet.xml preferences via the PortletDD if it needed to store/use them. The PortletPreferencesImpl always loads the default prefs then loads the stored prefs and then merges them, no default preference information will be lost by removing the internalStore() call and PLT.14.1 should be fulfilled.

        I'll bring this up on the Pluto list as well but perhaps the internalStore() method can just be removed.

        Show
        Eric Dalquist added a comment - The more I read PLT.14.1 and look at the code I'm not sure why the PortletPreferencesImpl calls internalStore() at all in the constructor. PLT.14.1 seems to be more worded towards ensuring the PortletPreferencesImpl does not contain stale preferences, especially since the spec doesn't address how preferences are actually persisted or the layering is done when loading them. As far as I can tell from the code the only downside of not calling internalStore() in the constructor is that the PortletPreferencesService does not get a chance to store the default preferences post-merge. This seems like a minor down-side as the PortletPreferencesService impl would have access to the default portlet.xml preferences via the PortletDD if it needed to store/use them. The PortletPreferencesImpl always loads the default prefs then loads the stored prefs and then merges them, no default preference information will be lost by removing the internalStore() call and PLT.14.1 should be fulfilled. I'll bring this up on the Pluto list as well but perhaps the internalStore() method can just be removed.
        Hide
        Nikita Dubrovsky added a comment -

        I had assumed that internalStore() is called in the constructor because of the last paragraph in PLT.14.1. For me, it sounds good to not make the call at all, but let me double-check this in more detail tomorrow.

        Show
        Nikita Dubrovsky added a comment - I had assumed that internalStore() is called in the constructor because of the last paragraph in PLT.14.1. For me, it sounds good to not make the call at all, but let me double-check this in more detail tomorrow.
        Hide
        Nikita Dubrovsky added a comment - - edited

        Hi Eric, sorry for the delay! The one use-case I was considering is whether the portal might expect to be able to read a portlet's preferences from the persistent store, without the portlet having ever called store() explicitly. However, I don't think this is a valid use-case, because it assumes that the portlet must have retrieved its preferences at least once (which currently would store them). So, I think it makes sense to remove the call to internalStore() from the PortletPreferencesImpl constructor. Removing the call would also be very good for efficiency and performance; for example, right now if 1000 users view the same portlet, there will be 1000 copies of the same default preferences in the persistent store, even if none of the users actually changed the portlet's preferences.

        The one suggestion I have is to make internalStore() protected instead of private, so that the method can be used if someone wants to modify the behavior by extending the class.

        Show
        Nikita Dubrovsky added a comment - - edited Hi Eric, sorry for the delay! The one use-case I was considering is whether the portal might expect to be able to read a portlet's preferences from the persistent store, without the portlet having ever called store() explicitly. However, I don't think this is a valid use-case, because it assumes that the portlet must have retrieved its preferences at least once (which currently would store them). So, I think it makes sense to remove the call to internalStore() from the PortletPreferencesImpl constructor. Removing the call would also be very good for efficiency and performance; for example, right now if 1000 users view the same portlet, there will be 1000 copies of the same default preferences in the persistent store, even if none of the users actually changed the portlet's preferences. The one suggestion I have is to make internalStore() protected instead of private, so that the method can be used if someone wants to modify the behavior by extending the class.
        Hide
        Eric Dalquist added a comment -

        I'll remove the initial call and make internalStore() protected for subclasses. I should get this in within the next week so it will make 1.1.6

        Show
        Eric Dalquist added a comment - I'll remove the initial call and make internalStore() protected for subclasses. I should get this in within the next week so it will make 1.1.6
        Hide
        Eric Dalquist added a comment -

        Removed call to internalStore() in the constructor and made internalStore() a protected final method for sub-classes to use if needed.

        Show
        Eric Dalquist added a comment - Removed call to internalStore() in the constructor and made internalStore() a protected final method for sub-classes to use if needed.
        Hide
        Craig Doremus added a comment -

        This fix needs to be applied to trunk and 2.0-refactoring branch.

        Show
        Craig Doremus added a comment - This fix needs to be applied to trunk and 2.0-refactoring branch.
        Hide
        Eric Dalquist added a comment -

        Applied to refactoring branch

        Show
        Eric Dalquist added a comment - Applied to refactoring branch

          People

          • Assignee:
            Eric Dalquist
            Reporter:
            Nikita Dubrovsky
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development