You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by tr...@apache.org on 2013/10/15 02:42:22 UTC
svn commit: r1532157 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
Author: tripod
Date: Tue Oct 15 00:42:21 2013
New Revision: 1532157
URL: http://svn.apache.org/r1532157
Log:
OAK-1086 NodeTypes of successive calls to node.getPrimaryNodetypes() are not equal
- implement equals and hashcode based on the CND of the node type definition.
Added:
jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java?rev=1532157&r1=1532156&r2=1532157&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java Tue Oct 15 00:42:21 2013
@@ -31,6 +31,8 @@ import static org.apache.jackrabbit.oak.
import static org.apache.jackrabbit.oak.plugins.nodetype.NodeTypeConstants.JCR_IS_QUERYABLE;
import static org.apache.jackrabbit.oak.plugins.nodetype.NodeTypeConstants.RESIDUAL_NAME;
+import java.io.IOException;
+import java.io.StringWriter;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.ArrayList;
@@ -49,6 +51,7 @@ import javax.jcr.nodetype.ItemDefinition
import javax.jcr.nodetype.NoSuchNodeTypeException;
import javax.jcr.nodetype.NodeDefinition;
import javax.jcr.nodetype.NodeType;
+import javax.jcr.nodetype.NodeTypeDefinition;
import javax.jcr.nodetype.NodeTypeIterator;
import javax.jcr.nodetype.PropertyDefinition;
@@ -57,6 +60,8 @@ import com.google.common.collect.Iterabl
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
+
+import org.apache.jackrabbit.commons.cnd.CompactNodeTypeDefWriter;
import org.apache.jackrabbit.commons.iterator.NodeTypeIteratorAdapter;
import org.apache.jackrabbit.oak.api.PropertyState;
import org.apache.jackrabbit.oak.api.Tree;
@@ -97,6 +102,8 @@ class NodeTypeImpl extends AbstractTypeD
private static final String[] NO_NAMES = new String[0];
+ private String cnd;
+
NodeTypeImpl(Tree type, NamePathMapper mapper) {
super(type, mapper);
}
@@ -405,13 +412,68 @@ class NodeTypeImpl extends AbstractTypeD
return internalCanRemoveItem(propertyName, Arrays.asList(getPropertyDefinitions()));
}
+ /**
+ * Returns the namespace neutral CND of the given node type definition.
+ * @param def the node type definition
+ * @return the CND
+ * @throws IOException if an error occurs.
+ */
+ private static String getCnd(NodeTypeDefinition def) throws IOException {
+ StringWriter out = new StringWriter();
+ CompactNodeTypeDefWriter cndWriter = new CompactNodeTypeDefWriter(out, new CompactNodeTypeDefWriter.NamespaceMapping(){
+ @Override
+ public String getNamespaceURI(String s) {
+ return s;
+ }
+ }, false);
+ cndWriter.write(def);
+ return out.toString();
+ }
+
+ /**
+ * Returns the namespace neutral CND of the this node type definition.
+ * @return the CND
+ */
+ private String getCnd() {
+ if (cnd == null) {
+ try {
+ cnd = getCnd(this);
+ } catch (IOException e) {
+ log.error("Internal error while writing CND for {}", this);
+ cnd = getName();
+ }
+ }
+ return cnd;
+ }
+
//-------------------------------------------------------------< Object >---
@Override
public String toString() {
return getName();
}
- //-----------------------------------------------------------< internal >---
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (o instanceof NodeTypeImpl) {
+ return getCnd().equals(((NodeTypeImpl) o).getCnd());
+ } else if (o instanceof NodeType) {
+ try {
+ return getCnd().equals(getCnd((NodeType) o));
+ } catch (IOException e) {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ }
+
+ @Override
+ public int hashCode() {
+ return getCnd().hashCode();
+ }
+
+//-----------------------------------------------------------< internal >---
private boolean internalCanRemoveItem(String itemName,
Iterable<? extends ItemDefinition> definitions) {
Added: jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java?rev=1532157&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java (added)
+++ jackrabbit/oak/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java Tue Oct 15 00:42:21 2013
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.jcr.nodetype;
+
+import javax.jcr.nodetype.NodeType;
+
+import org.apache.jackrabbit.test.AbstractJCRTest;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ *
+ */
+public class NodeTypeEqualsTest extends AbstractJCRTest {
+
+ /**
+ * Tests if 2 NodeType objects are equals if they refer to the same node type. OAK-1086.
+ * @throws Exception
+ */
+ @Test
+ public void testNodeTypeEquals() throws Exception {
+ NodeType nt = testRootNode.getPrimaryNodeType();
+ Assert.assertEquals("Same NoteType could be equal", nt, testRootNode.getPrimaryNodeType());
+ }
+
+
+}
\ No newline at end of file
Re: svn commit: r1532157 - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
Posted by Michael Dürig <md...@apache.org>.
On 15.10.13 8:04 , Tobias Bocanegra wrote:
> I think the semantics of NodeType.equals() should be mentioned in the JCR
> API. I'm indifferent if we should consider the comparison between 2
> different implementations.
The symmetry requirement comes from the general contract of equals and
we can't specify this away. Its a tough requirement to get right across
inheritance. That's why I'd prefer to not take inheritance into account
and simply return false for NodeType.equals() between different
implementations.
The current implementation would still return true for n.equals(m) but
might return false for m.equals(n) if getCnd(m).equals(getCnd(n)) and n
instanceOf NodeTypeImpl and !(m instanceOf NodeTypeImpl).
Michael
Re: svn commit: r1532157 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
Posted by Tobias Bocanegra <tr...@apache.org>.
Hi,
On Tue, Oct 15, 2013 at 12:05 PM, Jukka Zitting <ju...@gmail.com> wrote:
> Hi,
>
> On Tue, Oct 15, 2013 at 2:47 PM, Tobias Bocanegra <tr...@apache.org> wrote:
>> Sure, but in this particular case I think that comparing the actual
>> definition is desired. Imagine a NodeType editor that operates on a session
>> and wants to list the nodetypes that were modified.
>
> Makes sense.
>
>> OTOH, in most cases comparing the names should be good enough. If you think
>> the comparing the CND is overhead, please re-open [0] and I'll change it.
>
> CND comparison should be good here.
>
> In fact the main change I'd make to the implementation is to drop the
> caching of the CND string, and simply regenerate it whenever needed.
> That should simplify the equals method to:
>
> public boolean equals(Object o) {
> return o instanceof NodeTypeDefinition
> && getCnd(this).equals(getCnd((NodeTypeDefinition) o));
> }
>
> I'd value simplicity over performance here as AFAICT no
> performance-sensitive code uses this feature and correctly
> invalidating the cached value would be non-trivial.
good point.
> Also, as a lesser concern, I'd drop the fallback in the "catch
> (IOException e)" clause. Instead of logging the error and continuing
> with different semantics, I think it would be better to just fail fast
> with an IllegalStateException.
ok.
regards, toby
Re: svn commit: r1532157 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Tue, Oct 15, 2013 at 2:47 PM, Tobias Bocanegra <tr...@apache.org> wrote:
> Sure, but in this particular case I think that comparing the actual
> definition is desired. Imagine a NodeType editor that operates on a session
> and wants to list the nodetypes that were modified.
Makes sense.
> OTOH, in most cases comparing the names should be good enough. If you think
> the comparing the CND is overhead, please re-open [0] and I'll change it.
CND comparison should be good here.
In fact the main change I'd make to the implementation is to drop the
caching of the CND string, and simply regenerate it whenever needed.
That should simplify the equals method to:
public boolean equals(Object o) {
return o instanceof NodeTypeDefinition
&& getCnd(this).equals(getCnd((NodeTypeDefinition) o));
}
I'd value simplicity over performance here as AFAICT no
performance-sensitive code uses this feature and correctly
invalidating the cached value would be non-trivial.
Also, as a lesser concern, I'd drop the fallback in the "catch
(IOException e)" clause. Instead of logging the error and continuing
with different semantics, I think it would be better to just fail fast
with an IllegalStateException.
BR,
Jukka Zitting
Re: svn commit: r1532157 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
Posted by Tobias Bocanegra <tr...@apache.org>.
Hi,
On Tue, Oct 15, 2013 at 11:34 AM, Jukka Zitting <ju...@gmail.com>wrote:
> Hi,
>
> On Tue, Oct 15, 2013 at 2:04 PM, Tobias Bocanegra <tr...@apache.org>
> wrote:
> > Initially I thought that comparing just the names would be
> > enough, but then it wouldn't work correctly when a 2 nodetypes of
> different
> > sessions are compared that were modified.
>
> The Object.equals() contract doesn't require that the compared objects
> are *exactly equal*. Any equivalence relationship, including name
> comparison, should be fine.
>
Sure, but in this particular case I think that comparing the actual
definition is desired. Imagine a NodeType editor that operates on a session
and wants to list the nodetypes that were modified.
OTOH, in most cases comparing the names should be good enough. If you think
the comparing the CND is overhead, please re-open [0] and I'll change it.
Regards, Toby
[0] https://issues.apache.org/jira/browse/OAK-1086
Re: svn commit: r1532157 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Tue, Oct 15, 2013 at 2:04 PM, Tobias Bocanegra <tr...@apache.org> wrote:
> Initially I thought that comparing just the names would be
> enough, but then it wouldn't work correctly when a 2 nodetypes of different
> sessions are compared that were modified.
The Object.equals() contract doesn't require that the compared objects
are *exactly equal*. Any equivalence relationship, including name
comparison, should be fine.
BR,
Jukka Zitting
Re: svn commit: r1532157 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
Posted by Tobias Bocanegra <tr...@apache.org>.
Hi Michael, thanks for reviewing this.
On Tue, Oct 15, 2013 at 12:49 AM, Michael Dürig <md...@apache.org> wrote:
> Hi,
>
> On 15.10.13 2:42 , tripod@apache.org wrote:
>
>> Author: tripod
>> Date: Tue Oct 15 00:42:21 2013
>> New Revision: 1532157
>>
>> URL: http://svn.apache.org/r1532157
>> Log:
>> OAK-1086 NodeTypes of successive calls to node.getPrimaryNodetypes() are
>> not equal
>>
>> - implement equals and hashcode based on the CND of the node type
>> definition.
>>
>
> [...]
>
> + @Override
>> + public boolean equals(Object o) {
>> + if (this == o) return true;
>> + if (o instanceof NodeTypeImpl) {
>> + return getCnd().equals(((**NodeTypeImpl) o).getCnd());
>> + } else if (o instanceof NodeType) {
>>
>
> Should we even consider this case? If so, below implementation is not
> symmetric wrt. e.g. the NodeType implementation of JR2.
>
I think the semantics of NodeType.equals() should be mentioned in the JCR
API. I'm indifferent if we should consider the comparison between 2
different implementations.
> I'd rather return false for unknown NodeType instances.
>
ok. Maybe we could add a NodeTypeComparator() to JCR commons that can do
this check. Initially I thought that comparing just the names would be
enough, but then it wouldn't work correctly when a 2 nodetypes of different
sessions are compared that were modified. for the implementation specific
comparison, we could use the revision/hash of the definition tree.
>
> + try {
>> + return getCnd().equals(getCnd((**NodeType) o));
>> + } catch (IOException e) {
>> + return false;
>> + }
>> + } else {
>> + return false;
>> + }
>> + }
>>
>
> Michael
>
Regards, Toby
Re: svn commit: r1532157 - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/nodetype/NodeTypeImpl.java
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/nodetype/NodeTypeEqualsTest.java
Posted by Michael Dürig <md...@apache.org>.
Hi,
On 15.10.13 2:42 , tripod@apache.org wrote:
> Author: tripod
> Date: Tue Oct 15 00:42:21 2013
> New Revision: 1532157
>
> URL: http://svn.apache.org/r1532157
> Log:
> OAK-1086 NodeTypes of successive calls to node.getPrimaryNodetypes() are not equal
>
> - implement equals and hashcode based on the CND of the node type definition.
[...]
> + @Override
> + public boolean equals(Object o) {
> + if (this == o) return true;
> + if (o instanceof NodeTypeImpl) {
> + return getCnd().equals(((NodeTypeImpl) o).getCnd());
> + } else if (o instanceof NodeType) {
Should we even consider this case? If so, below implementation is not
symmetric wrt. e.g. the NodeType implementation of JR2.
I'd rather return false for unknown NodeType instances.
> + try {
> + return getCnd().equals(getCnd((NodeType) o));
> + } catch (IOException e) {
> + return false;
> + }
> + } else {
> + return false;
> + }
> + }
Michael