Error loading mapping whem migrating from 1.0.1 to 1.0.3

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

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

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 ... .

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

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?

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.

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.

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.

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

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 ?

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.

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.

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?

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?

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.

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?

Patch for review.

+1.

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.

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.

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?

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