ODF Toolkit
  1. ODF Toolkit
  2. ODFTOOLKIT-110

Attribute namespace declaration now usable by XPath

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: odfdom-0.8.5
    • Fix Version/s: odfdom-0.9
    • Component/s: java
    • Labels:
      None
    • Environment:
      Operating System: Linux
      Platform: PC

      Description

      When a slide deck is very simple (like attachment 1) it can't be used in

      OdfPresentationDocument.copyForeignSlide()

      It throws the following exceptions:

      ------------------
      javax.xml.xpath.XPathExpressionException
      at com.sun.org.apache.xpath.internal.jaxp.XPathImpl.evaluate(XPathImpl.java:289)
      at org.odftoolkit.odfdom.doc.OdfPresentationDocument.copyForeignLinkRef(OdfPresentationDocument.java:644)
      at org.odftoolkit.odfdom.doc.OdfPresentationDocument.InsertCollectedStyle(OdfPresentationDocument.java:840)
      at org.odftoolkit.odfdom.doc.OdfPresentationDocument.copyForeignStyleRef(OdfPresentationDocument.java:723)
      at org.odftoolkit.odfdom.doc.OdfPresentationDocument.copyForeignSlide(OdfPresentationDocument.java:618)
      at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

      -----------------------

      and

      This runs on a German system, so a rough translation is:
      "SEVERE The prefix xlink must be resoved"
      ------------------------

      SCHWERWIEGEND: null
      com.sun.org.apache.xpath.internal.domapi.XPathStylesheetDOM3Exception: Das Präfix muss in einen Namensbereich aufgelöst werden: xlink
      at com.sun.org.apache.xpath.internal.compiler.XPathParser.errorForDOM3(XPathParser.java:653)
      at com.sun.org.apache.xpath.internal.compiler.Lexer.mapNSTokens(Lexer.java:638)
      at com.sun.org.apache.xpath.internal.compiler.Lexer.tokenize(Lexer.java:265)
      at com.sun.org.apache.xpath.internal.compiler.Lexer.tokenize(Lexer.java:96)
      at com.sun.org.apache.xpath.internal.compiler.XPathParser.initXPath(XPathParser.java:110)
      at com.sun.org.apache.xpath.internal.XPath.<init>(XPath.java:176)
      at com.sun.org.apache.xpath.internal.XPath.<init>(XPath.java:264)
      at com.sun.org.apache.xpath.internal.jaxp.XPathImpl.eval(XPathImpl.java:193)
      at com.sun.org.apache.xpath.internal.jaxp.XPathImpl.evaluate(XPathImpl.java:275)
      at org.odftoolkit.odfdom.doc.OdfPresentationDocument.copyForeignLinkRef(OdfPresentationDocument.java:644)
      at org.odftoolkit.odfdom.doc.OdfPresentationDocument.InsertCollectedStyle(OdfPresentationDocument.java:840)
      at org.odftoolkit.odfdom.doc.OdfPresentationDocument.copyForeignStyleRef(OdfPresentationDocument.java:749)
      at org.odftoolkit.odfdom.doc.OdfPresentationDocument.copyForeignSlide(OdfPresentationDocument.java:618)
      ------------------------

      once a slide deck contains an image the parsing go through.

      I assume this is true for all high level methods that call OdfPresentationDocument.copyForeignLinkRef() and methods like
      OdfPresentationDocument.deleteLinkRef

        Issue Links

          Activity

          Hide
          j-wicz added a comment -

          Created an attachment (id=263)
          attachment 1, very simple slide deck with text only

          Show
          j-wicz added a comment - Created an attachment (id=263) attachment 1, very simple slide deck with text only
          Hide
          j-wicz added a comment -

          Created an attachment (id=264)
          attachment 2, one image in the slide deck

          Show
          j-wicz added a comment - Created an attachment (id=264) attachment 2, one image in the slide deck
          Hide
          j-wicz added a comment -

          I tried saving the file in OpenOffice 3.2.1 as "ODF 1.2 with extensions" and "ODF 1.2" (pure), with no difference

          Show
          j-wicz added a comment - I tried saving the file in OpenOffice 3.2.1 as "ODF 1.2 with extensions" and "ODF 1.2" (pure), with no difference
          Hide
          Wei Hua Wang added a comment -

          Hi J-Wicz,

          I can not reproduce this issue in my machine, the copyForeignSlide operation runs well, so I want to ask your OS and JRE version, and pls attach your sample code? I use sun jdk1.5.0, jdk1.6.0_18, IBM jdk 1.6, they all work well.

          My code :
          ----------------------------------
          private static final String SOURCE_FILE_1="slideDeckWithTwoSlides.odp";
          private static final String SOURCE_FILE_2="slideDeckWithXlinkSlide.odp";

          @Test
          public void testCopyForeignSlide(){
          try {
          OdfPresentationDocument targetodp = (OdfPresentationDocument) OdfPresentationDocument.loadDocument(ResourceUtilities.getTestResourceAsStream(SOURCE_FILE_1));
          OdfPresentationDocument sourceodp = (OdfPresentationDocument) OdfPresentationDocument.loadDocument(ResourceUtilities.getTestResourceAsStream(SOURCE_FILE_2));

          int slidecount = sourceodp.getSlideCount();
          for(int i=0;i<slidecount;i++)

          { targetodp.copyForeignSlide(i, sourceodp, i); }

          targetodp.save(ResourceUtilities.newTestOutputFile("CopyForeignSlide2.odp"));
          targetodp.close();
          System.out.println("Done");
          }catch (Exception e)

          { e.printStackTrace(); }

          }
          ------------------------------------------------
          While from your description, I can tell that it is caused by XPath miss binding to the namespace, I use xpath expression "//*[@xlink:href]" which the "xlink" is the prefix for namespace "http://www.w3.org/1999/xlink", if namespace prefixes are used in an XPath expression, the XPath processor needs to know what namespace URIs the prefixes are bound to.
          But you can see my stack for getting the namespace for prefix "xlink" when call Xpath.evaluation("//*[@xlink:href]");
          ------------------------------
          OdfNamespace.getNamespaceURI(String) line: 176
          JAXPPrefixResolver.getNamespaceForPrefix(String) line: 45
          Lexer.mapNSTokens(String, int, int, int) line: 587
          Lexer.tokenize(String, Vector) line: 265
          Lexer.tokenize(String) line: 96
          XPathParser.initXPath(Compiler, String, PrefixResolver) line: 108
          XPath.<init>(String, SourceLocator, PrefixResolver, int, ErrorListener) line: 162
          XPath.<init>(String, SourceLocator, PrefixResolver, int) line: 198
          XPathImpl.eval(String, Object) line: 193
          XPathImpl.evaluate(String, Object, QName) line: 275
          OdfPresentationDocument.copyForeignLinkRef(OdfElement) line: 644
          OdfPresentationDocument.copyForeignSlide(int, OdfPresentationDocument, int) line: 616
          ---------------------------------
          JAXPPrefixResolver can be initialized with OdfDOM specific namespaceContext org.odftoolkit.odfdom.OdfNamespace when XPath set any namespace context(pls refer to implementation of OdfDocument.getXPath(), Slide API use the such xpath from OdfDocument). So when xpath evaluate any expression with the prefix, JAXPPrefixResolver will use the OdfDOM namespaceContext to parse the prefix.

          Show
          Wei Hua Wang added a comment - Hi J-Wicz, I can not reproduce this issue in my machine, the copyForeignSlide operation runs well, so I want to ask your OS and JRE version, and pls attach your sample code? I use sun jdk1.5.0, jdk1.6.0_18, IBM jdk 1.6, they all work well. My code : ---------------------------------- private static final String SOURCE_FILE_1="slideDeckWithTwoSlides.odp"; private static final String SOURCE_FILE_2="slideDeckWithXlinkSlide.odp"; @Test public void testCopyForeignSlide(){ try { OdfPresentationDocument targetodp = (OdfPresentationDocument) OdfPresentationDocument.loadDocument(ResourceUtilities.getTestResourceAsStream(SOURCE_FILE_1)); OdfPresentationDocument sourceodp = (OdfPresentationDocument) OdfPresentationDocument.loadDocument(ResourceUtilities.getTestResourceAsStream(SOURCE_FILE_2)); int slidecount = sourceodp.getSlideCount(); for(int i=0;i<slidecount;i++) { targetodp.copyForeignSlide(i, sourceodp, i); } targetodp.save(ResourceUtilities.newTestOutputFile("CopyForeignSlide2.odp")); targetodp.close(); System.out.println("Done"); }catch (Exception e) { e.printStackTrace(); } } ------------------------------------------------ While from your description, I can tell that it is caused by XPath miss binding to the namespace, I use xpath expression "//* [@xlink:href] " which the "xlink" is the prefix for namespace "http://www.w3.org/1999/xlink", if namespace prefixes are used in an XPath expression, the XPath processor needs to know what namespace URIs the prefixes are bound to. But you can see my stack for getting the namespace for prefix "xlink" when call Xpath.evaluation("//* [@xlink:href] "); ------------------------------ OdfNamespace.getNamespaceURI(String) line: 176 JAXPPrefixResolver.getNamespaceForPrefix(String) line: 45 Lexer.mapNSTokens(String, int, int, int) line: 587 Lexer.tokenize(String, Vector) line: 265 Lexer.tokenize(String) line: 96 XPathParser.initXPath(Compiler, String, PrefixResolver) line: 108 XPath.<init>(String, SourceLocator, PrefixResolver, int, ErrorListener) line: 162 XPath.<init>(String, SourceLocator, PrefixResolver, int) line: 198 XPathImpl.eval(String, Object) line: 193 XPathImpl.evaluate(String, Object, QName) line: 275 OdfPresentationDocument.copyForeignLinkRef(OdfElement) line: 644 OdfPresentationDocument.copyForeignSlide(int, OdfPresentationDocument, int) line: 616 --------------------------------- JAXPPrefixResolver can be initialized with OdfDOM specific namespaceContext org.odftoolkit.odfdom.OdfNamespace when XPath set any namespace context(pls refer to implementation of OdfDocument.getXPath(), Slide API use the such xpath from OdfDocument). So when xpath evaluate any expression with the prefix, JAXPPrefixResolver will use the OdfDOM namespaceContext to parse the prefix.
          Hide
          j-wicz added a comment -

          Created an attachment (id=269)
          Extended Unit Case written in Java

          Show
          j-wicz added a comment - Created an attachment (id=269) Extended Unit Case written in Java
          Hide
          j-wicz added a comment -

          Created an attachment (id=270)
          Extended Unit Case written in Groovy

          Show
          j-wicz added a comment - Created an attachment (id=270) Extended Unit Case written in Groovy
          Hide
          j-wicz added a comment -

          Dear Wei Hua Wang

          I'm on Ubuntu 10.04 AMD 64bit with java-6-sun-1.6.0.16 using Eclipse Helios

          I tested your test case in a clean Eclipse project and it passes.

          I extended the example with the test method testCopyForeignSlideInNewDoc (see attachment 3) which is closer to my use and it still passes

          I rewrote my test case in Groovy, which is part of our project (see attachment 4) and it fails the previously reported xpath problem.

          So I assume it's some class loading problem maybe related to groovy-all-1.7.3.jar and xerces 2.9.1

          I'm not sure if Groovy compatibility is in your scope, but it would be very nice when Odftools would work with it.

          Maybe the title of this issue should be adapted then.

          Show
          j-wicz added a comment - Dear Wei Hua Wang I'm on Ubuntu 10.04 AMD 64bit with java-6-sun-1.6.0.16 using Eclipse Helios I tested your test case in a clean Eclipse project and it passes. I extended the example with the test method testCopyForeignSlideInNewDoc (see attachment 3) which is closer to my use and it still passes I rewrote my test case in Groovy, which is part of our project (see attachment 4) and it fails the previously reported xpath problem. So I assume it's some class loading problem maybe related to groovy-all-1.7.3.jar and xerces 2.9.1 I'm not sure if Groovy compatibility is in your scope, but it would be very nice when Odftools would work with it. Maybe the title of this issue should be adapted then.
          Hide
          Svante Schubert added a comment -

          Hi J-Wicz,

          I double checked the unit test and test documents you provided.
          It works for me under Windows7 and the issue reminds me on a ODFTOOLKIT-72, I had fixed for last version.

          To be sure we talking about the same thing, I have adapted your test documents and test case for everyone in a single patch file.

          There where only three minor things I have adapted:

          1) Load/Save test files in our Maven environment via
          ResourceUtilities.getTestResourceAsStream(..)
          ResourceUtilities.newTestOutputFile(..)

          2) Renamed your test to org.odftoolkit.odfdom.presentation.SlideTest.

          3) Used Java Logging instead of System.out..

          After that I commited your files via
          hg commit -A -m"#ODFTOOLKIT-110 Slide XPATH regression test" -u"svanteschubert"
          and created the patch via
          hg export -ago ../bug179-slide-xpath-regression-test.patch tip

          Those patches help us to avoid mistakes when applying suggested changes and save time (when taking all times developer and reviewer together).

          Your files as patch will follow, can you check out which version do you use.
          "hg log -l 3" or make a "hg in" to see if there is still something new on the repository.

          If you have only an odfdom.jar you are working with, use "java -jar odfdom.jar" to get version information.

          Thanks for your efforts, J!
          Svante

          Show
          Svante Schubert added a comment - Hi J-Wicz, I double checked the unit test and test documents you provided. It works for me under Windows7 and the issue reminds me on a ODFTOOLKIT-72 , I had fixed for last version. To be sure we talking about the same thing, I have adapted your test documents and test case for everyone in a single patch file. There where only three minor things I have adapted: 1) Load/Save test files in our Maven environment via ResourceUtilities.getTestResourceAsStream(..) ResourceUtilities.newTestOutputFile(..) 2) Renamed your test to org.odftoolkit.odfdom.presentation.SlideTest. 3) Used Java Logging instead of System.out.. After that I commited your files via hg commit -A -m"# ODFTOOLKIT-110 Slide XPATH regression test" -u"svanteschubert" and created the patch via hg export -ago ../bug179-slide-xpath-regression-test.patch tip Those patches help us to avoid mistakes when applying suggested changes and save time (when taking all times developer and reviewer together). Your files as patch will follow, can you check out which version do you use. "hg log -l 3" or make a "hg in" to see if there is still something new on the repository. If you have only an odfdom.jar you are working with, use "java -jar odfdom.jar" to get version information. Thanks for your efforts, J! Svante
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=273)
          Java test and test files bundled as a Mercurial patch

          Show
          Svante Schubert added a comment - Created an attachment (id=273) Java test and test files bundled as a Mercurial patch
          Hide
          j-wicz added a comment -

          Some more information

          I'm using the 0.8.5 release
          on the classpath are only the following libs/jars:

          java-6-sun-1.6.0.16
          xercesImpl.jar aka Xerces-J 2.9.1
          groovy-all-1.7.3.jar
          odfdom.jar aka odfdom 0.8.5 (build 20100603-1912)

          as ResourceUtilities is not part of the odfdom.jar but rather your test setup and source code I used java.io.File before. I can switch to ResourceUtilities for your convenience.

          I did not have time to check your Mercurial patch yet

          Show
          j-wicz added a comment - Some more information I'm using the 0.8.5 release on the classpath are only the following libs/jars: java-6-sun-1.6.0.16 xercesImpl.jar aka Xerces-J 2.9.1 groovy-all-1.7.3.jar odfdom.jar aka odfdom 0.8.5 (build 20100603-1912) as ResourceUtilities is not part of the odfdom.jar but rather your test setup and source code I used java.io.File before. I can switch to ResourceUtilities for your convenience. I did not have time to check your Mercurial patch yet
          Hide
          Svante Schubert added a comment -

          The XPATH implementation being used is from the Sun JDK and not from Xerces, look at the stacktrace:

          com.sun.org.apache.xpath.internal.jaxp.XPathImpl.evaluate(XPathImpl.java:289

          The com.sun.* reveals it. I have been using the latest JDK 1.6.0_20.

          In addition I cloned ODFDOM on a Ubuntu 10.04 LTS platform and applied my Java test and test file patch based on your work.
          It runs smooth, but using JDK 1.6.0_20 as well.

          Could you try to test on a machine with updated JDK? It seems the JDK is the problem.

          Thanks & a nice week-end,
          Svante

          Show
          Svante Schubert added a comment - The XPATH implementation being used is from the Sun JDK and not from Xerces, look at the stacktrace: com.sun.org.apache.xpath.internal.jaxp.XPathImpl.evaluate(XPathImpl.java:289 The com.sun.* reveals it. I have been using the latest JDK 1.6.0_20. In addition I cloned ODFDOM on a Ubuntu 10.04 LTS platform and applied my Java test and test file patch based on your work. It runs smooth, but using JDK 1.6.0_20 as well. Could you try to test on a machine with updated JDK? It seems the JDK is the problem. Thanks & a nice week-end, Svante
          Hide
          j-wicz added a comment -

          Dear Odftoolkit Team

          I think your unit tests are passing, because the code of the method copyForeignLinkRef is in a try-catch-block and when the catch block gets activated, it just writes to the logger.

          I investigated the issue and I believe the problem is the call line
          XPath xpath = getXPath();

          the XPath that is constructed in the method getXPath() is explictly initialised with the OFFICE namespace OdfNamespaceNames.OFFICE "urn:oasis:names:tc:opendocument:xmlns:office:1.0"

          even though the static method OdfNamespace.newNamespace() returns an Object with the interface javax.xml.namespace.NamespaceContext the only namespace registered is OdfNamespaceNames.OFFICE

          if I initialize mXPath.setNamespaceContext(OdfNamespace.newNamespace(OdfNamespaceNames.XLINK))
          my errors disappear. But this is most likely not the desired solution, because getXPath() is called at various places.

          I think a clean solution would be to set a NamespaceContext with all enumeration members in OdfNamespaceNames.

          btw, you could also programtically check if a namespace is registered with DOM Level 3 with calls like:
          sourceCloneEle.getOwnerDocument().lookupNamespaceURI("xlink")
          sourceCloneEle.lookupNamespaceURI("xlink"))
          xpath.getNamespaceContext().getNamespaceURI("xlink"));

          Show
          j-wicz added a comment - Dear Odftoolkit Team I think your unit tests are passing, because the code of the method copyForeignLinkRef is in a try-catch-block and when the catch block gets activated, it just writes to the logger. I investigated the issue and I believe the problem is the call line XPath xpath = getXPath(); the XPath that is constructed in the method getXPath() is explictly initialised with the OFFICE namespace OdfNamespaceNames.OFFICE "urn:oasis:names:tc:opendocument:xmlns:office:1.0" even though the static method OdfNamespace.newNamespace() returns an Object with the interface javax.xml.namespace.NamespaceContext the only namespace registered is OdfNamespaceNames.OFFICE if I initialize mXPath.setNamespaceContext(OdfNamespace.newNamespace(OdfNamespaceNames.XLINK)) my errors disappear. But this is most likely not the desired solution, because getXPath() is called at various places. I think a clean solution would be to set a NamespaceContext with all enumeration members in OdfNamespaceNames. btw, you could also programtically check if a namespace is registered with DOM Level 3 with calls like: sourceCloneEle.getOwnerDocument().lookupNamespaceURI("xlink") sourceCloneEle.lookupNamespaceURI("xlink")) xpath.getNamespaceContext().getNamespaceURI("xlink"));
          Hide
          j-wicz added a comment -

          Dear Svante

          Xerces does not have any xpath implementation to my knowledge. The implementation is provided by Apache Xalan.

          A (always problematic) version of Xerces and a version of Xalan are bundeled in JDK since 1.5.
          So the ODFToolkit most likely uses the 2.9.1 version of Xerces (btw it looks like 2.10 was released recently http://xerces.apache.org/xerces2-j/ ) and the repackage Xalan XPathImpl that comes with the JDK

          (In reply to comment #11)
          > The XPATH implementation being used is from the Sun JDK and not from Xerces,
          > look at the stacktrace:
          >
          > com.sun.org.apache.xpath.internal.jaxp.XPathImpl.evaluate(XPathImpl.java:289
          >
          > The com.sun.* reveals it. I have been using the latest JDK 1.6.0_20.
          >

          Show
          j-wicz added a comment - Dear Svante Xerces does not have any xpath implementation to my knowledge. The implementation is provided by Apache Xalan. A (always problematic) version of Xerces and a version of Xalan are bundeled in JDK since 1.5. So the ODFToolkit most likely uses the 2.9.1 version of Xerces (btw it looks like 2.10 was released recently http://xerces.apache.org/xerces2-j/ ) and the repackage Xalan XPathImpl that comes with the JDK (In reply to comment #11) > The XPATH implementation being used is from the Sun JDK and not from Xerces, > look at the stacktrace: > > com.sun.org.apache.xpath.internal.jaxp.XPathImpl.evaluate(XPathImpl.java:289 > > The com.sun.* reveals it. I have been using the latest JDK 1.6.0_20. >
          Hide
          Svante Schubert added a comment -

          Hi J-Wicz,

          checked it, you are right about XPATH implementation belonging to Xalan.
          Can you adjust the given Mercurial patch (see above) in a way that it breaks?
          I had been working on this issue once and would work myself again into it, unless you can even provide a solution with the patch given, which would be most welcome.

          For the SVN users a quick commandline example how to load and create the patch:

          hg clone https://odftoolkit.org/hg/odfdom~developer

          to build through via maven by commandline to see if the new version is running:
          cd odfdom~developer
          mvn

          After that the download of the patch
          http://odftoolkit.org/bugzilla/attachment.cgi?id=273

          and import without commit into the repository (no commit has the advantage, that you see the changes in the IDE and still might adapt sources)
          hg import PATH_TO_PATCHFILE --no-commit

          When this works, please adjust the test, perhaps add a solution and export everything into a new patch

          Show
          Svante Schubert added a comment - Hi J-Wicz, checked it, you are right about XPATH implementation belonging to Xalan. Can you adjust the given Mercurial patch (see above) in a way that it breaks? I had been working on this issue once and would work myself again into it, unless you can even provide a solution with the patch given, which would be most welcome. For the SVN users a quick commandline example how to load and create the patch: hg clone https://odftoolkit.org/hg/odfdom~developer to build through via maven by commandline to see if the new version is running: cd odfdom~developer mvn After that the download of the patch http://odftoolkit.org/bugzilla/attachment.cgi?id=273 and import without commit into the repository (no commit has the advantage, that you see the changes in the IDE and still might adapt sources) hg import PATH_TO_PATCHFILE --no-commit When this works, please adjust the test, perhaps add a solution and export everything into a new patch
          Hide
          Svante Schubert added a comment -

          Sorry, incentally pressed key combo to submit the patch (tab than return)
          You can create the patch via
          hg export -ago ../PATCHFILENAME.patch tip

          and upload the patch to the issue, deprecating the old one.

          Thanks for any help!
          Svante

          Show
          Svante Schubert added a comment - Sorry, incentally pressed key combo to submit the patch (tab than return) You can create the patch via hg export -ago ../PATCHFILENAME.patch tip and upload the patch to the issue, deprecating the old one. Thanks for any help! Svante
          Hide
          j-wicz added a comment -

          Created an attachment (id=283)
          Unit test that fails with com.sun.org.apache.xpath.internal.domapi.XPathStylesheetDOM3Exception

          Dear Odftoolkit team

          I'm new to Mercurial and all the binary ODF files in the checkout make it exporting a patch a bit challenging for me.

          please find attached a traditional java source file attachment that should fail on your system as well.

          Show
          j-wicz added a comment - Created an attachment (id=283) Unit test that fails with com.sun.org.apache.xpath.internal.domapi.XPathStylesheetDOM3Exception Dear Odftoolkit team I'm new to Mercurial and all the binary ODF files in the checkout make it exporting a patch a bit challenging for me. please find attached a traditional java source file attachment that should fail on your system as well.
          Hide
          j-wicz added a comment -

          About attachment 283

          please be awre that the order in which the test methods are called are crucial. I assume this is due some lazy-initialization patterns in your library.

          Once you comment out the first method the test will pass, even though the last method is an exact copy of the first. So some magic is happening in the middle method

          Show
          j-wicz added a comment - About attachment 283 please be awre that the order in which the test methods are called are crucial. I assume this is due some lazy-initialization patterns in your library. Once you comment out the first method the test will pass, even though the last method is an exact copy of the first. So some magic is happening in the middle method
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=285)
          Patch to reproduce XPath problem based on J-Wicz work

          With this patch the test fails as in your environemnt, J-Wicz.
          I have deprecated the previous patches (except Groovy) and will take a look into its reason.

          Show
          Svante Schubert added a comment - Created an attachment (id=285) Patch to reproduce XPath problem based on J-Wicz work With this patch the test fails as in your environemnt, J-Wicz. I have deprecated the previous patches (except Groovy) and will take a look into its reason.
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=286)
          Attribute namespace declaration now usable by XPath

          Show
          Svante Schubert added a comment - Created an attachment (id=286) Attribute namespace declaration now usable by XPath
          Hide
          Svante Schubert added a comment -

          Hi J-Wicz,

          are you able to review the patch?

          Take a look at the Wiki for more details:
          http://odftoolkit.org/projects/odfdom/pages/Development#Get_and_Build_the_Source_Code

          If there are questions left, don't hesitate to ask, and I will answer and improve the Wiki meanwhile.

          PS: The trick was, that xmlns:xxx="yyy" are now added to the JDK NamespaceContext
          http://java.sun.com/javase/6/docs/api/javax/xml/namespace/NamespaceContext.html
          with the prefix xxx and the URL yyy.
          Seems Java can not handle multiple URLs with the same prefix, I wrote and removed the test again.

          Thanks,
          Svante

          Show
          Svante Schubert added a comment - Hi J-Wicz, are you able to review the patch? Take a look at the Wiki for more details: http://odftoolkit.org/projects/odfdom/pages/Development#Get_and_Build_the_Source_Code If there are questions left, don't hesitate to ask, and I will answer and improve the Wiki meanwhile. PS: The trick was, that xmlns:xxx="yyy" are now added to the JDK NamespaceContext http://java.sun.com/javase/6/docs/api/javax/xml/namespace/NamespaceContext.html with the prefix xxx and the URL yyy. Seems Java can not handle multiple URLs with the same prefix, I wrote and removed the test again. Thanks, Svante
          Hide
          Svante Schubert added a comment -

          I have reviewed my patch myself after a view days and would like to hear your opinion on a potential problem.

          I have read that the NamespaceConext interface of the JDK, where the JDK XPath implementation gets its mapping from prefix to URL to uniqly identify expressions as "//table:table" are not supporting the same prefix with multiple URLs as all mappings are global.

          In our case they are really global for all documents being used by ODFDOM.
          That might be on a server even for different users.
          I feel potential danger, although I am not able to grap it.

          My suggestion is to separate the now static Maps from OdfNamespace and create a new class OdfNamespaceContext, created for each package and/or each document.

          It also makes the code more logical/cleaner, as a user does not have to create a specific OdfNamespace (eg. for office: prefix) to get in favor of the OdfNamespaceContext required for XPath.

          What are the others opinion on this?

          Show
          Svante Schubert added a comment - I have reviewed my patch myself after a view days and would like to hear your opinion on a potential problem. I have read that the NamespaceConext interface of the JDK, where the JDK XPath implementation gets its mapping from prefix to URL to uniqly identify expressions as "//table:table" are not supporting the same prefix with multiple URLs as all mappings are global. In our case they are really global for all documents being used by ODFDOM. That might be on a server even for different users. I feel potential danger, although I am not able to grap it. My suggestion is to separate the now static Maps from OdfNamespace and create a new class OdfNamespaceContext, created for each package and/or each document. It also makes the code more logical/cleaner, as a user does not have to create a specific OdfNamespace (eg. for office: prefix) to get in favor of the OdfNamespaceContext required for XPath. What are the others opinion on this?
          Hide
          j-wicz added a comment -

          Dear Odfdom Team

          I run the patch in attachment 286 on my Ubuntu system and it passes now.

          Also my project specific tests (including tests written in groovy-1.7.3) are passing as well.

          So it looks good

          Show
          j-wicz added a comment - Dear Odfdom Team I run the patch in attachment 286 on my Ubuntu system and it passes now. Also my project specific tests (including tests written in groovy-1.7.3) are passing as well. So it looks good
          Hide
          j-wicz added a comment -

          Dear Odfdom Team

          The line OdfDocument.java:1282 in the patch attachment 286

          if (!attrQname.startsWith("xmlns:")) {

          looked a bit odd to me as it is testing against a prefix but rather a URI, but I looked up http://www.w3.org/TR/xml-names/#ns-decl

          and to quote
          "
          Namespace constraint: Reserved Prefixes and Namespace Names
          [...]
          The prefix xmlns is used only to declare namespace bindings and is by definition bound to the namespace name http://www.w3.org/2000/xmlns/. It MUST NOT be declared . Other prefixes MUST NOT be bound to this namespace name, and it MUST NOT be declared as the default namespace. Element names MUST NOT have the prefix xmlns.
          "

          so the usage in Odfdom seems to be conforming

          Show
          j-wicz added a comment - Dear Odfdom Team The line OdfDocument.java:1282 in the patch attachment 286 if (!attrQname.startsWith("xmlns:")) { looked a bit odd to me as it is testing against a prefix but rather a URI, but I looked up http://www.w3.org/TR/xml-names/#ns-decl and to quote " Namespace constraint: Reserved Prefixes and Namespace Names [...] The prefix xmlns is used only to declare namespace bindings and is by definition bound to the namespace name http://www.w3.org/2000/xmlns/ . It MUST NOT be declared . Other prefixes MUST NOT be bound to this namespace name, and it MUST NOT be declared as the default namespace. Element names MUST NOT have the prefix xmlns. " so the usage in Odfdom seems to be conforming
          Hide
          j-wicz added a comment -

          (In reply to comment #21)
          >
          > In our case they are really global for all documents being used by ODFDOM.
          > That might be on a server even for different users.
          > I feel potential danger, although I am not able to grap it.
          >
          > My suggestion is to separate the now static Maps from OdfNamespace and create a
          > new class OdfNamespaceContext, created for each package and/or each document.
          >
          > It also makes the code more logical/cleaner, as a user does not have to create
          > a specific OdfNamespace (eg. for office: prefix) to get in favor of the
          > OdfNamespaceContext required for XPath.
          >
          > What are the others opinion on this?

          Dear Svante

          I personally think that there should be one namespace context per document / DOM.

          people might upload documents with different URIs but equal prefixes

          xmlns:my="http:/some.magic.extension.from.organisation.a"
          xmlns:my="http:/some.totally.awesome.namespace.from.organisation.b"

          or different versions of staff which is not controlled by OASIS
          xmlns:dc="http://purl.org/dc/elements/1.1/"
          xmlns:dc="http://purl.org/dc/elements/5.5/"

          they might do important stuff and xpath queries with this, but they want to use
          an infrastructure library like "ODFdom" as commodity.

          But this is only a potential/made up use case. For instance we are using vanilla ODF and we don't dare to mix in other XML.

          And while I think that compared to all the DOM heavy lifting the CPU cost of one-namespace-per-document might be small. It might require some bigger refactoring.

          Show
          j-wicz added a comment - (In reply to comment #21) > > In our case they are really global for all documents being used by ODFDOM. > That might be on a server even for different users. > I feel potential danger, although I am not able to grap it. > > My suggestion is to separate the now static Maps from OdfNamespace and create a > new class OdfNamespaceContext, created for each package and/or each document. > > It also makes the code more logical/cleaner, as a user does not have to create > a specific OdfNamespace (eg. for office: prefix) to get in favor of the > OdfNamespaceContext required for XPath. > > What are the others opinion on this? Dear Svante I personally think that there should be one namespace context per document / DOM. people might upload documents with different URIs but equal prefixes xmlns:my="http:/some.magic.extension.from.organisation.a" xmlns:my="http:/some.totally.awesome.namespace.from.organisation.b" or different versions of staff which is not controlled by OASIS xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:dc="http://purl.org/dc/elements/5.5/" they might do important stuff and xpath queries with this, but they want to use an infrastructure library like "ODFdom" as commodity. But this is only a potential/made up use case. For instance we are using vanilla ODF and we don't dare to mix in other XML. And while I think that compared to all the DOM heavy lifting the CPU cost of one-namespace-per-document might be small. It might require some bigger refactoring.
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=350)
          #ODFTOOLKIT-110# XPath namespace fix adding identical XML comparison test

          Some of the problems fixed by this patch were already fixed by issue 69.

          I did a resynch of the patch to the latest revision and added an XML regression test, which does an identical comparison of XML in the DocumentTest class.

          The reason to do this, was that with this fix unnoticed the namespace attributes starting with xmlns: were no longer written to the ODF root element, but instead repeated all over the XML, increasing the size.

          This patch reveals this problem, the patch has the fix disabled by comment to proof the working test.

          A final patch will follow.

          @Reviewer: I find it hard to compare the XML. Aside of the encoding setting to UTF-8, I had to save and reload the XML to be certain that all line ending encodings were similar.

          Show
          Svante Schubert added a comment - Created an attachment (id=350) # ODFTOOLKIT-110 # XPath namespace fix adding identical XML comparison test Some of the problems fixed by this patch were already fixed by issue 69. I did a resynch of the patch to the latest revision and added an XML regression test, which does an identical comparison of XML in the DocumentTest class. The reason to do this, was that with this fix unnoticed the namespace attributes starting with xmlns: were no longer written to the ODF root element, but instead repeated all over the XML, increasing the size. This patch reveals this problem, the patch has the fix disabled by comment to proof the working test. A final patch will follow. @Reviewer: I find it hard to compare the XML. Aside of the encoding setting to UTF-8, I had to save and reload the XML to be certain that all line ending encodings were similar.
          Hide
          Svante Schubert added a comment -

          I would like to discuss on tomorrows dev call the solution of the namespace scope problem (see http://odftoolkit.org/bugzilla/show_bug.cgi?id=179#c21

          Facts & Design Suggestions:
          1) An XML file might use more than one URLs for namespace prefixes, but JDK XPATH/NamespaceContext does not support this.

          2) Currently ODFDOM only uses static maps to align namespace prefix with URL for all documents on the server. This worked fine as long we could not extend the maps, what we now need to do for user XML.

          3) Instead of having static mapping tables for namespace prefix to URL.
          The mapping might be per XML file, for instance for every given file DOM.
          Used only once.

          4) An XPath taken from a OdfDocument getXPath() should be initialized with all ODF namespace prefix/URL mappings (given by the enumeration org.odftoolkit.odfdom.dom.OdfNamespaceNames

          5) A possible design would be to add getXPath() to OdfFileDom.
          In addition add further files OdfContentDom, OdfStylesDom,.. and be able to reveive getXPath() including the ODF XML prefixes from them (and from the Document). The use OdfNameSpace Context might be taken out of the API, making the class using a package modifier.

          Svante

          Show
          Svante Schubert added a comment - I would like to discuss on tomorrows dev call the solution of the namespace scope problem (see http://odftoolkit.org/bugzilla/show_bug.cgi?id=179#c21 Facts & Design Suggestions: 1) An XML file might use more than one URLs for namespace prefixes, but JDK XPATH/NamespaceContext does not support this. 2) Currently ODFDOM only uses static maps to align namespace prefix with URL for all documents on the server. This worked fine as long we could not extend the maps, what we now need to do for user XML. 3) Instead of having static mapping tables for namespace prefix to URL. The mapping might be per XML file, for instance for every given file DOM. Used only once. 4) An XPath taken from a OdfDocument getXPath() should be initialized with all ODF namespace prefix/URL mappings (given by the enumeration org.odftoolkit.odfdom.dom.OdfNamespaceNames 5) A possible design would be to add getXPath() to OdfFileDom. In addition add further files OdfContentDom, OdfStylesDom,.. and be able to reveive getXPath() including the ODF XML prefixes from them (and from the Document). The use OdfNameSpace Context might be taken out of the API, making the class using a package modifier. Svante
          Hide
          Svante Schubert added a comment -

          The getXPath method and the map of the namespace prefix is now only available on the OdfFileDom class an its subclasses.

          New are the subclasses OdfContentDom, OdfStylesDom, OdfMetaDom and OdfSettingsDom, providing a XPath with a preinitialized JDK NamespaceContext.

          Trying to remove the NamespaceContext completly from the ODFDOM API, let it become an implementation detail.

          If we can save all the namespace prefixes and urls in the root element of an OdfFileDom during the save operation of an XML DOM, all new namespaces will be saved.

          Adapted tests, but review and tweaking my work on Monday before releasing it as a patch.

          Making this patch a predecessor of the style refactoring (72).

          Show
          Svante Schubert added a comment - The getXPath method and the map of the namespace prefix is now only available on the OdfFileDom class an its subclasses. New are the subclasses OdfContentDom, OdfStylesDom, OdfMetaDom and OdfSettingsDom, providing a XPath with a preinitialized JDK NamespaceContext. Trying to remove the NamespaceContext completly from the ODFDOM API, let it become an implementation detail. If we can save all the namespace prefixes and urls in the root element of an OdfFileDom during the save operation of an XML DOM, all new namespaces will be saved. Adapted tests, but review and tweaking my work on Monday before releasing it as a patch. Making this patch a predecessor of the style refactoring (72).
          Hide
          Svante Schubert added a comment -

          Hi Daisy,

          Rob asked to move the style handling in separated task, some for you some for me.
          I have done some style depending changes with this issue and worked several days on the design of this issue.
          Could you give me an early feed-back and help me on the tests and if you are able on the implementation?
          I would continue on the implementation tomorrow, pinging you in IRC.

          I believe the design is quite stable now.
          Still some of the implementation are missing and therefore tests are failing. BTW I fixed an issue in the XPath/Slide test making the problem visible, making it fail.

          The design:
          OdfNamespaceContext was removed.
          The OdfFileDom is now implementing the interface NamespaceContext, embracing the get methods from the XPath required interface and in addition set methods:

          public void setNamespace(String prefix, String uri)
          public void setNamespace(NamespaceName name)

          to add new namespaces during SAX parsing and DOM node creation.

          The specific ODF subclasses of

          org.odftoolkit.odfdom.dom.
          OdfContentDom, OdfStylesDom, OdfMetaDom and OdfSettingsDom

          and the later following
          org.odftoolkit.odfdom.pkg.manifest
          OdfManifestDom

          will initialize the Namespace mapping map with its ODF namespaces taken from enums generated by source code generation.
          (During this redesign the namespace enum ..dom.OdfNamespaceNames has been renamed to ..dom.OdfDocumentNamespace, making it easy to distinguish to the upcoming pkg.manifest.OdfPackageNamespace - see issue 71, e.g. OdfDocumentNamespace.OFFICE)
          These mapping are made persistent as XML during the save of a OdfFileDom within the OdfPackage. The mapping will be saved as namespace attributes to the root element.

          Note: The OdfFileDom subclasses are now the entry point for each DOM tree, e.g. for OdfContentDom
          public OfficeDocumentContentElement getRootElement()

          Following additional changes/suggestions:
          1)
          org.odftoolkit.odfdom.pkg.OdfXMLHelper.java was only used by tests and moved to test package, as the class is more a tool based on ODFDOM than a required part of ODFDOM.
          2) Not used/required and removed for simplification:
          org.odftoolkit.odfdom.dom.OdfAttributeNames.java
          org.odftoolkit.odfdom.pkg.OdfPackageStream
          org.odftoolkit.odfdom.pkg.OdfPackageResource.java
          3)
          The class
          org.odftoolkit.odfdom.OdfFileDom
          has to be moved to the pkg Java package.
          The reason: All DOM need to be saved when the package is saved (precondition of 219). To archive this the class
          have to be on the same level as OdfPackage to be able to add itself during construction (constructor) to the collection of DOM the OdfPackage will hold.
          Exact the same principle as the OdfPackageDocument adds itself to the Package (updated the issue 219).
          After the move of this class, I believe only the JarManifest class remains in org.odftoolkit.odfdom.
          all other classes should similar moved to pkg Java package. For example OdfExample would describe XML elements within the package.
          Not certain yet if alien attribute/element classes should remain. (NOTE: This multiple class move should be done after first review to minimize the changes in files. You might even want to change the package of OdfFileDom back before review to avoid so many tiny changes.)

          What is missing...

          The tests:
          I) A test document where other namespace prefixes were used than usual in ODF.
          II) A test document where within a document the similar prefix is defined to a different URI

          The implementation:
          I would suggest to continue on OdfFileDom:
          1) The setNamespace() method needs to be called by SAX Parsing and all DOM creation adding a new namespace.
          2) In addition we might do some collision detection, when a new prefix/URI pair is being added and
          a) the URI was already used:
          Use the existing prefix, neglect the given
          b) the prefix was already used:
          Change the prefix in a symetric manner, e.g. append "_1".
          If the changed prefix was already used and NOT the same URI, incrementing the number to "_2" and loop this.
          We would document this and users have to take care for their attribute values using namespace prefixes.
          I neglected the thought on parsing all attribute values for ':' or the feature of allowing the change of a namespace prefix.
          3) OdfFileDom needs a package level access method to provide the map (the one where the URI is the key - as only doubled prefixes might cause trouble).
          4) OdfContent, etc. would loop over the enum, ie. for(NamespaceName name : OdfDocumentNamespace.values()) and fill the map.

          After this issue, I would ask you to devide the doc.OdfDocument (and subclasses) into dom.OdfDocument (and subclasses).
          (I have created ODFTOOLKIT-158 - "Separation of DOM dependencies of doc.OdfDocument to dom.OdfDocument")

          Only the DOM documents should be aware of XML details as content.xml, etc.
          From pkg.OdfFileDom the following methods have to be moved to dom.OdfDocument (or removed):
          public OdfOfficeAutomaticStyles getOrCreateAutomaticStyles()
          public OdfOfficeAutomaticStyles getAutomaticStyles()
          public OdfOfficeStyles getOfficeStyles()

          We have to move those, as no user would like to care if he has the automatic styles of the content.xml or the styles.xml, he would only access these information at the OdfDocument. In addition the pkg layer is not allowed to access classes from the upper ODF XML layer.

          An interesting question will become obvious (again) how DOM to DOC documents access each other? Both are the entry points for the DOM and DOC API. Most likely by composition having references to each other (or creating them on demand), while DOM is created during loading anyway.

          Show
          Svante Schubert added a comment - Hi Daisy, Rob asked to move the style handling in separated task, some for you some for me. I have done some style depending changes with this issue and worked several days on the design of this issue. Could you give me an early feed-back and help me on the tests and if you are able on the implementation? I would continue on the implementation tomorrow, pinging you in IRC. I believe the design is quite stable now. Still some of the implementation are missing and therefore tests are failing. BTW I fixed an issue in the XPath/Slide test making the problem visible, making it fail. The design: OdfNamespaceContext was removed. The OdfFileDom is now implementing the interface NamespaceContext, embracing the get methods from the XPath required interface and in addition set methods: public void setNamespace(String prefix, String uri) public void setNamespace(NamespaceName name) to add new namespaces during SAX parsing and DOM node creation. The specific ODF subclasses of org.odftoolkit.odfdom.dom. OdfContentDom, OdfStylesDom, OdfMetaDom and OdfSettingsDom and the later following org.odftoolkit.odfdom.pkg.manifest OdfManifestDom will initialize the Namespace mapping map with its ODF namespaces taken from enums generated by source code generation. (During this redesign the namespace enum ..dom.OdfNamespaceNames has been renamed to ..dom.OdfDocumentNamespace, making it easy to distinguish to the upcoming pkg.manifest.OdfPackageNamespace - see issue 71, e.g. OdfDocumentNamespace.OFFICE) These mapping are made persistent as XML during the save of a OdfFileDom within the OdfPackage. The mapping will be saved as namespace attributes to the root element. Note: The OdfFileDom subclasses are now the entry point for each DOM tree, e.g. for OdfContentDom public OfficeDocumentContentElement getRootElement() Following additional changes/suggestions: 1) org.odftoolkit.odfdom.pkg.OdfXMLHelper.java was only used by tests and moved to test package, as the class is more a tool based on ODFDOM than a required part of ODFDOM. 2) Not used/required and removed for simplification: org.odftoolkit.odfdom.dom.OdfAttributeNames.java org.odftoolkit.odfdom.pkg.OdfPackageStream org.odftoolkit.odfdom.pkg.OdfPackageResource.java 3) The class org.odftoolkit.odfdom.OdfFileDom has to be moved to the pkg Java package. The reason: All DOM need to be saved when the package is saved (precondition of 219). To archive this the class have to be on the same level as OdfPackage to be able to add itself during construction (constructor) to the collection of DOM the OdfPackage will hold. Exact the same principle as the OdfPackageDocument adds itself to the Package (updated the issue 219). After the move of this class, I believe only the JarManifest class remains in org.odftoolkit.odfdom. all other classes should similar moved to pkg Java package. For example OdfExample would describe XML elements within the package. Not certain yet if alien attribute/element classes should remain. (NOTE: This multiple class move should be done after first review to minimize the changes in files. You might even want to change the package of OdfFileDom back before review to avoid so many tiny changes.) What is missing... The tests: I) A test document where other namespace prefixes were used than usual in ODF. II) A test document where within a document the similar prefix is defined to a different URI The implementation: I would suggest to continue on OdfFileDom: 1) The setNamespace() method needs to be called by SAX Parsing and all DOM creation adding a new namespace. 2) In addition we might do some collision detection, when a new prefix/URI pair is being added and a) the URI was already used: Use the existing prefix, neglect the given b) the prefix was already used: Change the prefix in a symetric manner, e.g. append "_1". If the changed prefix was already used and NOT the same URI, incrementing the number to "_2" and loop this. We would document this and users have to take care for their attribute values using namespace prefixes. I neglected the thought on parsing all attribute values for ':' or the feature of allowing the change of a namespace prefix. 3) OdfFileDom needs a package level access method to provide the map (the one where the URI is the key - as only doubled prefixes might cause trouble). 4) OdfContent, etc. would loop over the enum, ie. for(NamespaceName name : OdfDocumentNamespace.values()) and fill the map. After this issue, I would ask you to devide the doc.OdfDocument (and subclasses) into dom.OdfDocument (and subclasses). (I have created ODFTOOLKIT-158 - "Separation of DOM dependencies of doc.OdfDocument to dom.OdfDocument") Only the DOM documents should be aware of XML details as content.xml, etc. From pkg.OdfFileDom the following methods have to be moved to dom.OdfDocument (or removed): public OdfOfficeAutomaticStyles getOrCreateAutomaticStyles() public OdfOfficeAutomaticStyles getAutomaticStyles() public OdfOfficeStyles getOfficeStyles() We have to move those, as no user would like to care if he has the automatic styles of the content.xml or the styles.xml, he would only access these information at the OdfDocument. In addition the pkg layer is not allowed to access classes from the upper ODF XML layer. An interesting question will become obvious (again) how DOM to DOC documents access each other? Both are the entry points for the DOM and DOC API. Most likely by composition having references to each other (or creating them on demand), while DOM is created during loading anyway.
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=356)
          ZIP of Design review patch - implementation missing, test fail

          In addition to the previous mentioned missing changes the code generation have to be adapted to generate the new source code.

          Note: The office formula namespace of is not part of the schema and will not be generated into OdfDocumentNamespace. It have to be explicitly addressed in generation clauses.

          Show
          Svante Schubert added a comment - Created an attachment (id=356) ZIP of Design review patch - implementation missing, test fail In addition to the previous mentioned missing changes the code generation have to be adapted to generate the new source code. Note: The office formula namespace of is not part of the schema and will not be generated into OdfDocumentNamespace. It have to be explicitly addressed in generation clauses.
          Hide
          Ying Chun Guo added a comment -

          Hi, Svante

          It's really a big patch, and thank you for your work. See my questions and comments below.

          (1) I agree with all your changes about name spaces. Just remind one thing, I think we need to make sure all namespaces in xml file should be added to the corresponding instances of OdfFileDom, including the foreign namespaces.

          (2) I see you separate OdfFileDom into several sub classes: OdfContentDom, OdfStyleDom and etc. But I don't see the much differences in these sub classes besides getRootElement(). Can you explain more about why you introduce sub classes of OdfFileDom and what kind of functions you want to put in these sub classes?

          (3) I see you put OdfFileDom under pkg layer. I agree that OdfFileDom should be moved out of package org.odftoolkit.odfdom, and getOrCreateAutomaticStyles() getAutomaticStyles() and getOfficeStyles() should be moved out of OdfFileDom. But I think it's better to put OdfFileDom under dom package, in the same package with the its 4 sub classes. OdfFileDom extends DocumentImpl, which is obviously in DOM scope, so I personally think it is more reasonable to put OdfFileDom in package org.odftoolkit.odfdom.dom. I read your comments about why moving OdfFileDom to pkg layer, but I cannot understand clearly.

          (4) In OdfPresentationDocument, method "copyForeignLinkRef(OdfElement sourceCloneEle)", do you think the following codes:

          OdfFileDom fileDom = (OdfFileDom) sourceCloneEle.getOwnerDocument();
          XPath xpath;
          if(fileDom instanceof OdfContentDom)

          { xpath = ((OdfContentDom)fileDom).getXPath(); }

          else

          { xpath = ((OdfStylesDom)fileDom).getXPath(); }

          OdfPackageDocument srcDoc = fileDom.getDocument();

          can be changed to:
          OdfFileDom fileDom = (OdfFileDom) sourceCloneEle.getOwnerDocument();
          XPath xpath;
          xpath = fileDom.getXPath();

          Regards
          Daisy

          Show
          Ying Chun Guo added a comment - Hi, Svante It's really a big patch, and thank you for your work. See my questions and comments below. (1) I agree with all your changes about name spaces. Just remind one thing, I think we need to make sure all namespaces in xml file should be added to the corresponding instances of OdfFileDom, including the foreign namespaces. (2) I see you separate OdfFileDom into several sub classes: OdfContentDom, OdfStyleDom and etc. But I don't see the much differences in these sub classes besides getRootElement(). Can you explain more about why you introduce sub classes of OdfFileDom and what kind of functions you want to put in these sub classes? (3) I see you put OdfFileDom under pkg layer. I agree that OdfFileDom should be moved out of package org.odftoolkit.odfdom, and getOrCreateAutomaticStyles() getAutomaticStyles() and getOfficeStyles() should be moved out of OdfFileDom. But I think it's better to put OdfFileDom under dom package, in the same package with the its 4 sub classes. OdfFileDom extends DocumentImpl, which is obviously in DOM scope, so I personally think it is more reasonable to put OdfFileDom in package org.odftoolkit.odfdom.dom. I read your comments about why moving OdfFileDom to pkg layer, but I cannot understand clearly. (4) In OdfPresentationDocument, method "copyForeignLinkRef(OdfElement sourceCloneEle)", do you think the following codes: OdfFileDom fileDom = (OdfFileDom) sourceCloneEle.getOwnerDocument(); XPath xpath; if(fileDom instanceof OdfContentDom) { xpath = ((OdfContentDom)fileDom).getXPath(); } else { xpath = ((OdfStylesDom)fileDom).getXPath(); } OdfPackageDocument srcDoc = fileDom.getDocument(); can be changed to: OdfFileDom fileDom = (OdfFileDom) sourceCloneEle.getOwnerDocument(); XPath xpath; xpath = fileDom.getXPath(); Regards Daisy
          Hide
          Svante Schubert added a comment -

          1) OdfFileDom should indeed fulfill these requirements. In addition it should be able to cope with new added Namespaces that have similar URI or prefix as existent. I call it collision detection and prevention.

          2) The subclasses are for type safety. They explicitly return their specific parent elemement and they are returning the XML layer OdfDocument instead of an OdfPackageDocument when calling getOdfDocument() - should be getDocument().

          3) OdfFileDom will add namespace handling for XML files. This functionality should be available for application that are only using ODF PKG and not the ODF XML layer, for instance audio books. Therefore it is necessary that OdfFileDom is not in the dom java package, but in a java package of the ODF pkg layer.
          There are two reasons to write the class into the package layer:
          a) The OdfFileDom constructor should add the new object to the OdfPackage, so it can be saved when the package is being saved.
          This adding functionality should not be public, but package level access.
          b) When the DOM is being saved by the package, all namespace pairs have to be written as XML namespace attributes to the root element (e.g. xmlns:office="urn:oasis:names:tc:opendocument:xmlns:office:1.0")
          The OdfPackage have to iterate over the complete Namespace Map of OdfFileDom.
          The access on the namespace map of OdfFileDom should not be public, but package level access.

          4) I had to fix in OdfPresentationDocument, method "copyForeignLinkRef(OdfElement
          sourceCloneEle)" manually. At one time it is a styles.xml you are calling.
          Try to fix it yourself, you will run into a ClassCastexception
          Is there a way to get rid of this function? The need is uncertain to me the method seems a little complex. Perhaps we might solve the problem the function is handling in a different way?

          Show
          Svante Schubert added a comment - 1) OdfFileDom should indeed fulfill these requirements. In addition it should be able to cope with new added Namespaces that have similar URI or prefix as existent. I call it collision detection and prevention. 2) The subclasses are for type safety. They explicitly return their specific parent elemement and they are returning the XML layer OdfDocument instead of an OdfPackageDocument when calling getOdfDocument() - should be getDocument(). 3) OdfFileDom will add namespace handling for XML files. This functionality should be available for application that are only using ODF PKG and not the ODF XML layer, for instance audio books. Therefore it is necessary that OdfFileDom is not in the dom java package, but in a java package of the ODF pkg layer. There are two reasons to write the class into the package layer: a) The OdfFileDom constructor should add the new object to the OdfPackage, so it can be saved when the package is being saved. This adding functionality should not be public, but package level access. b) When the DOM is being saved by the package, all namespace pairs have to be written as XML namespace attributes to the root element (e.g. xmlns:office="urn:oasis:names:tc:opendocument:xmlns:office:1.0") The OdfPackage have to iterate over the complete Namespace Map of OdfFileDom. The access on the namespace map of OdfFileDom should not be public, but package level access. 4) I had to fix in OdfPresentationDocument, method "copyForeignLinkRef(OdfElement sourceCloneEle)" manually. At one time it is a styles.xml you are calling. Try to fix it yourself, you will run into a ClassCastexception Is there a way to get rid of this function? The need is uncertain to me the method seems a little complex. Perhaps we might solve the problem the function is handling in a different way?
          Hide
          Svante Schubert added a comment -

          Hi Daisy,

          as it is a complicated patch, I would like to release often.
          Please make a diff (e.g. using tools as TotalCommander (Menu:Commands-Synchronize Dirs) to sources with this and last patch applied.
          1) Added the collision handling for OdfFileDom, when an existing URI or prefix is being added. It is simple in the beginning always appending "_1" to prefix
          2) Added the namespace handling to OdfXMLFactory (add namespace to file whenever an alien attribute/element) is encountered
          3) Adapted the adding of new namespaces in OdfDocument (no longer add them by creating an OdfNamespace, but by explicitly add them to OdfFileDom)
          4) Added ODF 1.2 XML namespace initializiation to our four OdfFileDom subclasses
          5) Removed exceptions from OdfPackage methods signatures, with the idea in mind to provide a robust loading behavior. Even problematic packages should be loaded if possible.
          6) I adapted the test of SlideTest.testCopyThreeDoc() to compare the draw:name of each copied slide instead its automatic style, which might change when the test file is being opened and saved once.

          Could you please review it, I might add additional tests. Like a test document using daisy:document instead of office:document, still using the same namespace for both prefix.

          Regards,
          Svante

          Show
          Svante Schubert added a comment - Hi Daisy, as it is a complicated patch, I would like to release often. Please make a diff (e.g. using tools as TotalCommander (Menu:Commands-Synchronize Dirs) to sources with this and last patch applied. 1) Added the collision handling for OdfFileDom, when an existing URI or prefix is being added. It is simple in the beginning always appending "_1" to prefix 2) Added the namespace handling to OdfXMLFactory (add namespace to file whenever an alien attribute/element) is encountered 3) Adapted the adding of new namespaces in OdfDocument (no longer add them by creating an OdfNamespace, but by explicitly add them to OdfFileDom) 4) Added ODF 1.2 XML namespace initializiation to our four OdfFileDom subclasses 5) Removed exceptions from OdfPackage methods signatures, with the idea in mind to provide a robust loading behavior. Even problematic packages should be loaded if possible. 6) I adapted the test of SlideTest.testCopyThreeDoc() to compare the draw:name of each copied slide instead its automatic style, which might change when the test file is being opened and saved once. Could you please review it, I might add additional tests. Like a test document using daisy:document instead of office:document, still using the same namespace for both prefix. Regards, Svante
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=358)
          179 - implementation ready, all test run smooth, perhaps further tests will be added

          I might add some test before our meeting. If you review it earlier, you might do a quick diff to this one.

          Thanks,
          Svante

          Show
          Svante Schubert added a comment - Created an attachment (id=358) 179 - implementation ready, all test run smooth, perhaps further tests will be added I might add some test before our meeting. If you review it earlier, you might do a quick diff to this one. Thanks, Svante
          Hide
          Ying Chun Guo added a comment -

          Hi, Svante

          Thank you for detail information. I have some small questions below:

          (In reply to comment #32)
          > Hi Daisy,
          > as it is a complicated patch, I would like to release often.
          > Please make a diff (e.g. using tools as TotalCommander
          > (Menu:Commands-Synchronize Dirs) to sources with this and last patch applied.
          > 1) Added the collision handling for OdfFileDom, when an existing URI or prefix
          > is being added. It is simple in the beginning always appending "_1" to prefix
          What to do if collision happens a third time?

          > 2) Added the namespace handling to OdfXMLFactory (add namespace to file
          > whenever an alien attribute/element) is encountered
          > 3) Adapted the adding of new namespaces in OdfDocument (no longer add them by
          > creating an OdfNamespace, but by explicitly add them to OdfFileDom)
          > 4) Added ODF 1.2 XML namespace initializiation to our four OdfFileDom
          > subclasses
          > 5) Removed exceptions from OdfPackage methods signatures, with the idea in mind
          > to provide a robust loading behavior. Even problematic packages should be
          > loaded if possible.
          Have you ever tested problematic packages to be loaded? Do you plan to test?

          > 6) I adapted the test of SlideTest.testCopyThreeDoc() to compare the draw:name
          > of each copied slide instead its automatic style, which might change when the
          > test file is being opened and saved once.
          > Could you please review it, I might add additional tests. Like a test document
          > using daisy:document instead of office:document, still using the same namespace
          > for both prefix.
          I believe additional tests are required. It's a big patch, and changed a lot of things. We need enough test to make sure there are not regression.

          > Regards,
          > Svante

          Thanks and regards
          Daisy

          Show
          Ying Chun Guo added a comment - Hi, Svante Thank you for detail information. I have some small questions below: (In reply to comment #32) > Hi Daisy, > as it is a complicated patch, I would like to release often. > Please make a diff (e.g. using tools as TotalCommander > (Menu:Commands-Synchronize Dirs) to sources with this and last patch applied. > 1) Added the collision handling for OdfFileDom, when an existing URI or prefix > is being added. It is simple in the beginning always appending "_1" to prefix What to do if collision happens a third time? > 2) Added the namespace handling to OdfXMLFactory (add namespace to file > whenever an alien attribute/element) is encountered > 3) Adapted the adding of new namespaces in OdfDocument (no longer add them by > creating an OdfNamespace, but by explicitly add them to OdfFileDom) > 4) Added ODF 1.2 XML namespace initializiation to our four OdfFileDom > subclasses > 5) Removed exceptions from OdfPackage methods signatures, with the idea in mind > to provide a robust loading behavior. Even problematic packages should be > loaded if possible. Have you ever tested problematic packages to be loaded? Do you plan to test? > 6) I adapted the test of SlideTest.testCopyThreeDoc() to compare the draw:name > of each copied slide instead its automatic style, which might change when the > test file is being opened and saved once. > Could you please review it, I might add additional tests. Like a test document > using daisy:document instead of office:document, still using the same namespace > for both prefix. I believe additional tests are required. It's a big patch, and changed a lot of things. We need enough test to make sure there are not regression. > Regards, > Svante Thanks and regards Daisy
          Hide
          Svante Schubert added a comment -

          Hi Daisy,

          as suspected there were problems when loading a valid ODF document using a different XML prefix than the one in OpenOffice.
          These problems already existed prior this patch and were now only revealed.

          The reason:
          In OdfXMLFactory the class to load e.g. OfficeBodyElement was based on the prefix found during loading (SAX event).

          The solution:
          To loose no performance I added a fall-back approach to OdfXMLFactory.. Whenever no class was found, e.g. there is an element called <daisy:document> instead of <office:document>.
          I add this new namespace to the OdfFileDom, which realizes due to the collision detection that there is already a different prefix for this URI.
          It realizes the collision, as the OdfFileDom is in reality a OdfContentDom, which was initialized with the ODF 1.2 namespaces.
          When realizing the collision, it will return the 'correct' OdfName.

          During review I realized the OdfXmlFactory for get/set for the type DOM class to be loaded (e.g. <office:document> would return OfficeDocumentElement) does not work, neither.
          Even more the static map for all classes on the server have to be questioned.
          My approach was to remove this questionable feature for the 0.9 release and made the APIs private and disabled the test.

          It took me hours to get the correct a solution for multiple identical namespace prefixes within a file (see OdfFileDom source), tested them for attributes and elements.
          I have already adapted the source code generation and took the liberty to deactivate the generation of OdfDocumentNamespace.

          Please do me the favor and make a performance test and build under UNIX and JDK5.

          What might be missing. I am uncertain about the exception handling for OdfPackage (shall we remove them, when do they make sense?)
          Perhaps Loading and Saving should work as much as possible, working on the document should give clues via exceptions?
          Open for suggestions.

          Two further tasks you might help me in, during my work with the API in tests, I find it annoying to cast to a specific document after loading on the specific document.
          Perhaps we should shadow the static method to avoid the casting like:

          /**

          • Creates an OdfPresentationDocument from an input stream.
            *
          • @param inStream - the InputStream of the ODF document.
          • @return the document created from the given InputStream
          • @throws java.lang.Exception - if the document could not be created.
            */
            public static OdfPresentationDocument loadDocument(InputStream inStream) throws Exception { return (OdfPresentationDocument) OdfDocument.loadDocument(inStream); }

          This might apply to more functions than above.
          And finally please move all files from org.odftoolkit.odfdom.* to pkg (aside of the JarManifest), which we might call as well JarMetadata.
          I suggest to do this change by a search/replace the package string for each class. Thank you!

          PS: Code generation will follow, the problems of 179 took to long..

          Enjoy my absence
          Svante

          Show
          Svante Schubert added a comment - Hi Daisy, as suspected there were problems when loading a valid ODF document using a different XML prefix than the one in OpenOffice. These problems already existed prior this patch and were now only revealed. The reason: In OdfXMLFactory the class to load e.g. OfficeBodyElement was based on the prefix found during loading (SAX event). The solution: To loose no performance I added a fall-back approach to OdfXMLFactory.. Whenever no class was found, e.g. there is an element called <daisy:document> instead of <office:document>. I add this new namespace to the OdfFileDom, which realizes due to the collision detection that there is already a different prefix for this URI. It realizes the collision, as the OdfFileDom is in reality a OdfContentDom, which was initialized with the ODF 1.2 namespaces. When realizing the collision, it will return the 'correct' OdfName. During review I realized the OdfXmlFactory for get/set for the type DOM class to be loaded (e.g. <office:document> would return OfficeDocumentElement) does not work, neither. Even more the static map for all classes on the server have to be questioned. My approach was to remove this questionable feature for the 0.9 release and made the APIs private and disabled the test. It took me hours to get the correct a solution for multiple identical namespace prefixes within a file (see OdfFileDom source), tested them for attributes and elements. I have already adapted the source code generation and took the liberty to deactivate the generation of OdfDocumentNamespace. Please do me the favor and make a performance test and build under UNIX and JDK5. What might be missing. I am uncertain about the exception handling for OdfPackage (shall we remove them, when do they make sense?) Perhaps Loading and Saving should work as much as possible, working on the document should give clues via exceptions? Open for suggestions. Two further tasks you might help me in, during my work with the API in tests, I find it annoying to cast to a specific document after loading on the specific document. Perhaps we should shadow the static method to avoid the casting like: /** Creates an OdfPresentationDocument from an input stream. * @param inStream - the InputStream of the ODF document. @return the document created from the given InputStream @throws java.lang.Exception - if the document could not be created. */ public static OdfPresentationDocument loadDocument(InputStream inStream) throws Exception { return (OdfPresentationDocument) OdfDocument.loadDocument(inStream); } This might apply to more functions than above. And finally please move all files from org.odftoolkit.odfdom.* to pkg (aside of the JarManifest), which we might call as well JarMetadata. I suggest to do this change by a search/replace the package string for each class. Thank you! PS: Code generation will follow, the problems of 179 took to long.. Enjoy my absence Svante
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=360)
          XPATH enabling further tests, fixes and code generation included

          Show
          Svante Schubert added a comment - Created an attachment (id=360) XPATH enabling further tests, fixes and code generation included
          Hide
          Svante Schubert added a comment -

          Resolved Fixed, I will be on vacation till Tuesday..

          Show
          Svante Schubert added a comment - Resolved Fixed, I will be on vacation till Tuesday..
          Hide
          Ying Chun Guo added a comment -

          Hi, Svante

          I reviewed your patch. I'm OK with your current updates. See some comments below:

          (1) Why do you change "OdfXMLFactory.setOdfElementClass" from public to private? I see you remove some unit test code: FactoryManipulationTest.testFactoryManipulation(). So we won't support this function from now on?

          (2) you want me to add new loadDocument() methods in sub classes of OdfDocument, e.g public static OdfPresentationDocument loadDocument(InputStream inStream). If we have such method, OdfPresentation will have 6 loadDocument():
          OdfPresentationDocument loadDocument(InputStream inStream)
          OdfPresentationDocument loadDocument(String documentPath)
          OdfPresentationDocument loadDocument(File file)
          OdfDocument loadDocument(InputStream inStream)
          OdfDocument loadDocument(String documentPath)
          OdfDocument loadDocument(File file)

          Some of them are inherited from OdfDocumet, some of them are created in OdfPresentationDocument. I wonder if users will feel strange to 6 loadDocument() methods, some with same parameter but different return type.

          (3) It's easily to move all files from org.odftoolkit.odfdom.* to pkg (aside of
          the JarManifest), but I wonder if it is reasonable. Sorry I argue with you again about packaging.

          "The ODF Package API covers all features from the third part of the ODF 1.2 specification defining the ODF Package features. The ODF 1.2 package features are build on top technologies as ZIP package handling, W3C encryption, W3C signature and W3C metadata. "

          Are OdfXMLFactory, OdfNamespace, OdfName, OdfAttribute, and OdfElement defined in the third part of ODF 1.2 specifications?

          Looking forward to your answer.

          Regards
          Daisy

          Show
          Ying Chun Guo added a comment - Hi, Svante I reviewed your patch. I'm OK with your current updates. See some comments below: (1) Why do you change "OdfXMLFactory.setOdfElementClass" from public to private? I see you remove some unit test code: FactoryManipulationTest.testFactoryManipulation(). So we won't support this function from now on? (2) you want me to add new loadDocument() methods in sub classes of OdfDocument, e.g public static OdfPresentationDocument loadDocument(InputStream inStream). If we have such method, OdfPresentation will have 6 loadDocument(): OdfPresentationDocument loadDocument(InputStream inStream) OdfPresentationDocument loadDocument(String documentPath) OdfPresentationDocument loadDocument(File file) OdfDocument loadDocument(InputStream inStream) OdfDocument loadDocument(String documentPath) OdfDocument loadDocument(File file) Some of them are inherited from OdfDocumet, some of them are created in OdfPresentationDocument. I wonder if users will feel strange to 6 loadDocument() methods, some with same parameter but different return type. (3) It's easily to move all files from org.odftoolkit.odfdom.* to pkg (aside of the JarManifest), but I wonder if it is reasonable. Sorry I argue with you again about packaging. "The ODF Package API covers all features from the third part of the ODF 1.2 specification defining the ODF Package features. The ODF 1.2 package features are build on top technologies as ZIP package handling, W3C encryption, W3C signature and W3C metadata. " Are OdfXMLFactory, OdfNamespace, OdfName, OdfAttribute, and OdfElement defined in the third part of ODF 1.2 specifications? Looking forward to your answer. Regards Daisy
          Hide
          Svante Schubert added a comment -

          Hi Daisy,

          It would be great if you could do the performance and JDK 5 test and take a look at possible JavaDoc problems during generation (minor typos).

          Going to do a Unix check tomorrow, if you agree on pkg renaming, you might adapt it in this issue or write a follow up issue.

          We should have pkg refactoring done before our next snapshot.

          Thanks,
          Svante

          Show
          Svante Schubert added a comment - Hi Daisy, It would be great if you could do the performance and JDK 5 test and take a look at possible JavaDoc problems during generation (minor typos). Going to do a Unix check tomorrow, if you agree on pkg renaming, you might adapt it in this issue or write a follow up issue. We should have pkg refactoring done before our next snapshot. Thanks, Svante
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=363)
          ZIP with XPATH enabling further tests, fixes and code generation included

          Hi Daisy,

          I have fixed the JavaDoc. During this fix, I have realized an issue in
          OdfFileDom.getPrefixes(NamespaceURI), fixed it and added a test for it.

          Finally as discussed in our call today, I did the final pkg refactorings to have a stable target for Devin and myself in our source code generation task.
          Adapted the existing source code generation as well.

          It has been tested under Windows and Unix, please test with JDK5.
          From my side this issue is finally to be pushed to the master.

          Thanks,
          Svante

          Show
          Svante Schubert added a comment - Created an attachment (id=363) ZIP with XPATH enabling further tests, fixes and code generation included Hi Daisy, I have fixed the JavaDoc. During this fix, I have realized an issue in OdfFileDom.getPrefixes(NamespaceURI), fixed it and added a test for it. Finally as discussed in our call today, I did the final pkg refactorings to have a stable target for Devin and myself in our source code generation task. Adapted the existing source code generation as well. It has been tested under Windows and Unix, please test with JDK5. From my side this issue is finally to be pushed to the master. Thanks, Svante
          Hide
          Ying Chun Guo added a comment -

          Created an attachment (id=366)
          Revise one error, add one failure test, add loadDocument() methods based on Svante's version

          Hi, Svante

          In this patch, I did following changes.

          (1) Add 3 loadDocument() methods to sub classes of OdfDocument to avoid class cast when loading documents.

          (2) Fix an exception happened under IBM JDK. The exception is "java.lang.IllegalArgumentException: parameter is invalid for datatype duration". The reason is caused by initializing an object of javax.xml.datatype.Duration with zero, which is not allowed by IBM JDK. The duration time is calcuated by the saving time plus the openning time, sometimes the duration time is so short that it is zero. I reset the duration time to one if it is zero.

          (3) Change OdfFileDom.getNamespacesByURI() to public in order to disclose a bug. Sometimes, a 'xlmns:null=""' is added to the namespace of an output ODF documents. IBM JDK doesn't allow 'xmlns:null=""', so that I found this issue. But I didn't debug it. Leave it to yourself to debug. If you correct it, you can change OdfFileDom.getNamespacesByURI() to package level.

          Some unit tests code are added in org.odftoolkit.odfdom.doc.LoadSaveTest. You can easily reproduce this issue.

          Till now, there are still three exceptions left.
          (1) java.lang.NullPointerException
          at org.odftoolkit.odfdom.doc.DocumentCreationTest.accessEmbeddedWithinEmbeddedDocs(DocumentCreationTest.java:306)

          It will happen under Sun JDK 5, IBM JDK 5 and 6. It's a new exception for me. I guess, it should be a problem of code. So I suggest you to debug under a Sun JDK 1.5.

          (2) java.lang.NullPointerException
          at org.apache.xml.serializer.dom3.DOM3TreeWalker.isAttributeWellFormed(DOM3TreeWalker.java:1566)

          It will happen under IBM JDK 5 and 6. It's an old exception for me. I think ODFTOOLKIT-133 in bugzilla is caused by this exception too. I'm not sure if it is a bug of IBM JDK. Or maybe IBM JDK does strict validation than Sun JDK.

          (3) java.io.IOException: Access is denied
          at java.io.WinNTFileSystem.createFileExclusively(Native Method)

          It will happen under SUN JDK5. It's an old exception for me too. I met with this exception on 02/11/2010, and we had exchanged some personal mails to discuss about it. I think it may be a bug of Sun JDK5. BTW, I run it under a Windows XP OS. I don't know if it will reproduce under other OS.

          If you can fix the first exception, I'm OK if you push this patch to main repository, because I think exception 2 and 3 is not easy to handle. We may create new issues to follow these two exceptions.

          Regards
          Daisy

          Show
          Ying Chun Guo added a comment - Created an attachment (id=366) Revise one error, add one failure test, add loadDocument() methods based on Svante's version Hi, Svante In this patch, I did following changes. (1) Add 3 loadDocument() methods to sub classes of OdfDocument to avoid class cast when loading documents. (2) Fix an exception happened under IBM JDK. The exception is "java.lang.IllegalArgumentException: parameter is invalid for datatype duration". The reason is caused by initializing an object of javax.xml.datatype.Duration with zero, which is not allowed by IBM JDK. The duration time is calcuated by the saving time plus the openning time, sometimes the duration time is so short that it is zero. I reset the duration time to one if it is zero. (3) Change OdfFileDom.getNamespacesByURI() to public in order to disclose a bug. Sometimes, a 'xlmns:null=""' is added to the namespace of an output ODF documents. IBM JDK doesn't allow 'xmlns:null=""', so that I found this issue. But I didn't debug it. Leave it to yourself to debug. If you correct it, you can change OdfFileDom.getNamespacesByURI() to package level. Some unit tests code are added in org.odftoolkit.odfdom.doc.LoadSaveTest. You can easily reproduce this issue. Till now, there are still three exceptions left. (1) java.lang.NullPointerException at org.odftoolkit.odfdom.doc.DocumentCreationTest.accessEmbeddedWithinEmbeddedDocs(DocumentCreationTest.java:306) It will happen under Sun JDK 5, IBM JDK 5 and 6. It's a new exception for me. I guess, it should be a problem of code. So I suggest you to debug under a Sun JDK 1.5. (2) java.lang.NullPointerException at org.apache.xml.serializer.dom3.DOM3TreeWalker.isAttributeWellFormed(DOM3TreeWalker.java:1566) It will happen under IBM JDK 5 and 6. It's an old exception for me. I think ODFTOOLKIT-133 in bugzilla is caused by this exception too. I'm not sure if it is a bug of IBM JDK. Or maybe IBM JDK does strict validation than Sun JDK. (3) java.io.IOException: Access is denied at java.io.WinNTFileSystem.createFileExclusively(Native Method) It will happen under SUN JDK5. It's an old exception for me too. I met with this exception on 02/11/2010, and we had exchanged some personal mails to discuss about it. I think it may be a bug of Sun JDK5. BTW, I run it under a Windows XP OS. I don't know if it will reproduce under other OS. If you can fix the first exception, I'm OK if you push this patch to main repository, because I think exception 2 and 3 is not easy to handle. We may create new issues to follow these two exceptions. Regards Daisy
          Hide
          Ying Chun Guo added a comment -

          Assign to Svante.

          Show
          Ying Chun Guo added a comment - Assign to Svante.
          Hide
          Ying Chun Guo added a comment -

          Created an attachment (id=367)
          Outputs of mvn under Sun JDK 5 and 6, IBM JDK 5 and 6

          I uploaded the outputs of mvn for you to reference.

          Show
          Ying Chun Guo added a comment - Created an attachment (id=367) Outputs of mvn under Sun JDK 5 and 6, IBM JDK 5 and 6 I uploaded the outputs of mvn for you to reference.
          Hide
          Svante Schubert added a comment -

          Hi Daisy,

          thanks for the update on 1 and 2.
          I fixed 3)
          You could easily fixed 3) as well, by adding two lines
          if(uri == null)
          System.out.println("ERROR");
          in the OdfFileDom method filling the uri and giving a breakpoint on the system.out.

          With your IDE you can go via the method call stack to OdfXmlFactory.newOdfElement(..) and see that
          String oldPrefix = name.getPrefix();
          OdfName adaptedName = addNamespaceToDom(name, dom);

          is called even for XML nodes without a namespace (ie. my test XML <foreign>). This was an easy fix.

          BUT what you have revealed is that we have to make hidden method public to run the test. THIS is weired, certainly we want to keep the test AND keep the method hidden.

          The correct way is certainly to have a loadSaveTest in the pkg test folder, what I tried copying the doc test into it and using only PKG functions.

          It is not possible! A good catch, I am working to make it possible otherwise the API is not usable for PKG only access, which we need in our tests.
          Only next time you might point such problem out right away

          You see an API have to be used to see its limitation.

          Thanks,
          Svante

          Show
          Svante Schubert added a comment - Hi Daisy, thanks for the update on 1 and 2. I fixed 3) You could easily fixed 3) as well, by adding two lines if(uri == null) System.out.println("ERROR"); in the OdfFileDom method filling the uri and giving a breakpoint on the system.out. With your IDE you can go via the method call stack to OdfXmlFactory.newOdfElement(..) and see that String oldPrefix = name.getPrefix(); OdfName adaptedName = addNamespaceToDom(name, dom); is called even for XML nodes without a namespace (ie. my test XML <foreign>). This was an easy fix. BUT what you have revealed is that we have to make hidden method public to run the test. THIS is weired, certainly we want to keep the test AND keep the method hidden. The correct way is certainly to have a loadSaveTest in the pkg test folder, what I tried copying the doc test into it and using only PKG functions. It is not possible! A good catch, I am working to make it possible otherwise the API is not usable for PKG only access, which we need in our tests. Only next time you might point such problem out right away You see an API have to be used to see its limitation. Thanks, Svante
          Hide
          Ying Chun Guo added a comment -

          Created an attachment (id=368)
          A new patch with two issues fixed

          Hi, Svante

          This is the new patch. In this patch,
          (1) I merged part of code in the private patch you sent to me, to fix the issue related with 'xlmns:null=""'. I changed OdfFileDom.getNamespacesByURI() back to package level, and removed the new added test codes in doc.LoadSaveTest.

          I noticed in your private patch, you added a new LoadSaveTest to package "pkg". I didn't do that because pkg.LoadSaveTest covers more changes in your private package. I want to make this patch as small as possible and push it as soon as possible.

          (2) I fixed Exception #1 in comment #41. It is caused by a logic error in test code.

          Till now, two exceptions left in this patch in Sun JDK 5 and IBM JDK 5 and 6. They are "java.io.IOException: Access is denied" and "java.lang.NullPointerException at org.apache.xml.serializer.dom3.DOM3TreeWalker.isAttributeWellFormed(DOM3TreeWalker.java:1566)". I tried to figure out the reason, but failed. I think these two exceptions might need more time. I would suggest to push this patch and create two new issues to follow on these two exceptions.

          For your changes in DOM and PKG layer about embeded document, how about you create a new issue and a new patch? A bigger and bigger patch is not easy to review and test.

          What is your opinion?
          Daisy

          Show
          Ying Chun Guo added a comment - Created an attachment (id=368) A new patch with two issues fixed Hi, Svante This is the new patch. In this patch, (1) I merged part of code in the private patch you sent to me, to fix the issue related with 'xlmns:null=""'. I changed OdfFileDom.getNamespacesByURI() back to package level, and removed the new added test codes in doc.LoadSaveTest. I noticed in your private patch, you added a new LoadSaveTest to package "pkg". I didn't do that because pkg.LoadSaveTest covers more changes in your private package. I want to make this patch as small as possible and push it as soon as possible. (2) I fixed Exception #1 in comment #41. It is caused by a logic error in test code. Till now, two exceptions left in this patch in Sun JDK 5 and IBM JDK 5 and 6. They are "java.io.IOException: Access is denied" and "java.lang.NullPointerException at org.apache.xml.serializer.dom3.DOM3TreeWalker.isAttributeWellFormed(DOM3TreeWalker.java:1566)". I tried to figure out the reason, but failed. I think these two exceptions might need more time. I would suggest to push this patch and create two new issues to follow on these two exceptions. For your changes in DOM and PKG layer about embeded document, how about you create a new issue and a new patch? A bigger and bigger patch is not easy to review and test. What is your opinion? Daisy
          Hide
          Svante Schubert added a comment -

          Thanks for the update, Daisy.

          1) Are those exceptions occur for the previous revision as well?If so we should fix them now.
          2) I will review your latest changes in the patch
          3) Still we have to fix getPrefixes(String URI).
          4) For the other changes, I will reopen the embedded document feature issue, as embedding documents only works for ODF 1.2 spec part1 documents (XML spec, our XML Layer), although the feature should work in the package layer as it is a Odf 1.2 part3 package feature.

          Regards,
          Svante

          Show
          Svante Schubert added a comment - Thanks for the update, Daisy. 1) Are those exceptions occur for the previous revision as well?If so we should fix them now. 2) I will review your latest changes in the patch 3) Still we have to fix getPrefixes(String URI). 4) For the other changes, I will reopen the embedded document feature issue, as embedding documents only works for ODF 1.2 spec part1 documents (XML spec, our XML Layer), although the feature should work in the package layer as it is a Odf 1.2 part3 package feature. Regards, Svante
          Hide
          Svante Schubert added a comment -

          The following fixes in the patch that follows:

          1) I added to the XPathTest the nullpointer scenario - using in the test document an element and attribute without namespace.
          2) Fixed the getPrefixes(String uri) function of the NamespaceContext interface in sources and test.
          3) Added further documentation to the XPath test - copy/paste of the relevant XML of the test document.

          Please review and push the test.
          You mentioned that the occuring JDK 5 issues happened even without the patch, please fix or write follow up issues and paste the issue numbers into the issue.

          Thanks in advance,
          Svante

          Show
          Svante Schubert added a comment - The following fixes in the patch that follows: 1) I added to the XPathTest the nullpointer scenario - using in the test document an element and attribute without namespace. 2) Fixed the getPrefixes(String uri) function of the NamespaceContext interface in sources and test. 3) Added further documentation to the XPath test - copy/paste of the relevant XML of the test document. Please review and push the test. You mentioned that the occuring JDK 5 issues happened even without the patch, please fix or write follow up issues and paste the issue numbers into the issue. Thanks in advance, Svante
          Hide
          Svante Schubert added a comment -

          Created an attachment (id=369)
          #ODFTOOLKIT-110# Enabling JDK XPath for all XML files of the package

          ZIP including the patch

          Show
          Svante Schubert added a comment - Created an attachment (id=369) # ODFTOOLKIT-110 # Enabling JDK XPath for all XML files of the package ZIP including the patch
          Hide
          Ying Chun Guo added a comment -

          Hi, Svante

          I'm sure exception #2 and #3 in comment #41 happened without this patch.

          Exception #2. java.lang.NullPointerException at
          org.apache.xml.serializer.dom3.DOM3TreeWalker.isAttributeWellFormed(DOM3TreeWalker.java:1566)

          The status of this issue will be followed in ODFTOOLKIT-133 "NullPointerException sometimes happened when parsing a centain kind of XML under IBM JDK".

          Exception #3. java.io.IOException: Access is denied at java.io.WinNTFileSystem.createFileExclusively(Native Method)

          The status of this issue will be followed in ODFTOOLKIT-161 " "java.io.IOException: Access is denied" sometimes happened under Oracle JDK 1.5"

          I have reviewed your code and tested the patch. If you have no other updates, I will push it.

          Regards
          Daisy

          Show
          Ying Chun Guo added a comment - Hi, Svante I'm sure exception #2 and #3 in comment #41 happened without this patch. Exception #2. java.lang.NullPointerException at org.apache.xml.serializer.dom3.DOM3TreeWalker.isAttributeWellFormed(DOM3TreeWalker.java:1566) The status of this issue will be followed in ODFTOOLKIT-133 "NullPointerException sometimes happened when parsing a centain kind of XML under IBM JDK". Exception #3. java.io.IOException: Access is denied at java.io.WinNTFileSystem.createFileExclusively(Native Method) The status of this issue will be followed in ODFTOOLKIT-161 " "java.io.IOException: Access is denied" sometimes happened under Oracle JDK 1.5" I have reviewed your code and tested the patch. If you have no other updates, I will push it. Regards Daisy

            People

            • Assignee:
              Ying Chun Guo
              Reporter:
              j-wicz
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development