497040: fix overzealous recording of task list xml orphans

Fixes an issue where all task list elements after the first encountered
orphans would be recorded as orphans. This lead to duplication of task
list elements on read/write round trips.

Change-Id: I7176eb4fda0ede6459ec1d7cf2dd0c78adec1c98
Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=497040

2 files changed, 137 insertions, 1 deletions

diff --git a/org.eclipse.mylyn.tasks.core.tests/src/org/eclipse/mylyn/internal/tasks/core/externalization/SaxOrphanBuilderTest.java b/org.eclipse.mylyn.tasks.core.tests/src/org/eclipse/mylyn/internal/tasks/core/externalization/SaxOrphanBuilderTest.java
new file mode 100644
index 000000000..9c140474b
--- /dev/null
+++ b/org.eclipse.mylyn.tasks.core.tests/src/org/eclipse/mylyn/internal/tasks/core/externalization/SaxOrphanBuilderTest.java
@@ -0,0 +1,131 @@
+/*******************************************************************************
+ * Copyright (c) 2016 Tasktop Technologies and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Tasktop Technologies - initial API and implementation
+ *******************************************************************************/
+
+package org.eclipse.mylyn.internal.tasks.core.externalization;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.w3c.dom.Document;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
+import org.w3c.dom.Text;
+import org.xml.sax.helpers.AttributesImpl;
+
+public class SaxOrphanBuilderTest {
+
+	private SaxOrphanBuilder builder;
+
+	@Before
+	public void setUp() throws Exception {
+		builder = new SaxOrphanBuilder();
+	}
+
+	@Test
+	public void parseWithNoCommit() {
+		prepareTask("123");
+		builder.endElement();
+
+		prepareTask("456");
+		builder.endElement();
+
+		assertOrphans(0);
+	}
+
+	@Test
+	public void parseWithCommit() {
+		prepareTask("123");
+		builder.endElement();
+
+		prepareTask("456");
+		builder.commitOrphan();
+		builder.endElement();
+
+		assertOrphans(1, "456");
+
+	}
+
+	@Test
+	public void parseWithMiddleCommit() {
+		prepareTask("123");
+		builder.endElement();
+
+		prepareTask("345");
+		builder.commitOrphan();
+		builder.endElement();
+
+		prepareTask("567");
+		builder.endElement();
+
+		assertOrphans(1, "345");
+	}
+
+	@Test
+	public void parseWithMultipleCommit() {
+		prepareTask("123");
+		builder.endElement();
+
+		prepareTask("o1");
+		builder.commitOrphan();
+		builder.endElement();
+
+		prepareTask("o2");
+		builder.commitOrphan();
+		builder.endElement();
+
+		prepareTask("o3");
+		builder.commitOrphan();
+		builder.endElement();
+
+		prepareTask("567");
+		builder.endElement();
+
+		prepareTask("o4");
+		builder.commitOrphan();
+		builder.endElement();
+
+		prepareTask("o5");
+		builder.commitOrphan();
+		builder.endElement();
+
+		assertOrphans(5, "o1", "o2", "o3", "o4", "o5");
+	}
+
+	private void prepareTask(String attributeValue) {
+		start("task");
+		start("attribute");
+		text(attributeValue);
+		builder.endElement();
+	}
+
+	private void start(String name) {
+		builder.startElement(name, new AttributesImpl());
+	}
+
+	private void text(String text) {
+		builder.acceptCharacters(text.toCharArray(), 0, text.length());
+	}
+
+	private void assertOrphans(int numOrphans, String... attributeValue) {
+		Document document = builder.getOrphans();
+		assertEquals(1, document.getChildNodes().getLength());
+		NodeList orphans = document.getDocumentElement().getChildNodes();
+		assertEquals(numOrphans, orphans.getLength());
+		for (int i = 0; i < attributeValue.length; i++) {
+			Node text = orphans.item(i).getFirstChild().getFirstChild();
+			assertTrue(text instanceof Text);
+			assertEquals(attributeValue[i], ((Text) text).getTextContent());
+		}
+	}
+
+}
diff --git a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/externalization/SaxOrphanBuilder.java b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/externalization/SaxOrphanBuilder.java
index 5a9228f31..13dac579d 100644
--- a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/externalization/SaxOrphanBuilder.java
+++ b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/internal/tasks/core/externalization/SaxOrphanBuilder.java
@@ -33,8 +33,11 @@ public class SaxOrphanBuilder {
 
 	private StringBuilder currentStringContent;
 
+	private final Element orphansElement;
+
 	public SaxOrphanBuilder() throws CoreException {
 		this.document = createDocument();
+		this.orphansElement = document.getDocumentElement();
 	}
 
 	private Document createDocument() throws CoreException {
@@ -79,11 +82,13 @@ public class SaxOrphanBuilder {
 		if (currentElement != null) {
 			appendTextNode();
 			Node parentNode = currentElement.getParentNode();
-			if (parentNode instanceof Element) {
+			// do not set the current element to be the orphans element or all tasks will end up as orphans
+			if (parentNode instanceof Element && parentNode != orphansElement) {
 				currentElement = (Element) parentNode;
 				currentStringContent = new StringBuilder();
 			} else {
 				currentElement = null;
+				currentStringContent = null;
 			}
 		}
 	}