Bug 30878 - [PATCH] org.apache.xindice.core.xupdate.XUpdateImpl
Summary: [PATCH] org.apache.xindice.core.xupdate.XUpdateImpl
Status: CLOSED FIXED
Alias: None
Product: Xindice
Classification: Unclassified
Component: XML:DB API (show other bugs)
Version: cvs head (1.1)
Hardware: All All
: P3 blocker
Target Milestone: ---
Assignee: Xindice Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-08-26 22:20 UTC by Todd Byrne
Modified: 2005-05-25 19:19 UTC (History)
0 users



Attachments
Possible patch (12.65 KB, patch)
2004-08-26 22:22 UTC, Todd Byrne
Details | Diff
New cleaned up patch. (2.54 KB, patch)
2004-08-30 16:25 UTC, Todd Byrne
Details | Diff
Contains to xml data files and a xupdate file (360 bytes, application/octet-stream)
2004-08-30 16:41 UTC, Todd Byrne
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Todd Byrne 2004-08-26 22:20:29 UTC
Patch that fixes various issues with xupdate commands. 

Has a temp hack to get rid of "temporaryXUpdateTree" nodes. Going to look into a
better solution. 

Fixed xupdate commands being ran more then once on a document when a set of
commands was applied on a collection. 
Patch below....  Side note willing to write up some test cases to prove this
works and to supply xindice with some basic xupdate testing..

--- xml-xindice/java/src/org/apache/xindice/core/xupdate/XUpdateImpl.java
2004-02-07 19:50:54.000000000 -0700
+++
/home/byrne/jbproject/xindice-cvs-src/xml-xindice/java/src/org/apache/xindice/core/xupdate/XUpdateImpl.java
2004-08-26 13:50:53.000000000 -0600
@@ -27,6 +27,7 @@
 
 import org.w3c.dom.Document;
 import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
 import org.xml.sax.SAXException;
 import org.xmldb.xupdate.lexus.XUpdateQueryImpl;
 import org.xmldb.xupdate.lexus.commands.CommandConstants;
@@ -35,6 +36,7 @@
 
 import java.util.Enumeration;
 import java.util.Hashtable;
+import org.apache.xindice.core.data.Key;
 
 /**
  * Provides Collection and document based XUpdate capabilities.
@@ -44,148 +46,194 @@
  *
  * @version CVS $Revision: 1.12 $, $Date: 2004/02/08 02:50:54 $
  */
-public class XUpdateImpl extends XUpdateQueryImpl {
+public class XUpdateImpl
+    extends XUpdateQueryImpl {
 
-    protected int nodesModified = 0;
-    protected NamespaceMap nsMap;
+  protected int nodesModified = 0;
+  protected NamespaceMap nsMap;
 
-    /**
-     * If set to true, then namespaces set explicitly via an API call will take
precendence.
-     * If set to false, then namespaces set implicitly within query string will
take precedence.
-     */
-    private static final boolean API_NS_PRECEDENCE = true;
-
-    /**
-     * Set the namespace map to be used when resolving queries
-     */
-    public void setNamespaceMap(NamespaceMap nsMap) {
-        if (nsMap == null) {
-            return;
-        }
-
-        if (this.nsMap == null) {
-            this.nsMap = nsMap;
-        } else {
-            this.nsMap.includeNamespaces(nsMap, API_NS_PRECEDENCE);
-        }
+  /**
+   * If set to true, then namespaces set explicitly via an API call will take
precendence.
+   * If set to false, then namespaces set implicitly within query string will
take precedence.
+   */
+  private static final boolean API_NS_PRECEDENCE = true;
+
+  /**
+   * Set the namespace map to be used when resolving queries
+   */
+  public void setNamespaceMap(NamespaceMap nsMap) {
+    if (nsMap == null) {
+      return;
     }
 
-
-    /**
-     * Sets the query string to be used when executing update
-     */
-    public void setQString(String query) throws SAXException {
-        super.setQString(query);
-        if (nsMap == null) {
-            nsMap = new NamespaceMap();
-        }
-        nsMap.includeNamespaces(super.namespaces, !API_NS_PRECEDENCE);
+    if (this.nsMap == null) {
+      this.nsMap = nsMap;
     }
+    else {
+      this.nsMap.includeNamespaces(nsMap, API_NS_PRECEDENCE);
+    }
+  }
 
+  /**
+   * Sets the query string to be used when executing update
+   */
+  public void setQString(String query) throws SAXException {
+    super.setQString(query);
+    if (nsMap == null) {
+      nsMap = new NamespaceMap();
+    }
+    nsMap.includeNamespaces(super.namespaces, !API_NS_PRECEDENCE);
+  }
 
-    /**
-     * Execute the XUpdate commands against a document.
-     */
-    public void execute(Node contextNode) throws Exception {
-        CommandObject currentCommand = new DefaultCommand(contextNode);
-        Enumeration commands = super.query[0].elements();
-        Enumeration attributes = super.query[1].elements();
-        Enumeration characters = super.query[2].elements();
-        Node origNode = contextNode;
-        CommandObject.getXPath().setNamespace(nsMap.getContextNode());
-
-        while (commands.hasMoreElements()) {
-            int id = ((Integer) commands.nextElement()).intValue();
-
-            if (id == CommandConstants.ATTRIBUTES) {
-                currentCommand.submitAttributes((Hashtable)
attributes.nextElement());
-            } else if (id == CommandConstants.CHARACTERS) {
-                currentCommand.submitCharacters((String) characters.nextElement());
-            } else if (id > 0) {
-                if (!currentCommand.submitInstruction(id)) {
-                    super.commandConstants.setContextNode(contextNode);
-                    currentCommand = super.commandConstants.commandForID(id);
-                    if (currentCommand == null) {
-                        throw new Exception("Operation can not have any
XUpdate-instruction!");
-                    }
-                    currentCommand.reset();
-                }
-            } else {
-                if (!currentCommand.executeInstruction()) {
-                    try {
-                        contextNode = currentCommand.execute();
-                    } catch (Exception e) {
-                        // While not ideal, CommandObject.execute throws
-                        // Exception("no nodes selected !") if nothing is
-                        // selected for modification we trap that case
-                        // and ignore allowing continued processing
-                        // of remaining xupdate instructions that may be present
-                        if (!"no nodes selected !".equals(e.getMessage())) {
-                            throw e;
-                        }
-                    }
-                    // Default do-nothing command will soak up anything
-                    // (characters, attributes, etc.) encountered until we
-                    // come across the next xupdate instruction
-                    // (e.g. remove, append, insert, etc.)
-                    currentCommand = new DefaultCommand(contextNode);
-                }
+  /**
+   * Execute the XUpdate commands against a document.
+   */
+  public void execute(Node contextNode) throws Exception {
+    CommandObject currentCommand = new DefaultCommand(contextNode);
+    Enumeration commands = super.query[0].elements();
+    Enumeration attributes = super.query[1].elements();
+    Enumeration characters = super.query[2].elements();
+    Node origNode = contextNode;
+    CommandObject.getXPath().setNamespace(nsMap.getContextNode());
+    boolean contextNodeNeedsCleaning = false;
+
+    while (commands.hasMoreElements()) {
+      int id = ( (Integer) commands.nextElement()).intValue();
+
+      if (id == CommandConstants.ATTRIBUTES) {
+        currentCommand.submitAttributes( (Hashtable) attributes.nextElement());
+      }
+      else if (id == CommandConstants.CHARACTERS) {
+        currentCommand.submitCharacters( (String) characters.nextElement());
+      }
+      else if (id > 0) {
+        if (!currentCommand.submitInstruction(id)) {
+          super.commandConstants.setContextNode(contextNode);
+          currentCommand = super.commandConstants.commandForID(id);
+          if (currentCommand == null) {
+            throw new Exception(
+                "Operation can not have any XUpdate-instruction!");
+          }
+          currentCommand.reset();
+        }
+      }
+      else {
+        if (!currentCommand.executeInstruction()) {
+          try {
+            contextNode = currentCommand.execute();
+          }
+          catch (Exception e) {
+            // While not ideal, CommandObject.execute throws
+            // Exception("no nodes selected !") if nothing is
+            // selected for modification we trap that case
+            // and ignore allowing continued processing
+            // of remaining xupdate instructions that may be present
+            if (!"no nodes selected !".equals(e.getMessage())) {
+              throw e;
             }
-        }
-
-        if (origNode instanceof CompressedNode) {
-            CompressedNode cn = (CompressedNode) origNode;
-            if (cn.isDirty()) {
-                nodesModified++;
+            // from my tests I figure out it adds one temporary node per
+            // command, but when the exception gets thrown it never cleans it
self up
+            // this one node is always at the end
+            String lastNodeName = origNode.getLastChild().getNodeName();
+            if (lastNodeName.equals("temporaryXUpdateTree")) {
+              // assuming that one command one temporary node.
+              origNode.removeChild(origNode.getLastChild());
+            }
+            else { // search for it?
+              contextNodeNeedsCleaning = true;
             }
+          }
+          // Default do-nothing command will soak up anything
+          // (characters, attributes, etc.) encountered until we
+          // come across the next xupdate instruction
+          // (e.g. remove, append, insert, etc.)
+          currentCommand = new DefaultCommand(contextNode);
         }
+      }
     }
+    if (contextNodeNeedsCleaning) {
+      // search entire tree and to verify no extra nodes where left around.
+      cleanUpNode(origNode);
+    }
+    if (origNode instanceof CompressedNode) {
+      CompressedNode cn = (CompressedNode) origNode;
+      if (cn.isDirty()) {
+        nodesModified++;
+      }
+    }
+  }
+
+  /**
+   * Execute the set of XUpdate commands against a collection.
+   *
+   * @param col The collection against which the command will be executed
+   * @exception Exception Description of Exception
+   */
+  public void execute(Collection col) throws Exception {
+
+    int attribIndex = 0;
+    int temp = 0;
+    Hashtable docsUpdated = new Hashtable();
+
+    for (int i = 0; i < super.query[0].size(); i++) {
+      int cmdID = ( (Integer)super.query[0].elementAt(i)).intValue();
+
+      if (cmdID == CommandConstants.ATTRIBUTES) {
+        Hashtable attribs = (Hashtable)super.query[1].elementAt(attribIndex);
+        attribIndex++;
+        String selector = (String) attribs.get("select");
+
+        // If we found an XPath selector we need to execute the commands.
+        // save to skip variable because at this point we have already run the
commands on the
+        // files.
+        if (selector != null && !selector.startsWith("$")) {
+          NodeSet ns = col.queryCollection("XPath", selector, nsMap);
+
+          while (ns != null && ns.hasMoreNodes()) {
+            DBNode node = (DBNode) ns.getNextNode();
+            Document doc = node.getOwnerDocument();
+            NodeSource source = node.getSource();
+            Node contextNode = doc.getDocumentElement();
 
-    /**
-     * Execute the set of XUpdate commands against a collection.
-     *
-     * @param col The collection against which the command will be executed
-     * @exception Exception Description of Exception
-     */
-    public void execute(Collection col) throws Exception {
-
-        int attribIndex = 0;
-        for (int i = 0; i < super.query[0].size(); i++) {
-            int cmdID = ((Integer) super.query[0].elementAt(i)).intValue();
-
-            if (cmdID == CommandConstants.ATTRIBUTES) {
-                Hashtable attribs = (Hashtable)
super.query[1].elementAt(attribIndex);
-                attribIndex++;
-                String selector = (String) attribs.get("select");
-
-                // If we found an XPath selector we need to execute the commands.
-                if (selector != null) {
-                    NodeSet ns = col.queryCollection("XPath", selector, nsMap);
-                    Document lastDoc = null;
-                    while (ns != null && ns.hasMoreNodes()) {
-                        DBNode node = (DBNode) ns.getNextNode();
-                        Document doc = node.getOwnerDocument();
-
-                        if (doc == lastDoc) {
-                            continue; // We only have to process it once
-                        } else {
-                            lastDoc = doc;
-                        }
-
-                        NodeSource source = node.getSource();
-
-                        Node contextNode = doc.getDocumentElement();
-                        execute(contextNode);
-
-                        col.setDocument(source.getKey(), doc);
-                    }
-                }
+            if (docsUpdated.containsKey(source.getKey())) {
+              continue; // We only have to process it once
             }
+            else {
+              docsUpdated.put(source.getKey(), doc);
+            }
+            execute(contextNode);
+          }
         }
+      }
+    }
+    Enumeration keys = docsUpdated.keys();
+    while (keys.hasMoreElements()) {
+      Key docID = (Key) keys.nextElement();
+      col.setDocument(docID, (Document) docsUpdated.get(docID));
     }
+  }
+
+  public int getModifiedCount() {
+    return nodesModified;
+  }
+/**
+ * Search the document tree for nodes left around from the xupdate commands.
+ *
+ */
 
-    public int getModifiedCount() {
-        return nodesModified;
+  private void cleanUpNode(Node n) {
+    NodeList list = n.getChildNodes();
+    int listLength = list.getLength();
+
+    for (int i = listLength - 1; i >= 0; i--) {
+      Node currentNode = list.item(i);
+      if (currentNode.getNodeName().equals("temporaryXUpdateTree")) {
+        n.removeChild(currentNode);
+      }
+      else {
+        cleanUpNode(currentNode);
+      }
     }
-}
 
+  }
+}
\ No newline at end of file
Comment 1 Todd Byrne 2004-08-26 22:22:04 UTC
Created attachment 12540 [details]
Possible patch
Comment 2 Vadim Gritsenko 2004-08-29 00:50:36 UTC
Todd,

I'm having hard time to understand what you patch here. Please keep original
formatting of the file when submitting patches.

Having said that, I see that you try to clean up something when xupdate fails.
Why don't fix it in xupdate implementation instead?

Vadim
Comment 3 Todd Byrne 2004-08-30 16:25:33 UTC
Created attachment 12568 [details]
New cleaned up patch.
Comment 4 Todd Byrne 2004-08-30 16:31:01 UTC
My new patch removes the document tree clean up as I have found the problem to
lie  in the XUpdate code. I will work on trying to get the xupdate guys to fix
or accept some patchs to fix the problem. 

The orginal idea of the execute(Collection col) code was to do a pre xpath query
to prune down the possible documents to run the update commands. The problem
occurs when their is more then one update command. This causes the xupdate
commands to be run multiple times on the same file. The solution to this problem
it to check to see what documents we have run the commands on and only run the
xupdate commands once per document. 

Comment 5 Todd Byrne 2004-08-30 16:41:17 UTC
Created attachment 12569 [details]
Contains to xml data files and a xupdate file
Comment 6 Todd Byrne 2004-08-30 16:46:15 UTC
Unpack bugfiles.tgz in a dir then run the following commands:

 $XINDICE_HOME/bin/xindice dc  -c xmldb:xindice://localhost:8080/db/ -n test
 $XINDICE_HOME/bin/xindice ac  -c xmldb:xindice://localhost:8080/db/ -n test
 $XINDICE_HOME/bin/xindice ad  -c xmldb:xindice://localhost:8080/db/test -n
doc1.xml -f doc1.xml
 $XINDICE_HOME/bin/xindice ad  -c xmldb:xindice://localhost:8080/db/test -n
doc2.xml -f doc2.xml
 $XINDICE_HOME/bin/xindice xupdate  -c xmldb:xindice://localhost:8080/db/test -f
update.xml


First thing that pops up is the 3 documents updated.

Then if you take a look at doc2.xml you see:
<?xml version="1.0" encoding="UTF-8"?>
<ROOT>
  <HEAD>
    <test2/>
    <test2/>
  </HEAD>
  <HEAD1>
    <test1/>
    <test1/>
  </HEAD1>
</ROOT>

Where both append commands got run twice.
Comment 7 Vadim Gritsenko 2005-05-26 03:18:23 UTC
applied patch:
http://marc.theaimsgroup.com/?l=xindice-dev&m=111695214418879