Prepared SQL cache ordering problem with subqueries.

Details

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

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.

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:

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

Martin Dirichs
added a comment - 24/Jul/10 21:02 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.)

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.

Catalina Wei
added a comment - 25/Jul/10 18:53 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.