ODF Toolkit
  1. ODF Toolkit
  2. ODFTOOLKIT-104

Create new spreadsheet, save it, load it, add a sheet, save it, then load it causes failure

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: odfdom-0.8.5
    • Fix Version/s: 0.5-incubating
    • Component/s: java
    • Labels:
      None
    • Environment:
      Operating System: Linux
      Platform: PC

      Description

      Created an attachment (id=257)
      Test app demonstrating the problem

      Very basic test of creating a new spreadsheet, saving it, then loading it and adding a new sheet with 2 columns, then saving it, and when loaded again it gives

      java.lang.IllegalArgumentException: Document contains incorrect ODF Mediatype 'null'
      at org.odftoolkit.odfdom.doc.OdfDocument.loadDocument(OdfDocument.java:356)
      at org.odftoolkit.odfdom.doc.OdfDocument.loadDocument(OdfDocument.java:344)
      at org.datanucleus.test.Main.main(Main.java:51)

      Such things never happened with 0.6.16. Sample app attached, and is as below.
      This was reported previously via the forum/mailing list and issue 106 got raised, but that hasn't addressed this basic problem.

      If there is some missing step in the provided app (that makes it work) then please let me know, but the documentation and examples for creating spreadsheets seem lacking.

      OdfDocument doc = null;
      String filename = "test.ods";

      File file = new File(filename);
      System.out.println(">> file=" + file);
      if (!file.exists())

      { // ODF spreadsheet doesn't exist, so create System.out.println(">> OdfDocument.newSpreadsheetDocument()"); doc = OdfSpreadsheetDocument.newSpreadsheetDocument(); System.out.println(">> OdfDocument.save(" + file + ")"); doc.save(file); }

      // Load the document
      System.out.println(">> OdfDocument.loadDocument(" + file + ")");
      doc = OdfDocument.loadDocument(file);

      // Add the table and its columns
      OdfTable table = OdfTable.newTable(doc);
      String sheetName = "FirstSheet";
      table.setTableName(sheetName);

      int numCols = 2;
      for (int i=0;i<numCols;i++)

      { OdfTableColumn col = table.appendColumn(); }

      // Save the document
      System.out.println(">> OdfDocument.save(" + file + ")");
      doc.save(file);

      // Load the document
      System.out.println(">> OdfDocument.loadDocument(" + file + ")");
      doc = OdfDocument.loadDocument(file);

        Activity

        Hide
        Svante Schubert added a comment -

        Closing this issue, as there is neither a test case nor a stakeholder who can verify a problem or test a potential fix.

        Show
        Svante Schubert added a comment - Closing this issue, as there is neither a test case nor a stakeholder who can verify a problem or test a potential fix.
        Hide
        Devin Han added a comment -

        We got a new feedback from user. It looks like the issue he was having with the ODF lockups is due to the JDK version they are running on Linux (JDK1.6.0_18 ).
        And JDK1.6.0_18 seems to have a number of changes regarding garbage collection.

        If they change to an older version JDK (JDK1.6.0_13) or a new version JDK1.6.0_27 (64bit), they could not recreate this issue.

        Anyway, we will continue keeping an eye on it and keep this issue open.

        Devin

        Show
        Devin Han added a comment - We got a new feedback from user. It looks like the issue he was having with the ODF lockups is due to the JDK version they are running on Linux (JDK1.6.0_18 ). And JDK1.6.0_18 seems to have a number of changes regarding garbage collection. If they change to an older version JDK (JDK1.6.0_13) or a new version JDK1.6.0_27 (64bit), they could not recreate this issue. Anyway, we will continue keeping an eye on it and keep this issue open. Devin
        Hide
        Devin Han added a comment -

        Feedback from user, this issue still exists on 64bits Linux plateform, so reopen this issue.

        I found a bug in ODFDOM that maybe the reason of this issue. see the following code from OdfPackage

        // Initialize using memory
        private void initializeZip(InputStream odfStream) throws Exception {
        ByteArrayOutputStream tempBuf = new ByteArrayOutputStream();
        StreamHelper.transformStream(odfStream, tempBuf);
        byte[] mTempByteBuf = tempBuf.toByteArray();
        tempBuf.close(); ---------------------------------------------------------------------------------------------------------->!!!!!!!!!!!!!! odfStream doesn't close
        if (mTempByteBuf.length < 3) {
        OdfValidationException ve = new OdfValidationException(OdfPackageConstraint.PACKAGE_IS_NO_ZIP, getBaseURI());
        if (mErrorHandler != null)

        { mErrorHandler.fatalError(ve); }

        throw new IllegalArgumentException(ve);
        }
        mZipFile = new ZipHelper(this, mTempByteBuf);
        readZip();
        }

        In this method, the opened file stream isn't closed, which will lead OS can't recovery the file handler.

        add the following code to the target:
        // close the input stream to notify OS release file handle.
        odfStream.close();

        the update code has been commited to svn.

        waiting for the user's new feedback to make sure this issue has been fixed.

        Devin

        Show
        Devin Han added a comment - Feedback from user, this issue still exists on 64bits Linux plateform, so reopen this issue. I found a bug in ODFDOM that maybe the reason of this issue. see the following code from OdfPackage // Initialize using memory private void initializeZip(InputStream odfStream) throws Exception { ByteArrayOutputStream tempBuf = new ByteArrayOutputStream(); StreamHelper.transformStream(odfStream, tempBuf); byte[] mTempByteBuf = tempBuf.toByteArray(); tempBuf.close(); ---------------------------------------------------------------------------------------------------------->!!!!!!!!!!!!!! odfStream doesn't close if (mTempByteBuf.length < 3) { OdfValidationException ve = new OdfValidationException(OdfPackageConstraint.PACKAGE_IS_NO_ZIP, getBaseURI()); if (mErrorHandler != null) { mErrorHandler.fatalError(ve); } throw new IllegalArgumentException(ve); } mZipFile = new ZipHelper(this, mTempByteBuf); readZip(); } In this method, the opened file stream isn't closed, which will lead OS can't recovery the file handler. add the following code to the target: // close the input stream to notify OS release file handle. odfStream.close(); the update code has been commited to svn. waiting for the user's new feedback to make sure this issue has been fixed. Devin
        Hide
        Svante Schubert added a comment -

        Again, could please verify if this issue is fixed with the latest 0.8.7 SNAPSHOT from
        https://oss.sonatype.org/content/repositories/snapshots//org/odftoolkit/odfdom-java/0.8.7-SNAPSHOT/

        Show
        Svante Schubert added a comment - Again, could please verify if this issue is fixed with the latest 0.8.7 SNAPSHOT from https://oss.sonatype.org/content/repositories/snapshots//org/odftoolkit/odfdom-java/0.8.7-SNAPSHOT/
        Hide
        Svante Schubert added a comment -

        Hi Devin,

        thanks for reminding me on this one.

        I have to take a look into the issue/patch and read myself into it again.
        Going to do this most likely after my short vacation next Tuesday.

        This issue triggered the changed save handling, ie. all open documents and open DOMs should be filed.

        In addition we plan a single save() method without parameter to save the document on itself, skipping the performance cost of copying aside (as ZIPs can not be saved on itself).

        The above changes were triggered by 173.

        • Svante
        Show
        Svante Schubert added a comment - Hi Devin, thanks for reminding me on this one. I have to take a look into the issue/patch and read myself into it again. Going to do this most likely after my short vacation next Tuesday. This issue triggered the changed save handling, ie. all open documents and open DOMs should be filed. In addition we plan a single save() method without parameter to save the document on itself, skipping the performance cost of copying aside (as ZIPs can not be saved on itself). The above changes were triggered by 173. Svante
        Hide
        devin added a comment -

        Hi, All
        May be we need a conclusion for this issue.
        In my opinion, clarify in the Javadoc the underlying implementation dependencies is the best solution now. If you agree with me, I would like to resynchronized weihua's path to the newest revision and submit a new one.

        Devin

        Show
        devin added a comment - Hi, All May be we need a conclusion for this issue. In my opinion, clarify in the Javadoc the underlying implementation dependencies is the best solution now. If you agree with me, I would like to resynchronized weihua's path to the newest revision and submit a new one. Devin
        Hide
        Ying Chun Guo added a comment -

        I agree to use JavaDoc to let user know if he/she wants to reload a document, he has to close it at first. I think Wei Hua has add these comments in her patch.

        I have no idea about using zip methods to justify if a ZipFile is openned.

        > What I find much more disturbing during code review is that when we save the
        > document to its origin, we are currently loading the complete document into the
        > memory
        > See in OdfPackage line 639:
        > /**
        > * If this file is saved to itself, we have to cache it. It is not possible
        > * to read and write from the same zip file at the same time, so the
        > content
        > * must be read and stored in memory.
        > */
        > private void cacheContent()
        > The user should have the change to avoid the load of every source, if he knows
        > he would like to stop working with the file.

        We have to read the content into memory if user wants to save to the same input file. If the user doesn't save to the same file, this method won't be invoked.

        Show
        Ying Chun Guo added a comment - I agree to use JavaDoc to let user know if he/she wants to reload a document, he has to close it at first. I think Wei Hua has add these comments in her patch. I have no idea about using zip methods to justify if a ZipFile is openned. > What I find much more disturbing during code review is that when we save the > document to its origin, we are currently loading the complete document into the > memory > See in OdfPackage line 639: > /** > * If this file is saved to itself, we have to cache it. It is not possible > * to read and write from the same zip file at the same time, so the > content > * must be read and stored in memory. > */ > private void cacheContent() > The user should have the change to avoid the load of every source, if he knows > he would like to stop working with the file. We have to read the content into memory if user wants to save to the same input file. If the user doesn't save to the same file, this method won't be invoked.
        Hide
        Svante Schubert added a comment -

        I fully agree that we should clarify in the Javadoc the underlying implementation dependencies.

        Not certain if we get from the underlying ZIP implementation a distinguishable error/exception that we can differentiate for a better output, but if someone finds a solution, I am happy to review it.

        What I find much more disturbing during code review is that when we save the document to its origin, we are currently loading the complete document into the memory
        See in OdfPackage line 639:
        /**

        • If this file is saved to itself, we have to cache it. It is not possible
        • to read and write from the same zip file at the same time, so the content
        • must be read and stored in memory.
          */
          private void cacheContent()

        The user should have the change to avoid the load of every source, if he knows he would like to stop working with the file.

        Anyone any suggestions to the comments above?
        Daisy, who does your team think about this?
        --> Reassign to Daisy

        Show
        Svante Schubert added a comment - I fully agree that we should clarify in the Javadoc the underlying implementation dependencies. Not certain if we get from the underlying ZIP implementation a distinguishable error/exception that we can differentiate for a better output, but if someone finds a solution, I am happy to review it. What I find much more disturbing during code review is that when we save the document to its origin, we are currently loading the complete document into the memory See in OdfPackage line 639: /** If this file is saved to itself, we have to cache it. It is not possible to read and write from the same zip file at the same time, so the content must be read and stored in memory. */ private void cacheContent() The user should have the change to avoid the load of every source, if he knows he would like to stop working with the file. Anyone any suggestions to the comments above? Daisy, who does your team think about this? --> Reassign to Daisy
        Hide
        datanucleus added a comment -

        I can only comment from a user perspective. If a method is called "load()" then I expect it to load into that object that is returned. If a method is called "save()" then I expect it to save the object to the destination. The javadocs of loadDocument make no reference to only being able to have it open once.

        What if a user is in some spreadsheet application, and they want to open the same doc twice ? The first time it opens ok, then they make some changes to that (but not saved), and then they say "Open" a second time. It usually opens it, but with suffix "-2" or something.

        How to improve it ?
        Throw an exception like OdfDocumentAlreadyOpenException with message like
        "This document is already open" and have an accessor on the exception for the OdfDocument that is already open. That way the user has the chance to close it, or continue from that point.

        Show
        datanucleus added a comment - I can only comment from a user perspective. If a method is called "load()" then I expect it to load into that object that is returned. If a method is called "save()" then I expect it to save the object to the destination. The javadocs of loadDocument make no reference to only being able to have it open once. What if a user is in some spreadsheet application, and they want to open the same doc twice ? The first time it opens ok, then they make some changes to that (but not saved), and then they say "Open" a second time. It usually opens it, but with suffix "-2" or something. How to improve it ? Throw an exception like OdfDocumentAlreadyOpenException with message like "This document is already open" and have an accessor on the exception for the OdfDocument that is already open. That way the user has the chance to close it, or continue from that point.
        Hide
        Svante Schubert added a comment -

        Why it can not be opened?
        I assume why we are handling internally with streams, which are locking the object. Certainly we can copy the document before starting, but isn't this an overhead?

        Do we agree on following scenario:
        A user might desire to load a document, do changes, save doc, do further changes, save again, and so on.. Like when sending one letter 1000 times with different addresses.
        When doing so, the user does not want to unzip the document every time again, load the file as stream and parse the XML after every save.

        On the other hand, I do not think it is a good idea for the user to open the document multiple times with write access, due to data inconsistencies.

        Do you have any suggestion to improve the current situation?

        Show
        Svante Schubert added a comment - Why it can not be opened? I assume why we are handling internally with streams, which are locking the object. Certainly we can copy the document before starting, but isn't this an overhead? Do we agree on following scenario: A user might desire to load a document, do changes, save doc, do further changes, save again, and so on.. Like when sending one letter 1000 times with different addresses. When doing so, the user does not want to unzip the document every time again, load the file as stream and parse the XML after every save. On the other hand, I do not think it is a good idea for the user to open the document multiple times with write access, due to data inconsistencies. Do you have any suggestion to improve the current situation?
        Hide
        datanucleus added a comment -

        > The document is not being able to be loaded a second time, while it is open

        Why ? How is 'it' "open" ?

        If I do
        doc = OdfSpreadsheetDocument.newSpreadsheetDocument();
        doc.save(file);
        this should (from a user point of view) write the spreadsheet to that file, so we have a spreadsheet. The user doesn't care if it has DOM trees or whatever internally, saved.

        Then the user, in a different part of their application does
        OdfDocument doc = OdfDocument.loadDocument(file);

        This should just go to that File on the filesystem and load it into memory from there. Why can't they have two copies of the same doc in the application in memory at once ? It's an OdfDocument Java object. Is it somehow keeping the "File" object open? I would expect it to read the File create whatever DOM tree and then close the File at that point. Thereafter it is an ODFDocument object in-memory.

        Since OdfDocument.save(...) takes a File, then there is no reason for it to keep any reference to the originating location when it was opened.

        Show
        datanucleus added a comment - > The document is not being able to be loaded a second time, while it is open Why ? How is 'it' "open" ? If I do doc = OdfSpreadsheetDocument.newSpreadsheetDocument(); doc.save(file); this should (from a user point of view) write the spreadsheet to that file, so we have a spreadsheet. The user doesn't care if it has DOM trees or whatever internally, saved. Then the user, in a different part of their application does OdfDocument doc = OdfDocument.loadDocument(file); This should just go to that File on the filesystem and load it into memory from there. Why can't they have two copies of the same doc in the application in memory at once ? It's an OdfDocument Java object. Is it somehow keeping the "File" object open? I would expect it to read the File create whatever DOM tree and then close the File at that point. Thereafter it is an ODFDocument object in-memory. Since OdfDocument.save(...) takes a File, then there is no reason for it to keep any reference to the originating location when it was opened.
        Hide
        Svante Schubert added a comment -

        I am not sure if I understand you correctly, therefore let me ask different. First lets summarize the current status quo:

        When a document is currently loaded into ODFDOM and a user accesses its content, a file is being loaded and the XML is parsed. A DOM tree is availble to be edited.

        Currently a save() serializes this DOM tree and stores everyting in a new zipped ODF. Still it allows to reuse the the DOM tree in memory, therefore does not require reloading the file and reparsing the XML. On the other hand it will not free resources. The document is not being able to be loaded a second time, while it is open (from Windows it seem to work due to error correction, but other platforms throw exceptions).

        Do you think the above handling that save does not free resources and require a close before reloading the doc should be changed?

        Best regards,
        Svante

        Show
        Svante Schubert added a comment - I am not sure if I understand you correctly, therefore let me ask different. First lets summarize the current status quo: When a document is currently loaded into ODFDOM and a user accesses its content, a file is being loaded and the XML is parsed. A DOM tree is availble to be edited. Currently a save() serializes this DOM tree and stores everyting in a new zipped ODF. Still it allows to reuse the the DOM tree in memory, therefore does not require reloading the file and reparsing the XML. On the other hand it will not free resources. The document is not being able to be loaded a second time, while it is open (from Windows it seem to work due to error correction, but other platforms throw exceptions). Do you think the above handling that save does not free resources and require a close before reloading the doc should be changed? Best regards, Svante
        Hide
        datanucleus added a comment -

        Not sure about your scenarios but ...

        save() implies (to me) to leave the document (File) in the state of being reopenable/reloadable (maybe from a different application, or different Thread, or different part of the application). So save() has to do all that is required for that to be the case.

        All that close() implies to me (over save()) is that it is freeing off resources. Whereas save() leaves some internal resources available. If a user doesn't call close() then some resources aren't freed, ... until garbage collection.

        If I do save(someFile) I would expect to be able to do save(someOtherFile) at some time later, unless I called doc.close().

        Show
        datanucleus added a comment - Not sure about your scenarios but ... save() implies (to me) to leave the document (File) in the state of being reopenable/reloadable (maybe from a different application, or different Thread, or different part of the application). So save() has to do all that is required for that to be the case. All that close() implies to me (over save()) is that it is freeing off resources. Whereas save() leaves some internal resources available. If a user doesn't call close() then some resources aren't freed, ... until garbage collection. If I do save(someFile) I would expect to be able to do save(someOtherFile) at some time later, unless I called doc.close().
        Hide
        Svante Schubert added a comment -

        I forgot one scenario:
        If a user opens a file and only reads data, but does not want to change anything, he still have to close() the document.

        Here a redefinition of the scenarios

        1)As it was and you voted for:
        a) save();close();
        b) doingChanges();save();doingChanges();save();close();

        2)And another way, where save() closes the doc as well:
        a) save()
        b) doingChanges();write()doingChanges();write();close();
        or
        b) doingChanges();write()doingChanges();save();

        3) Opening a documnent without change
        a) close()

        @datanucleus: What does the author of the bug would find most intuitive?

        Svante

        Show
        Svante Schubert added a comment - I forgot one scenario: If a user opens a file and only reads data, but does not want to change anything, he still have to close() the document. Here a redefinition of the scenarios 1)As it was and you voted for: a) save();close(); b) doingChanges();save();doingChanges();save();close(); 2)And another way, where save() closes the doc as well: a) save() b) doingChanges();write()doingChanges();write();close(); or b) doingChanges();write()doingChanges();save(); 3) Opening a documnent without change a) close() @datanucleus: What does the author of the bug would find most intuitive? Svante
        Hide
        Svante Schubert added a comment -

        Hi Wei Hua,

        if the IBM team prefers the save() with explicit close, I am most curious to hear the reasons.

        To speed up the discussion here is comees my view:

        First the scenario, where we might want to quickly agree on:

        A user has loaded a document and would like to save the document.
        a) To stop working afterwards
        b) To continue to work and save again

        There are two suggestions on the table.

        1)As it was and you voted for:
        a) save();close();
        b) doingChanges();save();doingChanges();save();close();

        2)And another way, where save() already embraces close()
        a) save()
        b) doingChanges();write()doingChanges();write();save();

        Here come my reasons to chose the second version:
        PRO:
        I) Errorsome:
        People often use to forget to call close()

        II) Not intuitive:
        People, not forgetting it, stil believe there is an issue (see this issue)

        III) More methods to be called by user:
        Scenario a) needs an additional call from the user, while b) has the same amount

        CON:
        I) API change to ODFDOM
        We are still in the 0.x I do not care about this as long it is an improvement.

        If it does not convince you, I am eager to hear your arguments.

        Anybody else is invited to give his opinion, too.

        Kind regards,
        Svante

        Show
        Svante Schubert added a comment - Hi Wei Hua, if the IBM team prefers the save() with explicit close, I am most curious to hear the reasons. To speed up the discussion here is comees my view: First the scenario, where we might want to quickly agree on: A user has loaded a document and would like to save the document. a) To stop working afterwards b) To continue to work and save again There are two suggestions on the table. 1)As it was and you voted for: a) save();close(); b) doingChanges();save();doingChanges();save();close(); 2)And another way, where save() already embraces close() a) save() b) doingChanges();write()doingChanges();write();save(); Here come my reasons to chose the second version: PRO: I) Errorsome: People often use to forget to call close() II) Not intuitive: People, not forgetting it, stil believe there is an issue (see this issue) III) More methods to be called by user: Scenario a) needs an additional call from the user, while b) has the same amount CON: I) API change to ODFDOM We are still in the 0.x I do not care about this as long it is an improvement. If it does not convince you, I am eager to hear your arguments. Anybody else is invited to give his opinion, too. Kind regards, Svante
        Hide
        Wei Hua Wang added a comment -

        Hi Svante, I have upload the patch, you can apply it and modify it if you are not satisfied with it,because I have not add the write() method.
        Daisy and I all vote to close the file explicitly by user if they want to reload this document again. So as to datanucleus's sample code, he has to close the document before reload it again.

        Show
        Wei Hua Wang added a comment - Hi Svante, I have upload the patch, you can apply it and modify it if you are not satisfied with it,because I have not add the write() method. Daisy and I all vote to close the file explicitly by user if they want to reload this document again. So as to datanucleus's sample code, he has to close the document before reload it again.
        Hide
        Wei Hua Wang added a comment -

        Created an attachment (id=267)
        can not get the resources of the document if it has been closed

        1)If user want to reload the document, the document should be first closed. This is described in Javadoc of OdfDocument.loadDocument method.
        2)When user want to access the resources of the closed document, an IllegalStateException will be thrown to inform that the file has been closed.

        Show
        Wei Hua Wang added a comment - Created an attachment (id=267) can not get the resources of the document if it has been closed 1)If user want to reload the document, the document should be first closed. This is described in Javadoc of OdfDocument.loadDocument method. 2)When user want to access the resources of the closed document, an IllegalStateException will be thrown to inform that the file has been closed.
        Hide
        Svante Schubert added a comment -

        Weihua,

        would you like to come up with a suggestion? (patch)

        Bests,
        Svante

        Show
        Svante Schubert added a comment - Weihua, would you like to come up with a suggestion? (patch) Bests, Svante
        Hide
        Svante Schubert added a comment -

        The reason to call close() is to release system resources.

        1) You are right that it is an issue that there are still resources held after a close(), e.g. dom from a document. I do not know how a memory/profiler test might verify this. Anyone an idea?

        2) I like the idea of returning something more verbose than a NPE, and would a similar to ZIPFILE throw an IllegalStateException.

        Show
        Svante Schubert added a comment - The reason to call close() is to release system resources. 1) You are right that it is an issue that there are still resources held after a close(), e.g. dom from a document. I do not know how a memory/profiler test might verify this. Anyone an idea? 2) I like the idea of returning something more verbose than a NPE, and would a similar to ZIPFILE throw an IllegalStateException.
        Hide
        Wei Hua Wang added a comment -

        Hi Svante,
        Why no longer possible to work on the document when the document is closed, although the resouces of the document have been released, the doc handler is still there, user might still use this doc handler to get the content dom.
        for example,
        OdfTextDocument doc = OdfTextDocument.newTextDocument();
        doc.save(test.odt);//include close document
        OdfFileDom contentDom = doc.getContentDom(); // can get the content dom successfully
        InputStream contentStream = doc.getContentStream();// will throw NPE because reource mPackageEntries = null.
        while I think
        1) when close the document, the dom which is also the resource of the document should also set to null.
        2) throw NPE might make user puzzled, we should explicitly throw the exception to notify user reload the document.
        So i would like to refer the implementation of java.util.zip.ZipFile which use a flag to ensure the zipfile is open, when user manipulate the closed zipfile, an IllegalStatexception will be thrown to notify user that the zipfile is closed. What do you think?

        Show
        Wei Hua Wang added a comment - Hi Svante, Why no longer possible to work on the document when the document is closed, although the resouces of the document have been released, the doc handler is still there, user might still use this doc handler to get the content dom. for example, OdfTextDocument doc = OdfTextDocument.newTextDocument(); doc.save(test.odt);//include close document OdfFileDom contentDom = doc.getContentDom(); // can get the content dom successfully InputStream contentStream = doc.getContentStream();// will throw NPE because reource mPackageEntries = null. while I think 1) when close the document, the dom which is also the resource of the document should also set to null. 2) throw NPE might make user puzzled, we should explicitly throw the exception to notify user reload the document. So i would like to refer the implementation of java.util.zip.ZipFile which use a flag to ensure the zipfile is open, when user manipulate the closed zipfile, an IllegalStatexception will be thrown to notify user that the zipfile is closed. What do you think?
        Hide
        Svante Schubert added a comment -

        Hi Weihua,

        regarding to your questions:
        1) If we change the logic that a save() operation will close() the document, releasing all resources, equal that the document is no longer loaded, it is therefore no longer possible to work on the document, but have to be reloaded.

        As we change the logic, our test cases have to be adapted.

        2) The problem we face here is the same on all platforms, but I assume Windows has a better error handling and is releasing the resources, if they are not accessible and access the document again.

        Does this clarify all questions, otherwise we might discuss the topic as well tomorrow in our dev call (not announced so far).

        Best regards,
        Svante

        Show
        Svante Schubert added a comment - Hi Weihua, regarding to your questions: 1) If we change the logic that a save() operation will close() the document, releasing all resources, equal that the document is no longer loaded, it is therefore no longer possible to work on the document, but have to be reloaded. As we change the logic, our test cases have to be adapted. 2) The problem we face here is the same on all platforms, but I assume Windows has a better error handling and is releasing the resources, if they are not accessible and access the document again. Does this clarify all questions, otherwise we might discuss the topic as well tomorrow in our dev call (not announced so far). Best regards, Svante
        Hide
        Wei Hua Wang added a comment -

        Hi Svante,
        I have tested the case that user use the previous document handler after saving the document(include write() and close()), an exception will be thrown if we insert any stream to the package or modify the dom and save it again, because the close() will release all the resources of the document.
        PackageTest.testPackage() and DocumentTest.testParsingOfInvalidAttribute() will fail for this reason if close is done implicitly in save operation.

        We can not avoid user to do this, so what should we do for this condition?

        Other question:
        1.Why bug173 only happens on Linux, rather than Windows? How to explain the java platform independent feature.
        2.The bug173 can only reproduce when the test.ods is not there, and user new a document first then saving several versions of the same document. If test.ods is there, the bug will not happen. Why?

        Show
        Wei Hua Wang added a comment - Hi Svante, I have tested the case that user use the previous document handler after saving the document(include write() and close()), an exception will be thrown if we insert any stream to the package or modify the dom and save it again, because the close() will release all the resources of the document. PackageTest.testPackage() and DocumentTest.testParsingOfInvalidAttribute() will fail for this reason if close is done implicitly in save operation. We can not avoid user to do this, so what should we do for this condition? Other question: 1.Why bug173 only happens on Linux, rather than Windows? How to explain the java platform independent feature. 2.The bug173 can only reproduce when the test.ods is not there, and user new a document first then saving several versions of the same document. If test.ods is there, the bug will not happen. Why?
        Hide
        Wei Hua Wang added a comment -

        The new added write() method just like the OutputStreamWriter.write(), right? I
        like this idea.
        But if user call save() which include write() and close() , they have to reload
        the document, what will happen and what should we do if user still use the
        previous OdfDocument handler after save()?

        Show
        Wei Hua Wang added a comment - The new added write() method just like the OutputStreamWriter.write(), right? I like this idea. But if user call save() which include write() and close() , they have to reload the document, what will happen and what should we do if user still use the previous OdfDocument handler after save()?
        Hide
        Svante Schubert added a comment -

        The question remains is this a bug at all.

        If you save() and would like to stop working, you need to have a close().
        If the close is done implicitly, there is no way in saving several versions of the same document only by changing a minor part (e.g. the address).

        Usually I would avoid to put two actions into one method. In our case, people might too often assume, that save() includes a close().

        Therefore I suggest to add close() to save() and allow some intermediate save() method, with a different name, for instance write().

        write() would be our current save() method and the upcoming save() would ave write() and close().

        Do you like this idea? What do you think?

        Bests,
        Svante

        Show
        Svante Schubert added a comment - The question remains is this a bug at all. If you save() and would like to stop working, you need to have a close(). If the close is done implicitly, there is no way in saving several versions of the same document only by changing a minor part (e.g. the address). Usually I would avoid to put two actions into one method. In our case, people might too often assume, that save() includes a close(). Therefore I suggest to add close() to save() and allow some intermediate save() method, with a different name, for instance write(). write() would be our current save() method and the upcoming save() would ave write() and close(). Do you like this idea? What do you think? Bests, Svante
        Hide
        Wei Hua Wang added a comment -

        simple test case to reproduce this issue
        try

        { OdfDocument doc = null; String filename = "test.ods"; File file = new File(filename); System.out.println(">> file=" + file); //1. create a new spreadsheet System.out.println(">> OdfDocument.newSpreadsheetDocument()"); doc = OdfSpreadsheetDocument.newSpreadsheetDocument(); //2. save it System.out.println(">> OdfDocument.save(" + file + ")"); doc.save(file); //3. Load the document System.out.println(">> OdfDocument.loadDocument(" + file + ")"); doc = OdfDocument.loadDocument(file); //3.1. insert a image or append any element on the content dom doc.newImage(URI.create("test.jpg")); //4. save to the same file System.out.println(">> OdfDocument.save(" + file + ")"); doc.save(file); //5. Load the document System.out.println(">> OdfDocument.loadDocument(" + file + ")"); doc = OdfDocument.loadDocument(file); }

        catch (Exception e)

        { e.printStackTrace(); throw e; }

        The test case will fail when test.ods is not on the disk, otherwise if the test.ods exist, the above test case will not fail.

        Show
        Wei Hua Wang added a comment - simple test case to reproduce this issue try { OdfDocument doc = null; String filename = "test.ods"; File file = new File(filename); System.out.println(">> file=" + file); //1. create a new spreadsheet System.out.println(">> OdfDocument.newSpreadsheetDocument()"); doc = OdfSpreadsheetDocument.newSpreadsheetDocument(); //2. save it System.out.println(">> OdfDocument.save(" + file + ")"); doc.save(file); //3. Load the document System.out.println(">> OdfDocument.loadDocument(" + file + ")"); doc = OdfDocument.loadDocument(file); //3.1. insert a image or append any element on the content dom doc.newImage(URI.create("test.jpg")); //4. save to the same file System.out.println(">> OdfDocument.save(" + file + ")"); doc.save(file); //5. Load the document System.out.println(">> OdfDocument.loadDocument(" + file + ")"); doc = OdfDocument.loadDocument(file); } catch (Exception e) { e.printStackTrace(); throw e; } The test case will fail when test.ods is not on the disk, otherwise if the test.ods exist, the above test case will not fail.
        Hide
        Wei Hua Wang added a comment -

        i can reproduce this issue on my SUSE, and i can always reproduce it by the following step:
        1. newSpreadsheetDocument()
        2. save it
        3. load it, append any element on the content dom(i have not test other cases, such as insert a stream to the package)
        4. save to the same file
        5. reload it then the exception is thrown like
        "java.lang.IllegalArgumentException: Document contains incorrect ODF Mediatype
        'null'"
        From my investiation, at step 5, the name of the zip entries are all messy code, so we can not get the media type and throw the above exception. While if in step 4, we save to another file, the class can run successfully.
        I think this issue might has relationship with http://odftoolkit.org/bugzilla/show_bug.cgi?id=50 and lazy loading machanism.
        Steffeng is the owner of these two issue,so Svante, could you pls help to look at this bug? I think you are more familiar with the package layer and the Linux platform.
        BTW, in the test case of ODFTOOLKIT-33, PackageTest.testPackage() always close the document after save it, i am not sure what will happen on Unix if we don't call close there, and reload the document at the last.
        Because I am at home now and do not have Unix at hand, I will upload the simple sample file to reproduce this bug.

        Show
        Wei Hua Wang added a comment - i can reproduce this issue on my SUSE, and i can always reproduce it by the following step: 1. newSpreadsheetDocument() 2. save it 3. load it, append any element on the content dom(i have not test other cases, such as insert a stream to the package) 4. save to the same file 5. reload it then the exception is thrown like "java.lang.IllegalArgumentException: Document contains incorrect ODF Mediatype 'null'" From my investiation, at step 5, the name of the zip entries are all messy code, so we can not get the media type and throw the above exception. While if in step 4, we save to another file, the class can run successfully. I think this issue might has relationship with http://odftoolkit.org/bugzilla/show_bug.cgi?id=50 and lazy loading machanism. Steffeng is the owner of these two issue,so Svante, could you pls help to look at this bug? I think you are more familiar with the package layer and the Linux platform. BTW, in the test case of ODFTOOLKIT-33 , PackageTest.testPackage() always close the document after save it, i am not sure what will happen on Unix if we don't call close there, and reload the document at the last. Because I am at home now and do not have Unix at hand, I will upload the simple sample file to reproduce this bug.
        Hide
        Svante Schubert added a comment -

        If we have one running into this situation, there will be others.
        A save that closes the file instantly would solve this.
        On the other hand, if a user saves, but would like to do some minor changes before saving the model again into a different document, he would need to reload the file.

        Any suggestions how the API might express the given scenarios (are there more?) more clearly?

        Thanks,
        Svante

        Show
        Svante Schubert added a comment - If we have one running into this situation, there will be others. A save that closes the file instantly would solve this. On the other hand, if a user saves, but would like to do some minor changes before saving the model again into a different document, he would need to reload the file. Any suggestions how the API might express the given scenarios (are there more?) more clearly? Thanks, Svante
        Hide
        Ying Chun Guo added a comment -

        Hi,

        It's an interesting issue.

        "OdfDocument.loadDocument(file)" will open this ODF document as a zip file and
        read the zip entries. "OdfDocument.save()" will save it as a zip file but won't
        close this zip file. If you re-load again, the ODF document will be openned and
        read again as a zip file. If the zip file cannot be openned and read correctly,
        exception will be thrown and there will be no mediatype information. So that
        there comes the error message "Document contains incorrect ODF Mediatype
        'null'".

        I think it looks like different operation systems handle it differently so that
        Wei Hua and I won't reproduce it in Windows system.

        I'm thinking about how we can fix this issue. Maybe we can catch the exception
        thrown when the zip file cannot be openned and read correctly, and throw a new
        exception "The file has been openned by another user."

        What's your opinion, Svante?

        Regards
        Daisy

        Show
        Ying Chun Guo added a comment - Hi, It's an interesting issue. "OdfDocument.loadDocument(file)" will open this ODF document as a zip file and read the zip entries. "OdfDocument.save()" will save it as a zip file but won't close this zip file. If you re-load again, the ODF document will be openned and read again as a zip file. If the zip file cannot be openned and read correctly, exception will be thrown and there will be no mediatype information. So that there comes the error message "Document contains incorrect ODF Mediatype 'null'". I think it looks like different operation systems handle it differently so that Wei Hua and I won't reproduce it in Windows system. I'm thinking about how we can fix this issue. Maybe we can catch the exception thrown when the zip file cannot be openned and read correctly, and throw a new exception "The file has been openned by another user." What's your opinion, Svante? Regards Daisy
        Hide
        Wei Hua Wang added a comment -

        I will test it on linux tomorrow and loop the test several times.

        Show
        Wei Hua Wang added a comment - I will test it on linux tomorrow and loop the test several times.
        Hide
        datanucleus added a comment -

        Running the test several times
        Run 1 : failed
        Run 2 : succeeded
        Run 3 : failed
        Run 4 : failed
        Run 5 : failed

        so perhaps there is some timing issue involved there around what you're doing with the "File" object ?

        Show
        datanucleus added a comment - Running the test several times Run 1 : failed Run 2 : succeeded Run 3 : failed Run 4 : failed Run 5 : failed so perhaps there is some timing issue involved there around what you're doing with the "File" object ?
        Hide
        datanucleus added a comment -

        Just ran the test again (FYI, to run the test I just type "mvn clean compile exec:java") and it definitely fails for me. Yes the version of ODF is the most recent release (0.8.5).

        andy@rookie.22> mvn clean compile exec:java
        [INFO] Scanning for projects...
        [INFO] Searching repository for plugin with prefix: 'exec'.
        [INFO] ------------------------------------------------------------------------
        [INFO] Building DataNucleus TestSamples
        [INFO] task-segment: [clean, compile, exec:java]
        [INFO] ------------------------------------------------------------------------
        [INFO] [clean:clean]
        [INFO] Deleting file-set: /home/andy/work/test/odf_bug_173 (included: [*.log, testDB, test.xls, test.ods, test.ooxml], excluded: [])
        [INFO] [resources:resources]
        [WARNING] File encoding has not been set, using platform encoding UTF-8, i.e. build is platform dependent!
        [WARNING] Using platform encoding (UTF-8 actually) to copy filtered resources, i.e. build is platform dependent!
        [INFO] Copying 0 resource
        [INFO] skip non existing resourceDirectory /home/andy/work/test/odf_bug_173/src/conf
        [INFO] Copying 0 resource
        [INFO] Copying 0 resource to META-INF
        Downloading: http://repo1.maven.org/maven2/org/odftoolkit/odfdom-java/0.8.5/odfdom-java-0.8.5.pom
        [INFO] [compiler:compile]
        [INFO] Compiling 1 source file to /home/andy/work/test/odf_bug_173/target/classes
        [INFO] Preparing exec:java
        [INFO] No goals needed for project - skipping
        [INFO] [exec:java]
        >> file=test.ods
        >> OdfDocument.newSpreadsheetDocument()
        >> OdfDocument.save(test.ods)
        >> OdfDocument.loadDocument(test.ods)
        >> OdfDocument.save(test.ods)
        >> OdfDocument.loadDocument(test.ods)
        java.lang.IllegalArgumentException: Document contains incorrect ODF Mediatype 'null'
        at org.odftoolkit.odfdom.doc.OdfDocument.loadDocument(OdfDocument.java:356)
        at org.odftoolkit.odfdom.doc.OdfDocument.loadDocument(OdfDocument.java:344)
        at org.datanucleus.test.Main.main(Main.java:51)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:597)
        at org.codehaus.mojo.exec.ExecJavaMojo$1.run(ExecJavaMojo.java:290)
        at java.lang.Thread.run(Thread.java:619)
        [INFO] ------------------------------------------------------------------------
        [ERROR] BUILD ERROR
        [INFO] ------------------------------------------------------------------------
        [INFO] An exception occured while executing the Java class. null

        Document contains incorrect ODF Mediatype 'null'
        [INFO] ------------------------------------------------------------------------
        [INFO] For more information, run Maven with the -e switch
        [INFO] ------------------------------------------------------------------------
        [INFO] Total time: 5 seconds
        [INFO] Finished at: Mon Jun 07 11:39:00 BST 2010
        [INFO] Final Memory: 13M/494M
        [INFO] ------------------------------------------------------------------------

        Note that this was run on Linux using JDK 1.6.0_18 if that makes a difference. The pom.xml defines what jars I used it with

        Show
        datanucleus added a comment - Just ran the test again (FYI, to run the test I just type "mvn clean compile exec:java") and it definitely fails for me. Yes the version of ODF is the most recent release (0.8.5). andy@rookie.22> mvn clean compile exec:java [INFO] Scanning for projects... [INFO] Searching repository for plugin with prefix: 'exec'. [INFO] ------------------------------------------------------------------------ [INFO] Building DataNucleus TestSamples [INFO] task-segment: [clean, compile, exec:java] [INFO] ------------------------------------------------------------------------ [INFO] [clean:clean] [INFO] Deleting file-set: /home/andy/work/test/odf_bug_173 (included: [*.log, testDB, test.xls, test.ods, test.ooxml] , excluded: []) [INFO] [resources:resources] [WARNING] File encoding has not been set, using platform encoding UTF-8, i.e. build is platform dependent! [WARNING] Using platform encoding (UTF-8 actually) to copy filtered resources, i.e. build is platform dependent! [INFO] Copying 0 resource [INFO] skip non existing resourceDirectory /home/andy/work/test/odf_bug_173/src/conf [INFO] Copying 0 resource [INFO] Copying 0 resource to META-INF Downloading: http://repo1.maven.org/maven2/org/odftoolkit/odfdom-java/0.8.5/odfdom-java-0.8.5.pom [INFO] [compiler:compile] [INFO] Compiling 1 source file to /home/andy/work/test/odf_bug_173/target/classes [INFO] Preparing exec:java [INFO] No goals needed for project - skipping [INFO] [exec:java] >> file=test.ods >> OdfDocument.newSpreadsheetDocument() >> OdfDocument.save(test.ods) >> OdfDocument.loadDocument(test.ods) >> OdfDocument.save(test.ods) >> OdfDocument.loadDocument(test.ods) java.lang.IllegalArgumentException: Document contains incorrect ODF Mediatype 'null' at org.odftoolkit.odfdom.doc.OdfDocument.loadDocument(OdfDocument.java:356) at org.odftoolkit.odfdom.doc.OdfDocument.loadDocument(OdfDocument.java:344) at org.datanucleus.test.Main.main(Main.java:51) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.codehaus.mojo.exec.ExecJavaMojo$1.run(ExecJavaMojo.java:290) at java.lang.Thread.run(Thread.java:619) [INFO] ------------------------------------------------------------------------ [ERROR] BUILD ERROR [INFO] ------------------------------------------------------------------------ [INFO] An exception occured while executing the Java class. null Document contains incorrect ODF Mediatype 'null' [INFO] ------------------------------------------------------------------------ [INFO] For more information, run Maven with the -e switch [INFO] ------------------------------------------------------------------------ [INFO] Total time: 5 seconds [INFO] Finished at: Mon Jun 07 11:39:00 BST 2010 [INFO] Final Memory: 13M/494M [INFO] ------------------------------------------------------------------------ Note that this was run on Linux using JDK 1.6.0_18 if that makes a difference. The pom.xml defines what jars I used it with
        Hide
        Svante Schubert added a comment -

        Weihua,

        have you checked under Linux? I can not reproduce the issue under Windows, neither, no Unix at hand today. Otherwise I will run it under Unix tomorrow.

        PS: If you want him to recheck on his issue, pls change the owner. The issue owner is responsible for the the next action.

        Thanks,
        Svante

        Show
        Svante Schubert added a comment - Weihua, have you checked under Linux? I can not reproduce the issue under Windows, neither, no Unix at hand today. Otherwise I will run it under Unix tomorrow. PS: If you want him to recheck on his issue, pls change the owner. The issue owner is responsible for the the next action. Thanks, Svante
        Hide
        Wei Hua Wang added a comment -

        My colleague and I can not reproduce this bug
        could you pls get the latest odfdom jar and recheck it?

        Show
        Wei Hua Wang added a comment - My colleague and I can not reproduce this bug could you pls get the latest odfdom jar and recheck it?
        Hide
        datanucleus added a comment -

        Adding a doc.close() before the loadDocument() avoids the error.
        1. It wasn't necessary in 0.6.16
        2. The error message does not imply "you ought to close that first before reopening"

        Show
        datanucleus added a comment - Adding a doc.close() before the loadDocument() avoids the error. 1. It wasn't necessary in 0.6.16 2. The error message does not imply "you ought to close that first before reopening"

          People

          • Assignee:
            Unassigned
            Reporter:
            datanucleus
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development