Author: rhauch Date: 2009-04-13 15:13:43 -0400 (Mon, 13 Apr 2009) New Revision: 815 Modified: trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/AbstractJcrNode.java trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrI18n.java trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrMultiValueProperty.java trunk/dna-jcr/src/main/resources/org/jboss/dna/jcr/JcrI18n.properties trunk/dna-jcr/src/test/java/org/jboss/dna/jcr/JcrTckTest.java Log: DNA-346 JR TCK Tests for SetPropertyStringTest and SetPropertyValueTest Fail Applied the patch, which fixed a couple of minor defects that were keeping the tests from passing as per Javadoc in section 7.1.5 of the 1.0.1 spec: - Added code to "compact" value arrays, that is, Node.setProperty(String, Value[]) and Node.setProperty(String, Value[], int) should ignore any null values in the value array. - Added code to throw a ValueFormatException if the values in the value arrays in Node.setProperty(String, Value[]) and Node.setProperty(String, Value[], int) are not all of the same type. The spec permits coercing the values into the type expected by the property, but it does not permit different types of values in the array. - Added code (AbstractJcrNode.checkCardinalityOfExistingProperty) to throw a ValueFormatException if one attempts to set a multi-valued value onto an existing single-valued array (or vice versa) directly through the node. A few minor changes were made to the patch, including supplying useful error messages for exceptions, and some minor refactoring of some code to compute the Name object only once. Modified: trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/AbstractJcrNode.java =================================================================== --- trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/AbstractJcrNode.java 2009-04-09 20:07:42 UTC (rev 814) +++ trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/AbstractJcrNode.java 2009-04-13 19:13:43 UTC (rev 815) @@ -49,6 +49,7 @@ import javax.jcr.RepositoryException; import javax.jcr.UnsupportedRepositoryOperationException; import javax.jcr.Value; +import javax.jcr.ValueFormatException; import javax.jcr.lock.Lock; import javax.jcr.lock.LockException; import javax.jcr.nodetype.ConstraintViolationException; @@ -68,11 +69,11 @@ import org.jboss.dna.graph.property.NamespaceRegistry; import org.jboss.dna.graph.property.Path; import org.jboss.dna.graph.property.ValueFactories; -import org.jboss.dna.graph.property.ValueFormatException; import org.jboss.dna.jcr.SessionCache.NodeEditor; import org.jboss.dna.jcr.cache.ChildNode; import org.jboss.dna.jcr.cache.Children; import org.jboss.dna.jcr.cache.NodeInfo; +import org.jboss.dna.jcr.cache.PropertyInfo; /** * An abstract implementation of the JCR {@link Node} interface. Instances of this class are created and managed by the @@ -151,13 +152,16 @@ final JcrValue[] valuesFrom( int propertyType, Object[] values ) { + /* + * Null values in the array are "compacted" (read: ignored) as per section 7.1.6 in the JCR 1.0.1 specification. + */ int len = values.length; ValueFactories factories = cache.factories(); - JcrValue[] results = new JcrValue[values.length]; + List<JcrValue> results = new ArrayList<JcrValue>(len); for (int i = 0; i != len; ++i) { - results[i] = new JcrValue(factories, cache, propertyType, values[i]); + if (values[i] != null) results.add(new JcrValue(factories, cache, propertyType, values[i])); } - return results; + return results.toArray(new JcrValue[results.size()]); } @Override @@ -905,7 +909,7 @@ Path path = null; try { path = cache.pathFactory().create(relPath); - } catch (ValueFormatException e) { + } catch (org.jboss.dna.graph.property.ValueFormatException e) { throw new RepositoryException(JcrI18n.invalidPathParameter.text(relPath, "relPath")); } if (path.size() == 0) { @@ -954,7 +958,7 @@ if (primaryNodeTypeName != null) { try { childPrimaryTypeName = cache.nameFactory().create(primaryNodeTypeName); - } catch (ValueFormatException e) { + } catch (org.jboss.dna.graph.property.ValueFormatException e) { throw new RepositoryException(JcrI18n.invalidNodeTypeNameParameter.text(primaryNodeTypeName, "primaryNodeTypeName")); } @@ -976,6 +980,33 @@ } /** + * Checks whether there is an existing property with this name that does not match the given cardinality. If such a property + * exists, a {@code javax.jcr.ValueFormatException} is thrown, as per section 7.1.5 of the JCR 1.0.1 specification. + * + * @param propertyName the name of the property + * @param isMultiple whether the property must have multiple values + * @throws javax.jcr.ValueFormatException if the property exists but has the opposite cardinality + * @throws RepositoryException if any other error occurs + */ + private void checkCardinalityOfExistingProperty( Name propertyName, + boolean isMultiple ) + throws javax.jcr.ValueFormatException, RepositoryException { + // Check for existing single-valued property - can't set multiple values on single-valued property + PropertyInfo propInfo = this.nodeInfo().getProperty(propertyName); + if (propInfo != null && propInfo.isMultiValued() != isMultiple) { + if (isMultiple) { + I18n msg = JcrI18n.unableToSetSingleValuedPropertyUsingMultipleValues; + throw new ValueFormatException(msg.text(getPath(), + propertyName.getString(cache.namespaces), + cache.workspaceName())); + } + I18n msg = JcrI18n.unableToSetMultiValuedPropertyUsingSingleValue; + throw new ValueFormatException(msg.text(getPath(), propertyName, cache.workspaceName())); + } + + } + + /** * {@inheritDoc} * * @see javax.jcr.Node#setProperty(java.lang.String, boolean) @@ -983,7 +1014,9 @@ public final Property setProperty( String name, boolean value ) throws ValueFormatException, VersionException, LockException, ConstraintViolationException, RepositoryException { - return cache.findJcrProperty(editor().setProperty(nameFrom(name), valueFrom(PropertyType.BOOLEAN, value))); + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, false); + return cache.findJcrProperty(editor().setProperty(propertyName, valueFrom(PropertyType.BOOLEAN, value))); } @@ -999,8 +1032,11 @@ // If there is an existing property, then remove it ... return removeExistingValuedProperty(name); } - return cache.findJcrProperty(editor().setProperty(nameFrom(name), valueFrom(value))); + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, false); + return cache.findJcrProperty(editor().setProperty(propertyName, valueFrom(value))); + } /** @@ -1011,7 +1047,9 @@ public final Property setProperty( String name, double value ) throws ValueFormatException, VersionException, LockException, ConstraintViolationException, RepositoryException { - return cache.findJcrProperty(editor().setProperty(nameFrom(name), valueFrom(PropertyType.DOUBLE, value))); + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, false); + return cache.findJcrProperty(editor().setProperty(propertyName, valueFrom(PropertyType.DOUBLE, value))); } @@ -1027,7 +1065,9 @@ // If there is an existing property, then remove it ... return removeExistingValuedProperty(name); } - return cache.findJcrProperty(editor().setProperty(nameFrom(name), valueFrom(value))); + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, false); + return cache.findJcrProperty(editor().setProperty(propertyName, valueFrom(value))); } /** @@ -1038,8 +1078,9 @@ public final Property setProperty( String name, long value ) throws ValueFormatException, VersionException, LockException, ConstraintViolationException, RepositoryException { - return cache.findJcrProperty(editor().setProperty(nameFrom(name), valueFrom(PropertyType.LONG, value))); - + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, false); + return cache.findJcrProperty(editor().setProperty(propertyName, valueFrom(PropertyType.LONG, value))); } /** @@ -1054,7 +1095,10 @@ // If there is an existing property, then remove it ... return removeExistingValuedProperty(name); } - return cache.findJcrProperty(editor().setProperty(nameFrom(name), valueFrom(value))); + + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, false); + return cache.findJcrProperty(editor().setProperty(propertyName, valueFrom(value))); } /** @@ -1069,7 +1113,9 @@ // If there is an existing property, then remove it ... return removeExistingValuedProperty(name); } - return cache.findJcrProperty(editor().setProperty(nameFrom(name), valueFrom(PropertyType.STRING, value))); + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, false); + return cache.findJcrProperty(editor().setProperty(propertyName, valueFrom(PropertyType.STRING, value))); } /** @@ -1085,7 +1131,9 @@ // If there is an existing property, then remove it ... return removeExistingValuedProperty(name); } - return cache.findJcrProperty(editor().setProperty(nameFrom(name), valueFrom(type, value))); + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, false); + return cache.findJcrProperty(editor().setProperty(propertyName, valueFrom(type, value))); } /** @@ -1100,7 +1148,9 @@ // If there is an existing property, then remove it ... return removeExistingValuedProperty(name); } - return cache.findJcrProperty(editor().setProperty(nameFrom(name), valuesFrom(PropertyType.STRING, values))); + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, true); + return cache.findJcrProperty(editor().setProperty(propertyName, valuesFrom(PropertyType.STRING, values))); } /** @@ -1116,7 +1166,9 @@ // If there is an existing property, then remove it ... return removeExistingValuedProperty(name); } - return cache.findJcrProperty(editor().setProperty(nameFrom(name), valuesFrom(type, values))); + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, true); + return cache.findJcrProperty(editor().setProperty(propertyName, valuesFrom(type, values))); } /** @@ -1131,7 +1183,9 @@ // If there is an existing property, then remove it ... return removeExistingValuedProperty(name); } - return cache.findJcrProperty(editor().setProperty(nameFrom(name), (JcrValue)value)); + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, false); + return cache.findJcrProperty(editor().setProperty(propertyName, (JcrValue)value)); } protected final Property removeExistingValuedProperty( String name ) @@ -1159,7 +1213,9 @@ // If there is an existing property, then remove it ... return removeExistingValuedProperty(name); } - return cache.findJcrProperty(editor().setProperty(nameFrom(name), ((JcrValue)value).asType(type))); + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, false); + return cache.findJcrProperty(editor().setProperty(propertyName, ((JcrValue)value).asType(type))); } /** @@ -1180,9 +1236,25 @@ newValues = JcrMultiValueProperty.EMPTY_VALUES; } else { List<Value> valuesWithDesiredType = new ArrayList<Value>(len); + int expectedType = -1; for (int i = 0; i != len; ++i) { Value value = values[i]; if (value == null) continue; + if (expectedType == -1) { + expectedType = value.getType(); + } else if (value.getType() != expectedType) { + // Make sure the type of each value is the same, as per Javadoc in section 7.1.5 of the JCR 1.0.1 spec + StringBuilder sb = new StringBuilder(); + sb.append('['); + for (int j = 0; j != values.length; ++j) { + if (j != 0) sb.append(","); + sb.append(values[j].toString()); + } + sb.append(']'); + String propType = PropertyType.nameFromValue(expectedType); + I18n msg = JcrI18n.allPropertyValuesMustHaveSameType; + throw new ValueFormatException(msg.text(name, values, propType, getPath(), cache.workspaceName())); + } valuesWithDesiredType.add(value); } if (valuesWithDesiredType.isEmpty()) { @@ -1191,8 +1263,11 @@ newValues = valuesWithDesiredType.toArray(new Value[valuesWithDesiredType.size()]); } } + + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, true); // Set the value, perhaps to an empty array ... - return cache.findJcrProperty(editor().setProperty(nameFrom(name), newValues)); + return cache.findJcrProperty(editor().setProperty(propertyName, newValues)); } /** @@ -1208,15 +1283,33 @@ // If there is an existing property, then remove it ... return removeExistingValuedProperty(name); } + int len = values.length; Value[] newValues = null; if (len == 0) { newValues = JcrMultiValueProperty.EMPTY_VALUES; } else { List<Value> valuesWithDesiredType = new ArrayList<Value>(len); + int expectedType = -1; for (int i = 0; i != len; ++i) { Value value = values[i]; + if (value == null) continue; + if (expectedType == -1) { + expectedType = value.getType(); + } else if (value.getType() != expectedType) { + // Make sure the type of each value is the same, as per Javadoc in section 7.1.5 of the JCR 1.0.1 spec + StringBuilder sb = new StringBuilder(); + sb.append('['); + for (int j = 0; j != values.length; ++j) { + if (j != 0) sb.append(","); + sb.append(values[j].toString()); + } + sb.append(']'); + String propType = PropertyType.nameFromValue(expectedType); + I18n msg = JcrI18n.allPropertyValuesMustHaveSameType; + throw new ValueFormatException(msg.text(name, values, propType, getPath(), cache.workspaceName())); + } if (value.getType() != type) { value = ((JcrValue)value).asType(type); } @@ -1228,8 +1321,11 @@ newValues = valuesWithDesiredType.toArray(new Value[valuesWithDesiredType.size()]); } } + + Name propertyName = nameFrom(name); + checkCardinalityOfExistingProperty(propertyName, true); // Set the value, perhaps to an empty array ... - return cache.findJcrProperty(editor().setProperty(nameFrom(name), newValues)); + return cache.findJcrProperty(editor().setProperty(propertyName, newValues)); } /** Modified: trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrI18n.java =================================================================== --- trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrI18n.java 2009-04-09 20:07:42 UTC (rev 814) +++ trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrI18n.java 2009-04-13 19:13:43 UTC (rev 815) @@ -80,6 +80,9 @@ public static I18n unableToCreateNodeWithPrimaryTypeThatDoesNotExist; public static I18n unableToCreateNodeWithNoDefaultPrimaryTypeOnChildNodeDefinition; public static I18n unableToSaveNodeThatWasCreatedSincePreviousSave; + public static I18n unableToSetMultiValuedPropertyUsingSingleValue; + public static I18n unableToSetSingleValuedPropertyUsingMultipleValues; + public static I18n allPropertyValuesMustHaveSameType; public static I18n unableToRemoveRootNode; public static I18n unableToMoveNodeToBeChildOfDecendent; Modified: trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrMultiValueProperty.java =================================================================== --- trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrMultiValueProperty.java 2009-04-09 20:07:42 UTC (rev 814) +++ trunk/dna-jcr/src/main/java/org/jboss/dna/jcr/JcrMultiValueProperty.java 2009-04-13 19:13:43 UTC (rev 815) @@ -37,6 +37,7 @@ import javax.jcr.nodetype.ConstraintViolationException; import javax.jcr.version.VersionException; import net.jcip.annotations.NotThreadSafe; +import org.jboss.dna.common.i18n.I18n; import org.jboss.dna.graph.property.Property; import org.jboss.dna.graph.property.ValueFactory; @@ -193,6 +194,7 @@ Value[] newValues = null; if (values.length != 0) { int numValues = values.length; + int valueType = -1; List<Value> valuesList = new ArrayList<Value>(numValues); ValueFactory<?> factory = null; for (int i = 0; i != numValues; ++i) { @@ -200,7 +202,24 @@ if (value == null) { // skip null values ... continue; - } else if (value instanceof JcrValue) { + } + if (valueType == -1) { + valueType = value.getType(); + } else if (value.getType() != valueType) { + // Make sure the type of each value is the same, as per Javadoc in section 7.1.5 of the JCR 1.0.1 spec + StringBuilder sb = new StringBuilder(); + sb.append('['); + for (int j = 0; j != values.length; ++j) { + if (j != 0) sb.append(","); + sb.append(values[j].toString()); + } + sb.append(']'); + String propType = PropertyType.nameFromValue(valueType); + I18n msg = JcrI18n.allPropertyValuesMustHaveSameType; + throw new ValueFormatException(msg.text(getName(), values, propType, getPath(), cache.workspaceName())); + } + + if (value instanceof JcrValue) { // just use the value ... valuesList.add(value); } else { Modified: trunk/dna-jcr/src/main/resources/org/jboss/dna/jcr/JcrI18n.properties =================================================================== --- trunk/dna-jcr/src/main/resources/org/jboss/dna/jcr/JcrI18n.properties 2009-04-09 20:07:42 UTC (rev 814) +++ trunk/dna-jcr/src/main/resources/org/jboss/dna/jcr/JcrI18n.properties 2009-04-13 19:13:43 UTC (rev 815) @@ -70,6 +70,9 @@ unableToCreateNodeWithPrimaryTypeThatDoesNotExist = Unable to create child "{1}" in workspace "{2}" because the node type "{0}" does not exist unableToCreateNodeWithNoDefaultPrimaryTypeOnChildNodeDefinition = Unable to create child "{2}" in workspace "{3}" because the node definition "{0}" on the "{1}" node type has no default primary type unableToSaveNodeThatWasCreatedSincePreviousSave = Unable to save node "{0}" in workspace "{1}" because it was created since the last save +unableToSetMultiValuedPropertyUsingSingleValue = Unable to set existing multi-valued property "{0}" on node "{1}" in workspace "{2}" using single-value setter methods +unableToSetSingleValuedPropertyUsingMultipleValues = Unable to set existing single-valued property "{0}" on node "{1}" in workspace "{2}" using multi-value setter methods +allPropertyValuesMustHaveSameType = All values of property "{0}" on node "{3}" in workspace "{4}" must all be {2} values (values were: {1}) unableToRemoveRootNode = Unable to remove the root node in workspace "{1}" unableToMoveNodeToBeChildOfDecendent = Node "{0}" in workspace "{2}" cannot be moved under a decendant node ("{1}") Modified: trunk/dna-jcr/src/test/java/org/jboss/dna/jcr/JcrTckTest.java =================================================================== --- trunk/dna-jcr/src/test/java/org/jboss/dna/jcr/JcrTckTest.java 2009-04-09 20:07:42 UTC (rev 814) +++ trunk/dna-jcr/src/test/java/org/jboss/dna/jcr/JcrTckTest.java 2009-04-13 19:13:43 UTC (rev 815) @@ -50,6 +50,8 @@ import org.apache.jackrabbit.test.api.SetPropertyDoubleTest; import org.apache.jackrabbit.test.api.SetPropertyInputStreamTest; import org.apache.jackrabbit.test.api.SetPropertyLongTest; +import org.apache.jackrabbit.test.api.SetPropertyStringTest; +import org.apache.jackrabbit.test.api.SetPropertyValueTest; import org.apache.jackrabbit.test.api.SetValueBinaryTest; import org.apache.jackrabbit.test.api.SetValueBooleanTest; import org.apache.jackrabbit.test.api.SetValueConstraintViolationExceptionTest; @@ -183,7 +185,7 @@ // addTestSuite(NodeUUIDTest.class); // addTestSuite(NodeOrderableChildNodesTest.class); addTestSuite(PropertyTest.class); - // + addTestSuite(SetValueBinaryTest.class); addTestSuite(SetValueBooleanTest.class); addTestSuite(SetValueDateTest.class); @@ -201,8 +203,8 @@ addTestSuite(SetPropertyInputStreamTest.class); addTestSuite(SetPropertyLongTest.class); // addTestSuite(SetPropertyNodeTest.class); - // addTestSuite(SetPropertyStringTest.class); - // addTestSuite(SetPropertyValueTest.class); + addTestSuite(SetPropertyStringTest.class); + addTestSuite(SetPropertyValueTest.class); addTestSuite(SetPropertyConstraintViolationExceptionTest.class); // addTestSuite(SetPropertyAssumeTypeTest.class); // @@ -210,11 +212,11 @@ // addTestSuite(NodeItemIsNewTest.class); // addTestSuite(PropertyItemIsModifiedTest.class); // addTestSuite(PropertyItemIsNewTest.class); - // + addTestSuite(NodeAddMixinTest.class); addTestSuite(NodeCanAddMixinTest.class); addTestSuite(NodeRemoveMixinTest.class); - // + addTestSuite(WorkspaceCloneReferenceableTest.class); addTestSuite(WorkspaceCloneSameNameSibsTest.class); addTestSuite(WorkspaceCloneTest.class);