Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatibility with Hibernate < 5.2.11 broken for projections on native queries [DATAJPA-1209] #1543

Closed
spring-projects-issues opened this issue Oct 20, 2017 · 14 comments
Assignees
Labels
type: bug

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

Robert Yeates opened DATAJPA-1209 and commented

After the release of DATAJPA-980 in Ingalls SR8, Spring Boot is now broken. The Spring Boot BOM has the following version for Hibernate.

<hibernate.version>5.0.12.Final</hibernate.version>

Most teams would override this as it is somewhat old. DATAJPA-980 requires an update to 5.2.11.Final and the only place I've found that mentioned is in the linked ticket. To have to step one by one through spring-data-releasetrain.versions to find the breaking change and then scour the changelog is a huge pain. Can we either document this change in a more visible way or move Spring Boot to the required Hibernate version? Hibernate version is not specified in spring-data-releasetrain.


Affects: 1.10.11 (Hopper SR11), 1.11.8 (Ingalls SR8)

Issue Links:

  • DATAJPA-1247 Interface-based projection mixes data order

  • DATAJPA-980 Projections with native queries don't work as expected

  • DATAJPA-1562 Got IndexOutOfBoundsException when projecting data on maps list

  • SPR-16100 Allow declaration-stable lookup of declared methods via ASM

Backported to: 1.10.12 (Hopper SR12)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

Oliver Drotbohm commented

Let's take a step back: the projection execution for native queries never worked as there was no way to properly interact with Hibernate before. So SD JPA in Ingalls SR7 doesn't support projections on native queries, SD JPA in Ingalls SR8 on Hibernate 5.0.12 also fails, but works with 5.2.11. That's quite intentional or at least there's just no way to magically make this work for earlier versions of Hibernate. So we don't strictly require that upgrade. You simply need to upgrade if you want to see that particular bug fixed. Am I missing something here?

The compatibility baseline for Ingalls is even Hibernate 3 assuming that certain things won't work if Hibernate does not properly support some JPA functionality in those earlier versions.

So in how far is Spring Boot broken? I have just created a branch of our 1.x examples and tweaked it to run on Boot's default Hibernate version. All test cases – including the ones that use projections – succeed. Would you mind elaborating on what exactly is not working for you?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

Robert Yeates commented

I think the missing part from my description is that Projections on native queries have been working prior to this.

Previous working query

@Query(value = "SELECT sT.ID, sT.NAME, sT.EDITABLE_STATE from SomeTable sT INNER JOIN SomeLinkTable somelinkTable ON wT.ID = somelinkTable.TEMPLATE_ID WHERE somelinkTable.SOME_ID=?1", nativeQuery = true)
List<SomeProjection> findIdNameStateBySomeId(UUID someId);

Projection

public interface SomeProjection {
    byte[] getId();
    String getName();
    String getState();
}

Prior to this change the projection is populated correctly. Post this change the query or projection has to change to match the field names to the columns extracted. In the above case the change could look like

@Query(value = "SELECT sT.ID, sT.NAME, sT.EDITABLE_STATE as state from SomeTable sT INNER JOIN SomeLinkTable somelinkTable ON wT.ID = somelinkTable.TEMPLATE_ID WHERE somelinkTable.SOME_ID=?1", nativeQuery = true)
List<SomeProjection> findIdNameStateBySomeId(UUID someId);

For the users who have been using projections, this can be a breaking change. Resolving the issue hinges on finding the comments in a Jira

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

Oliver Drotbohm commented

That's weird as the reporter of DATAJPA-980 claimed it wasn't and the test case we added was clearly showing that that's the case. Can you maybe add

What I am kind of wondering from your example: even assuming it works as you claim, how would you expect Spring Data to match the returned values against the method invocations on SomeProjection. In particular how would the call to getState() know to return what your query returns as sT.EDITABLE_STATE?

In general: are you able to upgrade to Hibernate 5.2.11 (by setting hibernate.version to that version)?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

Robert Yeates commented

We are up and working using the following.

dependencyManagement {
     imports {
          mavenBom("org.springframework.boot:spring-boot-starter-parent:$spring_boot.version"){
               bomProperty 'hibernate.version', "$hibernate.version"

The impact of the change and the difficulty of resolving it is why I've created the ticket. The error you see is Unknown entity: javax.persistence.Tuple which leads to all sorts of terrible S/O threads.

I can't answer for the existing code, given the sparse nature of the documentation I'd imagine it would be a trial and error approach. How would we go about requesting Boot to bump the Hibernate version and how can I help to make that happen?

Regarding test cases, I'd imagine a PR would be suitable?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

Oliver Drotbohm commented

Boot is not going to raise the Hibernate version in 1.5 more than bugfix versions due to compatibility guarantees. Boot 2.0 however is already on Hibernate 5.2.

The change we deployed wasn't one that requires an upgrade to Hibernate 5.2, but enable something to work in 5.2 that wasn't in neither Ingalls SR7, nor on Hibernate 5.0 on SR8. That is the reason there hasn't been more explicit documentation about that change besides the remark in the commit message.

I'd argue that given the test case we have deployed alongside the change, feel free to create a branch on a clone and mess with it. I'd still like to see some actually running code that show projections for native queries working on SR7 as you claim. The test cases we have added indicated, that's not the case. Happy to reconsider though if we actually have something to act on

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

Robert Yeates commented

The difference in view is that there is more confidence in the existing test than the comments of some unknown user. I appreciate that.

The code snippets above are from a live production system that is working well. When I add the test case to support this I'll add a suggestion for the change to the documentation as well

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

Oliver Drotbohm commented

I appreciate that as well. It's just that that production system is not something I can run to verify the allegedly erroneous behavior. :) We also apparently both still have no clue how the currently running code is supposed to reliably map getState() to sT.EDITABLE_STATE. So what I am actually trying to say is that I'd like to get my hands on some stuff that shows breakage over guesswork on code snippets :)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

Robert Yeates commented

The original test case from 980 works for this. https://github.com/roberthunt/spring-data-native-query-projection

Step 1.
Make the Projection field name difference to column name. Data from the query gets shoved in anyway.

-    @Query(value = "SELECT name AS name, age AS age FROM Person", nativeQuery = true)
+    @Query(value = "SELECT age, name as potato FROM Person", nativeQuery = true)
     List<PersonSummary> findAllProjectedNativeQuery();

uk.co.rbrt.SpringDataNativeQueryProjectionApplicationTests#testFindAllProjectedNativeQuery is green

Step 2.
Change Boot version

 	<parent>
 		<groupId>org.springframework.boot</groupId>
 		<artifactId>spring-boot-starter-parent</artifactId>
-		<version>1.4.1.RELEASE</version>
+		<version>1.5.8.RELEASE</version>
 		<relativePath/> <!-- lookup parent from repository -->
 	</parent>

Rerun same test and it fails with
org.springframework.orm.jpa.JpaSystemException: Unknown entity: javax.persistence.Tuple; nested exception is org.hibernate.MappingException: Unknown entity: javax.persistence.Tuple

Step 4.
Change the query back

-    @Query(value = "SELECT age, name as potato FROM Person", nativeQuery = true)
+   @Query(value = "SELECT age, name FROM Person", nativeQuery = true)
     List<PersonSummary> findAllProjectedNativeQuery();

Rerun same test and it continues to fails with
org.springframework.orm.jpa.JpaSystemException: Unknown entity: javax.persistence.Tuple; nested exception is org.hibernate.MappingException: Unknown entity: javax.persistence.Tuple

Step 5.

Explicitly add the version of Hibernate required by DATAJPA-980 and the test goes green

+		<dependency>
+			<groupId>org.hibernate</groupId>
+			<artifactId>hibernate-core</artifactId>
+			<version>5.2.11.Final</version>
+		</dependency>
+
+		<dependency>
+			<groupId>org.hibernate</groupId>
+			<artifactId>hibernate-entitymanager</artifactId>
+			<version>5.2.11.Final</version>
+		</dependency>

The original working implementation (as noted on 980) is fragile. The change to Boot 1.5.8 breaks it in a surprising way. Should the documentation not say something like, "This feature was not working as intended in previous versions, by resolving that you may need to update queries and set the version of Hibernate to 5.2.11.Final or newer."

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2017

Oliver Drotbohm commented

Thanks for the detailed writeup, Robert. I'll have a look first thing Monday morning CET

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 23, 2017

Oliver Drotbohm commented

To add a bit more context here: the "fix" is not tweaking the aliases as they don't seem to affect the way we get results returned from Hibernate in the first place (unless explicitly being asked for to return a Tuple on Hibernate 5.2). By default, we will always get an Object[] back so that a match to the methods depends on the order in which we find our input properties to the projection. That currently depends on PropertyDescriptors being returned from the reflection API which does not guarantee a stable order. So while it actually might work in some cases. I have actually even seen subsequent test runs succeeding and failing with both query declarations.

So I guess I'll do the following:

  • I'll add a guard to the code introduced in SR8 to only follow the new code path if we're on Hibernate 5.2 in the first place.
  • I have opened SPR-16100 for the core framework to allow us inspecting the declared projection accessors and actually rely on the order of the properties returned.

Still, as the match between SQL column selection order and the declaration in the interface is a pretty weak link, I'd recommend to use aliases and Hibernate 5.2 to make sure you reliably get properly matched results

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 23, 2017

Oliver Drotbohm commented

I've just pushed the fix re-instantiating compatibility with Hibernate versions before 5.2.11. Would you mind setting the Spring Data version of your Boot 1.5 based project to Ingalls-BUILD-SNAPSHOT and see whether that allows it to run on Boot's Hibernate default version?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 23, 2017

Robert Yeates commented

Using bomProperty 'spring-data-releasetrain', 'Ingalls-BUILD-SNAPSHOT'

Boot version is 1.5.8.RELEASE, with this setup

+--- org.springframework.data:spring-data-jpa:1.11.8.RELEASE (*)
+--- org.hibernate:hibernate-envers:4.3.11.Final -> 5.2.10.Final
|    +--- org.jboss.logging:jboss-logging:3.3.0.Final -> 3.3.1.Final
|    \--- org.hibernate:hibernate-core:5.2.10.Final

Tests are green. Commit looks good.

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 25, 2018

Caleb Cushing commented

hmm... getting this same error with hibernate 5.0.12, and Kay-SR7

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 25, 2018

Caleb Cushing commented

(Platform Brussels-SR10 works fine) We cannot easily just upgrade to Hibernate 5.2, if that's required we'll have to refactor or hold off upgrading Spring Data until we can deal with whatever hibernate throws at us (which means keeping Spring Data (along with Spring Security, and Hibernate) back during the migration from platform to SB 2, parent)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug
Projects
None yet
Development

No branches or pull requests

2 participants