incubator-odf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Svante Schubert (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ODFTOOLKIT-302) getNamespaceURI() implementation in ODFDOM is not aware of duplicate namespace prefixes
Date Mon, 06 Feb 2012 22:30:59 GMT

    [ https://issues.apache.org/jira/browse/ODFTOOLKIT-302?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13201698#comment-13201698
] 

Svante Schubert commented on ODFTOOLKIT-302:
--------------------------------------------

Hi guys,

I have been travelling end of last week, therefore I was just able to review the patch in
detail today as I needed some time to work myself into the context of the XPath implementation
 (especially as JDK7 did a 'fix' in this area, which caused error messages/confusion -  more
details below).

Ashok was completely right about the problem his fix was quite good, still I fear he added
a small side effect.
As JavaDoc of the fixed/changed method, the method should return an empty iterator for prefixes,
if the namespaceURI was not found in the XML file. 
Frankly, spoken this was working before, but the new line
   mDuplicatePrefixesByUri.put(namespaceURI, prefixes);
alerted me to change the map (state) during a get-method call.

Therefore I would have reopened the issue and added a new patch, but this is not an available
option in JIRA to me - either I am missing rights or JIRA expects a different handling, therefore
some notes inline:

Changed in OdfFileDom.java:
@@ -509,9 +509,11 @@
 		Set<String> prefixes = mDuplicatePrefixesByUri.get(namespaceURI);
 		if (prefixes == null) {
 			prefixes = new HashSet<String>();
-			mDuplicatePrefixesByUri.put(namespaceURI, prefixes);
 		}
-		prefixes.add(mPrefixByUri.get(namespaceURI));
+		String givenPrefix = mPrefixByUri.get(namespaceURI);
+		if(givenPrefix != null){
+			prefixes.add(givenPrefix);
+		}
 		return prefixes.iterator();

I have added a test for the empty iterator and fixed a JavaDoc issue with of a renamed test
file:
Changed in XPathTest.java:
@@ -45,7 +45,7 @@
         private static final String SOURCE_FILE_3 = "XPathTest-duplicate-prefix.odt";
 	/**
-	 * 1) The first test document "slideDeckWithTwoSlides.odp" uses the prefix "daisy" instead
of "office" for ODF XML elements.
+	 * 1) The first test document "XPathTest-foreignPrefix.odp" uses the prefix "daisy" instead
of "office" for ODF XML elements.
 	   <daisy:document-content xmlns:daisy="urn:oasis:names:tc:opendocument:xmlns:office:1.0"
xmlns:style="ur...
 			<daisy:scripts/>
 			<daisy:automatic-styles>
@@ -131,6 +131,10 @@
 			String alienElementValue = (String) xpath.evaluate("//text:p/test", node, XPathConstants.STRING);
 			LOG.log(Level.INFO, "The value of the alien element is {0}, expected is ''good''!", alienElementValue);
 			Assert.assertEquals("good", alienElementValue);
+			
+			// Test if an empty iterator is being returned for a none existing URL
+			Iterator<String> prefixes3 = contentDom.getPrefixes("urn://this-prefix-does-not-exist-in-the-xml");
+			Assert.assertFalse("Not used prefix returned a none-empty iterator!", prefixes3.hasNext());
 		} catch (Exception e) {

Finally, JDK7 changed the TreeSet implementation, see http://www.oracle.com/technetwork/java/javase/jdk7-relnotes-418459.html
"Area: API: Utilities
Synopsis: Due to an error in java.util.TreeMap, it was previously possible to insert invalid
null elements and elements not implementing Comparable into empty TreeMaps and TreeSets. Only
a single invalid element could be inserted into the empty TreeMaps or TreeSets; additional
elements would cause the expected NullPointerException or ClassCastException. Most other operations
upon the collection would also fail. As of JDK 7, inserting an invalid null element or an
element not implementing Comparable into an empty TreeMap or TreeSet throws a NullPointerException.
RFE: 5045147 - http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5045147

>From my current view, we have used with OdfStyleFamily a TreeSet, which implements a Comparator
that does allow NULL, so I can not follow their argumentation, as Set in general do allow
NULL, but I will not pick a battle here and decided to do a work around for OdfStyleFamily
as it happened in a private constructor:
private OdfStyleFamily(String name, OdfStyleProperty[] props), so I changed the constructor
and added a second one.

In addition, I  followed advises of Netbeans 7.1 (e.g. added brackets, etc.).
Changed in OdfStyleFamily.java:
@@ -26,31 +26,13 @@
  */
 package org.odftoolkit.odfdom.dom.style;
 
-import org.odftoolkit.odfdom.dom.attribute.style.StyleFamilyAttribute;
+import java.util.*;
+import org.odftoolkit.odfdom.dom.element.style.*;
 import org.odftoolkit.odfdom.dom.style.props.OdfStyleProperty;
-import java.util.Set;
-import java.util.TreeSet;
-import java.util.Map;
-import java.util.HashMap;
-import org.odftoolkit.odfdom.dom.element.style.StyleChartPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleDrawingPagePropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleGraphicPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleHeaderFooterPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleListLevelPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StylePageLayoutPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleParagraphPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleRubyPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleSectionPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleTableCellPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleTableColumnPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleTablePropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleTableRowPropertiesElement;
-import org.odftoolkit.odfdom.dom.element.style.StyleTextPropertiesElement;
 
 public class OdfStyleFamily implements Comparable<OdfStyleFamily> {
 
 	private String m_name;
-//    private Class m_styleClass;
 	private Set<OdfStyleProperty> m_properties = new TreeSet<OdfStyleProperty>();
 	private static Map<String, OdfStyleFamily> m_familyByName = new HashMap<String,
OdfStyleFamily>();
 
@@ -58,12 +40,14 @@
 		return m_familyByName.get(name);
 	}
 
-	private OdfStyleFamily(String name /*, Class styleClass*/, OdfStyleProperty[] props) {
+	private OdfStyleFamily(String name, OdfStyleProperty[] props) {
 		m_name = name;
-//        m_styleClass = styleClass;
-		for (OdfStyleProperty prop : props) {
-			m_properties.add(prop);
+		m_properties.addAll(Arrays.asList(props));
+		m_familyByName.put(name, this);
 		}
+
+	private OdfStyleFamily(String name) {
+		m_name = name;
 		m_familyByName.put(name, this);
 	}
 
@@ -77,18 +61,20 @@
 
 	public static OdfStyleFamily valueOf(String name) {
 		OdfStyleFamily family = getByName(name);
-		if (family == null)
-			family = new OdfStyleFamily(name,  new OdfStyleProperty[]{ null });
-
+		if (family == null) {
+			family = new OdfStyleFamily(name);
+		} 
 		return family;
 	}
 
 	public static String toString(OdfStyleFamily family) {
-		if (family != null)
+		if (family != null) {
 			return family.toString();
-		else
+		}
+		else {
 			return new String();
 	}
+	}
 
 	@Override
 	public String toString() {
@@ -96,7 +82,7 @@
 	}
 
 	public Set<OdfStyleProperty> getProperties() {
-		return m_properties;
+		return Collections.unmodifiableSet(m_properties);
 	}
 
 	public final static OdfStyleFamily Chart = new OdfStyleFamily("chart",

Works afterwards with JDK6 and JDK7 (after the previous pom.xml fix from the list had been
applied).
My question is now, where do I add the patch for the changes? Do we have to clone this issue
first?

Thanks,
Svante
                
> getNamespaceURI() implementation in ODFDOM is not aware of duplicate namespace prefixes
> ---------------------------------------------------------------------------------------
>
>                 Key: ODFTOOLKIT-302
>                 URL: https://issues.apache.org/jira/browse/ODFTOOLKIT-302
>             Project: ODF Toolkit
>          Issue Type: Bug
>          Components: odfdom, unittest
>    Affects Versions: 0.8.8
>         Environment: Ubuntu 11.04 64 bit, Intel i5, OpenJDK 1.6.0_20
>            Reporter: Ashok Hariharan
>            Assignee: Devin Han
>              Labels: java, odf, patch, test, test-patch
>             Fix For: 0.8.8
>
>         Attachments: XPathTest-duplicate-prefix.odt, odffiledom-patch.txt, xpathtest-case-patch.txt
>
>   Original Estimate: 2h
>  Remaining Estimate: 2h
>
> The getNamespaceURI() implementation in OdfFileDom is not aware of duplicate namespace
prefixes. It is aware only of the default namespace prefixes, despite there being support
for duplicate namespace prefixes in the implementation of OdfFileDom.
> I am submitting a patched test case "xpathtest-case-patch.txt" and the corresponding
odt file used by the test.
> To simulate the problem --
> 1) apply the patch xpathtest-case-patch.txt on the ODFDOM source
> 2) put the XPathTest-duplicate-prefix.odt in odfdom/src/test/resources
> 3) Run the src/test/java/org/odftoolkit/odfdom/dom/XPathTest.java unit test. It fails.
> 4) apply the patch odffiledom-patch.txt on OdfFileDom and run the unit test again. It
passes.
> I have added explanatory comments in the patched unit test and the patched getNamespaceURI().

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message