Docs: read-only parent's operations are still cascaded to its child associations #10772
Conversation
….java - read-only parent's operations are still cascaded to its child associations
|
Thanks for your pull request! This pull request does not follow the contribution rules. Could you have a look? ❌ All commit messages should start with a JIRA issue key matching pattern › This message was automatically generated. |
There was a problem hiding this comment.
I'm not at all keen on these proposed changes, which seem to reflect a confusion about the whole notion of "read-only" and what it is about.
Read-only mode refers to the entity being in a state where its changes to its fields are not tracked by Hibernate and not made persistent. It is not relevant to what happens to the state of other associated entities.
For example, a child entity is the owner of the association to its parent, the FK is not a field of the parent entity, and so the readonliness of the parent is irrelevant.
This is, I believe, failry adequately explained in the Javadoc for Session.setReadOnly() and Session.setDefaultReadOnly().
| * In some ways, Hibernate treats read-only entities the same as | ||
| * entities that are not read-only; for example, it cascades | ||
| * operations to associations as defined in the entity mapping. | ||
| * <p> |
There was a problem hiding this comment.
I strongly disagree with this misleading language. Please remove it.
Cascading operations have absolutely nothing to do with read-onliness of an entity.
There was a problem hiding this comment.
I see what you mean: when the parent has a child association, which uses mappedBy to make it owned by the child, you don't view this child association as being one of the fields of the parent.
Personally, I strongly disagree with that perspective, because even though it is technically correct in some sense, I am not aware of any special definition of a "field" that excludes the @OneToMany from being considered a field. In a Hibernate entity, not all fields may be "persistable" for the purposes of a final query, but what I'm saying is that we are setting the read-only on the parent entity, implying all fields of this specific entity as defined by its data model, not only as defined by SQL.
Note that this is not the scenario where you explicitly persist the child. You only persist (via loading) the parent, which causes a side-effect of persistence even though you never "authorized" the persistence of the child itself.
I agree though that it should be re-phrased to include your definition that is more correct. My intention is to be explicit about covering this "severe risk" in the JavaDoc, and for that it is vital to explicitly inform the user of the practical side effect in the form of "persisting any added child association", which I also believe to be the most likely scenario to happen, as this is really close to the most trivial data model example.
For example, we could be more explicit about the definition by saying "This only changes all owned fields of an entity to be read-only. Any fields not owned by the entity, such as child associations, will not be considered read-only."
Can you help me come up with a reasonable comment here, considering this is the most frequent and important place any first-time users of this function will likely see? Thanks!
There was a problem hiding this comment.
Personally, I strongly disagree with that perspective, because even though it is technically correct in some sense, I am not aware of any special definition of a "field" that excludes the @onetomany from being considered a field. In a Hibernate entity, not all fields may be "persistable" for the purposes of a final query, but what I'm saying is that we are setting the read-only on the parent entity, implying all fields of this specific entity as defined by its data model, not only as defined by SQL.
Well, in this case at least, that's actually quite wrong. There is indeed a
special definition of a "field" that excludes the
@OneToManyfrom being considered a field
This @OneToMany is an "unowned collection" in the language of JPA, and is not considered part of the state of the parent. If you make a change (only) to an unowned collection, that change is never made persistent.
Now, on the other hand, if this were an owned collection, you would have a strong point. I believe that the situation today is that read-only mode doesn't affect collections at all. And I think that's simply wrong. It should affect owned collections. But, again, this isn't really a problem with the documentation, it's a problem with the actual functionality.
| * In some ways, Hibernate treats read-only entities the same as | ||
| * entities that are not read-only; for example, it cascades | ||
| * operations to associations as defined in the entity mapping. | ||
| * <p> |
There was a problem hiding this comment.
Same as above; I'm hoping an expert in Hibernate terminology can fine-tune this critical point of visibility. This was the first place we analyzed when we observed this issue in a production code...
| @@ -1134,6 +1134,11 @@ Additionally, the `CascadeType.ALL` will propagate any Hibernate-specific operat | |||
| `REPLICATE`:: cascades the entity replicate operation. | |||
| `LOCK`:: cascades the entity lock operation. | |||
|
|
|||
| [WARNING] | |||
There was a problem hiding this comment.
| [WARNING] | |
| [NOTE] |
There was a problem hiding this comment.
Just to confirm, you suggest changing it to a note here, but keeping it as a warning below? Also, maybe you could provide some reasoning for future maintainers why this is not semantically more of a warning of a non-intuitive behavior with potentially dangerous default usage. In any case, I agree that it should probably be just a note here, while the official definition with any warnings should be located in a document dedicated to "read-onliness", similar to Hibernate 3's docs. Thanks!
| @@ -1134,6 +1134,11 @@ Additionally, the `CascadeType.ALL` will propagate any Hibernate-specific operat | |||
| `REPLICATE`:: cascades the entity replicate operation. | |||
| `LOCK`:: cascades the entity lock operation. | |||
|
|
|||
| [WARNING] | |||
| ==== | |||
| When a parent entity is in a **read-only state**, operations are still cascaded to its child associations. For example, adding a new child to a collection on a read-only parent will result in the child entity being persisted. If this behavior is not desired, the parent entity needs to be evicted from the session before making modifications. | |||
There was a problem hiding this comment.
| When a parent entity is in a **read-only state**, operations are still cascaded to its child associations. For example, adding a new child to a collection on a read-only parent will result in the child entity being persisted. If this behavior is not desired, the parent entity needs to be evicted from the session before making modifications. | |
| When a parent entity is in a read-only mode, operations are still cascaded to its child associations. For example, adding a new child to a collection on a read-only parent will result in the child entity being persisted. If this behavior is not desired, the parent entity needs to be evicted from the session before making modifications. |
There was a problem hiding this comment.
Actually, this section is not dealing with read-only mode at all, so this note is inappropriate in here. Please remove it.
| ==== | ||
| [source, java, indent=0] | ||
| ---- | ||
| include::{example-dir-query}/HQLTest.java[tags=hql-read-only-entities-native-example] | ||
| ---- | ||
| ==== | ||
|
|
||
| [WARNING] | ||
| ==== | ||
| When a parent entity is in a **read-only state**, operations are still cascaded to its child associations. For example, adding a new child to a collection on a read-only parent will result in the child entity being persisted. If this behavior is not desired, the parent entity needs to be evicted from the session before making modifications. |
There was a problem hiding this comment.
| When a parent entity is in a **read-only state**, operations are still cascaded to its child associations. For example, adding a new child to a collection on a read-only parent will result in the child entity being persisted. If this behavior is not desired, the parent entity needs to be evicted from the session before making modifications. | |
| When a parent entity is in a read-only mode, operations are still cascaded to its child associations. For example, adding a new child to a collection on a read-only parent will result in the child entity being persisted. If this behavior is not desired, the parent entity needs to be evicted from the session before making modifications. |
My primary concern is that there was no change to any state of a child entity, because the child entity was transient.
To quote
There is not a single word about this peculiar case. If anything, it misleadingly pretends that we should actually be able to trust this mode. It says the "entity" can be safely modified, not "some of entity's fields that are directly owned". |
Perhaps if you could explain where in the documentation of Hibernate you picked up on the idea that read-only mode for the parent had something to do with state mapped by the child, I could do a better job of reviewing this PR. As it is, I've no idea where you got this idea. It's not implied by the Javadoc. To me, it's pretty clear that foreign keys belong to child entities, not to parent entities.
I have quite literally no idea why a new developer would make this assumption. To two concepts are not related, and are not linked together in any documentation that I know of. |
Ok, so, reading between the lines here, I think the mental model you're working from is that [And the operation definitely has no effect on a new or detached child, because a new or detached entity can't possibly be in read-only mode.] |
Well, I can only tell you that I have quite literally no idea why any new developer would not make this assumption 😄 If you set it on a session, you should automatically inherit it on both parents and children. But yes, I wasn't literally implying
Well my point is that I didn't pick it up in a documentation, because it was missing in the documentation. So I had to assume the intuitive behavior applies. But again, I am not saying about cascading operations "to children" - especially not to persistent children. I am talking about cascading an operation to "persisting a new child, which was added as a transient entity to a read-only DTO". (You literally create an object via |
But there actually was a change to the state of a child entity. You called Now, all that said, I actually happen to agree that this behavior is less-than-intuitive. I even opened this issue a while ago: https://hibernate.atlassian.net/issues/?filter=21198&selectedIssue=HHH-19398 If it weren't a breaking change, and if it weren't for those tests, I probably would turn off cascade processing for read-only entities. And so, sure, I'm OK with stating explicitly that read-only mode has no relationship with and no effect on cascade processing. But I don't like anything else about how you're framing this discussion. The documentation accurately describes the effect of read-only mode. |
Alright, look, while I agree with you on the limited point that it might be more intuitive to turn of cascade persist for read-only entities, I really do think the rest of what you're saying here is a bit nuts. I don't think any of these expectations are reasonable, given the semantics of |
|
Oh, so the only issue which I found and linked in the original post is your issue, that's great 😃 Well yeah, I'm framing the discussion primarily with the intention of shining a light on the fact that it isn't intuitive. It's not just a tiny issue, but a problem where seasoned devs can be using Hibernate with this feature for 10 years and only today figure out a "bug" as they modify a child for the very first time... I'm strongly opinionated on this being a design flaw of the API, so I'm hoping for the best solution in the form of waiting for a major version to deprecate & rename this method. (And tbh, people often don't really memorize the entire docs, so it's common to actually forget these edge cases even if you learn them once. I usually rely on JavaDocs to double-check the definition as long as it's written in a user-friendly way.) Thanks for the technical explanation of the implicit persist flow. Now it does make sense from a technical standpoint, but it's definitely crazy that this problem appeared in the first place :) We can never have enough English words for concepts; there are so many "read-onlys", so many "mutables", and we need both low-level and high-level ones... (Based on that definition, I subjectively re-classify it from a
I mean, if you exclude my imperfect terminology (where I made the analogy between read-only session and detached entities), can you list which of my expectations do you consider unreasonable? Honestly, literally speaking, I do agree with you that "there's nothing in the existing documentation which would justify these expectations", because that's my primary complaint: there is nothing in the existing documentation 😅 it doesn't say it "doesn't persist new children", but how can a developer make the assumption that it means it "does persist them"? 😄 Would you honestly come to that conclusion if you were not a Hibernate contributor? In any case, I believe we're clear on this issue, so thanks for all the details. I'd suggest inviting a few reviewers to suggest & review possible JavaDocs or any relevant places in the docs, so that some simple explanation of the current behavior can be merged asap. |
Sure, I guess the disconnect is I see this more as a functional problem rather than a documentation problem. My preference would be to change the actual behavior, but we can't take any action on that until we see what happens with this stuff in JPA 4. (I've proposed to introduce a read-only mode to the spec.)
I wasn't involved in the design of this, so I'm really not clear on what were the considerations which went into the current behavior, but I do see that they were intentional decisions, because we have tests. That said, I do think that:
Actually there was no issue for (1) so I just opened https://hibernate.atlassian.net/browse/HHH-19715.
Absolutely correct. Read-only mode is essentially just a performance optimization for cases where you have a large number of entities in the session and want to manage the cost of dirty-checking. It's really a very low-level thing, which is why you won't see it mentioned in many places. If this becomes part of JPA 4, we'll need to make sure the semantics are much more thoroughly reconsidered. |
|
Yeah, it'll be great if you can change the behavior for collections, thanks for opening the ticket. I'm sure there's enough space for feature flags or new method overloads in the worst case if multiple solutions need to be supported; I mainly care about newcomers having the ability to open the docs and find one recommended default solution with an intuitive explanation of where it should be used. In my team's case, we just need to cover the independent needs of both "read only transaction" and "read only hibernate entities". That said, it's still vital to merge my documentation changes, because many developers will keep using old versions for a very long time. Honestly, I'm hoping that it'll be possible for maintainers to backintegrate these doc changes into old Hibernate versions too, because it's certainly not a problem that appeared in the version 7.1. Considering old docs were documented correctly (though still not in JavaDocs I guess), it may need some individual consideration. I'm just hoping this issue can be considered serious enough to warrant any doc backintegration effort :) One more thing: if we do indeed intend to use the
|
Hibernate uses a misleading concept name of "read-only" for entities whose modifications to child associations are persisted.
Imagine the following use-case: a new Hibernate developer will figure out and assume they can wrap a session using
to achieve the behavior equivalent to detached entities. Developers will continue maintaining this so-called "read-only" branch of code execution and eventually modify a child association, as they consider this Entity to be a simple DTO, to be read by other functions in the code. As a result, a severe bug (or an actual data loss in production) will be introduced.
Hibernate Docs have terrible SEO indexing, but an AI found for me that in version 3.5 there was an excellent documentation:
It had mentioned the following critical points:
I wasn't able to find any similar information in the latest docs (7.1). All the sections arbitrarily link to each other in a way that there is no single place dedicated to describing the concept of read-only anymore.
I followed the reference of
and added a warning to
Persistence Contexts.Similarly, there was an example of a usage in
Read-only entities native query example, so I added an explicit cascade usage example below that.SessionandQuery(SelectionQuery)'s JavaDocs to explicitly document this behavior inside thesetReadOnlyandsetDefaultReadOnlymethods. This is the least we can do - I'd actually propose that you could consider renaming the methods.Please perform a thorough review of the documentation from the standpoint of ensuring that this concept is searchable.
Additionally, please consider preparing examples of correct usages for users who want to use an actual read-only state, especially at the beginning (right after opening a session). These patterns are currently very difficult to learn and understand. Namely, it's very dangerous to assume all developers will remember to evict an entity before mutating it if we don't open a read-only transaction. (We're currently analyzing the downsides of read-only transactions.)
I also found one ticket that may be related, which seems to consider the idea of "fixing" the behavior of read-only to be what one would intuitively assume (though it would be a breaking change):
Accountentity was missing in the example data model, which I noticed when trying to reproduce your example from Docs, so I also fixed it - please review the comment-docs references etc.Read-only entities native query example, because it's very misleading to include a phrasenative query, which is a very distinct concept.entity-immutabilityreference that linked to an anchor that no longer exists (clicking on the link had no effect).entity immutabilityshould be reviewed, because it doesn't really relate to the concept ofread-only. I mean, it kinda does, but it's very ambiguous, as it mentions that themutabilityhas various relative meanings; but it doesn't explicitly describe theread-only-kind of mutability. (It's far from the usefulness of the old docs.)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.