Bugzilla – Full Text Bug Listing
| Summary: | Transient eOpposites are not set when loading from persistence | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Caspar D. <caspar_d> | ||||||||
| Component: | cdo.core | Assignee: | Caspar D. <caspar_d> | ||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | saulius.tvarijonas, stepper | ||||||||
| Version: | 4.0 | Flags: | stepper:
review+
|
||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Caspar D.
Created attachment 186467 [details]
Testcases (as a patch)
I have doubts myself as to whether it ever makes sense for one end of a bi-di ref to be persistent and the other end to be transient. It seems like a modeling error, and so I've raised the issue on the newsgroup: http://www.eclipse.org/forums/index.php?t=msg&th=202817 On the NG, Ed has made the point that we might as well treat a bidi ref with one persistent end and one transient end, as completely persistent, since this makes no difference to the user. I agree with this. But CDO (with auditingSupport enabled) has a special problem: the object holding the transient end will be revisioned every time its 'pseudo-transient' reference changes... which is probably just what the user is trying to avoid by setting transient=true. But what other solutions are there? If we don't persist the transient end, then it'll have to be set on the fly when the object gets loaded. But how can we set it on the fly? Clearly it's not an option to scan the entire repo for the persistent other end of the bidi ref. So what do we scan then? Only the objects that are already loaded on the client? That's doable (although still potentially slow without an optimization mechanism) but it means the value of the transient end of the ref becomes a function of the client's local state -- which makes me think that it can't be of much use to the client. Moreover, if we take this approach, we also need to deal with the case when the object holding the transient end is already loaded, and the (or 'a') persistent end is loaded later. In that case, we need to set the transient end when the persistent end gets loaded. This has a curious general implication: loading one object might cause changes to another object, with notifications getting fired etc. I'm not sure if that's a good direction to go in... Another option is to persist the transient end as Ed suggested, but NOT revision the object when *only* the transient ref is updated. That would work perfectly, but I think it'll be very hard to implement, as it would affect lots of code :-( Last option is to just not support it, and consider a transient eOpposite of a persistent reference, to be a modeling error. (In reply to comment #3) > On the NG, Ed has made the point that we might as well treat > a bidi ref with one persistent end and one transient end, > as completely persistent, since this makes no difference to > the user. I agree with this. I agree with this, too. > But CDO (with auditingSupport enabled) has a special problem: > the object holding the transient end will be revisioned every > time its 'pseudo-transient' reference changes... which is > probably just what the user is trying to avoid by setting > transient=true. I think this is, in the context of CDO, not a valid argumentation. If a bidi reference with at least one persistent end is modified then both ends must reflect that in the history. I propose that we modify EMFUtil.isPersistent(EStructuralFeature) so that it returns true for transient bidi refs with a persistent opposite. In other words, we just ignore the "modeling error". OK, based on NG discussion, we will go with what Ed suggested: transient eOpposities of persistent refs, will be considered *persistent*. Will provide patch shortly. Created attachment 186605 [details]
Patch v1 (including testcase)
Created attachment 186624 [details]
Patch v2 - ready to be committed
Just minor reformatting...
Eike, Actually, it wasn't modeling error. We wanted to have a back reference in model without changing persistent state. We intentionally used transient flag to say to CDO not to persist. So fixing transient to be persistent doesn't make sense to me. Maybe it is better to step back and explain why we wanted to use transient. In model we created back opposite references for performance reasons. While semantically it might look strange, it is very convenient and fast. Some specific logic is much easier to calculate from opposite end. We have considered non-model approach also with cache/index outside of model. It is problematic with OCL to access non-model data. Plus we want to have notifications also. Why transient? Because semantically it is not real bidi ref and we don't want to create new revision every time new ref to this object is created. Another aspect is access control(permissions). Normally we might not have write access to these referenced objects. So we want to persist one end. One more critical aspect to us is scope concept. Maybe this should be discussed as separate bugzilla, but is related to this issue. Let me know if you want to open new. CDO currently has only one scope whole repository. From our application point user is working on project scope (collection of resources). Some operations might be performed on whole repository, some on project scope. For instance search or model validation. I think it would be good to introduce a *scope* or *context* concept into CDO. We could use it with queries or OCL also. With back refs in this situation we wanted to have scope also. Eike, let us know what you think about it. Does it make sense? Maybe you see better approach? (In reply to comment #8) As I've already discussed with Saulius on Skype, I think what's being requested is possible, but not at all easy to realize. I think so far it's been assumed that 'transient' means: the user app will take responsibility for computing this value. The current request invalidates that assumption, and so may be viewed as a special case of a more general problem/request: to have CDO compute the values of non-persistent attributes. In this specific case, the meaning of the value is obvious, but that doesn't eliminate any of the general problems. In my opinion, those are: 1. The value has to be computed (at least initially) on the server-side; anything else would sabotage the lazy loading. 2. The value has to be transmitted from the server to the client; currently transient features are not transmitted. 3. How to make such computations efficient, is a big question. (Saulius' idea of scopes' might play a role in this, although it's not yet clear to me what their general validity is.) 4. When one client commits a change that affects the computed value of a transient value held by another client, then that 2nd client must somehow become aware of the change to the transient value in case of passiveUpdates/refreshes. This means either the server side must notice the impact of the change to the persistent value on the transient value, recompute the latter, and include it in the commit notification or the refresh; or, alternatively, the client must recompute the value based on the changes to the persistent values included in the passiveUpdate/refresh. I'm puzzled as to whether the 2nd approach is always possible... needs more thought. (Note by the way that there is no *general* way of determining which transients should be recomputed in response to the change in a persistent value, since that would depend on the nature of the relationship. But in the specific case it's obvious.) Now, for the specific case of computing transient eOpposites, I think (3) can be solved by persisting the computed values outside the model. In fact, I don't see any other way of maintaining any semblance of efficiency. This persistent cache of computed values would have to be versioned if we want the transient eOpps to also be computed correctly in audit views... In summary, I think realizing the requested behavior will involve new server side computations, a new server-side storage mechanism, changes to the revision transmission protocol, changes to the passiveUpdates/refresh mechanisms, and new client-side computations. All in all, I think it's going to be a big undertaking. (In reply to comment #9) > I think so far it's been assumed that 'transient' means: the > user app will take responsibility for computing this value. I fully agree. transient=true means there's not even a slot in a CDORevision for storage and transport. We don't want to break this easy assumption that every code of CDO and of any user relies upon. > The current request invalidates that assumption, and so may > be viewed as a special case of a more general > problem/request: to have CDO compute the values of > non-persistent attributes. CDO client or CDO server. The latter is not possible, see above. Computing it on the client is identical to computing it in the model where computations belong. Use derived/transient features. I think you could even use CDOView.queryXRefs() inside a derived feature. What about change notification from remote commits? I still fail to see any value in persistent/transient opposites. Any attempt to implement them is very expensive and extremely risky. While solutions in your specific model are cheap and easy (derived features). The whole scope topic is a separate one, right? Committed to trunk, rev. 6921 Reopening on Saulius' request. Resolving, as there's no reason specified that justifies the REOPEN. Eike, The way it was fixed is even bigger problem for us. We cannot use transient opposite reference any more, because it is always persistent now. Can we make it more flexible/configurable? For example allow to plug our own implementation how to compute opposite transient. By default implementation could be based on queryXRefs(). Could you reopen and discuss all details? Saulius I still fail to see why you just don't want this tiny opposite persisted. See, this is really not just an optimization question. If the ooposite is transient the CDOFeatureDeltas are impacted. That can lead to many many bad things, including uncheckable inconsistencies. Referential integrity checks as well as containment cycle checks heavily rely on the feature deltas. I can not even think of an argument to contradict that, but you may still try ;-) (In reply to comment #15) > I still fail to see why you just don't want this tiny opposite persisted. In our meta-model it will change semantic meaning. Plus we get unnecessary revision/version. And finally I might not have write permissions on this object. > > See, this is really not just an optimization question. If the ooposite is > transient the CDOFeatureDeltas are impacted. That can lead to many many bad > things, including uncheckable inconsistencies. Referential integrity checks as > well as containment cycle checks heavily rely on the feature deltas. How is this related to deltas? Can you give an example of bad scenario? > > I can not even think of an argument to contradict that, but you may still try > ;-) Attempt #2 :) (In reply to comment #16) > In our meta-model it will change semantic meaning. The term "semantic" is commonly used to refer to the business aspects of a model. The aspect of being transient or not is orthogonal to that. It is not a semantic apsect. If you used it in that way you made a usage/modeling error. The interpretation of this property is reserved for persistence solutions. > Plus we get unnecessary revision/version. And I said, it's not unnecessary but needed that the opposite object gets a new version number, hence a revision. But I will look for code examples that illustrate it (see below). > And finally I might not have write permissions on this object. An indication for how difficult and problematic bidirectional references are in general. Can you explain why it must be a bidirectional reference and not just a transient derived back reference? > > See, this is really not just an optimization question. If the ooposite is > > transient the CDOFeatureDeltas are impacted. That can lead to many many bad > > things, including uncheckable inconsistencies. Referential integrity checks as > > well as containment cycle checks heavily rely on the feature deltas. > > How is this related to deltas? Can you give an example of bad scenario? Give me some time to find them... (In reply to comment #16) > How is this related to deltas? Can you give an example of bad scenario? Well, for example exactly the mechansim you would use to actually implement the "missing" end of the reference, i.e. queryXRefs(), would no longer work for the other end of that reference. Because there's no longer a revision with the pointing ID in the table of the DBStore. Looking for more examples... And consider this: Client A changes the persistent end of a bidi ref. Client B changes an attribute of the object at the transient end. Both are comitting at the same time. Usually one of them would fail with a CME. But this state could no longer be detected by the store. Is this an implicit conflict resolution at the server now? And notification: Adapters on the object with the transient end would no longer get notifications. I already hear people crying "my local state is corrupted because my adapter logic is no longer triggered". (In reply to comment #17) > (In reply to comment #16) > > In our meta-model it will change semantic meaning. > > The term "semantic" is commonly used to refer to the business aspects of a > model. The aspect of being transient or not is orthogonal to that. It is not a > semantic aspect. If you used it in that way you made a usage/modeling error. > The interpretation of this property is reserved for persistence solutions. What I try to say, semantically we have directed (not bidi) reference. An opposite transient reference added only for performance reasons and OCL. We can treat it as modeling error, but what are solutions to model correctly? Using xrefs query is CDO specific option and not available in OCL. > > > Plus we get unnecessary revision/version. > > And I said, it's not unnecessary but needed that the opposite object gets a new > version number, hence a revision. But I will look for code examples that > illustrate it (see below). > I understand it is needed for current implementation. Need to check your examples. But we marked it transient to not create new version. > > And finally I might not have write permissions on this object. > > An indication for how difficult and problematic bidirectional references are in > general. Can you explain why it must be a bidirectional reference and not just > a transient derived back reference? An opposite I think has semantic meaning and technically ensures both ends are in sync. As we created opposite not for semantic reasons, it is not relevant here. From technical side it is convenient if you can set one end and another end is automatically updated. If an opposite is used as read only feature not a big advantage of keeping it opposite. Maybe we could live with derived transient only ... Discussion seems to focus on whether or not the requested behavior is useful/reasonable from the client app's perspective. But regard- less of whether it is or not, I have already made the point that this would be *very* hard to implement. See my comment of 2011-01-17, in particular the last paragraph. I still see no attempt whatsoever to address the huge implementation problems involved. (In reply to comment #18) > (In reply to comment #16) > > How is this related to deltas? Can you give an example of bad scenario? > > Well, for example exactly the mechansim you would use to actually implement the > "missing" end of the reference, i.e. queryXRefs(), would no longer work for the > other end of that reference. Because there's no longer a revision with the > pointing ID in the table of the DBStore. Yes, queryXRefs is possible way to implement transient end. It will allow to query objects which have persistent references. And yes, it will not work for other end if you try to query transient information. I think it is expected behavior that queryXRefs is using persistent information only. > Client A changes the persistent end of a bidi ref. > Client B changes an attribute of the object at the transient end. > Both are comitting at the same time. Usually one of them would fail with a CME. > But this state could no longer be detected by the store. > Is this an implicit conflict resolution at the server now? Changing transient end also changes persistent end as they are opposites. So both clients will have dirty objects with persistent end. First commits, second fails with a CME. Nothing changes as I see. > And notification: > Adapters on the object with the transient end would no longer get > notifications. I already hear people crying "my local state is corrupted > because my adapter logic is no longer triggered". I agree notifications support is important. IMO it is similar situation like with other derived and transient features. An opposite is derived case also. Are people crying for other derived transient features? I guess it is general problem with derived transient features under CDO. > Yes, queryXRefs is possible way to implement transient end. It will allow to
> query objects which have persistent references.
Yes, you can easily do that on the client in the body of a transient derived reference. No need for eOpposite being set.
(In reply to comment #24) > > Yes, queryXRefs is possible way to implement transient end. It will allow to > > query objects which have persistent references. > > Yes, you can easily do that on the client in the body of a transient derived > reference. No need for eOpposite being set. Eike, we cannot make it non opposite. It will change meaning of relationship. I think CDO should not restrict user modeling abilities. We don't think opposite relationship with one end transient is modeling error. Can we make it optional and configurable so users like us could plug our own calculation mechanism which takes care of transient end? By default it could work as now. > Eike, we cannot make it non opposite. It will change meaning of relationship. Then make it non-transient, too. > I think CDO should not restrict user modeling abilities. We don't think opposite > relationship with one end transient is modeling error. We've clarified that already: Ed, I and my team, we all think, it is. And we've provided you with an automatic fix (re-interpretation). > Can we make it optional and configurable so users like us could plug our own > calculation mechanism which takes care of transient end? By default it could > work as now. No, this mechansim would be a huge monster everywhere in the CDO code. Only a couple of lines in your configuration but we'd have to maintain the monster forever. Please use one of the simple alternatives we've offered: 1) If you want to have it bidi make both ends persistent. 2) If you want one end transient make it non-bidi. Available in R20110608-1407 |