[
https://issues.apache.org/jira/browse/XMLBEANS-636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18132640#comment-18132640
]
François Rajotte commented on XMLBEANS-636:
-------------------------------------------
I saw activity in the mailing about a problem I thought was similar to this
one: XMLBEANS-661
Although it turned out to be unrelated, I did spend some more time on this
problem and thought I'd share some of the things I found.
I now believe that the regression with regards to multi-schema bean factory
methods was specifically introduced by this commit:
https://github.com/apache/poi-xmlbeans/commit/7a6f8e3159c897335aa1c96fac9b64f813ba2b53#diff-d6b1647188c1a397a29bdd622f14cc9e51fe2f9f63a8532bc2e96ac6584a90cf
In this commit, the SchemaTypeCodePrinter that is used to build the generated
java classes was updated. The generated Factory classes were changed to use the
SchemaTypeSystem (held by the TypeSystemHolder) rather than the context type
loader (XmlBeans.getContextTypeLoader).
With this change, the convenience Factory classes are now unable to resolve
types that are outside their own type system.
There are still remnants of the old behavior that remain documented in the
javadoc such as this comment here that mentions that the context type loader is
used by the convenient factory methods:
https://github.com/apache/poi-xmlbeans/blob/fd4458788634baa390de13796a09942a0ffa0a53/src/main/java/org/apache/xmlbeans/SchemaTypeLoader.java#L48-L57
Types that span multiple type systems are not really tested by the existing
tests. However, I did find one minor side effect that could have been observed
at the time.
In the ElementRestriction test, an element that was previously of concrete type
XmlString became a more generic XmlAnyType. This regression was found later
during the migration from maven to gradle and was addressed by changing the
test:
https://github.com/apache/poi-xmlbeans/commit/df29cd7d19ad683db5ca596b69155aa8625e3361#diff-33f73d05d94355392159e6d40f52d45d813a08f6f84ed29b63dc58992cd3dde6L82-R83
The reason why this regression had not been found sooner is because a previous
refactor (XMLBEANS-532) had disabled the test classes that did not end in
Test.java or Tests.java:
https://github.com/apache/poi-xmlbeans/blob/258e01096ca3f85cb4e71bfbb0828d6159dac6f8/build.xml#L418-L419
In any case, this small side effect is probably undefined behavior. But I
thought it was interesting to mention nonetheless.
I am not sure of the rationale of moving the Factory methods away from the
context type loader. It seems to have been somehow related to enhancements
related to java9 support. And I'm not sure what kind of issues we would have if
we brought back the context Type Loader by default for Factory classes.
If we were to revisit the Factory type loaders, I believe it would have to be
in the ElementFactory class. The getTypeLoader() method
(https://github.com/apache/poi-xmlbeans/blob/fd4458788634baa390de13796a09942a0ffa0a53/src/main/java/org/apache/xmlbeans/impl/schema/ElementFactory.java#L36-L38)
could be updated to return a different type loader.
However, there are a few problems with such a change.
One problem is that the getTypeLoader() method is public and therefore exposed
in the generated beans. Despite its name, this method's signature forces the
return type to be of SchemaTypeSystem rather than the more generic
SchemaTypeLoader. Changing the return type to match the method name would
introduce backwards-compatibility issues with the generated beans. To work
around this limitation, we could introduce a new package-private method that
returns a SchemaTypeLoader, for internal use and update all the factory classes
to use it rather than getTypeLoader().
If, for some reason, we still want to prefer looking up types in the local
schema type system before interrogating the context type loader (for
performance reasons perhaps?), we could build a TypeLoader union (similar to
this:
https://github.com/apache/poi-xmlbeans/blob/fd4458788634baa390de13796a09942a0ffa0a53/src/main/java/org/apache/xmlbeans/XmlBeans.java#L407-L414)
which checks the local type system first before checking the context type
loader.
I've worked on some prototypes of these changes and it appears to solve the
issue at first glance. But I'm a bit worried because I don't fully understand
why we moved away from the context type loader in the first place.
> Incorrect processing of abstract XML types defined in multiple schema files
> ---------------------------------------------------------------------------
>
> Key: XMLBEANS-636
> URL: https://issues.apache.org/jira/browse/XMLBEANS-636
> Project: XMLBeans
> Issue Type: Bug
> Affects Versions: Version 4.0.0, Version 5.1.1
> Reporter: François Rajotte
> Priority: Major
> Attachments: AbstractType_xmlbeans-3.0.0.log,
> AbstractType_xmlbeans-5.1.1.log
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> We have observed a regression when upgrading XMLBeans from version 3.0.0 to
> 5.1.1.
> The issue is related to how concrete and abstract types are handled in
> XMLBeans when they are defined in different schema files.
> I have enhanced the existing "AbstractTypes" sample provided in the XMLBeans
> repository to show the regression. You can find the code in my private branch
> here:
> [https://github.com/JeSuisAbraham/xmlbeans/tree/AbstractTypes/samples/AbstractTypes]
> The "AbstractTypes" sample consists of two schema files.
> The abstractBase.xsd schema defines the document root, which contains
> abstract "figures".
> The myfigures.xsd schema contains concrete implementations of these abstract
> figures: a "square" and a "circle".
> Three tests are performed:
> * buildDocument: builds a document "top-down". Creates a document, adds
> abstract objects and specializes them into concrete objects.
> * buildDocument2: builds a document "down-top". Creates a document, creates
> the concrete objects and adds them to the document.
> * parseDocument: parses a document containing concrete types.
> All of these tests are broken in XMLBeans 4.+ whereas they worked in XMLBeans
> 3.x.
> I have attached the output of this test program when ran against XMLBeans
> 3.0.0 and XMLBeans 5.1.1 for comparison.
> The differences between v3.0.0 and v5.1.1 are:
> * buildDocument: In v5.1.1, the test throws a ClassCastException when trying
> to coerce an abstract object down to a concrete implementation.
> * buildDocument2: In v5.1.1, the document output does not contain the
> xsi:type attributes. The resulting document is not schema-valid.
> * parseDocument: In v5.1.1, the concrete objects are not recognized as such.
> They remain of the abstract type. The parsed document object in not
> schema-valid.
>
> Using a debugger, we've found the following differences between v3.0.0 and
> v5.1.1:
> The concrete types are resolved here using a SchemaTypeLoader:
> [https://github.com/apache/xmlbeans/blob/fb3f015826f1378b10178b7d4fe71bfe133c30ca/src/main/java/org/apache/xmlbeans/impl/schema/SchemaTypeImpl.java#L937]
> In 3.x.x, the schema type loader is of type "SchemaTypeLoaderImpl", and will
> fetch both locally in its own schema and in other schema type systems loaded
> into the system:
> [https://github.com/apache/xmlbeans/blob/a7cdcb1a37319d69eeef31a90badf9d2bb02de81/src/main/java/org/apache/xmlbeans/impl/schema/SchemaTypeLoaderImpl.java#L372]
> In 5.x.x, the schema type loader is of type "TypeSchemaHolder", defined in
> the bean jar itself, and extends "SchemaTypeSystemImpl". This schema type
> loader only looks for types into its own schema:
> [https://github.com/apache/xmlbeans/blob/a7cdcb1a37319d69eeef31a90badf9d2bb02de81/src/main/java/org/apache/xmlbeans/impl/schema/SchemaTypeSystemImpl.java#L1009]
> This change in behavior seems related to XMLBEANS-536.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
