-
Notifications
You must be signed in to change notification settings - Fork 90
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
lazy collection fetching in Quarkus #663
Comments
/cc @DavideD @gavinking |
Yeah, I think we decided this was the correct thing to do: in JPA there's no notion of reassociating an entity instance with a session. That's a bit different to the really old usage pattern of Hibernate where you could use (And there's good reasons to not have this. Experience is that in practice users get themselves into a mess with reassociation.) Since loading a detached collection really implies reattaching its parent, we simply don't support this. However, IIRC, I do let you do this in a |
Hi @gavinking, thanks for the quick reply. I'm not quite sure whether I'm getting what you're saying. So what I implemented was exactly what is described in the documentation:
This is working if the author and his books have been persisted with the same session (i.e., in the same HTTP request), but it's not working if I use another session (i.e., another request). So you are implying that this is intended behavior, at least for a "normal" (stateful?) session? This seems to me like a really standard use case, because I will almost always try to get some data from the database some time after persisting it, so in a different session. This would mean that using a normal session, lazy associations are not supported. In that case, I would argue that it should be explicitly mentioned in the documentation that a |
I'm saying that in the new session you should use This is the standard JPA model, it's even a bit easier to use in HR because I overloaded |
@gavinking do you mean something like this?
|
Sure, but I made it even easier. You can directly pass the detached author to |
Okay, I tried those suggestions, but unfortunately they didn't work either. Maybe you can be so kind to point out if I'm doing something incorrectly here: The approach using
The other suggested solution using
|
I will have a look, it seems related to bytecode enhancements |
@DavideD, any update? I tried to reproduce the issue without Quarkus by just using the So this issue only seems to happen in Quarkus. |
I'm still looking into it. The problem happens because of bytecode enhancements that are enabled by default in Quarkus. |
I can now recreate the error in reactive. It requires two things:
@gavinking Am I correct in thinking that we should make it sure that in Quarkus |
Um. I think it only makes sense to have the collection in the default fetch group. I don't think it ever makes sense to exclude it. |
Sorry, yeah, I meant to write |
Ah OK good :-) Take a break :-) |
@DavideD That's some great findings, thank you :) Just wanted to point out, that according to the discussion in #374 and the related hibernate/hibernate-orm#3558, it seems like hibernate/hibernate-orm#3558 (comment) Anyways, it does look like Quarkus currently doesn't set that property to And if I checked it correctly, the default in Hibernate ORM still seems to be |
@Sanne needs to look at this one. |
If we only have to set it to true, it seems pretty easy to solve. I will create a fix and then check with @Sanne |
Well what I'm wondering is if it's set to false because that is what ORM needs, and if we need to be careful to not step on regular Hibernate? |
great find, thanks. Yes it's a little tricky because the two extensions ("regular" and "reactive") are highly coupled ATM. Need to separate them properly, that was developed in too much rush. |
(OTOH, I'm not sure why ORM would need it; as I've argued before, I think it should be on by default.) |
hum ok. Let's do this for both ORMs (in Quarkus). |
@gavinking, I think that title is wrong, isn't it? I guess you meant to put |
Ah yeah, sure, I guess you're right. you're saying that you see this bug with default settings in Quarkus? |
Yes, no special configuration is necessary in Quarkus to get this bug. @DavideD just had to do some special configuration to reproduce in plain HR (without Quarkus). |
Sure. The thing is it's not actually a bug in HR. I deliberately added that "collection in default fetch group" thing to core specifically because we need it on all the time in HR. So the bug is in the Quarkus extension which turns it off. (Or, arguably, in core for doing the wrong thing by default.) |
The default for ORM is false, but when we create the SessionFactory for Hibernate Reactive this value has to be true. For more details, see: hibernate/hibernate-reactive#663
I've sent a fix for Quarkus: quarkusio/quarkus#15818 |
Sounds great, thank you! 👍 |
Just tested it and can confirm that lazy collection fetching now works in Quarkus. Thanks once again 😄 |
Thank you @markusdlugi |
Thanks for reporting it @markusdlugi |
The default for ORM is false, but when we create the SessionFactory for Hibernate Reactive this value has to be true. For more details, see: hibernate/hibernate-reactive#663 (cherry picked from commit 76072c5)
The default for ORM is false, but when we create the SessionFactory for Hibernate Reactive this value has to be true. For more details, see: hibernate/hibernate-reactive#663
When trying to fetch an association lazily with Mutiny in Quarkus, it doesn't work if the fetch is performed in a different session than the one used to persist the data. If the same session is used (so fetching right after persisting in the same session), it works.
Expected behavior
It works also in different sessions.
Actual behavior
An exception is thrown when trying to fetch the association:
Reproducer
https://github.com/markusdlugi/hibernate-reactive-lazy-fetching
Steps to reproduce the behavior:
infrastructure/docker-compose.yml
.mvn quarkus:dev
POST http://localhost:8080/test/working
.3.1. This works and returns a list of books.
GET http://localhost:8080/test/failing/1
(or other authorId created viaPOST http://localhost:8080/test/failing
).4.1 This fails with the above exception.
The text was updated successfully, but these errors were encountered: