castor
  1. castor
  2. CASTOR-1586

Error loading mapping whem migrating from 1.0.1 to 1.0.3

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.0.3
    • Fix Version/s: 1.0.4
    • Component/s: XML
    • Labels:
      None
    • Environment:
      Windows XP SP2, JDK 1.5.0_07, Eclipse 3.2
    • Testcase included:
      yes
    • Number of attachments :
      5

      Description

      the following code worked under Castor 1.0.1 but doesn't under 1.0.3 A java.io.IOException: Stream closed is thrown when setting the mapping in the unmarshaller instance:

      mapping = new Mapping(getClass().getClassLoader());
      mapping.loadMapping(new InputSource(getClass().getResourceAsStream("mapping.xml")));

      // create Marshaller
      writer = new StringWriter();
      marshaller = new Marshaller(writer);
      marshaller.setMapping(mapping);
      marshaller.setEncoding("UTF-8");
      marshaller.setValidation(true);

      // create Unmarshaller
      unmarshaller = new Unmarshaller();
      unmarshaller.setClassLoader(getClass().getClassLoader());
      unmarshaller.setValidation(false);
      unmarshaller.setMapping(mapping); // --> Exception thrown here
      unmarshaller.setWhitespacePreserve(true);

      If the mapping is not loaded as input stream using

      mapping.loadMapping(new InputSource(getClass().getResource("mapping.xml")));

      everything works fine.

      1. c1586-patch.diff
        2 kB
        Edward Kuns
      2. castor-1586-test.patch
        7 kB
        Edward Kuns
      3. patch-C1586-20061016.txt
        10 kB
        Ralf Joachim
      4. patch-C1586-20061016-02.txt
        14 kB
        Ralf Joachim

        Activity

        Hide
        M.-Leander Reimer added a comment -

        Unit test case with mapping file and test entity that shows the exception.

        Show
        M.-Leander Reimer added a comment - Unit test case with mapping file and test entity that shows the exception.
        Hide
        M.-Leander Reimer added a comment -

        Use this unit test, the mapping file and the test entity now have the correct Java package declaration.

        Show
        M.-Leander Reimer added a comment - Use this unit test, the mapping file and the test entity now have the correct Java package declaration.
        Hide
        Werner Guttmann added a comment -

        Thans you very much. After some debugging related to the use of MappingSource, I do believe that this might be related to how MappingSource is coded. Iam not 100% sure, but ... .

        Show
        Werner Guttmann added a comment - Thans you very much. After some debugging related to the use of MappingSource, I do believe that this might be related to how MappingSource is coded. Iam not 100% sure, but ... .
        Hide
        Werner Guttmann added a comment -

        Guys, we really need to address this before shipping 1.0.4.

        Show
        Werner Guttmann added a comment - Guys, we really need to address this before shipping 1.0.4.
        Hide
        Edward Kuns added a comment -

        Having looked at this, I'm not sure it's a bug. I'm not sure the original code was truly working. The unit test loads a mapping and uses it twice. It is used and closed the first time, and thus cannot be used the second time. Before the line:

        unmarshaller.setMapping(mapping); // --> Exception thrown here

        if you insert

        mapping = new Mapping(getClass().getClassLoader());
        mapping.loadMapping(new InputSource(getClass().getResourceAsStream("mapping.xml")));

        then it works. While a previous version of Castor didn't throw an exception, did it in fact truly load the mapping both times, or did it load the mapping the first time and use an empty mapping the second time? If the mapping was actually used both times, then perhaps the fix is to have Castor always reset an inputsource (if that is possible) before using it. Or for Castor to never close() an input source.

        Thoughts?

        Show
        Edward Kuns added a comment - Having looked at this, I'm not sure it's a bug. I'm not sure the original code was truly working. The unit test loads a mapping and uses it twice. It is used and closed the first time, and thus cannot be used the second time. Before the line: unmarshaller.setMapping(mapping); // --> Exception thrown here if you insert mapping = new Mapping(getClass().getClassLoader()); mapping.loadMapping(new InputSource(getClass().getResourceAsStream("mapping.xml"))); then it works. While a previous version of Castor didn't throw an exception, did it in fact truly load the mapping both times, or did it load the mapping the first time and use an empty mapping the second time? If the mapping was actually used both times, then perhaps the fix is to have Castor always reset an inputsource (if that is possible) before using it. Or for Castor to never close() an input source. Thoughts?
        Hide
        Edward Kuns added a comment -

        Hmm. If I try the following

        mapping.loadMapping(new InputSource(getClass().getResource("mapping.xml")));

        I get a complaint that the constructor InputSource(URL) is undefined.

        Show
        Edward Kuns added a comment - Hmm. If I try the following mapping.loadMapping(new InputSource(getClass().getResource("mapping.xml"))); I get a complaint that the constructor InputSource(URL) is undefined.
        Hide
        Edward Kuns added a comment -

        Ignore what I said two comments above. I was taking what I saw too literally. This may have been introduced in CASTOR-1463 in Castor 1.0.2.

        Show
        Edward Kuns added a comment - Ignore what I said two comments above. I was taking what I saw too literally. This may have been introduced in CASTOR-1463 in Castor 1.0.2.
        Hide
        Edward Kuns added a comment -

        The problem is that the mapping is not cached. Each time you invoke setMapping, the mapping is freshly loaded from disk. When you provide an InputStream, the first time the mapping is unmarshaled from disk, it is loaded and the InputStream is exhausted and somewhere must be closed. When you later use the same org.exolab.castor.mapping.Mapping in a setMapping(mapping), the code tries again to load the mapping from disk, but the InputStream was closed the first time this was done.

        Depending on how this was implemented before, it may not have worked properly, maybe it just provided an empty mapping. That is, if the InputStream wasn't closed before, but it still tried to load from disk every time setMapping() was called, then it would have immediately read EOF and thus read an empty mapping. But in that case it wouldn't have thrown an exception. In that case, the current behavior is arguably better because now you KNOW it's not doing what you want.

        If previously it cached what it loaded from disk, then it would have worked before.

        Thus, depending on how it was in Castor 1.0.1, my first comment may or may not have been correct.

        Show
        Edward Kuns added a comment - The problem is that the mapping is not cached. Each time you invoke setMapping, the mapping is freshly loaded from disk. When you provide an InputStream, the first time the mapping is unmarshaled from disk, it is loaded and the InputStream is exhausted and somewhere must be closed. When you later use the same org.exolab.castor.mapping.Mapping in a setMapping(mapping), the code tries again to load the mapping from disk, but the InputStream was closed the first time this was done. Depending on how this was implemented before, it may not have worked properly, maybe it just provided an empty mapping. That is, if the InputStream wasn't closed before, but it still tried to load from disk every time setMapping() was called, then it would have immediately read EOF and thus read an empty mapping. But in that case it wouldn't have thrown an exception. In that case, the current behavior is arguably better because now you KNOW it's not doing what you want. If previously it cached what it loaded from disk, then it would have worked before. Thus, depending on how it was in Castor 1.0.1, my first comment may or may not have been correct.
        Hide
        Ralf Joachim added a comment -

        I expect the changed behaviour have been introduced by me with one of the refactorings of CASTOR-1455. CASTOR-1463 seams to be a reasonable candidat for the origin of the problem. Having said that I tried to keep behaviour unchanged when working at these patches

        Show
        Ralf Joachim added a comment - I expect the changed behaviour have been introduced by me with one of the refactorings of CASTOR-1455 . CASTOR-1463 seams to be a reasonable candidat for the origin of the problem. Having said that I tried to keep behaviour unchanged when working at these patches
        Hide
        Werner Guttmann added a comment -

        Don't worry whether it got altered as par of your work or not ... but what Edward said above is the current standing. When using InputSource(InputStream) - and only then - we ru into situations where the underlying parser seems to be 'closing' the InputStream porivided by the MappingSource. This could be a problem of e.g. Xerces 1.4, but I remember having seen this with more recent Xerces releases.

        If the user switched to using InputSource(String), providing a URO to the InoutSource, the InputSource will internally open new InputStreams everytime it is used.

        So how do we go about this ?

        Show
        Werner Guttmann added a comment - Don't worry whether it got altered as par of your work or not ... but what Edward said above is the current standing. When using InputSource(InputStream) - and only then - we ru into situations where the underlying parser seems to be 'closing' the InputStream porivided by the MappingSource. This could be a problem of e.g. Xerces 1.4, but I remember having seen this with more recent Xerces releases. If the user switched to using InputSource(String), providing a URO to the InoutSource, the InputSource will internally open new InputStreams everytime it is used. So how do we go about this ?
        Hide
        Edward Kuns added a comment -

        I'm also genuinely curious whether this worked before, or whether it just silently failed to do what it was supposed to do. If every call to setMapping caused an unmarshaling then this never did what the user wanted.

        Show
        Edward Kuns added a comment - I'm also genuinely curious whether this worked before, or whether it just silently failed to do what it was supposed to do. If every call to setMapping caused an unmarshaling then this never did what the user wanted.
        Hide
        M.-Leander Reimer added a comment -

        First of all sorry for the slightly faulty code I initially posted, of course the code to load the mapping from an URL should have been

        mapping.loadMapping(getClass().getResource("mapping.xml"));

        After reading all the comments I went back to my original test case to do some testing myself, don't know if these findings are of any help.
        To see whether everything is working, I extended the test case so that a test object is marshalled into a StringWriter and then immediately unmarshalled again right after the setup of the marshaller/unmarshaller instances.

        Then I took all Castor releases I had in my repository, from 0.9.5.3 to 1.0.3 and ran the tests with each version as a dependency. The problem definitely starts to appear from Castor 1.0.2 onwards.

        I have also tested several Xerces versions (1.4.0, 2.3.0, 2.4.0, 2.6.2, 2.8.0). The tests run successfull with all these versions together with Castor 1.0.1 but don't with Castor 1.0.2 or later. So the Xerces version doesn't seem to have an influence at all.

        Show
        M.-Leander Reimer added a comment - First of all sorry for the slightly faulty code I initially posted, of course the code to load the mapping from an URL should have been mapping.loadMapping(getClass().getResource("mapping.xml")); After reading all the comments I went back to my original test case to do some testing myself, don't know if these findings are of any help. To see whether everything is working, I extended the test case so that a test object is marshalled into a StringWriter and then immediately unmarshalled again right after the setup of the marshaller/unmarshaller instances. Then I took all Castor releases I had in my repository, from 0.9.5.3 to 1.0.3 and ran the tests with each version as a dependency. The problem definitely starts to appear from Castor 1.0.2 onwards. I have also tested several Xerces versions (1.4.0, 2.3.0, 2.4.0, 2.6.2, 2.8.0). The tests run successfull with all these versions together with Castor 1.0.1 but don't with Castor 1.0.2 or later. So the Xerces version doesn't seem to have an influence at all.
        Hide
        Edward Kuns added a comment -

        Here is what it looks like:

        In Castor 1.0.1 and earlier, calling

        mapping.loadMapping(new InputSource(getClass().getResourceAsStream("mapping.xml")));

        caused the mapping to be unmarshaled from disk. Calling the setMapping just provided references to the mapping. However, in Castor 1.0.2 and later, calling loadMapping() only adds information about the mapping to a list for later use the mapping is not loaded from disk until setMapping is called.

        Is there a reason why we want to defer loading the mapping until it is used, and then load it each time it is used with a setMapping?

        Show
        Edward Kuns added a comment - Here is what it looks like: In Castor 1.0.1 and earlier, calling mapping.loadMapping(new InputSource(getClass().getResourceAsStream("mapping.xml"))); caused the mapping to be unmarshaled from disk. Calling the setMapping just provided references to the mapping. However, in Castor 1.0.2 and later, calling loadMapping() only adds information about the mapping to a list for later use the mapping is not loaded from disk until setMapping is called. Is there a reason why we want to defer loading the mapping until it is used, and then load it each time it is used with a setMapping?
        Hide
        Edward Kuns added a comment -

        Attaching a proposed patch from Ralf. This patch has one downside – if you call setMapping() on a marshaler or unmarshaler more than one time, the call will be ignored after the first time.

        Thoughts? Do we expect people to call setMapping() more than once on a marshaler or unmarshaler?

        Show
        Edward Kuns added a comment - Attaching a proposed patch from Ralf. This patch has one downside – if you call setMapping() on a marshaler or unmarshaler more than one time, the call will be ignored after the first time. Thoughts? Do we expect people to call setMapping() more than once on a marshaler or unmarshaler?
        Hide
        Werner Guttmann added a comment -

        Good question .. . Why not ask our users ? Personally, I would think that one would not call setMapping() more than once on these two classes, but then again, I am just a service provider.

        Show
        Werner Guttmann added a comment - Good question .. . Why not ask our users ? Personally, I would think that one would not call setMapping() more than once on these two classes, but then again, I am just a service provider.
        Hide
        Ralf Joachim added a comment -

        We tend to commit the attached patch as this seams to fix the issue of M.-Leander Reimer. Still there are mixed feelings according to the drawback that setMapping() can only be called once on a Unmarshaller/mMarshaller instance. Consecutive calles to setMapping() will simply be ignored and have no effect.

        M.-Leander Reimer, any comments from your side on that patch?

        Show
        Ralf Joachim added a comment - We tend to commit the attached patch as this seams to fix the issue of M.-Leander Reimer. Still there are mixed feelings according to the drawback that setMapping() can only be called once on a Unmarshaller/mMarshaller instance. Consecutive calles to setMapping() will simply be ignored and have no effect. M.-Leander Reimer, any comments from your side on that patch?
        Hide
        Ralf Joachim added a comment -

        Patch for review.

        Show
        Ralf Joachim added a comment - Patch for review.
        Hide
        Werner Guttmann added a comment -

        +1.

        Show
        Werner Guttmann added a comment - +1.
        Hide
        Ralf Joachim added a comment -

        Just before committing I recognized that this patch breaks quite some tests of our XML test suite so we have to search for another solution. Will come back here when i found something.

        Show
        Ralf Joachim added a comment - Just before committing I recognized that this patch breaks quite some tests of our XML test suite so we have to search for another solution. Will come back here when i found something.
        Hide
        Edward Kuns added a comment -

        Attaching a newer version of attachment "bug1586-with-correct-package.zip" that includes a change to the Eclipse path so you can right-click on TestContentTest.java and run-as-junit test.

        Show
        Edward Kuns added a comment - Attaching a newer version of attachment "bug1586-with-correct-package.zip" that includes a change to the Eclipse path so you can right-click on TestContentTest.java and run-as-junit test.
        Hide
        Edward Kuns added a comment -

        Summarizing my understanding of this issue:

        Previously, loadMapping on the mapping itself caused the mapping to be loaded. This was not desirable as it isn't known at the time of loadMapping() whether the mapping will be used for XML or for JDO, and the full configuration of XML or JDO (which the mapping needs) isn't necessarily known at the time of loadMapping.

        Thus, loadMapping was changed to hold a reference to the schema that will be loaded, and setMapping() on the marshaler or unmarshaler loads the file. This works fine when the mapping is created with a URI, but when the mapping is created using a Stream, this causes problems.

        Regardless of anything else ... what are the odds that someone would create a Mapping destined for use both by XML and JDO? Or what are the odds that someone would create a Mapping destined for use by XML but with two different XML configurations? Either of these, if the mapping is loaded via Stream, would cause this to break, yes?

        Is there a way to load the mapping in a way that doesn't depend on JDO or XML configuration, and that wouldn't require the mapping to be reloaded if the JDO or XML configuration is changed?

        Show
        Edward Kuns added a comment - Summarizing my understanding of this issue: Previously, loadMapping on the mapping itself caused the mapping to be loaded. This was not desirable as it isn't known at the time of loadMapping() whether the mapping will be used for XML or for JDO, and the full configuration of XML or JDO (which the mapping needs) isn't necessarily known at the time of loadMapping. Thus, loadMapping was changed to hold a reference to the schema that will be loaded, and setMapping() on the marshaler or unmarshaler loads the file. This works fine when the mapping is created with a URI, but when the mapping is created using a Stream, this causes problems. Regardless of anything else ... what are the odds that someone would create a Mapping destined for use both by XML and JDO? Or what are the odds that someone would create a Mapping destined for use by XML but with two different XML configurations? Either of these, if the mapping is loaded via Stream, would cause this to break, yes? Is there a way to load the mapping in a way that doesn't depend on JDO or XML configuration, and that wouldn't require the mapping to be reloaded if the JDO or XML configuration is changed?
        Hide
        Ralf Joachim added a comment -

        I finally got it. The new patch fixes the problem reported with this issue whitout any drawbacks. At least as far as I know yet. I guess if there is one I get blamed about that very soon again

        Show
        Ralf Joachim added a comment - I finally got it. The new patch fixes the problem reported with this issue whitout any drawbacks. At least as far as I know yet. I guess if there is one I get blamed about that very soon again

          People

          • Assignee:
            Ralf Joachim
            Reporter:
            M.-Leander Reimer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: