OpenJPA
  1. OpenJPA
  2. OPENJPA-1719

Prepared SQL cache ordering problem with subqueries.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: 2.0.1, 2.1.0
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      I've found what appears to be an ordering issue with subqueries and the prepared SQL cache. The attached patch shows where I think the problem lies and adds a test method that shows the problem.

      To summarize: When the prepared SQL cache is enabled we reorder the parameter values provided by the user. If a query contains named parameters and a subquery which also contains named parameters the placement of the subquery becomes important.

      The following query will work :
      SELECT p FROM Person p WHERE p.id IN (SELECT p1.id FROM Person p1 WHERE p1.lastUpdated >= :date ) AND p.name = :name

      But this one fails with a SQLDataException.
      SELECT p FROM Person p WHERE p.name = :name AND p.id IN (SELECT p1.id FROM Person p1 WHERE p1.lastUpdated >= :date )

      Assuming that the query is executed something like this :
      Query query = em.createQuery(query);
      query.setParameter("name", "mike");
      query.setParameter("date", new java.sql.Date(1005397));

      1. OpenJPA-trunk_OJ1719.testcase.patch
        2 kB
        Martin Dirichs
      2. sql-cache-subqordering.diff.txt
        4 kB
        Michael Dick

        Activity

        Michael Dick created issue -
        Hide
        Michael Dick added a comment -

        Attaching a patch that illustrates the problem using the existing TestPreparedQueryCache testcase.

        Show
        Michael Dick added a comment - Attaching a patch that illustrates the problem using the existing TestPreparedQueryCache testcase.
        Michael Dick made changes -
        Field Original Value New Value
        Attachment sql-cache-subqordering.diff.txt [ 12448882 ]
        Catalina Wei made changes -
        Assignee Catalina Wei [ fancy ]
        Hide
        Catalina Wei added a comment -

        Fix checked in under trunk at revision 963139.

        Show
        Catalina Wei added a comment - Fix checked in under trunk at revision 963139.
        Catalina Wei made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Martin Dirichs added a comment -

        The fix as committed with revision 963139 still has bugs. An IndexOutOfBoundsException
        is thrown caused by line 174 in SQLBuffer. Sample stack trace:

        java.util.ArrayList.addAll(ArrayList.java:497)
        org.apache.openjpa.jdbc.sql.SQLBuffer.append(SQLBuffer.java:174)
        org.apache.openjpa.jdbc.sql.SQLBuffer.resolveSubselects(SQLBuffer.java:521)
        org.apache.openjpa.jdbc.sql.SQLBuffer.getSQL(SQLBuffer.java:477)
        org.apache.openjpa.jdbc.sql.SQLBuffer.getSQL(SQLBuffer.java:467)
        org.apache.openjpa.jdbc.sql.SQLBuffer.prepareStatement(SQLBuffer.java:563)
        org.apache.openjpa.jdbc.sql.SQLBuffer.prepareStatement(SQLBuffer.java:543)
        org.apache.openjpa.jdbc.sql.SelectImpl.prepareStatement(SelectImpl.java:479)
        org.apache.openjpa.jdbc.sql.SelectImpl.execute(SelectImpl.java:420)
        org.apache.openjpa.jdbc.sql.SelectImpl.execute(SelectImpl.java:391)
        org.apache.openjpa.jdbc.sql.LogicalUnion$UnionSelect.execute(LogicalUnion.java:427)
        org.apache.openjpa.jdbc.sql.LogicalUnion.execute(LogicalUnion.java:230)
        org.apache.openjpa.jdbc.sql.LogicalUnion.execute(LogicalUnion.java:220)
        org.apache.openjpa.jdbc.kernel.SelectResultObjectProvider.open(SelectResultObjectProvider.java:94)
        org.apache.openjpa.kernel.QueryImpl$PackingResultObjectProvider.open(QueryImpl.java:2068)
        org.apache.openjpa.lib.rop.EagerResultList.<init>(EagerResultList.java:34)
        org.apache.openjpa.kernel.QueryImpl.toResult(QueryImpl.java:1246)
        org.apache.openjpa.kernel.QueryImpl.execute(QueryImpl.java:1005)
        org.apache.openjpa.kernel.QueryImpl.execute(QueryImpl.java:861)
        org.apache.openjpa.kernel.QueryImpl.execute(QueryImpl.java:792)
        org.apache.openjpa.kernel.DelegatingQuery.execute(DelegatingQuery.java:542)
        org.apache.openjpa.persistence.QueryImpl.execute(QueryImpl.java:288)
        org.apache.openjpa.persistence.QueryImpl.getResultList(QueryImpl.java:302)
        ... [code outside of OpenJPA)]

        The problematic line in context is (lines 169++):

        if (buf._userIndex != null) {
        if (_userIndex == null)

        { _userIndex = new ArrayList(); _userIndex.addAll(buf._userIndex); }

        else
        _userIndex.addAll(paramIndex*2, buf._userIndex); // Wrong index leads to exception
        }

        These lines could be simplified to

        if (buf._userIndex != null)

        { if (_userIndex == null) _userIndex = new ArrayList(); _userIndex.addAll(buf._userIndex); }

        because it always suffices to append the elements of buf._userIndex
        to this._userIndex no matter what the paramIndex is. The structure
        of _userIndex is a list where each parameter is already prepended
        by its position in the sql buffer. Thus, the order of parameters in
        _userIndex is unimportant.

        Also, the code in line 184++ seems suspicious:

        if (_userIndex != null) {
        // fix up user parameter index
        for (int i = 0; i < _userIndex.size(); i+=2)

        { _userIndex.set(i, _userParams.indexOf(_userIndex.get(i+1))); }

        }

        Here, the indizes in _userIndex get corrected by looking into the
        _userParams - list, which contains the correct position. Not only
        does this implementation rely on a certain equals()-semantic when
        using _userParams.indexOf(). As I understand it, the reason for
        storing the user parameters in the list _userIndex with
        (position, parameter) pairs was that there may be multiple positions
        for the same parameter in the resulting statement. Does the
        code above really take into account these cases? I've not
        tested it, but it looks suspicious. You may have a look at my
        patch submitted with OPENJPA-1584 which takes a safer
        approach to adjust the positions in _userIndex.

        On a side note, I was a bit surprised to see many
        if-statements such as in line 131:

        if (sqlIndex == _sql.length())
        _sql.append(buf._sql.toString());
        else
        _sql.insert(sqlIndex, buf._sql.toString());

        This test is useless and these lines could just
        be replaced with

        _sql.insert(sqlIndex, buf._sql.toString());

        A similar if-statement is on line 145:

        if (paramIndex == _params.size())

        { _params.addAll(buf._params); [...] // code that is duplicated below }

        else

        { _params.addAll(paramIndex, buf._params); [...] // mere duplication of code above }

        The resulting code duplication made it more
        troublesome than necessary to review this fix.
        (Indeed, the reported IndexOutOfBounds-exception
        only shows up in one branch of the line 145 if-statement,
        making it harder to replicate the bug.)

        Show
        Martin Dirichs added a comment - The fix as committed with revision 963139 still has bugs. An IndexOutOfBoundsException is thrown caused by line 174 in SQLBuffer. Sample stack trace: java.util.ArrayList.addAll(ArrayList.java:497) org.apache.openjpa.jdbc.sql.SQLBuffer.append(SQLBuffer.java:174) org.apache.openjpa.jdbc.sql.SQLBuffer.resolveSubselects(SQLBuffer.java:521) org.apache.openjpa.jdbc.sql.SQLBuffer.getSQL(SQLBuffer.java:477) org.apache.openjpa.jdbc.sql.SQLBuffer.getSQL(SQLBuffer.java:467) org.apache.openjpa.jdbc.sql.SQLBuffer.prepareStatement(SQLBuffer.java:563) org.apache.openjpa.jdbc.sql.SQLBuffer.prepareStatement(SQLBuffer.java:543) org.apache.openjpa.jdbc.sql.SelectImpl.prepareStatement(SelectImpl.java:479) org.apache.openjpa.jdbc.sql.SelectImpl.execute(SelectImpl.java:420) org.apache.openjpa.jdbc.sql.SelectImpl.execute(SelectImpl.java:391) org.apache.openjpa.jdbc.sql.LogicalUnion$UnionSelect.execute(LogicalUnion.java:427) org.apache.openjpa.jdbc.sql.LogicalUnion.execute(LogicalUnion.java:230) org.apache.openjpa.jdbc.sql.LogicalUnion.execute(LogicalUnion.java:220) org.apache.openjpa.jdbc.kernel.SelectResultObjectProvider.open(SelectResultObjectProvider.java:94) org.apache.openjpa.kernel.QueryImpl$PackingResultObjectProvider.open(QueryImpl.java:2068) org.apache.openjpa.lib.rop.EagerResultList.<init>(EagerResultList.java:34) org.apache.openjpa.kernel.QueryImpl.toResult(QueryImpl.java:1246) org.apache.openjpa.kernel.QueryImpl.execute(QueryImpl.java:1005) org.apache.openjpa.kernel.QueryImpl.execute(QueryImpl.java:861) org.apache.openjpa.kernel.QueryImpl.execute(QueryImpl.java:792) org.apache.openjpa.kernel.DelegatingQuery.execute(DelegatingQuery.java:542) org.apache.openjpa.persistence.QueryImpl.execute(QueryImpl.java:288) org.apache.openjpa.persistence.QueryImpl.getResultList(QueryImpl.java:302) ... [code outside of OpenJPA)] The problematic line in context is (lines 169++): if (buf._userIndex != null) { if (_userIndex == null) { _userIndex = new ArrayList(); _userIndex.addAll(buf._userIndex); } else _userIndex.addAll(paramIndex*2, buf._userIndex); // Wrong index leads to exception } These lines could be simplified to if (buf._userIndex != null) { if (_userIndex == null) _userIndex = new ArrayList(); _userIndex.addAll(buf._userIndex); } because it always suffices to append the elements of buf._userIndex to this._userIndex no matter what the paramIndex is. The structure of _userIndex is a list where each parameter is already prepended by its position in the sql buffer. Thus, the order of parameters in _userIndex is unimportant. Also, the code in line 184++ seems suspicious: if (_userIndex != null) { // fix up user parameter index for (int i = 0; i < _userIndex.size(); i+=2) { _userIndex.set(i, _userParams.indexOf(_userIndex.get(i+1))); } } Here, the indizes in _userIndex get corrected by looking into the _userParams - list, which contains the correct position. Not only does this implementation rely on a certain equals()-semantic when using _userParams.indexOf(). As I understand it, the reason for storing the user parameters in the list _userIndex with (position, parameter) pairs was that there may be multiple positions for the same parameter in the resulting statement. Does the code above really take into account these cases? I've not tested it, but it looks suspicious. You may have a look at my patch submitted with OPENJPA-1584 which takes a safer approach to adjust the positions in _userIndex. On a side note, I was a bit surprised to see many if-statements such as in line 131: if (sqlIndex == _sql.length()) _sql.append(buf._sql.toString()); else _sql.insert(sqlIndex, buf._sql.toString()); This test is useless and these lines could just be replaced with _sql.insert(sqlIndex, buf._sql.toString()); A similar if-statement is on line 145: if (paramIndex == _params.size()) { _params.addAll(buf._params); [...] // code that is duplicated below } else { _params.addAll(paramIndex, buf._params); [...] // mere duplication of code above } The resulting code duplication made it more troublesome than necessary to review this fix. (Indeed, the reported IndexOutOfBounds-exception only shows up in one branch of the line 145 if-statement, making it harder to replicate the bug.)
        Hide
        Catalina Wei added a comment -

        Martin Dirichs,
        Could you provide a test case that reproduce the IndexOutOfBoundsException ?

        Thanks.

        Show
        Catalina Wei added a comment - Martin Dirichs, Could you provide a test case that reproduce the IndexOutOfBoundsException ? Thanks.
        Catalina Wei made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Catalina Wei added a comment -

        Some debugging work validates that in what order the parameters are added to _userIndex does not matter, because the positions of the parameters in _userIndex are corrected by look up the Param object in _userParams.

        The current code block in SQLBuffer.java (lines 169++):
        (1)
        if (buf._userIndex != null) {
        if (_userIndex == null)

        { _userIndex = new ArrayList(); _userIndex.addAll(buf._userIndex); }

        else
        _userIndex.addAll(paramIndex*2, buf._userIndex); // Wrong index leads to exception
        }

        Can be simplified to
        (2)
        if (buf._userIndex != null)

        { if (_userIndex == null) _userIndex = new ArrayList(); _userIndex.addAll(buf._userIndex); }

        There should be no IndexOutOfBoundException in (1). but (2) is more simplified code.

        Show
        Catalina Wei added a comment - Some debugging work validates that in what order the parameters are added to _userIndex does not matter, because the positions of the parameters in _userIndex are corrected by look up the Param object in _userParams. The current code block in SQLBuffer.java (lines 169++): (1) if (buf._userIndex != null) { if (_userIndex == null) { _userIndex = new ArrayList(); _userIndex.addAll(buf._userIndex); } else _userIndex.addAll(paramIndex*2, buf._userIndex); // Wrong index leads to exception } Can be simplified to (2) if (buf._userIndex != null) { if (_userIndex == null) _userIndex = new ArrayList(); _userIndex.addAll(buf._userIndex); } There should be no IndexOutOfBoundException in (1). but (2) is more simplified code.
        Hide
        Martin Dirichs added a comment -

        You find a test case attached in file OpenJPA-trunk_OJ1719.testcase.patch.

        Yes, with lines 172-174 simplified the code should work correctly.

        Perhaps you also find value in my objections to code safety and code duplication stated above.

        Show
        Martin Dirichs added a comment - You find a test case attached in file OpenJPA-trunk_OJ1719.testcase.patch. Yes, with lines 172-174 simplified the code should work correctly. Perhaps you also find value in my objections to code safety and code duplication stated above.
        Martin Dirichs made changes -
        Attachment OpenJPA-trunk_OJ1719.testcase.patch [ 12450432 ]
        Hide
        Catalina Wei added a comment -

        Martin,
        Thank you very much for the test case, it produced the IndexOutOfBoundException.
        I will commit the fix soon.

        Show
        Catalina Wei added a comment - Martin, Thank you very much for the test case, it produced the IndexOutOfBoundException. I will commit the fix soon.
        Hide
        Catalina Wei added a comment -

        Fix checked in under trunk revision 979326.
        Mike,
        Could you please apply the same fix to 2.0.x branch ?
        Thanks

        Show
        Catalina Wei added a comment - Fix checked in under trunk revision 979326. Mike, Could you please apply the same fix to 2.0.x branch ? Thanks
        Catalina Wei made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Fix Version/s 2.0.1 [ 12314532 ]
        Resolution Fixed [ 1 ]
        Michael Dick committed 979375 (2 files)
        Donald Woods made changes -
        Fix Version/s 2.0.1 [ 12314532 ]
        Patch Info [Patch Available]
        Hide
        Michael Dick added a comment -

        Closing issues which have been resolved for some time. If the problem persists, please reopen.

        Show
        Michael Dick added a comment - Closing issues which have been resolved for some time. If the problem persists, please reopen.
        Michael Dick made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Catalina Wei
            Reporter:
            Michael Dick
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development