Issue 157 follow refs #164
Conversation
|
|
jbornemann
force-pushed
the
issue-157-follow-refs
branch
from
ff98a80
to
30dcd61
Compare
jbornemann
force-pushed
the
issue-157-follow-refs
branch
3 times, most recently
from
2d314b6
to
9df5917
Compare
|
Would love some feedback on this when ever you guys get the chance :-) I was going to cut a 7.1 release with the user/group features after this guy. |
jbornemann
force-pushed
the
issue-157-follow-refs
branch
from
9df5917
to
e3f5727
Compare
| /** | ||
| * @return true if the node at the path being written is being written to an existing node | ||
| */ | ||
| static boolean writingToExistingNode(@Nonnull final String pathBeingWritten, @Nonnull final Session session) { |
There was a problem hiding this comment.
I think method name is not making the intent clear... To me it seems that this method would write data to node, which it is not doing
There was a problem hiding this comment.
I had a tough time coming up with a good name for this one. Suggestions?
There was a problem hiding this comment.
I am also having tough time thinking about a name for this.. :P writesToExistingNode or what you currently have should be fine.. 👍
|
Any additional thoughts on this? Was going to get it integrated? @sagarsane |
jbornemann
force-pushed
the
issue-157-follow-refs
branch
from
e3f5727
to
d31ae3c
Compare
|
@jbornemann will be able to go through this hopefully after this week (release). Btw, did you guys see https://blogs.adobe.com/contentmanagement/2017/03/20/aem-rockstar-tips-tricks-blog-series/ ? :) |
|
I did! So awesome! Congrats on being mentioned!!! |
|
Congrats to the whole team here :) |
Automatically avoid modifying admin user Extract parent existence logic into JCRUtil Clarification in log message within ACL decorator
jbornemann
force-pushed
the
issue-157-follow-refs
branch
from
d31ae3c
to
fdd8ded
Compare
Hey guys @viveksachdeva @sagarsane this should be good for a look over, with one minor outstanding question. It's a lot, so bear with me :-)
In general, what this PR means is that we can now have nodes write proper references.
If a node has weak (WEAK_REFERENCE) or strong reference (REFERENCE) properties on it, Grabbit will know to find referenced nodes, have them synched with the original node, and translate their original server jcr:uuid, to their new jcr:uuids on the receiving server. Protected reference properties that are not handled, do not have their reference nodes synched; just like they do not have their properties synched (e.g mix:versionable. Version storage nodes are not synched). Part of how we do this translation, is sending an identifier field along with mix:referenceable node messages. This identifier is only sent with referenceables, as it doesn't make sense for other node types - references can only reference mix:referenceables. The functionality also supports the case where weak references may have pointers to nodes which no longer exist, i.e groups with pointers to deleted users.
If an authorizable:
1.) Groups with members (ie. protected property 'rep:members', type WEAK_REFERENCE) will have their members synched as described above.
2.) If an existing authorizable is updated, it retains membership in other groups. In other words, when we update an authorizable, the node is recreated, and the jcr:uuid can potentially change. We don't want this to affect groups that have pointers to the original authorizable.
Framework for functionality demonstration, and testing:
1.) General node reference preservation. Create a node 'a' that has a weak reference to another node 'b'. Have a strong reference on 'b' that references a new node 'c'. Synching just 'a' should result in these references being preserved, and 'a', 'b', and 'c' being synched automatically. Order of events should not matter. Having a job path for 'a', 'b', and 'c' independently should not affect 'a's ability to point to 'b', or 'b' to 'c'. Verify this.
2.) Group membership. Create a new group 'test-group' in User Administration, and a few members 'user-a', 'user-b'. Add these members to 'test-group'. Synching 'test-group' should result in 'user-a', and 'user-b' being synched as well. Syncing 'test-group' independently of 'user-a', and 'user-b' should not affect this result due to test number 3 below.
3.) Group retention. After syncing updated 'user-a' alone, 'user-a' should still be a member of 'test-group'.
Other misc. changes:
Unsatisfied scenario with a few options :
Given a reference to a node whose parents are not yet synched on the receiving server, how should we proceed? If such as node's path was within a path configuration rather than reference, we would fail with VALIDATION_FAILED at configuration stage. We do this so that Jackrabbit does not write nt:unstructured intermediate nodes in place of actual parents. This is a easy scenario, because it is nice and deterministic feeling. Path within configuration are segregated, and easy to manage; and paths that we sync are known at configuration time.
This case is more difficult because a path configuration may contain a node which reference may point to a node synched as part of another path configuration. If parents are already synched, this is not a problem; but if they are not, it becomes a race condition.
Some options :
1.) Continue assuming segregation, and assume this is an unlikely corner case. No effort, no code, no expensive runtime checks. I feel unprotected reference properties on unstructured nodes are pretty rare in the wild. Any other option with the current fail-on-no-parent approach would require a runtime check on writing a given node - no one likes general performance degradation, especially for saving corner cases. If we hit 99% of cases, we are "good enough" for what it is. The downside to accepting, and acknowledging that particular scenarios are not handled well are that if our assumptions are wrong about what's common, or wild - this will come back to bite us.
2.) Keep 1 for now, but rethink, redevelop how we handle this missing parent scenario in general; especially with this in mind. We could always find a way to automatically sync bare minimum intermediate nodes, rather than fail. I think we mainly just fail now, rather than handle this automatically because at a certain point, it is better just to fix configuration than add code, and complexity to solve simple issues automatically; it's a non-trivial amount of code. With this, those days of thinking may be passing. These issues hopefully go away organically in this case. The downside is that if we are right about our assumptions above, we will have added unwarranted complexity to the project. Maybe not enough to panic though.
3.) Do a runtime check on node write, and fail the same as we would at configuration-time. This is simple effort wise, but it may result in a bad, confusing experience; and a bit more difficult to remedy than if it were a path configuration failure. Another down-side as mentioned above is performance. Reference nodes, vs your every day nt:unstructured nodes are written the same way - it's all generic, and should stay that way. This means we apply a penalty to all nodes, to save these cases.
I'm leaning on number 2. Thoughts? This is probably one of the longer PR descriptions I've written ;)