Hi, On Thu, Feb 28, 2013 at 1:13 PM, Michael Dürig <[email protected]> wrote: > Just some anecdotal evidence: a single call to Node.addNode() results in a > call to Node.getPrimaryNodeType() which in turn ends up calling > Property.getValue() and which eventually results in a call to > Property.isMultiple(). So there will be quite a bit of stuff to untangle. > Not sure how to go about this in order to catch them all.
Yeah, I find this pattern somewhat troublesome and much more prevalent than I'd expected. It would be good to avoid crossing the JCR API boundary multiple times during the execution of a single operation. To do this, I'd suggest we adjust the Impl/Delegate boundary as follows: Responsibilities of JCR Impl classes: - name/path mapping for both method arguments and return values -> NamePathMapper should be in SessionImpl instead of SessionDelegate - tracking and instantiation of other JCR Impl objects -> Delegate classes should refer to neither the JCR API nor the Impl classes -> Values should be returned as PropertyState instances that are mapped to JCR Values by an Impl class Delegate classes - access to the Oak API - the checkStatus() and perform() logic -> Something like: - all the "business logic" associated with complex operations -> the complex SessionObject classes from Impl classes should be pushed down to Delegates -> dlg.perform(dlg.getSomeOperation(oakName, ...)) To make it easier to enforce such a stricter design boundary with tools like JDepend, I'd move the Delegate classes to a separate package structure under o.a.j.oak.jcr.delegate. > OTOH as your main concern initially was to reduce boilerplate, we could also > try the other approach I proposed earlier: enrich SessionOperation with > preconditions to be checked before the core of the call is actually > executed. Agreed. One particular SessionOperation adjustment I'd like to add is information on whether the operation can change the state of a session. Since otherwise the session is immutable (see also below), we could shortcut the status checks to be performed only after such a state-changing operation (like refresh() and all transient updates) has been made. Since the majority of sessions are read-only and never change their states from the initial snapshot, this would allow us to avoid most status checks entirely. > performed. Currently there is still the chance, that the conditions become > invalid right after the check methods returned but before the operation is > actually performed. How would that happen? Since we're operating on an immutable snapshot of the repository, the only way for the state of a session to change is through a concurrent call from the client, possibly triggered by an observation thread. An easy way to prevent that from being a problem is to synchronize the perform() method. BR, Jukka Zitting
