Bugzilla – Full Text Bug Listing
| Summary: | Using ScrollableCursor with 1:M joining produces incorrect results | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | David Minsky <david.minsky> | ||||||||||||||||
| Component: | Eclipselink | Assignee: | Nobody - feel free to take it <nobody> | ||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||
| Severity: | normal | ||||||||||||||||||
| Priority: | P3 | CC: | jmturc | ||||||||||||||||
| Version: | unspecified | ||||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Created attachment 205954 [details]
Patch and testcase
Proposed fix and testcase
Whilst working on this bug, I also observed the scenario reported in this bug: Bug 337099 - NullPointerException in JoinedAttributeManager class with cursor The attached patch resolves the NPE issue. Created attachment 206012 [details]
Patch and testcase
Improved patch, added internal ScrollableCursor API for better clarity.
Created attachment 206022 [details]
Fix and testcase
Corrected patch
*** Bug 337099 has been marked as a duplicate of this bug. *** Whilst investigating this issue, I found the following: I tested a query between a non-cursored ReadAll with 1:M joining and a cursored ReadAll with 1:M joining (Employee -> Phones). I saw that the employees with 1 phonenumber were not included in query results, and that some employees were duplicated in the results, some employees had an incorrect number of phone numbers. I ran the EclipseLink generated query to verify the data returned from the database, and compared it against the results that I was seeing. Debugging through the ScrollableCursor code, alongside the raw SQL results, when ScrollableCursor.next() is called, it calls loadNext(), which checks if the cached 'nextObject' is null, and if it is null, loads it in retrieveNextObject(). In retrieveNextObject(), one row is read, unless there is a previously cached 'nextRow', in which case that is the row used. Where joining is in effect, joinManager.processDataResults() is invoked to handle the joining. This code reads rows until a row with a primaryKey which is different to the passed row is encountered. This 'extra' row is returned, and the joined rows are cached in the dataResultsByPrimaryKey Map. Back in retrieveNextObject(), the object is built, and the extra row that was read is cached in the 'nextRow' variable. The built object is returned and set to be the 'nextObject', however in the calling method - next() - the nextObject is cached (and returned), clearNextAndPrevious() is invoked, which nulls the previousObject, nextObject, previousRow and nextRow. In this scenario, nulling previousRow and nextRow is incorrect, as it has the effect of 'skipping' the first database row of the next object that should be read, explaining the incorrect and missing data. It is correct to null the previousObject and nextObject references, since the next/previous object has been returned by the call to next()/previous(), and should not be cached (also, if these references are not nulled, an infinite loop will occur because of the null checking of the next/previous object to determine whether to load the object or not). So, the fix which should be made is to create a new method 'clearNextAndPreviousObject()', which only nulls out the cached nextObject and previousObject, leaving the cached nextRow and previousRow intact. This should be called in next() and previous() instead of calling the clearNextAndPreviousObject() method. The nextRow and previousRow variables are only referenced and set within the retrieveNextObject() and retrievePreviousObject() methods, and only cleared by the clearNextAndPrevious() method. So the impact of not clearing them is local to these operations. The nextRow and previousRow variables should be cleared when the cursor is repositioned, and I believe that clearNextAndPrevious() should be left intact for this purpose, but the calls in next() and previous() should be changed to only clear the next and previous objects. Created attachment 206143 [details]
Fix and testcase
Updated testcase code to initialize identitymaps before read tests are executed
When testing in MySQL, the following error is received when ScrollableCursor.hasPrevious() is called: Internal Exception: java.sql.SQLException: After end of result set Error Code: 0 Local Exception Stack: Exception [EclipseLink-4002] (Eclipse Persistence Services - 2.4.0.qualifier): org.eclipse.persistence.exceptions.DatabaseException Internal Exception: java.sql.SQLException: After end of result set Error Code: 0 at org.eclipse.persistence.exceptions.DatabaseException.sqlException(DatabaseException.java:324) at org.eclipse.persistence.internal.databaseaccess.DatabaseAccessor.getObject(DatabaseAccessor.java:1237) at org.eclipse.persistence.internal.databaseaccess.DatabaseAccessor.fetchRow(DatabaseAccessor.java:969) at org.eclipse.persistence.internal.databaseaccess.DatabaseAccessor.cursorRetrievePreviousRow(DatabaseAccessor.java:457) at org.eclipse.persistence.queries.ScrollableCursor.retrievePreviousObject(ScrollableCursor.java:614) at org.eclipse.persistence.queries.ScrollableCursor.loadPrevious(ScrollableCursor.java:419) at org.eclipse.persistence.queries.ScrollableCursor.hasPrevious(ScrollableCursor.java:295) at org.eclipse.persistence.testing.tests.queries.ScrollableCursorJoiningVerificationTest.test(ScrollableCursorJoiningVerificationTest.java:71) at org.eclipse.persistence.testing.framework.TestCase.executeTest(TestCase.java:545) at org.eclipse.persistence.testing.framework.TestCase.execute(TestCase.java:156) at org.eclipse.persistence.testing.framework.TestCase.runBare(TestCase.java:265) at org.eclipse.persistence.testing.framework.TestExecutor.execute(TestExecutor.java:248) at org.eclipse.persistence.testing.framework.TestSuite.execute(TestSuite.java:75) at org.eclipse.persistence.testing.framework.TestCollection.run(TestCollection.java:313) at org.eclipse.persistence.testing.framework.TestExecutor.execute(TestExecutor.java:248) at org.eclipse.persistence.testing.framework.TestModel.execute(TestModel.java:211) at org.eclipse.persistence.testing.framework.TestCollection.run(TestCollection.java:313) at org.eclipse.persistence.testing.framework.TestExecutor.execute(TestExecutor.java:248) at org.eclipse.persistence.testing.framework.TestModel.execute(TestModel.java:211) at org.eclipse.persistence.testing.framework.TestCollection.run(TestCollection.java:313) Caused by: java.sql.SQLException: After end of result set at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:1073) at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:987) at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:982) at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:927) at com.mysql.jdbc.ResultSetImpl.checkRowPos(ResultSetImpl.java:841) at com.mysql.jdbc.UpdatableResultSet.checkRowPos(UpdatableResultSet.java:228) at com.mysql.jdbc.ResultSetImpl.getObject(ResultSetImpl.java:4853) at org.eclipse.persistence.internal.databaseaccess.DatabasePlatform.getObjectFromResultSet(DatabasePlatform.java:1257) at org.eclipse.persistence.internal.databaseaccess.DatabaseAccessor.getObject(DatabaseAccessor.java:1206) Need to investigate this issue on MySQL before the fix can be checked in. Wrote a piece of JDBC test code to compare Oracle and MySQL. It appears that there is an extra call to next() within the ScrollableCursor code, after all rows in the cursor have been read. Oracle tolerates this fine, however MySQL throws the following error: java.sql.SQLException: After end of result set at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:1073) at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:987) at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:982) at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:927) at com.mysql.jdbc.ResultSetImpl.checkRowPos(ResultSetImpl.java:841) at com.mysql.jdbc.UpdatableResultSet.checkRowPos(UpdatableResultSet.java:228) at com.mysql.jdbc.ResultSetImpl.getObject(ResultSetImpl.java:4853) at oracle.sustaining.test.JDBCScrollableCursorTest.run(JDBCScrollableCursorTest.java:64) at java.lang.Thread.run(Thread.java:662) Uploading JDBC testcase for reference. Created attachment 206359 [details]
JDBC ScrollableCursor test (Oracle & MySQL)
Created attachment 206412 [details]
Fix and testcase
Additional code to add in a flag indicating that the end of the stream has been reached, so that EclipseLink doesn't unnecessarily call ResultSet.next() when it shouldn't (which will fail when using the MySQL driver/db)
Created attachment 206532 [details]
Fix and testcase
small change to move block of code in retrievePrevious to avoid NPE, this is in-line with the code for retrieveNext
Checked into trunk (2.4) at revision: 10341 The Eclipselink project has moved to Github: https://github.com/eclipse-ee4j/eclipselink |
Using the following query: ReadAllQuery cursoredQuery = new ReadAllQuery(Employee.class); cursoredQuery.useScrollableCursor(); cursoredQuery.addJoinedAttribute(cursoredQuery.getExpressionBuilder().anyOfAllowingNone("phoneNumbers")); cursoredQuery.addOrdering(cursoredQuery.getExpressionBuilder().get("id")); ScrollableCursor cursor = (ScrollableCursor)getSession().executeQuery(cursoredQuery); while (cursor.hasNext()) { Object result = cursor.next(); cursoredResults.add(result); } Produces different results to: ReadAllQuery nonCursoredQuery = new ReadAllQuery(Employee.class); nonCursoredQuery.addJoinedAttribute(nonCursoredQuery.getExpressionBuilder().anyOfAllowingNone("phoneNumbers")); nonCursoredQuery.addOrdering(nonCursoredQuery.getExpressionBuilder().get("id")); Vector nonCursoredResults = getSession().executeQuery(nonCursoredQuery); This includes duplicate results, and missing results.