Pluto
  1. Pluto
  2. PLUTO-589

alter return types in org.apache.pluto.container.om.portlet.Filter?

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.0.0
    • Fix Version/s: None
    • Component/s: portlet container
    • Labels:
      None

      Description

      The return type for the getInitParams() method in org.apache.pluto.container.om.portlet.Filter is List<? extends InitParam>.
      This presents an awkward scenario, one particularly present when creating unit tests for a FilterChain implementation and attempting to create mock Filter implementations.

      The requirement that getInitParams return a class that implements an interface that extends InitParam. You cannot simply return a class that implements InitParam; you would need to define your own custom interface that extends InitParam, and implement your custom interface.

      If the InitParam interface satisfactorily defines what's needed, the getInitParams return should be a class that simply implements InitParam.

      Is it possible to modify the return value for getInitParams to simply?
      List<InitParam>

      This also applies to the getDescriptions and getDisplayNames method in the same class; both have return values of List<? extends someinterface>, it should simply be List<interface>.

        Activity

        Hide
        Nicholas Blair added a comment -

        Example test for a FilterChain impl, attempting to create a mock filter:

        Filter mockFilter = EasyMock.createMock(Filter.class);

        EasyMock.expect(mockFilter.getFilterClass()).andReturn("org.jasig.portal.portlet.container.MockRenderFilter");
        EasyMock.expect(mockFilter.getFilterName()).andReturn("mockFilterName");

        List<InitParam> params = new ArrayList<InitParam>();
        EasyMock.expect(mockFilter.getInitParams()).andReturn(params);

        These lines will not compile, since the return type of getInitParams() does not match the type of the params variable.
        One cannot switch the params variable to a concrete implementation of InitParam, like so:

        List<InitParamType> params = new ArrayList<InitParamType>();

        Also a compilation failure, due to type mismatch.

        This may simply be a problem to pin on EasyMock and not Pluto, however it is a somewhat awkward return type.

        Show
        Nicholas Blair added a comment - Example test for a FilterChain impl, attempting to create a mock filter: Filter mockFilter = EasyMock.createMock(Filter.class); EasyMock.expect(mockFilter.getFilterClass()).andReturn("org.jasig.portal.portlet.container.MockRenderFilter"); EasyMock.expect(mockFilter.getFilterName()).andReturn("mockFilterName"); List<InitParam> params = new ArrayList<InitParam>(); EasyMock.expect(mockFilter.getInitParams()).andReturn(params); These lines will not compile, since the return type of getInitParams() does not match the type of the params variable. One cannot switch the params variable to a concrete implementation of InitParam, like so: List<InitParamType> params = new ArrayList<InitParamType>(); Also a compilation failure, due to type mismatch. This may simply be a problem to pin on EasyMock and not Pluto, however it is a somewhat awkward return type.
        Hide
        David Jencks added a comment -

        I think this is a limitation of EasyMock and a good design on the part of pluto. I don't really understand what the problem is. If I change the types in pluto's FilterType implementation of Filter to the interfaces rather than concrete types, e.g. lines 76 and 197 InitParamType >> InitParam everything is fine. The point of this kind of <? extends interface> is so concrete implementations can use their own specialized concrete types internally without having to cast them every time they are used, whereas the framework using the concrete implementation can use the interface. This seems to me like exactly the situation <? extends interface> was designed for. Am I missing something?

        Show
        David Jencks added a comment - I think this is a limitation of EasyMock and a good design on the part of pluto. I don't really understand what the problem is. If I change the types in pluto's FilterType implementation of Filter to the interfaces rather than concrete types, e.g. lines 76 and 197 InitParamType >> InitParam everything is fine. The point of this kind of <? extends interface> is so concrete implementations can use their own specialized concrete types internally without having to cast them every time they are used, whereas the framework using the concrete implementation can use the interface. This seems to me like exactly the situation <? extends interface> was designed for. Am I missing something?
        Hide
        Eric Dalquist added a comment -

        It seems that if the goal is for implementations to use custom types the Filter interface itself should be parameterized.

        So instead of:

        public interface Filter

        { String getFilterName(); Description getDescription(Locale locale); List<? extends Description> getDescriptions(); Description addDescription(String lang); DisplayName getDisplayName(Locale locale); List<? extends DisplayName> getDisplayNames(); DisplayName addDisplayName(String lang); String getFilterClass(); void setFilterClass(String filterClass); InitParam getInitParam(String paramName); List<? extends InitParam> getInitParams(); InitParam addInitParam(String paramName); List<String> getLifecycles(); void addLifecycle(String lifecycle); }

        Do:
        public interface Filter<D extends Description, N extends DisplayName, I extends InitParam>

        { String getFilterName(); Description getDescription(Locale locale); List<D> getDescriptions(); Description addDescription(String lang); DisplayName getDisplayName(Locale locale); List<N> getDisplayNames(); DisplayName addDisplayName(String lang); String getFilterClass(); void setFilterClass(String filterClass); InitParam getInitParam(String paramName); List<I> getInitParams(); InitParam addInitParam(String paramName); List<String> getLifecycles(); void addLifecycle(String lifecycle); }

        Then you actually have a fully parameterized Filter object that can communicate the types used by the methods at the class level. From what I know of generics ? should only be used for fields if the actual type cannot be known. To allow the use case you're suggesting a fully parameterized class should be used.

        Show
        Eric Dalquist added a comment - It seems that if the goal is for implementations to use custom types the Filter interface itself should be parameterized. So instead of: public interface Filter { String getFilterName(); Description getDescription(Locale locale); List<? extends Description> getDescriptions(); Description addDescription(String lang); DisplayName getDisplayName(Locale locale); List<? extends DisplayName> getDisplayNames(); DisplayName addDisplayName(String lang); String getFilterClass(); void setFilterClass(String filterClass); InitParam getInitParam(String paramName); List<? extends InitParam> getInitParams(); InitParam addInitParam(String paramName); List<String> getLifecycles(); void addLifecycle(String lifecycle); } Do: public interface Filter<D extends Description, N extends DisplayName, I extends InitParam> { String getFilterName(); Description getDescription(Locale locale); List<D> getDescriptions(); Description addDescription(String lang); DisplayName getDisplayName(Locale locale); List<N> getDisplayNames(); DisplayName addDisplayName(String lang); String getFilterClass(); void setFilterClass(String filterClass); InitParam getInitParam(String paramName); List<I> getInitParams(); InitParam addInitParam(String paramName); List<String> getLifecycles(); void addLifecycle(String lifecycle); } Then you actually have a fully parameterized Filter object that can communicate the types used by the methods at the class level. From what I know of generics ? should only be used for fields if the actual type cannot be known. To allow the use case you're suggesting a fully parameterized class should be used.

          People

          • Assignee:
            Unassigned
            Reporter:
            Nicholas Blair
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development