diff --git a/source/java/org/alfresco/repo/domain/node/AbstractNodeDAOImpl.java b/source/java/org/alfresco/repo/domain/node/AbstractNodeDAOImpl.java index 92aae8a5d7..0eb0a335c0 100644 --- a/source/java/org/alfresco/repo/domain/node/AbstractNodeDAOImpl.java +++ b/source/java/org/alfresco/repo/domain/node/AbstractNodeDAOImpl.java @@ -1637,7 +1637,7 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO // Copy for modification Map propsToDelete = new HashMap(oldPropsRaw); Map propsToAdd = new HashMap(newPropsRaw); - + // Compare these fine-grained properties Map persistableDiff = EqualsHelper.getMapComparison( propsToDelete, @@ -1732,12 +1732,18 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO } } - // Remove by key - List propKeysToDeleteList = new ArrayList(propsToDelete.keySet()); - deleteNodeProperties(nodeId, propKeysToDeleteList); - try { + // Remove by key + List propKeysToDeleteList = new ArrayList(propsToDelete.keySet()); + int deleted = deleteNodeProperties(nodeId, propKeysToDeleteList); + if (deleted != propKeysToDeleteList.size()) + { + // The translation of the left-hand map didn't match the database + // This happens when we use d:any and switch from a simple value to a collection: an edge case! foreach + throw new DataIntegrityViolationException("Only deleted " + deleted + "/" + propKeysToDeleteList.size()); + } + // Add by key-value insertNodeProperties(nodeId, propsToAdd); } @@ -1745,7 +1751,18 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO { // Don't trust the properties cache for the node propertiesCache.removeByKey(nodeId); - throw e; + // Focused error + throw new AlfrescoRuntimeException( + "Failed to write property deltas: \n" + + " Node: " + nodeId + "\n" + + " Old: " + oldProps + "\n" + + " New: " + newProps + "\n" + + " Old Raw: " + oldPropsRaw + "\n" + + " New Raw: " + newPropsRaw + "\n" + + " Diff: " + persistableDiff + "\n" + + " Delete Tried: " + propsToDelete.keySet() + "\n" + + " Add Tried: " + propsToAdd, + e); } boolean updated = propsToDelete.size() > 0 || propsToAdd.size() > 0; diff --git a/source/java/org/alfresco/repo/domain/node/NodePropertyHelper.java b/source/java/org/alfresco/repo/domain/node/NodePropertyHelper.java index 5639b45ccc..b1605846f3 100644 --- a/source/java/org/alfresco/repo/domain/node/NodePropertyHelper.java +++ b/source/java/org/alfresco/repo/domain/node/NodePropertyHelper.java @@ -46,6 +46,7 @@ import org.alfresco.service.cmr.repository.ContentData; import org.alfresco.service.cmr.repository.MLText; import org.alfresco.service.cmr.repository.datatype.TypeConversionException; import org.alfresco.service.namespace.QName; +import org.alfresco.util.EqualsHelper; import org.alfresco.util.Pair; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -321,9 +322,11 @@ public class NodePropertyHelper catch (TypeConversionException e) { throw new TypeConversionException( - "The property value is not compatible with the type defined for the property: \n" + " property: " - + (propertyDef == null ? "unknown" : propertyDef) + "\n" + " value: " + value + "\n" - + " value type: " + value.getClass(), e); + "The property value is not compatible with the type defined for the property: \n" + + " property: " + (propertyDef == null ? "unknown" : propertyDef) + "\n" + + " value: " + value + "\n" + + " value type: " + value.getClass(), + e); } } @@ -544,12 +547,21 @@ public class NodePropertyHelper // Nothing to do return value; } + + // Do we definitely have MLText? + boolean isMLText = (propertyDef != null && propertyDef.getDataType().getName().equals(DataTypeDefinition.MLTEXT)); + + // Determine the default locale ID. The chance of it being null is vanishingly small, but ... + Pair defaultLocalePair = localeDAO.getDefaultLocalePair(); + Long defaultLocaleId = (defaultLocalePair == null) ? null : defaultLocalePair.getFirst(); + Integer listIndex = null; for (Map.Entry entry : propertyValues.entrySet()) { NodePropertyKey propertyKey = entry.getKey(); NodePropertyValue propertyValue = entry.getValue(); + // Check that the client code has gathered the values together correctly if (listIndex == null) { listIndex = propertyKey.getListIndex(); @@ -559,31 +571,56 @@ public class NodePropertyHelper throw new IllegalStateException("Expecting to collapse properties with same list index: " + propertyValues); } - if (propertyValuesSize == 1 - && (propertyDef == null || !propertyDef.getDataType().getName().equals(DataTypeDefinition.MLTEXT))) + // Get the locale of the current value + Long localeId = propertyKey.getLocaleId(); + boolean isDefaultLocale = EqualsHelper.nullSafeEquals(defaultLocaleId, localeId); + + // Get the local entry value + Serializable entryValue = makeSerializableValue(propertyDef, propertyValue); + + // A default locale indicates a simple value i.e. the entry represents the whole value. + if (isDefaultLocale) { - // This is the only value and it is NOT to be converted to MLText - value = makeSerializableValue(propertyDef, propertyValue); + // Check and warn if there are other values + if (propertyValuesSize > 1) + { + logger.warn( + "Found localized properties along with a 'null' value in the default locale. \n" + + " The localized values will be ignored; 'null' will be returned: \n" + + " Default locale ID: " + defaultLocaleId + "\n" + + " Property: " + propertyDef + "\n" + + " Values: " + propertyValues); + } + // The entry could be null or whatever value came out + value = entryValue; + break; } else { - // There are multiple values, so add them to MLText - MLText mltext = (value == null) ? new MLText() : (MLText) value; - try + // Non-default locales indicate MLText ONLY. + Locale locale = localeDAO.getLocalePair(localeId).getSecond(); + // Note that we force a non-null value here as a null MLText object is persisted + // just like any other null i.e. with the default locale. + if (value == null) { - String mlString = (String) propertyValue.getValue(DataTypeDefinition.TEXT); - // Get the locale - Long localeId = propertyKey.getLocaleId(); - Locale locale = localeDAO.getLocalePair(localeId).getSecond(); - // Add to the MLText object - mltext.addValue(locale, mlString); - } - catch (TypeConversionException e) + value = new MLText(); + } // We break for other entry values, so no need to check the non-null case + // Put the current value into the MLText object + if (entryValue == null || entryValue instanceof String) { - // Ignore - logger.warn("Unable to add property value to MLText instance: " + propertyValue); + // Can put in nulls and Strings + ((MLText)value).put(locale, (String)entryValue); // We've checked the casts + } + else + { + // It's a non-null non-String ... can't be added to MLText! + logger.warn( + "Found localized non-String properties. \n" + + " The non-String values will be ignored: \n" + + " Default locale ID: " + defaultLocaleId + "\n" + + " Property: " + propertyDef + "\n" + + " Values: " + propertyValues); } - value = mltext; } } // Done diff --git a/source/java/org/alfresco/repo/domain/node/NodePropertyHelperTest.java b/source/java/org/alfresco/repo/domain/node/NodePropertyHelperTest.java new file mode 100644 index 0000000000..240e153960 --- /dev/null +++ b/source/java/org/alfresco/repo/domain/node/NodePropertyHelperTest.java @@ -0,0 +1,289 @@ +/* + * Copyright (C) 2005-2010 Alfresco Software Limited. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + + * As a special exception to the terms and conditions of version 2.0 of + * the GPL, you may redistribute this Program in connection with Free/Libre + * and Open Source Software ("FLOSS") applications as described in Alfresco's + * FLOSS exception. You should have recieved a copy of the text describing + * the FLOSS exception, and it is also available here: + * http://www.alfresco.com/legal/licensing" + */ +package org.alfresco.repo.domain.node; + +import java.io.Serializable; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Locale; +import java.util.Map; + +import junit.framework.TestCase; + +import org.alfresco.model.ContentModel; +import org.alfresco.repo.domain.contentdata.ContentDataDAO; +import org.alfresco.repo.domain.locale.LocaleDAO; +import org.alfresco.repo.domain.qname.QNameDAO; +import org.alfresco.repo.transaction.RetryingTransactionHelper; +import org.alfresco.repo.transaction.RetryingTransactionHelper.RetryingTransactionCallback; +import org.alfresco.repo.version.VersionModel; +import org.alfresco.service.ServiceRegistry; +import org.alfresco.service.cmr.dictionary.DictionaryService; +import org.alfresco.service.cmr.repository.MLText; +import org.alfresco.service.cmr.repository.NodeRef; +import org.alfresco.service.namespace.QName; +import org.alfresco.service.transaction.TransactionService; +import org.alfresco.util.ApplicationContextHelper; +import org.alfresco.util.EqualsHelper; +import org.springframework.context.ApplicationContext; + +/** + * Test low-level marshalling and unmarshalling of node properties + * + * @see NodePropertyHelper + * + * @author Derek Hulley + * @since 3.4 + */ +public class NodePropertyHelperTest extends TestCase +{ + private static final QName QN_BOOLEAN = createQName("boolean"); + private static final QName QN_INTEGER = createQName("integer"); + private static final QName QN_LONG = createQName("long"); + private static final QName QN_FLOAT = createQName("float"); + private static final QName QN_TEXT = createQName("text"); + private static final QName QN_MLTEXT = createQName("mltext"); + private static final QName QN_REF = createQName("ref"); + private static final QName QN_ANY = createQName("any"); + + /** + * @return Returns a QName that uses the localname + */ + private static QName createQName(String localName) + { + return QName.createQName("test", "local-" + localName); + } + + private ApplicationContext ctx = ApplicationContextHelper.getApplicationContext(); + + private NodePropertyHelper helper; + private TransactionService transactionService; + private RetryingTransactionHelper txnHelper; + + @Override + public void setUp() + { + ServiceRegistry serviceRegistry = (ServiceRegistry) ctx.getBean(ServiceRegistry.SERVICE_REGISTRY); + DictionaryService dictionaryService = serviceRegistry.getDictionaryService(); + QNameDAO qnameDAO = (QNameDAO) ctx.getBean("qnameDAO"); + LocaleDAO localeDAO = (LocaleDAO) ctx.getBean("localeDAO"); + ContentDataDAO contentDataDAO = (ContentDataDAO) ctx.getBean("contentDataDAO"); + + helper = new NodePropertyHelper(dictionaryService, qnameDAO, localeDAO, contentDataDAO); + transactionService = serviceRegistry.getTransactionService(); + txnHelper = transactionService.getRetryingTransactionHelper(); + txnHelper.setMinRetryWaitMs(10); + txnHelper.setRetryWaitIncrementMs(10); + txnHelper.setMaxRetryWaitMs(50); + } + + /** + * Converts properties back and forth and ensures that the result is unchanged or + * at least doesn't show up in a deep equals check. + */ + private void marshallAndUnmarshall(final Map in, final boolean exact) throws Throwable + { + RetryingTransactionCallback txnCallback = new RetryingTransactionCallback() + { + public Void execute() throws Throwable + { + String diffReport; + // Convert to raw and back + Map rawProps1 = helper.convertToPersistentProperties(in); + Map props1 = helper.convertToPublicProperties(rawProps1); + // We can't be sure that we have what we started with, because there may have been + // some mandatory conversions to the types defined by the model i.e. the values + // will get converted to the dictionary type rather than the incoming type + if (exact) + { + diffReport = EqualsHelper.getMapDifferenceReport(props1, in); + assertNull(diffReport, diffReport); + } + // Convert back to raw again + Map rawProps2 = helper.convertToPersistentProperties(in); + diffReport = EqualsHelper.getMapDifferenceReport(rawProps2, rawProps1); + assertNull(diffReport, diffReport); + // But now, on the second time out, we expect to get exactly what we got before + Map props2 = helper.convertToPublicProperties(rawProps2); + diffReport = EqualsHelper.getMapDifferenceReport(props2, props1); + assertNull(diffReport, diffReport); + + return null; + } + }; + txnHelper.doInTransaction(txnCallback); + } + + /** + * Tests simple, well-typed nulls + */ + public void testNullKnownValues() throws Throwable + { + Map in = new HashMap(17); + in.put(ContentModel.PROP_AUTO_VERSION, null); // d:boolean + in.put(ContentModel.PROP_HITS, null); // d:int + in.put(ContentModel.PROP_SIZE_CURRENT, null); // d:long + in.put(ContentModel.PROP_RATING_SCORE, null); // d:float + in.put(ContentModel.PROP_NAME, null); // d:text + in.put(ContentModel.PROP_TITLE, null); // d:mltext + in.put(ContentModel.PROP_REFERENCE, null); // d:noderef + in.put(VersionModel.PROP_QNAME_VALUE, null); // d:any + + marshallAndUnmarshall(in, true); + } + + /** + * Tests simple, well-typed values + */ + public void testSimpleKnownValues() throws Throwable + { + Map in = new HashMap(17); + in.put(ContentModel.PROP_AUTO_VERSION, Boolean.TRUE); + in.put(ContentModel.PROP_HITS, new Integer(1)); + in.put(ContentModel.PROP_SIZE_CURRENT, new Long(2L)); + in.put(ContentModel.PROP_RATING_SCORE, new Float(3.0)); + in.put(ContentModel.PROP_NAME, "four"); + in.put(ContentModel.PROP_TITLE, new MLText("five")); + in.put(ContentModel.PROP_REFERENCE, new NodeRef("protocol://identifier/six")); + in.put(VersionModel.PROP_QNAME_VALUE, Locale.CANADA); + + marshallAndUnmarshall(in, true); + } + + /** + * Tests simple, well-typed values that need conversion + */ + public void testConvertableKnownValues() throws Throwable + { + Map in = new HashMap(17); + in.put(ContentModel.PROP_AUTO_VERSION, "TRUE"); + in.put(ContentModel.PROP_HITS, "1"); + in.put(ContentModel.PROP_SIZE_CURRENT, "2"); + in.put(ContentModel.PROP_RATING_SCORE, "3.0"); + in.put(ContentModel.PROP_NAME, new MLText("four")); + in.put(ContentModel.PROP_TITLE, "five"); + in.put(ContentModel.PROP_REFERENCE, "protocol://identifier/six"); + in.put(VersionModel.PROP_QNAME_VALUE, "en_CA_"); + + marshallAndUnmarshall(in, false); + } + + /** + * Tests simple, residual nulls + */ + public void testNullResidualValues() throws Throwable + { + Map in = new HashMap(17); + in.put(QN_TEXT, null); + + marshallAndUnmarshall(in, true); + } + + /** + * Tests simple, residual values + */ + public void testSimpleResidualValues() throws Throwable + { + Map in = new HashMap(17); + in.put(QN_BOOLEAN, Boolean.TRUE); + in.put(QN_INTEGER, new Integer(1)); + in.put(QN_LONG, new Long(2L)); + in.put(QN_FLOAT, new Float(3.0)); + in.put(QN_TEXT, "four"); + in.put(QN_MLTEXT, new MLText("five")); + in.put(QN_REF, new NodeRef("protocol://identifier/six")); + in.put(QN_ANY, Locale.CANADA); + + marshallAndUnmarshall(in, true); + } + + /** + * Tests simple multi-value type + */ + public void testSimpleMultiValue() throws Throwable + { + Map in = new HashMap(17); + + in.put(ContentModel.PROP_ADDRESSEES, null); + marshallAndUnmarshall(in, true); + + in.put(ContentModel.PROP_ADDRESSEES, (Serializable) Arrays.asList()); + marshallAndUnmarshall(in, true); + + in.put(ContentModel.PROP_ADDRESSEES, (Serializable) Arrays.asList("A")); + marshallAndUnmarshall(in, true); + + in.put(ContentModel.PROP_ADDRESSEES, (Serializable) Arrays.asList("A", "B")); + marshallAndUnmarshall(in, true); + } + + /** + * Tests d:any multi-value type + */ + public void testAnyMultiValue() throws Throwable + { + Map in = new HashMap(17); + + in.put(VersionModel.PROP_QNAME_VALUE, null); + marshallAndUnmarshall(in, true); + + in.put(VersionModel.PROP_QNAME_VALUE, (Serializable) Arrays.asList()); + marshallAndUnmarshall(in, true); + + in.put(VersionModel.PROP_QNAME_VALUE, (Serializable) Arrays.asList("A")); + marshallAndUnmarshall(in, true); + + in.put(VersionModel.PROP_QNAME_VALUE, (Serializable) Arrays.asList("A", "B")); + marshallAndUnmarshall(in, true); + } + + /** + * Tests residual multi-value type + */ + public void testResidualMultiValue() throws Throwable + { + Map in = new HashMap(17); + + in.put(QN_ANY, null); + marshallAndUnmarshall(in, true); + + in.put(QN_ANY, (Serializable) Arrays.asList()); + marshallAndUnmarshall(in, true); + + in.put(QN_ANY, (Serializable) Arrays.asList("A")); + marshallAndUnmarshall(in, true); + + in.put(QN_ANY, (Serializable) Arrays.asList("A", "B")); + marshallAndUnmarshall(in, true); + + // Collection of collections + ArrayList arrayListVal = new ArrayList(2); + HashSet hashSetVal = new HashSet(2); + in.put(QN_ANY, (Serializable) Arrays.asList(arrayListVal, hashSetVal)); + marshallAndUnmarshall(in, true); + } +} \ No newline at end of file diff --git a/source/java/org/alfresco/repo/domain/node/NodePropertyValue.java b/source/java/org/alfresco/repo/domain/node/NodePropertyValue.java index 8bf1e2c68c..a76d978d80 100644 --- a/source/java/org/alfresco/repo/domain/node/NodePropertyValue.java +++ b/source/java/org/alfresco/repo/domain/node/NodePropertyValue.java @@ -568,7 +568,11 @@ public class NodePropertyValue implements Cloneable, Serializable { return ValueType.BOOLEAN; } - else if ((value instanceof Integer) || (value instanceof Long)) + else if (value instanceof Integer) + { + return ValueType.INTEGER; + } + else if (value instanceof Long) { return ValueType.LONG; } diff --git a/source/java/org/alfresco/repo/node/BaseNodeServiceTest.java b/source/java/org/alfresco/repo/node/BaseNodeServiceTest.java index 5e078f793f..233059ce26 100644 --- a/source/java/org/alfresco/repo/node/BaseNodeServiceTest.java +++ b/source/java/org/alfresco/repo/node/BaseNodeServiceTest.java @@ -1909,7 +1909,7 @@ public abstract class BaseNodeServiceTest extends BaseSpringTest QName.createQName("pathA"), TYPE_QNAME_TEST_MANY_PROPERTIES).getChildRef(); - for (int inc = 0; inc < 6; inc++) + for (int inc = 0; inc < 3; inc++) { System.out.println("----------------------------------------------"); int collectionSize = (int) Math.pow(10, inc); diff --git a/source/java/org/alfresco/repo/node/FullNodeServiceTest.java b/source/java/org/alfresco/repo/node/FullNodeServiceTest.java index af45bb1fb7..9d63b9e96c 100644 --- a/source/java/org/alfresco/repo/node/FullNodeServiceTest.java +++ b/source/java/org/alfresco/repo/node/FullNodeServiceTest.java @@ -130,18 +130,24 @@ public class FullNodeServiceTest extends BaseNodeServiceTest // Now create an MLText object with a null entry mlText = new MLText(null); - nodeService.setProperty(rootNodeRef, BaseNodeServiceTest.PROP_QNAME_ML_TEXT_VALUE, null); + nodeService.setProperty(rootNodeRef, BaseNodeServiceTest.PROP_QNAME_ML_TEXT_VALUE, mlText); MLText mlTextCheck = (MLText) nodeService.getProperty(rootNodeRef, BaseNodeServiceTest.PROP_QNAME_ML_TEXT_VALUE); assertNull("MLText value should have been converted to a null String", mlTextCheck); + // Set an ML value to null + nodeService.setProperty(rootNodeRef, BaseNodeServiceTest.PROP_QNAME_ML_TEXT_VALUE, null); + // Do the same as ML-aware MLPropertyInterceptor.setMLAware(true); try { mlText = new MLText(null); - nodeService.setProperty(rootNodeRef, BaseNodeServiceTest.PROP_QNAME_ML_TEXT_VALUE, null); + nodeService.setProperty(rootNodeRef, BaseNodeServiceTest.PROP_QNAME_ML_TEXT_VALUE, mlText); mlTextCheck = (MLText) nodeService.getProperty(rootNodeRef, BaseNodeServiceTest.PROP_QNAME_ML_TEXT_VALUE); assertEquals("MLText value was not pulled out the same as it went in", mlText, mlTextCheck); + + // Set an ML value to null + nodeService.setProperty(rootNodeRef, BaseNodeServiceTest.PROP_QNAME_ML_TEXT_VALUE, null); } finally {