Node cache changes (step): properties and aspects are retrieved with the node's version

- Perform basic node version check when reading properties and aspects from DB
 - This is just the start.  Next step is to change the key of the cache itself.
 - Includes fix for rev 31109: Use setNodeAclId() to set ACL IDs


git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@31121 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

config/alfresco/ibatis/org.hibernate.dialect.Dialect/node-common-SqlMap.xml

@@ -82,6 +82,7 @@
     <resultMap id="result_NodeAspects" type="NodeAspects" groupBy="nodeId"> -->
     <resultMap id="result_NodeAspects" type="NodeAspects">
         <id property="nodeId" column="node_id" jdbcType="BIGINT" javaType="java.lang.Long"/>
+        <id property="nodeVersion" column="node_version" jdbcType="BIGINT" javaType="java.lang.Long"/>
         <association property="aspectQNameIds" resultMap="alfresco.node.result_AspectQNameIds"/>
 <!--
         <result property="aspectQNameIds" column="qname_id" jdbcType="BIGINT" javaType="java.lang.Long"/>
@@ -89,6 +90,7 @@
     </resultMap>
     <resultMap id="result_NodeProperty" type="NodeProperty">
         <result property="nodeId" column="node_id" jdbcType="BIGINT" javaType="java.lang.Long"/>
+        <result property="nodeVersion" column="node_version" jdbcType="BIGINT" javaType="java.lang.Long"/>
         <result property="key.qnameId" column="qname_id" jdbcType="BIGINT" javaType="java.lang.Long"/>
         <result property="key.localeId" column="locale_id" jdbcType="BIGINT" javaType="java.lang.Long"/>
         <result property="key.listIndex" column="list_index" jdbcType="INTEGER" javaType="java.lang.Integer"/>
@@ -720,7 +722,8 @@
     
     <sql id="select_NodeProperty_Results">
         select
-            prop.node_id            as node_id,
+            node.id                 as node_id,
+            node.version            as node_version,
             prop.qname_id           as qname_id,
             prop.locale_id          as locale_id,
             prop.list_index         as list_index,
@@ -737,7 +740,8 @@
     <select id="select_NodeProperties" parameterType="NodeProperty" resultMap="result_NodeProperty">
         <include refid="alfresco.node.select_NodeProperty_Results"/>
         from
-            alf_node_properties prop
+            alf_node node
+            join alf_node_properties prop on (prop.node_id = node.id)
         <where>
             <if test="nodeId != null">prop.node_id = #{nodeId}</if>
             <if test="nodeIds != null">
@@ -759,7 +763,8 @@
     <select id="select_PropertiesByTypes" parameterType="Ids" resultMap="result_NodeProperty">
         <include refid="alfresco.node.select_NodeProperty_Results"/>
         from
-            alf_node_properties prop
+            alf_node node
+            join alf_node_properties prop on (prop.node_id = node.id)
         <where>
             qname_id in
             <foreach item="item" index="index" collection="ids" open="(" separator="," close=")">
@@ -770,14 +775,16 @@
 
     <sql id="select_NodeAspects_Results">
         select
-            aspects.node_id            as node_id,
+            node.id                    as node_id,
+            node.version               as node_version,
             aspects.qname_id           as qname_id
     </sql>
     
     <select id="select_NodeAspects" parameterType="Ids" resultMap="result_NodeAspects">
         <include refid="alfresco.node.select_NodeAspects_Results"/>
         from
-            alf_node_aspects aspects
+            alf_node node
+            join alf_node_aspects aspects on (aspects.node_id = node.id)
         <where>
             <if test="nodeId != null">aspects.node_id = #{nodeId}</if>
             <if test="nodeIds != null">
@@ -786,15 +793,6 @@
                     #{item}
                 </foreach>
             </if>
-<!--
-            <if test="key != null and key.qnameId">and qname_id = #{key.qnameId}</if>
-            <if test="qnameIds">
-                and qname_id in
-                <foreach item="item" index="index" collection="qnameIds" open="(" separator="," close=")">
-                    #{item}
-                </foreach>
-            </if>
--->
         </where>
     </select>
     

source/java/org/alfresco/repo/domain/node/AbstractNodeDAOImpl.java

@@ -562,6 +562,9 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
         }
     }
     
+    /**
+     * @return          Returns a new transaction or an existing one if already active
+     */
     private TransactionEntity getCurrentTransaction()
     {
         TransactionEntity txn = AlfrescoTransactionSupport.getResource(KEY_TRANSACTION);
@@ -1216,6 +1219,9 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             }
         }
         
+        // Need the child node's name here in case it gets removed
+        final String childNodeName = (String) getNodeProperty(childNodeId, ContentModel.PROP_NAME);
+        
         // First attempt to move the node, which may rollback to a savepoint
         Node newChildNode = childNode;
         // Store
@@ -1281,11 +1287,7 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
                 // Because we are retrying in-transaction i.e. absorbing exceptions, we need a Savepoint
                 Savepoint savepoint = controlDAO.createSavepoint("DuplicateChildNodeNameException");
                 // We use the child node's UUID if there is no cm:name
-                String childNodeName = (String) getNodeProperty(childNodeId, ContentModel.PROP_NAME);
+                String childNodeNameToUse = childNodeName == null ? childNode.getUuid() : childNodeName;
-                if (childNodeName == null)
-                {
-                    childNodeName = childNode.getUuid();
-                }
 
                 try
                 {
@@ -1294,7 +1296,7 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
                             newParentNodeId,
                             assocTypeQName,
                             assocQName,
-                            childNodeName);
+                            childNodeNameToUse);
                     controlDAO.releaseSavepoint(savepoint);
                     return updated;
                 }
@@ -1565,7 +1567,7 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             Long optionalOldSharedAlcIdInAdditionToNull,
             Long newSharedAclId)
     {
-        Long txnId = getCurrentTransactionId();
+        Long txnId = getCurrentTransaction().getId();
         updatePrimaryChildrenSharedAclId(
                 txnId,
                 primaryParentNodeId,
@@ -2101,6 +2103,7 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             throw new DataIntegrityViolationException("Invalid node ID: " + nodeId);
         }
         Map<QName, Serializable> cachedProperties = cacheEntry.getSecond();
+        // Need to return a harmlessly mutable map
         Map<QName, Serializable> properties = copyPropertiesAgainstModification(cachedProperties);
         // Done
         return properties;
@@ -2153,7 +2156,27 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
 
         public Pair<Long, Map<QName, Serializable>> findByKey(Long nodeId)
         {
-            Map<NodePropertyKey, NodePropertyValue> propsRaw = selectNodeProperties(nodeId);
+            NodeVersionKey nodeVersionKey = getNodeNotNull(nodeId).getNodeVersionKey();
+            Map<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> propsRawByNodeVersionKey = selectNodeProperties(nodeId);
+            // Check the node Txn ID for mismatch
+            Map<NodePropertyKey, NodePropertyValue> propsRaw = propsRawByNodeVersionKey.get(nodeVersionKey);
+            if (propsRaw == null)
+            {
+                // Didn't find a match.  Is this because there are none?
+                if (propsRawByNodeVersionKey.size() == 0)
+                {
+                    // This is OK.  The node has no properties
+                    propsRaw = Collections.emptyMap();
+                }
+                else
+                {
+                    // We found properties associated with a different node ID and txn
+                    invalidateNodeCaches(nodeId);
+                    throw new DataIntegrityViolationException(
+                            "Detected stale node entry: " + nodeVersionKey +
+                            " (now " + propsRawByNodeVersionKey.keySet() + ")");
+                }
+            }
             // Convert to public properties
             Map<QName, Serializable> props = nodePropertyHelper.convertToPublicProperties(propsRaw);
             // Done
@@ -2351,9 +2374,27 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
 
         public Pair<Long, Set<QName>> findByKey(Long nodeId)
         {
-            Set<Long> nodeAspectQNameIds = selectNodeAspectIds(nodeId);
+            NodeVersionKey nodeVersionKey = getNodeNotNull(nodeId).getNodeVersionKey();
-            // Convert to QNames
+            Set<Long> nodeIds = Collections.singleton(nodeId);
-            Set<QName> nodeAspectQNames = qnameDAO.convertIdsToQNames(nodeAspectQNameIds);
+            Map<NodeVersionKey, Set<QName>> nodeAspectQNameIdsByVersionKey = selectNodeAspects(nodeIds);
+            Set<QName> nodeAspectQNames = nodeAspectQNameIdsByVersionKey.get(nodeVersionKey);
+            if (nodeAspectQNames == null)
+            {
+                // Didn't find a match.  Is this because there are none?
+                if (nodeAspectQNameIdsByVersionKey.size() == 0)
+                {
+                    // This is OK.  The node has no properties
+                    nodeAspectQNames = Collections.emptySet();
+                }
+                else
+                {
+                    // We found properties associated with a different node ID and txn
+                    invalidateNodeCaches(nodeId);
+                    throw new DataIntegrityViolationException(
+                            "Detected stale node entry: " + nodeVersionKey +
+                            " (now " + nodeAspectQNameIdsByVersionKey.keySet() + ")");
+                }
+            }
             // Done
             return new Pair<Long, Set<QName>>(nodeId, Collections.unmodifiableSet(nodeAspectQNames));
         }
@@ -3619,9 +3660,11 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
         // Get the nodes
         SortedSet<Long> aspectNodeIds = new TreeSet<Long>();
         SortedSet<Long> propertiesNodeIds = new TreeSet<Long>();
+        Map<Long, NodeVersionKey> nodeVersionKeysFromCache = new HashMap<Long, NodeVersionKey>(nodes.size()*2);    // Keep for quick lookup
         for (Node node : nodes)
         {
             Long nodeId = node.getId();
+            NodeVersionKey nodeVersionKey = node.getNodeVersionKey();
             nodesCache.setValue(nodeId, node);
             if (propertiesCache.getValue(nodeId) == null)
             {
@@ -3631,6 +3674,7 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             {
                 aspectNodeIds.add(nodeId);
             }
+            nodeVersionKeysFromCache.put(nodeId, nodeVersionKey);
         }
         
         if(logger.isDebugEnabled())
@@ -3639,14 +3683,13 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             logger.debug("Pre-loaded " + propertiesNodeIds.size() + " aspects");
         }
         
-        List<NodeAspectsEntity> nodeAspects = selectNodeAspects(aspectNodeIds);
+        Map<NodeVersionKey, Set<QName>> nodeAspects = selectNodeAspects(aspectNodeIds);
-        for (NodeAspectsEntity nodeAspect : nodeAspects)
+        for (Map.Entry<NodeVersionKey, Set<QName>> entry : nodeAspects.entrySet())
         {
-            Long nodeId = nodeAspect.getNodeId();
+            NodeVersionKey nodeVersionKeyFromDb = entry.getKey();
-            List<Long> qnameIds = nodeAspect.getAspectQNameIds();
+            Long nodeId = nodeVersionKeyFromDb.getNodeId();
-            HashSet<Long> qnameIdsSet = new HashSet<Long>(qnameIds);
+            Set<QName> qnames = entry.getValue();
-            Set<QName> qnames = qnameDAO.convertIdsToQNames(qnameIdsSet);
+            setNodeAspectsCached(nodeId, qnames);
-            aspectsCache.setValue(nodeId, qnames);
             aspectNodeIds.remove(nodeId);
         }
         // Cache the absence of aspects too!
@@ -3655,13 +3698,13 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             aspectsCache.setValue(nodeId, Collections.<QName>emptySet());            
         }
 
-        Map<Long, Map<NodePropertyKey, NodePropertyValue>> propsByNodeId = selectNodeProperties(propertiesNodeIds);
+        Map<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> propsByNodeId = selectNodeProperties(propertiesNodeIds);
-        for (Map.Entry<Long, Map<NodePropertyKey, NodePropertyValue>> entry : propsByNodeId.entrySet())
+        for (Map.Entry<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> entry : propsByNodeId.entrySet())
         {
-            Long nodeId = entry.getKey();
+            Long nodeId = entry.getKey().getNodeId();
             Map<NodePropertyKey, NodePropertyValue> propertyValues = entry.getValue();
             Map<QName, Serializable> props = nodePropertyHelper.convertToPublicProperties(propertyValues);
-            propertiesCache.setValue(nodeId, props);
+            setNodePropertiesCached(nodeId, props);
         }
     }
 
@@ -3847,14 +3890,13 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
     protected abstract NodeEntity selectNodeByNodeRef(NodeRef nodeRef, Boolean deleted);
     protected abstract List<Node> selectNodesByUuids(Long storeId, SortedSet<String> uuids);
     protected abstract List<Node> selectNodesByIds(SortedSet<Long> ids);
-    protected abstract Map<Long, Map<NodePropertyKey, NodePropertyValue>> selectNodeProperties(Set<Long> nodeIds);
+    protected abstract Map<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> selectNodeProperties(Set<Long> nodeIds);
-    protected abstract List<NodeAspectsEntity> selectNodeAspects(Set<Long> nodeIds);
+    protected abstract Map<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> selectNodeProperties(Long nodeId);
-    protected abstract Map<NodePropertyKey, NodePropertyValue> selectNodeProperties(Long nodeId);
+    protected abstract Map<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> selectNodeProperties(Long nodeId, Set<Long> qnameIds);
-    protected abstract Map<NodePropertyKey, NodePropertyValue> selectNodeProperties(Long nodeId, Set<Long> qnameIds);
     protected abstract int deleteNodeProperties(Long nodeId, Set<Long> qnameIds);
     protected abstract int deleteNodeProperties(Long nodeId, List<NodePropertyKey> propKeys);
     protected abstract void insertNodeProperties(Long nodeId, Map<NodePropertyKey, NodePropertyValue> persistableProps);
-    protected abstract Set<Long> selectNodeAspectIds(Long nodeId);
+    protected abstract Map<NodeVersionKey, Set<QName>> selectNodeAspects(Set<Long> nodeIds);
     protected abstract void insertNodeAspect(Long nodeId, Long qnameId);
     protected abstract int deleteNodeAspects(Long nodeId, Set<Long> qnameIds);
     protected abstract void selectNodesWithAspects(

source/java/org/alfresco/repo/domain/node/Node.java

@@ -28,6 +28,11 @@ import org.alfresco.util.Pair;
  */
 public interface Node extends NodeIdAndAclId
 {
+    /**
+     * Helper method to get a key that includes the node and its current version number
+     */
+    public NodeVersionKey getNodeVersionKey();
+    
     /**
      * Helper method to force the instance to be read-only
      */

source/java/org/alfresco/repo/domain/node/NodeAspectsEntity.java

@@ -29,6 +29,7 @@ import java.util.List;
 public class NodeAspectsEntity
 {
     private Long nodeId;
+	private Long nodeVersion;
     private List<Long> aspectQNameIds;
 
     /** Carries data for queries */
@@ -47,6 +48,7 @@ public class NodeAspectsEntity
         StringBuilder sb = new StringBuilder(512);
         sb.append("NodeAspectsEntity")
           .append(", nodeId=").append(nodeId)
+          .append(", nodeVersion=").append(nodeVersion)
           .append(", aspects=").append(aspectQNameIds)
           .append("]");
         return sb.toString();
@@ -62,6 +64,16 @@ public class NodeAspectsEntity
         this.nodeId = nodeId;
     }
     
+    public Long getNodeVersion()
+    {
+        return nodeVersion;
+    }
+
+    public void setNodeVersion(Long nodeVersion)
+    {
+        this.nodeVersion = nodeVersion;
+    }
+
     public List<Long> getAspectQNameIds()
     {
         return aspectQNameIds;

source/java/org/alfresco/repo/domain/node/NodeEntity.java

@@ -105,6 +105,17 @@ public class NodeEntity implements Node, PermissionCheckValue
         return sb.toString();
     }
     
+    @Override
+    // TODO: Must cache the key
+    public NodeVersionKey getNodeVersionKey()
+    {
+        if (id == null || version == null)
+        {
+            throw new IllegalStateException("The NodeEntity has not be filled: " + this);
+        }
+        return new NodeVersionKey(id, version);
+    }
+    
     /**
      * Lock the entity against further updates to prevent accidental modification
      */
@@ -125,8 +136,9 @@ public class NodeEntity implements Node, PermissionCheckValue
         }
     }
     
-    public void incrementVersion()
+    public synchronized void incrementVersion()
     {
+        checkLock();
         if (version >= Short.MAX_VALUE)
         {
             this.version = 0L;
@@ -162,7 +174,7 @@ public class NodeEntity implements Node, PermissionCheckValue
         return id;
     }
 
-    public void setId(Long id)
+    public synchronized void setId(Long id)
     {
         checkLock();
         this.id = id;
@@ -174,7 +186,7 @@ public class NodeEntity implements Node, PermissionCheckValue
         return version;
     }
 
-    public void setVersion(Long version)
+    public synchronized void setVersion(Long version)
     {
         checkLock();
         this.version = version;
@@ -186,7 +198,7 @@ public class NodeEntity implements Node, PermissionCheckValue
         return store;
     }
 
-    public void setStore(StoreEntity store)
+    public synchronized void setStore(StoreEntity store)
     {
         checkLock();
         this.store = store;
@@ -198,7 +210,7 @@ public class NodeEntity implements Node, PermissionCheckValue
         return uuid;
     }
 
-    public void setUuid(String uuid)
+    public synchronized void setUuid(String uuid)
     {
         checkLock();
         this.uuid = uuid;
@@ -210,7 +222,7 @@ public class NodeEntity implements Node, PermissionCheckValue
         return typeQNameId;
     }
 
-    public void setTypeQNameId(Long typeQNameId)
+    public synchronized void setTypeQNameId(Long typeQNameId)
     {
         checkLock();
         this.typeQNameId = typeQNameId;
@@ -222,7 +234,7 @@ public class NodeEntity implements Node, PermissionCheckValue
         return localeId;
     }
 
-    public void setLocaleId(Long localeId)
+    public synchronized void setLocaleId(Long localeId)
     {
         this.localeId = localeId;
     }
@@ -233,7 +245,7 @@ public class NodeEntity implements Node, PermissionCheckValue
         return aclId;
     }
 
-    public void setAclId(Long aclId)
+    public synchronized void setAclId(Long aclId)
     {
         checkLock();
         this.aclId = aclId;
@@ -245,7 +257,7 @@ public class NodeEntity implements Node, PermissionCheckValue
         return deleted;
     }
 
-    public void setDeleted(Boolean deleted)
+    public synchronized void setDeleted(Boolean deleted)
     {
         checkLock();
         this.deleted = deleted;
@@ -257,7 +269,7 @@ public class NodeEntity implements Node, PermissionCheckValue
         return transaction;
     }
 
-    public void setTransaction(TransactionEntity transaction)
+    public synchronized void setTransaction(TransactionEntity transaction)
     {
         checkLock();
         this.transaction = transaction;
@@ -269,7 +281,7 @@ public class NodeEntity implements Node, PermissionCheckValue
         return auditableProperties;
     }
 
-    public void setAuditableProperties(AuditablePropertiesEntity auditableProperties)
+    public synchronized void setAuditableProperties(AuditablePropertiesEntity auditableProperties)
     {
         checkLock();
         this.auditableProperties = auditableProperties;

source/java/org/alfresco/repo/domain/node/NodePropertyEntity.java

@@ -29,6 +29,7 @@ import java.util.List;
 public class NodePropertyEntity
 {
     private Long nodeId;
+    private Long nodeVersion;
     private NodePropertyKey key;
     private NodePropertyValue value;
     /** Carries data for queries and updates */
@@ -46,7 +47,7 @@ public class NodePropertyEntity
     @Override
     public String toString()
     {
-        return "NodePropertyEntity [node=" + nodeId + ", key=" + key + ", value=" + value + "]";
+        return "NodePropertyEntity [node=" + nodeId + ", nodeVersion=" + nodeVersion + ", key=" + key + ", value=" + value + "]";
     }
 
     public Long getNodeId()
@@ -59,6 +60,16 @@ public class NodePropertyEntity
         this.nodeId = nodeId;
     }
 
+    public Long getNodeVersion()
+    {
+        return nodeVersion;
+    }
+
+    public void setNodeVersion(Long nodeVersion)
+    {
+        this.nodeVersion = nodeVersion;
+    }
+
     public NodePropertyKey getKey()
     {
         return key;

source/java/org/alfresco/repo/domain/node/NodeVersionKey.java

@@ -0,0 +1,89 @@
+/*
+ * 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;
+
+/**
+ * Key for caches that need to be bound implicitly to the current version of a node.
+ * 
+ * @author Derek Hulley
+ * @since 4.0
+ */
+public class NodeVersionKey implements Serializable
+{
+    private static final long serialVersionUID = 2241045540959490539L;
+    
+    private final Long nodeId;
+    private final Long version;
+
+    public NodeVersionKey(Long nodeId, Long version)
+    {
+        this.nodeId = nodeId;
+        this.version = version;
+    }
+
+    @Override
+    public boolean equals(Object other)
+    {
+        if (this == other)
+        {
+            return true;
+        }
+        if (!(other instanceof NodeVersionKey))
+        {
+            return false;
+        }
+        NodeVersionKey o = (NodeVersionKey)other;
+        return nodeId.equals(o.nodeId) && version.equals(o.version);
+    }
+    
+    @Override
+    public int hashCode()
+    {
+        return nodeId.hashCode() + version.hashCode();
+    }
+    
+    @Override
+    public String toString()
+    {
+        StringBuilder builder = new StringBuilder();
+        builder.append("NodeVersionKey ")
+               .append("[nodeId=").append(nodeId)
+               .append(", version=").append(version)
+               .append("]");
+        return builder.toString();
+    }
+
+    public Long getNodeId()
+    {
+        return nodeId;
+    }
+
+    public Long getVersion()
+    {
+        return version;
+    }
+}

source/java/org/alfresco/repo/domain/node/ibatis/NodeDAOImpl.java

@@ -42,6 +42,7 @@ import org.alfresco.repo.domain.node.NodePropertyEntity;
 import org.alfresco.repo.domain.node.NodePropertyKey;
 import org.alfresco.repo.domain.node.NodePropertyValue;
 import org.alfresco.repo.domain.node.NodeUpdateEntity;
+import org.alfresco.repo.domain.node.NodeVersionKey;
 import org.alfresco.repo.domain.node.PrimaryChildrenAclUpdateEntity;
 import org.alfresco.repo.domain.node.ServerEntity;
 import org.alfresco.repo.domain.node.StoreEntity;
@@ -418,21 +419,23 @@ public class NodeDAOImpl extends AbstractNodeDAOImpl
     /**
      * Pull out the key-value pairs from the rows
      */
-    private Map<Long, Map<NodePropertyKey, NodePropertyValue>> makePersistentPropertiesMap(List<NodePropertyEntity> rows)
+    private Map<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> makePersistentPropertiesMap(List<NodePropertyEntity> rows)
     {
-        Map<Long, Map<NodePropertyKey, NodePropertyValue>> results = new HashMap<Long, Map<NodePropertyKey, NodePropertyValue>>(3);
+        Map<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> results = new HashMap<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>>(3);
         for (NodePropertyEntity row : rows)
         {
             Long nodeId = row.getNodeId();
-            if (nodeId == null)
+            Long nodeVersion = row.getNodeVersion();
+            if (nodeId == null || nodeVersion == null)
             {
-                throw new RuntimeException("Expect results with a Node ID: " + row);
+                throw new RuntimeException("Expect results with a Node and Version: " + row);
             }
-            Map<NodePropertyKey, NodePropertyValue> props = results.get(nodeId);
+            NodeVersionKey nodeTxnKey = new NodeVersionKey(nodeId, nodeVersion);
+            Map<NodePropertyKey, NodePropertyValue> props = results.get(nodeTxnKey);
             if (props == null)
             {
                 props = new HashMap<NodePropertyKey, NodePropertyValue>(17);
-                results.put(nodeId, props);
+                results.put(nodeTxnKey, props);
             }
             props.put(row.getKey(), row.getValue());
         }
@@ -460,22 +463,7 @@ public class NodeDAOImpl extends AbstractNodeDAOImpl
     
     @Override
     @SuppressWarnings("unchecked")
-    protected List<NodeAspectsEntity> selectNodeAspects(Set<Long> nodeIds)
+    protected Map<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> selectNodeProperties(Set<Long> nodeIds)
-    {
-        if (nodeIds.size() == 0)
-        {
-            return Collections.emptyList();
-        }
-        NodeAspectsEntity aspects = new NodeAspectsEntity();
-        aspects.setNodeIds(new ArrayList<Long>(nodeIds));
-
-        List<NodeAspectsEntity> rows = (List<NodeAspectsEntity>) template.selectList(SELECT_NODE_ASPECTS, aspects);
-        return rows;
-    }
-    
-    @Override
-    @SuppressWarnings("unchecked")
-    protected Map<Long, Map<NodePropertyKey, NodePropertyValue>> selectNodeProperties(Set<Long> nodeIds)
     {
         if (nodeIds.size() == 0)
         {
@@ -488,13 +476,13 @@ public class NodeDAOImpl extends AbstractNodeDAOImpl
         return makePersistentPropertiesMap(rows);
     }
     @Override
-    protected Map<NodePropertyKey, NodePropertyValue> selectNodeProperties(Long nodeId)
+    protected Map<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> selectNodeProperties(Long nodeId)
     {
         return selectNodeProperties(nodeId, Collections.<Long>emptySet());
     }
     @Override
     @SuppressWarnings("unchecked")
-    protected Map<NodePropertyKey, NodePropertyValue> selectNodeProperties(Long nodeId, Set<Long> qnameIds)
+    protected Map<NodeVersionKey, Map<NodePropertyKey, NodePropertyValue>> selectNodeProperties(Long nodeId, Set<Long> qnameIds)
     {
         NodePropertyEntity prop = new NodePropertyEntity();
         // Node
@@ -514,16 +502,7 @@ public class NodeDAOImpl extends AbstractNodeDAOImpl
         }
 
         List<NodePropertyEntity> rows = (List<NodePropertyEntity>) template.selectList(SELECT_NODE_PROPERTIES, prop);
-        Map<Long, Map<NodePropertyKey, NodePropertyValue>> results = makePersistentPropertiesMap(rows);
+        return makePersistentPropertiesMap(rows);
-        Map<NodePropertyKey, NodePropertyValue> props = results.get(nodeId);
-        if (props == null)
-        {
-            return Collections.emptyMap();
-        }
-        else
-        {
-            return props;
-        }
     }
 
     @Override
@@ -601,19 +580,34 @@ public class NodeDAOImpl extends AbstractNodeDAOImpl
         }
     }
 
+    @SuppressWarnings("unchecked")
     @Override
-    protected Set<Long> selectNodeAspectIds(Long nodeId)
+    protected Map<NodeVersionKey, Set<QName>> selectNodeAspects(Set<Long> nodeIds)
     {
-    	Set<Long> aspectIds = new HashSet<Long>();
+        if (nodeIds.size() == 0)
-    	Set<Long> nodeIds = new HashSet<Long>();
-    	nodeIds.add(nodeId);
-    	List<NodeAspectsEntity> nodeAspectEntities = selectNodeAspects(nodeIds);
-    	if(nodeAspectEntities.size() > 0)
         {
-    		NodeAspectsEntity nodeAspects = nodeAspectEntities.get(0);
+            return Collections.emptyMap();
-    		aspectIds.addAll(nodeAspects.getAspectQNameIds());
         }
-		return aspectIds;
+        NodeAspectsEntity aspects = new NodeAspectsEntity();
+        aspects.setNodeIds(new ArrayList<Long>(nodeIds));
+
+        List<NodeAspectsEntity> rows = (List<NodeAspectsEntity>) template.selectList(SELECT_NODE_ASPECTS, aspects);
+        
+        Map<NodeVersionKey, Set<QName>> results = new HashMap<NodeVersionKey, Set<QName>>(rows.size()*2);
+        for (NodeAspectsEntity nodeAspectsEntity : rows)
+        {
+            Long nodeId = nodeAspectsEntity.getNodeId();
+            Long nodeVersion = nodeAspectsEntity.getNodeVersion();
+            NodeVersionKey nodeVersionKey = new NodeVersionKey(nodeId, nodeVersion);
+            if (results.containsKey(nodeVersionKey))
+            {
+                throw new IllegalStateException("Found existing key while querying for node aspects: " + nodeIds);
+            }
+            Set<Long> aspectIds = new HashSet<Long>(nodeAspectsEntity.getAspectQNameIds());
+            Set<QName> aspectQNames = qnameDAO.convertIdsToQNames(aspectIds);
+            results.put(nodeVersionKey, aspectQNames);
+        }
+        return results;
     }
 
     @Override