ModeShape/modeshape — pull request #615 by rhauch (2012-11-30 23:28:01+00:00)
Title: MODE-1720 Corrected how REFERENCE properties are set 

The first commit refactors ExecutionContext to simplify the code and prepare for the second commit. ExecutionContext contains quite a few objects that represent the context-specific factories, namespace registries, binary stores, etc. The nature of these components made it difficult to created altered contexts with customized value factories, security contexts, binary stores, etc.

ExecutionContext was refactored to own all of the value factory implementations, and all of the factories were changed to take "holder"-type objects that the factories use to get the components they want. These two steps make it far easier for ExecutionContext to create alterations.

These changes are simple refactorings of existing behavior, and as such made it easier to verify that the functionality remained unchanged.

The second commit attempts to fix the error described by MODE-1720. When REFERENCE properties are set with Value objects created from node identifier strings, the values were not correctly being converted (internally) to NodeKey representations. The result was that the REFERENCE values were not usable. A new test case verifies this behavior.

The challenge was that the logic was using the ReferenceFactory (in the ValueFactories inside the Session's ExecutionContext) to perform the conversion. However, the conversion requires knowledge of the root NodeKey, of which the ReferenceFactory was not aware.

The fix was to give each Session a new ExecutionContext with a custom ReferenceFactory implementation that knows about the workspace's root node key, allowing the factory to create REFERENCE (and WEAKREFERENCE) values from string Node identifiers.

Note that this resulted in a failure of a TCK test, whereby a node of type "nt:unstructured" had an existing REFERENCE property that was set to a STRING value (e.g., "Hello World") that was not convertible to a reference. This failure actually highlighted an existing bug: the internal AbstractJcrNode.setProperty(...) method did not check whether the new value had the same node type as the required type on the existing property's property definition. Because the types differed in this case, a new property definition needed to be found given the value.