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

deprecate "old" methods of session and add a new one #4590

Merged
merged 3 commits into from
Jan 7, 2022

Conversation

gavinking
Copy link
Member

@gavinking gavinking commented Jan 7, 2022

  • deprecated load()
  • deprecated save(), saveOrUpdate(), and update()
  • deprecated delete()
  • deprecated "dangerous" overloads of refresh()
  • added getReference() taking an entity name (to replace the deprecated load())
  • improved the Javadoc for lots of operations

As discussed here: #4520

Note that I have attempted to obtain a more consistent and readable format to the Javadoc, without quite so many raggedy long lines sticking off the side of my monitor.

@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 7, 2022
@gavinking gavinking force-pushed the session_deprecations branch from 56894dc to a7d4374 Compare January 7, 2022 07:22
@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 7, 2022
@gavinking gavinking force-pushed the session_deprecations branch 2 times, most recently from 94750bb to f1a2ac7 Compare January 7, 2022 08:10
@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 7, 2022
@gavinking gavinking force-pushed the session_deprecations branch from f1a2ac7 to 6820e1e Compare January 7, 2022 08:16
- deprecated load()
- deprecated save(), saveOrUpdate(), and update()
- deprecated delete()
- deprecated "dangerous" overloads of refresh()
- added getReference() taking an entity name
- improved the Javadoc for lots of operations
@gavinking gavinking force-pushed the session_deprecations branch from 6820e1e to 8eb3d4e Compare January 7, 2022 08:17
@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 7, 2022
@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 7, 2022
@gavinking gavinking force-pushed the session_deprecations branch from 63e8dd4 to bf12a72 Compare January 7, 2022 08:51
@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 7, 2022
rewrite the incorrect javadoc for the class Hibernate
@gavinking gavinking force-pushed the session_deprecations branch from bf12a72 to e1b5ded Compare January 7, 2022 09:10
@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 7, 2022
@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 7, 2022
@hibernate hibernate deleted a comment from hibernate-github-bot bot Jan 7, 2022
@gavinking
Copy link
Member Author

gavinking commented Jan 7, 2022

Need to merge this, or I'm just going to get myself back into more conflict hell.

Note that there are no breaking changes here, and if anyone disagrees that one of more of these methods should be deprecated then that's not a problem. Just speak up! We can undeprecate operations now or later. It's easy to remove a @Deprecated annotation.

@gavinking gavinking merged commit 1b1790b into hibernate:main Jan 7, 2022
@ctapobep
Copy link

ctapobep commented Mar 15, 2023

if anyone disagrees that one of more of these methods should be deprecated then that's not a problem. Just speak up! We can undeprecate operations now or later. It's easy to remove a @deprecated annotation.

Speaking up! :) I like update() and saveOrUpdate() because they don't have to do any SELECT. While merge() first selects the entity if it's not in PersistenceContext. Shouldn't that count as a reason to keep it?

I always thought of merge() as a more confusing method because it can return a new entity, and we can accidentally forget and keep using the old one 🤔 The discussion that you referenced doesn't talk much about problems with update() - was there something else besides the "already-attached" exception?

@gavinking gavinking deleted the session_deprecations branch June 15, 2023 11:06
@elab
Copy link

elab commented Apr 9, 2024

JPA (API) unfortunatelly does not allow for a reattaching a detached object to the session. And all of the JPA entity state diagrams published everywhere, pretending merge(object) would do that, are simply wrong, as the merge doesn't attach that object but returns a new one, as we all know...

I vote for keeping and supporting update and saveOrUpdate methods in Hibernate, which really implement that important transition of the entity state from detached to attached.

These methods are also much more convenient in many cases, because you can continue to work with the same object without the need of keeping in mind not to forget reassigning of the object references.

@gavinking
Copy link
Member Author

JPA (API) unfortunatelly does not allow for a reattaching a detached object to the session. And all of the JPA entity state diagrams published everywhere, pretending merge(object) would do that, are simply wrong, as the merge doesn't attach that object but returns a new one, as we all know...

That is correct, except it's not unfortunate. It's a good thing.

These methods are also much more convenient in many cases, because you can continue to work with the same object without the need of keeping in mind not to forget reassigning of the object references.

We decided not include them in JPA, because they caused so many problems in the early days of Hibernate. We should actually have deprecated them long ago. That said, there is no immediate plan to actually remove them.

@elab
Copy link

elab commented Apr 9, 2024

Thank you, Gavin, for your comment. I also very appreciate your work on Hibernate, JPA, and Ceylon.

But it would be so much more consistent and conceptually understandable to have that transition officially supported!
And perhaps nowadays there are not much implementation problems anymore?

If you have some links to analysis/discussions of pro and contra retaining of that methods, it would help all of us, I think (besides the already mentioned possible exception that the object is already attached, which is pretty good manageable if you keep the transaction scope small).

@gavinking
Copy link
Member Author

I understand why you want this, I really do. But that was the primary model in Hibernate for the first like 5 years of its existence and the truth is it was simply a bloodbath. There was no good way for users to be sure that some referenced object didn't already belong to the PC when they tried to call saveOrUpdate(). So they got horrible exceptions thrown that were hard to debug.

If you want to work with detached objects today, there's still a way: StatelessSession. Sure, it doesn't give you everything that a stateful PC gives you, but it does give you the ability to "just call update() on a detached object". (And yes, we plan to add a StatelessEntityManager in JPA4.)

@ctapobep
Copy link

ctapobep commented Apr 9, 2024

... it was simply a bloodbath. There was no good way for users to be sure that some referenced object didn't already belong to the PC when they tried to call saveOrUpdate(). So they got horrible exceptions thrown that were hard to debug.

I think we're talking about 2 different types of software. There are stateful apps (which I think is what you're referring to) that benefit from merge() and there are stateless apps that benefit from update()/saveOrUpdate().

Stateful apps benefit from merge()

Let's take a desktop app as an example - you keep all the objects in memory, and from time to time you do DB-related operations. So the Persisted objects and New/Detached objects get intertwined and you have hard times differentiating between them.

This is sort of a project where we would frequently encounter the "bloodbath", and where the merge() will probably be beneficial. I've written only one such software in my life, and I think that most Hibernate-related problems showed up in it - including the one you're talking about. Writing this kind of apps is complicated, and we have to be more careful with ORMs.

Stateless apps benefit from update()

Web apps on the other hand don't usually keep DB-objects in memory for long. You get an HTTP request, you select/update/insert and you throw away those objects.

In Web apps we don't have issues with update() because most of the time we either load an object with that ID and update it from a DTO. Or we create a new instance from the DTO and pass it to saveOrUpdate(). And that's it.

This type of a projects will have issues with merge():

  1. First, merge() is simply inefficient because it issues unnecessary SELECTs. If I get a collection of 100 resources in an HTTP request, I don't want to issue 100 additional queries. I'd like to just UPDATE them, ideally in a bulk manner.
  2. merge() is more difficult and less intuitive:
    • On every project I had to teach someone (or everyone) that you need to work with the returned type. Every. Single. Project. People don't read docs, and there's nothing that screams at them that they need to throw away their previous copy of the object. Most of the time it doesn't matter because these objects rarely used after they're persisted, and so this goes unnoticed. But occasionally you'll run into a silent bug.
    • What if we have a hierarchy of objects: a(attached)->b(detached)->c(attached) and we merge(b)? We'll end up with 2 different b objects, and our attached a references the wrong one. So we need to consider more things when working with merge().

So yes, if it's a Stateful app we may not have much of a choice and we have to make our peace with merge(), but most projects these days are web - and they will be better off with saveOrUpdate().

@Przemeo
Copy link

Przemeo commented Sep 28, 2024

Is this the final decision? I've noticed that save, update and saveOrUpdate methods have been removed completely in Hibernate 7 (HHH-18196), despite stating that there is no immediate plan to actually remove them.
I feel that with decisions like this us, the developers, are treated like we don't know what we want to achieve, which can be seen in statements like there was no good way for users to be sure that some referenced object didn't already belong to the PC when they tried to call saveOrUpdate(). So they got horrible exceptions thrown that were hard to debug.
I know what I'm doing in my code, I have a really optimized way to handle database queries, and now I'm forced to change all of it because some developers don't know what happens in their code?

@beikov
Copy link
Member

beikov commented Oct 23, 2024

A replacement for that is being discussed on https://hibernate.atlassian.net/issues/HHH-18549

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

Successfully merging this pull request may close these issues.

5 participants