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

DeferredQueryInvocationHandler fails to unwrap QuerySqmImpl class outside of transaction #32766

Closed
beikov opened this issue May 6, 2024 · 4 comments
Assignees
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@beikov
Copy link

beikov commented May 6, 2024

Affects: All versions

DeferredQueryInvocationHandler should return method.invoke(this.target, args); in an else branch for the unwrap method, otherwise the returned object might not be of the requested type. Consider e.g.

QuerySqmImpl<?> hqlQuery = query.unwrap(QuerySqmImpl.class);

This will fail with a class cast exception when using a proxy with DeferredQueryInvocationHandler.

@jhoeller
Copy link
Contributor

jhoeller commented May 6, 2024

As far as I can infer, this proceeds down to the common method.invoke(target, args) call in line 320 already, it just happens to post-process the returned Query instance for a non-transactional EntityManager - in order to implicitly close that EntityManager for a query-terminating method. And that's the part that does not work when unwrap is being called with a concrete class rather than an interface type: since we create an interface-based proxy there, the result is not castable to the implementation type.

Now, the question is what to do on our side in such a case: We could reject the unwrap attempt for a concrete class, providing a clearer exception message... or we could return a raw unwrapped Query instance without proxying, at the expense of not explicitly closing the EntityManager when that Query object is then used with a query-terminating method.

I'm wondering what the goal of that unwrap(QuerySqmImpl.class) attempt actually is. Does this really have to be unwrapped to the concrete class, or could it also work with the SqmQueryImplementor interface? And what does the caller do with the returned Query instance then, in particular outside of a transaction?

@jhoeller jhoeller added the in: data Issues in data modules (jdbc, orm, oxm, tx) label May 6, 2024
@jhoeller jhoeller self-assigned this May 6, 2024
@jhoeller jhoeller changed the title DeferredQueryInvocationHandler doesn't handle unwrap properly DeferredQueryInvocationHandler fails to unwrap QuerySqmImpl class outside of transaction May 6, 2024
@beikov
Copy link
Author

beikov commented May 6, 2024

The goal is to potentially call some Hibernate ORM specific or internal methods. Why does the implementation return the target object if I pass null to unwrap?

Whatever you do, return the target or throw an explicit exception, that will help users to understand what they have to do, which is to e.g. create an explicit transaction. From the ClassCastException that is thrown right now, it's not clear at all what the reason for this behavior is.

@jhoeller
Copy link
Contributor

jhoeller commented May 6, 2024

Fair enough, the existing code path for unwrap(null) does indeed return the target instance unmodified. So we accept returning a non-decorated raw Query instance in that code path already.

Also, scratch the above in terms of details, I mixed up the EntityManager decoration of Query instances with the Query-backed proxy behavior there. The code path actually ends up in method.invoke(this.target, args) in line 416 and then hard-switches to the proxy instance if the call returned the target Query instance through return (retVal == this.target ? proxy : retVal) which is where I suppose your scenario actually fails. We can revise this to check the expected type there indeed.

As for calling internal provider methods: Hibernate exposes its extended operations on interfaces, this does not usually require a cast to the actual implementation class. I'm just wondering whether Hibernate's SqmQueryImplementor interface could be used instead of the QuerySqmImpl type there. It's certainly recommended to use interfaces as far as possible, especially for provider-specific operations on JDBC connections (where the connection pool often decorates via interfaces as well) but also for JPA.

That said, we are going to fix this through raw target Query exposure in case of a proxy mismatch. To be released in 6.1.7 and backported to 6.0.20 and 5.3.35.

@beikov
Copy link
Author

beikov commented May 6, 2024

Dankeschön 😊

jhoeller added a commit that referenced this issue May 6, 2024
jhoeller added a commit that referenced this issue May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants