OpenJPA
  1. OpenJPA
  2. OPENJPA-1025

AbstractResultList.subList throws UnsupportedOperationException

    Details

      Description

      AbstractResultList implements the basics for readonly result lists. When calling the subList(int,int) method, the following exception is raised:

      java.lang.UnsupportedOperationException
      at org.apache.openjpa.lib.rop.AbstractResultList.subList(AbstractResultList.java:84)
      at org.apache.openjpa.kernel.DelegatingResultList.subList(DelegatingResultList.java:308)
      ...

      Since the subList() method contract is to create a new List from the ResultList, this operation does not modify the original list: it only provides a view on the original list (see http://java.sun.com/docs/books/tutorial/collections/interfaces/list.html ).

      This problem is also found by other users: http://n2.nabble.com/DelegatingResultList.subList-not-implemented--td210389.html
      They found the (bad) workaround to build a new List (bad because this is not the same as calling subList()):

      List mySubList = new ArrayList(openjpaList).subList(from, to);

      The AbstractResultList class should be modified by one of this solution (sorted by decreasing preference order):
      1) the AbstractResultList class should extends java.util.AbstractList and the subList() method should be removed (because implemented by AbstractList)
      2) the subList() method should be implemented to return a view on the original list. See java.util.AbstractList for an implementation (http://www.koders.com/java/fidCFCB47A1819AB345234CC04B6A1EA7554C2C17C0.aspx?s=iso+3166 )
      3) the subList() method should throw the exception with the message "this method is not yet implemented. Workaround: new ArrayList(openjpaList).subList(from, to)"

      1. OPENJPA-1025-withTest.patch
        6 kB
        B.J. Reed
      2. OPENJPA-1025b.patch
        3 kB
        B.J. Reed
      3. OPENJPA-1025.patch
        3 kB
        B.J. Reed

        Activity

        Julien Kronegg created issue -
        Julien Kronegg made changes -
        Field Original Value New Value
        Original Estimate 2h [ 7200 ] 4h [ 14400 ]
        Remaining Estimate 2h [ 7200 ] 4h [ 14400 ]
        Description AbstractResultList implements the basics for readonly result lists. When calling the subList(int,int) method, the following exception is raised:

            java.lang.UnsupportedOperationException
            at org.apache.openjpa.lib.rop.AbstractResultList.subList(AbstractResultList.java:84)
            at org.apache.openjpa.kernel.DelegatingResultList.subList(DelegatingResultList.java:308)
            ...

        Since the subList() method contract is to create a new List from the ResultList, this operation does not modify the original list.
        Thus, the subList() method should not raise an exception but should return a new list such as:

            return new ArrayList(openjpaList).subList(from, to);

        This problem is also found by other users: http://n2.nabble.com/DelegatingResultList.subList-not-implemented--td210389.html
        AbstractResultList implements the basics for readonly result lists. When calling the subList(int,int) method, the following exception is raised:

            java.lang.UnsupportedOperationException
            at org.apache.openjpa.lib.rop.AbstractResultList.subList(AbstractResultList.java:84)
            at org.apache.openjpa.kernel.DelegatingResultList.subList(DelegatingResultList.java:308)
            ...

        Since the subList() method contract is to create a new List from the ResultList, this operation does not modify the original list: it only provides a *view* on the original list (see http://java.sun.com/docs/books/tutorial/collections/interfaces/list.html ).

        This problem is also found by other users: http://n2.nabble.com/DelegatingResultList.subList-not-implemented--td210389.html
        They found the (bad) workaround to build a new List (bad because this is not the same as calling subList()):

            List mySubList = new ArrayList(openjpaList).subList(from, to);

        The AbstractResultList class should be modified by one of this solution (sorted by decreasing preference order):
        1) the AbstractResultList class should extends java.util.AbstractList and the subList() method should be removed (because implemented by AbstractList)
        2) the subList() method should be implemented to return a view on the original list. See java.util.AbstractList for an implementation (http://www.koders.com/java/fidCFCB47A1819AB345234CC04B6A1EA7554C2C17C0.aspx?s=iso+3166 )
        3) the subList() method should throw the exception with the message "this method is not yet implemented. Workaround: new ArrayList(openjpaList).subList(from, to)"
        Hide
        B.J. Reed added a comment -

        Attaching patch OPENJPA-1025.patch. Update AbstractResultList to not implement subList, which forces subclasses to implement subList. Update 3 classes that extend AbstractResultList to implement subList and return the proper List.

        Show
        B.J. Reed added a comment - Attaching patch OPENJPA-1025 .patch. Update AbstractResultList to not implement subList, which forces subclasses to implement subList. Update 3 classes that extend AbstractResultList to implement subList and return the proper List.
        B.J. Reed made changes -
        Attachment OPENJPA-1025.patch [ 12405947 ]
        Hide
        Julien Kronegg added a comment -

        I'm not sure about the patch content for AbstractNonSequentialResultList ( https://issues.apache.org/jira/secure/attachment/12405947/OPENJPA-1025.patch ). In this class, the workaround "new ArrayList()" is used.
        The List.subList(int,int) contract is to provide a view on the original list, not a shallow copy (see "Range-View Operation" section in http://java.sun.com/docs/books/tutorial/collections/interfaces/list.html ). Thus, operations on the subList are in fact done on the original List.

        Let's try with the following code:

        myAbstractNonSequentialResultList.subList(first, last).add(myElement);

        If the AbstractNonSequentialResultList class implements the List.subList contract correctly, this should raise an exception because the underlying AbstractResultList is read only (this is the expected result). But with the patch, the above code works correctly and the programmer may think that the underlying List has been modified, while it has still the same number of elements.

        So IMHO, the patch is correct, except for class AbstractNonSequentialResultList which should use a true view of the original List and not a shallow copy. For this class, I see 3 solutions:
        1) the subList method return a true view of the original List and not a shallow copy: this would be the best solution (See java.util.AbstractList for an implementation)
        2) the subList method throws a "not implemented exception": this would be a quickfix (I'm not sure it would solve the issue)
        3) the subList method return a shallow copy of the original List (i.e. new ArrayList()): this solution is not desirable because it does not respect the List.subList contract as described above.

        Show
        Julien Kronegg added a comment - I'm not sure about the patch content for AbstractNonSequentialResultList ( https://issues.apache.org/jira/secure/attachment/12405947/OPENJPA-1025.patch ). In this class, the workaround "new ArrayList()" is used. The List.subList(int,int) contract is to provide a view on the original list, not a shallow copy (see "Range-View Operation" section in http://java.sun.com/docs/books/tutorial/collections/interfaces/list.html ). Thus, operations on the subList are in fact done on the original List. Let's try with the following code: myAbstractNonSequentialResultList.subList(first, last).add(myElement); If the AbstractNonSequentialResultList class implements the List.subList contract correctly, this should raise an exception because the underlying AbstractResultList is read only (this is the expected result). But with the patch, the above code works correctly and the programmer may think that the underlying List has been modified, while it has still the same number of elements. So IMHO, the patch is correct, except for class AbstractNonSequentialResultList which should use a true view of the original List and not a shallow copy. For this class, I see 3 solutions: 1) the subList method return a true view of the original List and not a shallow copy: this would be the best solution (See java.util.AbstractList for an implementation) 2) the subList method throws a "not implemented exception": this would be a quickfix (I'm not sure it would solve the issue) 3) the subList method return a shallow copy of the original List (i.e. new ArrayList()): this solution is not desirable because it does not respect the List.subList contract as described above.
        Hide
        B.J. Reed added a comment -

        Thanks for the quick review Julien.

        Attaching OPENJPA-1025b.patch. For the AbstractNonSequentialResultList case, I think we're going to have to just throw a "not implemented exception". The concrete classes for this use Object[] underneath in stead of a List. I don't know of a way to create a List "view" on to an Object[] and I agree that an exception would be better than returning a modifiable List. Also, logically, I really wasn't sure that getting a sub list on an object that is "non sequential" was really a good idea - doesn't seem like the data would be consistently returned.

        Show
        B.J. Reed added a comment - Thanks for the quick review Julien. Attaching OPENJPA-1025 b.patch. For the AbstractNonSequentialResultList case, I think we're going to have to just throw a "not implemented exception". The concrete classes for this use Object[] underneath in stead of a List. I don't know of a way to create a List "view" on to an Object[] and I agree that an exception would be better than returning a modifiable List. Also, logically, I really wasn't sure that getting a sub list on an object that is "non sequential" was really a good idea - doesn't seem like the data would be consistently returned.
        B.J. Reed made changes -
        Attachment OPENJPA-1025b.patch [ 12406031 ]
        Hide
        B.J. Reed added a comment -

        OPENJPA-1025-withTest.patch includes the code changes from 1025b.patch and an update to the ResultListTest to verify if subList is a supported method or not for each kind of AbstractResultList.

        Show
        B.J. Reed added a comment - OPENJPA-1025 -withTest.patch includes the code changes from 1025b.patch and an update to the ResultListTest to verify if subList is a supported method or not for each kind of AbstractResultList.
        B.J. Reed made changes -
        Attachment OPENJPA-1025-withTest.patch [ 12406035 ]
        Hide
        Julien Kronegg added a comment -

        I applied https://issues.apache.org/jira/secure/attachment/12406031/OPENJPA-1025b.patch : the UnsupportedOperationException is not raised anymore when using subList in my application (Seam CRUD application's EntityList.xhtml).
        Thanks B.J., you can apply the patch as a solution.

        Show
        Julien Kronegg added a comment - I applied https://issues.apache.org/jira/secure/attachment/12406031/OPENJPA-1025b.patch : the UnsupportedOperationException is not raised anymore when using subList in my application (Seam CRUD application's EntityList.xhtml). Thanks B.J., you can apply the patch as a solution.
        Michael Dick made changes -
        Assignee B.J. Reed [ bjreed ]
        Hide
        Michael Dick added a comment -

        I'm not sure removing the method from AbstractList is the best solution for released versions since we could break downstream consumers. Merely overriding the subList method in the appropriate subclasses seems like a better solution in this case.

        Show
        Michael Dick added a comment - I'm not sure removing the method from AbstractList is the best solution for released versions since we could break downstream consumers. Merely overriding the subList method in the appropriate subclasses seems like a better solution in this case.
        Michael Dick made changes -
        Fix Version/s 1.0.4 [ 12313301 ]
        Fix Version/s 1.2.2 [ 12313681 ]
        Fix Version/s 1.3.0 [ 12313326 ]
        Fix Version/s 2.0.0 [ 12313483 ]
        Michael Dick made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 1.0.4 [ 12313301 ]
        Fix Version/s 1.3.0 [ 12313326 ]
        Fix Version/s 2.0.0 [ 12313483 ]
        Fix Version/s 1.2.2 [ 12313681 ]
        Resolution Fixed [ 1 ]
        Hide
        Julien Kronegg added a comment -

        I agree. The subList method can be kept in AbstractResultList (BTW it's not AbstractList) and the subclasses overriden subList method will be called.
        This is a more "backward compatible" fix.

        Show
        Julien Kronegg added a comment - I agree. The subList method can be kept in AbstractResultList (BTW it's not AbstractList) and the subclasses overriden subList method will be called. This is a more "backward compatible" fix.
        Michael Dick made changes -
        Fix Version/s 1.0.4 [ 12313301 ]
        Fix Version/s 1.2.2 [ 12313681 ]
        Fix Version/s 1.3.0 [ 12313326 ]
        Fix Version/s 2.0.0 [ 12314019 ]
        Donald Woods made changes -
        Fix Version/s 2.0.0-beta [ 12314149 ]
        Fix Version/s 2.0.0 [ 12314019 ]
        Donald Woods made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            B.J. Reed
            Reporter:
            Julien Kronegg
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 4h
              4h
              Remaining:
              Remaining Estimate - 4h
              4h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development