Updated property to triple conversion to handle single values with the s... #212
Conversation
|
I guess? Honestly I don't fully understand all of the implications of this code except that: we did one thing for multivalued properites and something else for single valued properties. Whether a property is multiple or single valued shouldn't affect how we handle the individual values. The simpler thing we did for single valued properties didn't work for references across workspaces, now it does. |
| @@ -169,9 +169,11 @@ private Node traverseLink(final Property p, final Value v) | |||
| refNode = p.getSession().getNodeByIdentifier(v.getString()); | |||
| } | |||
| } else { | |||
There was a problem hiding this comment.
What behavior were you seeing? I thought #getNode() was the single-value equivalent to the above..
There was a problem hiding this comment.
2013-12-11 17:09:54.902:WARN:oejs.ServletHandler:/rest/jcr:system/jcr:versionStorage/8f/60/af/8f60af36d6ff6258bcdc743e5a02e234c73e8117/1.0/jcr:frozenNode
java.lang.RuntimeException: javax.jcr.ItemNotFoundException: No node exists at path '87a0a8c317f1e78d85087a-d29f-4e7f-b525-5f406e892c93' in workspace "default"
at com.google.common.base.Throwables.propagate(Throwables.java:160)
at org.fcrepo.kernel.rdf.impl.mappings.PropertyToTriple.propertyvalue2node(PropertyToTriple.java:152)
at org.fcrepo.kernel.rdf.impl.mappings.PropertyToTriple.propertyvalue2triple(PropertyToTriple.java:119)
at org.fcrepo.kernel.rdf.impl.mappings.PropertyToTriple.access$000(PropertyToTriple.java:53)
at org.fcrepo.kernel.rdf.impl.mappings.PropertyToTriple$1$1.apply(PropertyToTriple.java:101)
at org.fcrepo.kernel.rdf.impl.mappings.PropertyToTriple$1$1.apply(PropertyToTriple.java:97)
at com.google.common.collect.Iterators$8.transform(Iterators.java:794)
at com.google.common.collect.TransformedIterator.next(TransformedIterator.java:48)
at com.google.common.collect.Iterators$5.next(Iterators.java:553)
at com.google.common.collect.Iterators$5.next(Iterators.java:553)
at com.google.common.collect.Iterators$5.next(Iterators.java:553)
at com.google.common.collect.ForwardingIterator.next(ForwardingIterator.java:48)
at com.google.common.collect.Iterators$5.next(Iterators.java:553)
at com.google.common.collect.Iterators$5.next(Iterators.java:553)
at com.google.common.collect.ForwardingIterator.next(ForwardingIterator.java:48)
at com.google.common.collect.Iterators$5.next(Iterators.java:553)
at com.google.common.collect.Iterators$5.next(Iterators.java:553)
at com.google.common.collect.Iterators$5.next(Iterators.java:553)
at com.google.common.collect.Iterators$5.next(Iterators.java:553)
at com.google.common.collect.ForwardingIterator.next(ForwardingIterator.java:48)
at org.fcrepo.kernel.utils.iterators.RdfStream.asModel(RdfStream.java:293)
There was a problem hiding this comment.
They key being that the node isn't in the default workspace, it's in the jcr:system workspace.
There was a problem hiding this comment.
So is this a problem with the method retrieving the node, or is it that the method is okay, but it's being asked to retrieve a node for a triple that it shouldn't be getting?
There was a problem hiding this comment.
Weird. Sounds like something we should report upstream, just in case it's a bug.
Made a ticket https://www.pivotaltracker.com/story/show/62480780 to report it upstream.
There was a problem hiding this comment.
Or is it that some nodes have ref properties aimed at system nodes, and we didn't know that? Versioned nodes have properties that might aim back into the system workspace... should we be filtering the properties in a way that we haven't been, before using them to generate triples?
There was a problem hiding this comment.
If you look at the whole method, not just the diff it illustrates that someone already identified that p.getNode() is insufficient since for multivalued properties they had this special handling. While I personally don't know enough about what properties go through here and how properties of different types behave when getNode() is invoked, it is my understanding that the only difference between properties that return true from p.isMultiple() and those that return false is whether the have multiple values. Knowing that, it seems clear that for whatever reason justified this rigmarole for the values of multivalued properties we are justified in performing it on the values of nodes that happen to be single valued.
Honestly I think it would be cleaner to collapse the whole if statement here and always assume that the value is a value from the property and have a single execution path for this method. That likely changes the contract for the method since a javadoc implies that for single-valued properties the "value" parameter need not be set. (though it appears as if it always is)
There was a problem hiding this comment.
#getNode() only works for single-valued properties, and there's no way (until MODE-2098 is released) to get a node out of a multivalued reference property. It had nothing to do with trying to fix the issue at hand.
There was a problem hiding this comment.
+1 to collapsing them, though. If we already have a value, I guess we might as well use it, for now, at least. I'm sure we were going getting the value from the property so we could use the #getNode() method.
…e same consideration as multiple values.
| @@ -100,6 +100,37 @@ public void testAddAndRetrieveVersion() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testVersioningANodeWithAVersionableChild() throws Exception { | |||
| final String pid = "testVersioningANodeWithAChild"; | |||
There was a problem hiding this comment.
Any reason this should be a hard-coded pid and not dynamically calculated, e.g.:
final String pid = UUID.randomUUID().toString();
|
Merged in 0a5fbaa...cdf0540 |
...ame consideration as multiple values.
I added an integration test for the real-world case that caused me to discover this bug.