openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Martin Dirichs (JIRA)" <j...@apache.org>
Subject [jira] Commented: (OPENJPA-1719) Prepared SQL cache ordering problem with subqueries.
Date Sat, 24 Jul 2010 20:02:49 GMT

    [ https://issues.apache.org/jira/browse/OPENJPA-1719?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12892018#action_12892018
] 

Martin Dirichs commented on OPENJPA-1719:
-----------------------------------------

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.)


> Prepared SQL cache ordering problem with subqueries. 
> -----------------------------------------------------
>
>                 Key: OPENJPA-1719
>                 URL: https://issues.apache.org/jira/browse/OPENJPA-1719
>             Project: OpenJPA
>          Issue Type: Bug
>    Affects Versions: 2.0.0
>            Reporter: Michael Dick
>            Assignee: Catalina Wei
>             Fix For: 2.0.1, 2.1.0
>
>         Attachments: sql-cache-subqordering.diff.txt
>
>
> 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));  

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message