MODE-1856 Attempt to index changes only when there are real changes

The internal session nodes that track transient changes were not
correctly recording how a property was removed. If a property were
added and immediately removed (all before saving), the add was
cleared from the transient state but the remove was still kept.
(This was due to the fact that it's more difficult to track removes,
since we're using a single structure to track adds and sets, so
it's not really easy to tell when a property was added and then
removed.) This change works around that constraint by using the
persisted state of the node to determine if any added/set property
already exists in the persisted state. If not, then the remove
is considered to undo the prior add/set.

A single test was added to perform the add/remove prior to a save.
However, due to the eventing mechanism, it's not possible for the
test to verify that the node was not re-indexed. (Simple debugging
and trace logging was used to verify that such changes no longer
cause an update to the node's indexes. If there are better ideas
for how the test can verify, please say so.)

modeshape-jcr/src/main/java/org/modeshape/jcr/cache/document/SessionNode.java

@@ -892,17 +892,34 @@ public void removeProperty( SessionCache cache,
                                 Name name ) {
         writableSession(cache).assertInSession(this);
         changedProperties.remove(name);
-        if (!isNew) removedProperties.put(name, name);
+        if (!isNew) {
+            // Determine if the existing node already contained this property ...
+            AbstractSessionCache session = session(cache);
+            CachedNode raw = nodeInWorkspace(session);
+            if (raw.hasProperty(name, cache)) {
+                removedProperties.put(name, name);
+            }
+        }
         updateReferences(cache, name, null);
     }
 
     @Override
     public void removeAllProperties( SessionCache cache ) {
         writableSession(cache).assertInSession(this);
+        CachedNode raw = null;
         for (Iterator<Property> propertyIterator = getProperties(cache); propertyIterator.hasNext();) {
             Name name = propertyIterator.next().getName();
             changedProperties.remove(name);
-            if (!isNew) removedProperties.put(name, name);
+            if (!isNew) {
+                // Determine if the existing node already contained this property ...
+                if (raw == null) {
+                    AbstractSessionCache session = session(cache);
+                    raw = nodeInWorkspace(session);
+                }
+                if (raw.hasProperty(name, cache)) {
+                    removedProperties.put(name, name);
+                }
+            }
             updateReferences(cache, name, null);
         }
     }

modeshape-jcr/src/main/java/org/modeshape/jcr/cache/document/WritableSessionCache.java

@@ -1073,7 +1073,8 @@ protected ChangeSet persistChanges( Iterable<NodeKey> changedNodesInOrder,
                     boolean isExternal = !workspaceCache().getRootKey()
                                                           .getSourceKey()
                                                           .equalsIgnoreCase(node.getKey().getSourceKey());
-                    boolean externalNodeChanged = isExternal && node.hasChanges();
+                    boolean hasChanges = node.hasChanges();
+                    boolean externalNodeChanged = isExternal && hasChanges;
                     if (externalNodeChanged) {
                         // in the case of external nodes, only if there are changes should the update be called
                         documentStore.updateDocument(keyStr, doc, node);
@@ -1086,7 +1087,7 @@ protected ChangeSet persistChanges( Iterable<NodeKey> changedNodesInOrder,
                     // when linking/un-linking nodes (e.g. shareable node or jcr:system) this condition will be false.
                     // the downside of this is that there may be cases (e.g. back references when working with versions) in which
                     // we might loose information from the indexes
-                    if (monitor != null && queryable && (isSameWorkspace || externalNodeChanged)) {
+                    if (monitor != null && queryable && ((isSameWorkspace && hasChanges) || externalNodeChanged)) {
                         // Get the primary and mixin type names; even though we're passing in the session, the two properties
                         // should be there and shouldn't require a looking in the cache...
                         Name primaryType = node.getPrimaryType(this);

modeshape-jcr/src/test/java/org/modeshape/jcr/JcrSessionTest.java

@@ -63,13 +63,16 @@
 import javax.jcr.nodetype.NodeType;
 import javax.jcr.nodetype.NodeTypeManager;
 import javax.jcr.nodetype.NodeTypeTemplate;
+import javax.jcr.observation.EventIterator;
+import javax.jcr.observation.EventListener;
 import org.junit.Test;
 import org.mockito.Mockito;
 import org.modeshape.common.FixFor;
 import org.modeshape.common.statistic.Stopwatch;
 import org.modeshape.jcr.api.AnonymousCredentials;
 import org.modeshape.jcr.api.JcrTools;
 import org.modeshape.jcr.api.Namespaced;
+import org.modeshape.jcr.api.observation.Event;
 import org.modeshape.jcr.value.Path;
 
 public class JcrSessionTest extends SingleUseAbstractTest {
@@ -1062,6 +1065,57 @@ public void shouldGetLocalNameAndNamespaceUriFromNodeAndPropertyObjects() throws
         assertLocalNameAndNamespace(contentNode, "content", "jcr");
     }
 
+    @FixFor( "MODE-1856" )
+    @Test
+    public void shouldNotIndexNoOpChanges() throws Exception {
+        // Add a node under which we'll do our work ...
+        Node node1 = session.getRootNode().addNode("node1");
+        session.save();
+
+        // Register the listener
+        PropertyListener listener = new PropertyListener();
+        session.getWorkspace()
+               .getObservationManager()
+               .addEventListener(listener, Event.PROPERTY_ADDED | Event.PROPERTY_CHANGED | Event.PROPERTY_REMOVED, null, // node1.getPath(),
+                                 true,
+                                 null,
+                                 null,
+                                 false);
+
+        // Now, add a property and remove the property, then save ...
+        node1.setProperty("unchanged", "new value");
+        node1.getProperty("unchanged").remove();
+        session.save();
+
+        Thread.sleep(500L);
+        assertThat(listener.adds, is(0));
+        assertThat(listener.removes, is(0));
+        assertThat(listener.changes, is(0));
+    }
+
+    protected static class PropertyListener implements EventListener {
+        protected int adds, removes, changes;
+
+        @Override
+        public void onEvent( EventIterator events ) {
+            System.out.println("CALLED");
+            while (events.hasNext()) {
+                javax.jcr.observation.Event event = events.nextEvent();
+                switch (event.getType()) {
+                    case Event.PROPERTY_ADDED:
+                        ++adds;
+                        break;
+                    case Event.PROPERTY_CHANGED:
+                        ++changes;
+                        break;
+                    case Event.PROPERTY_REMOVED:
+                        ++removes;
+                        break;
+                }
+            }
+        }
+    }
+
     protected void assertLocalNameAndNamespace( Item item,
                                                 String expectedLocalName,
                                                 String namespacePrefix ) throws RepositoryException {