Invocations of populateVirtualProperties in MOS#handleSignalVertexUpdateFromEdge
Currently, populateVirtualProperties is only called on the new object state. This is likely incorrect - instead, populateVirtualProperties need only be called on the old object state. Rationale: MOS#handleSignalVertexUpdateFromEdge is called by the EdgeToVertexActionHandler in four circumstances: edge creation/deletion/update, or notification. The first three are invoked when edges are created/deleted/update, and the last is invoked when signals are propagated across vertices, or e.g. when temporal constraints are triggered.
The edge mutation cases are considered first. In these cases, the edge changes are persisted to the repo prior to signal delivery. Upon signal receipt, the vertex is read from managed, and then the signaled relationship field is read, thus retrieving post-mutation relationship field state. The signal includes the edge state pre-mutation, and this state is used to alter the relationship field state to its pre-mutation state so that the old object can be set with this state. Thus, in the delete case, the deleted edge is added to the old object relationship field; in the create case, the created edge is removed from the old object relationship field; in the update case, the edge state pre-update is set in the old object relationship field. Thus the signaled relationship field in the old and new objects will reflect the relationship field state pre and post mutation. This relationship field state is used to update the old object vertex state as read from managed.
Note that a read of a managed object will call MOS#onRetrieve, which is essentially the same as MOS#populateVirtualProperties - the only difference being that populateVirtualProperties will consider invoking the onRetrieve hook only for virtual properties - onRetrieve will do so for all properties. This means that the fields invoked in onRetrieve is a superset of populateVirtualProperties. Thus the fact that the vertex state is read from managed means that virtual properties will be calculated reflecting the relationship field state post-mutation. Thus when MOS#handleSignalVertexUpdateFromEdge is invoked, vertex virtual property state on the new object, reflecting updated relationship field state, will have already been generated. Thus the call to populateVirtualProperties on the new object is redundant, and populateVirtualProperties should be called on the old object, so that any virtual property state derived from pre-relationship-field-mutation state can be set.
Not fixing this bug would seem to prevent the correct effectiveRoles and effectiveAssignment calculation if the old script-based mechanism were used. Rationale: because the old object is derived from the new object read from managed with post-edge-mutation state, effectiveRoles/Assignments, calculated via the old script-based mechanism, would not be properly calculated as they would reflect current relationship field state, a problem when either roles or assignments were changed.
Now consider the notification case. In this case, to preserve the semantics of triggerSyncCheck, the old object is read from the repo, and the new object is read from managed (EdgeToVertexActionHandler#relationshipNotification). Because no edges were mutated, no pre and post relationship field adjustments need be performed prior to the call to MOS#handleSignalVertexUpdateFromEdge. But again, the populateVirtualProperties performed on the new object is superfluous (as it had already been read from managed prior to the invocation). The question is whether replacing the new object parameter to populateVirtualProperties with the old object is technically more correct, or instead might cause problems with e.g. temporal constraints which is one of the core use cases for relationship notifications. Problems would occur because temporal constraints on the old object would be calculated now, and thus reflect the current ‘temporal’ state, not the previous state. So the innovation of populateVirtualProperties with the old object parameter would have to be avoided in the relationship notification case.
Note that the invocations on populateVirtualProperties from IOS#handleSignalVertexUpdateFromEdge must also be examined, as they are a bit different than for MOS.