Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue caused by MSV shading #103

Closed
coheigea opened this issue Apr 16, 2020 · 9 comments · Fixed by #106
Closed

Issue caused by MSV shading #103

coheigea opened this issue Apr 16, 2020 · 9 comments · Fixed by #106
Milestone

Comments

@coheigea
Copy link
Contributor

Apache CXF currently uses Woodstox 5.3.0. However, we've run into a problem in looking into upgrading to Woodstox 6.x, due to the MSV shading. We have a MSV BaseSchemaFactory implementation here:

https://github.com/apache/cxf/blob/cff773f4f750b7bdc3f955c1a5b1b5f4a311dd71/core/src/main/java/org/apache/cxf/staxutils/validation/W3CMultiSchemaFactory.java

This won't work with Woodstox 6.x, as the Woodstox W3CSchema class now takes the shaded MSV XMLSchemaGrammar in the constructor. I can change all references to MSV to the shaded version. However then CXF won't work with older versions of Woodstox, which might be a problem in some projects that use both CXF and an older Woodstox version.

Is there a way around this that doesn't involve completely porting W3CSchema and all associated dependencies to CXF to avoid uses the shaded package name?

@cowtowncoder
Copy link
Member

Hmmmh. Shoot. Yes, this is unfortunate. I am not quite sure how to resolve this...
Presumably would have to use some other, shared non-shaded abstraction to pass information. I would not expect use of shaded version, unless Woodstox itself was to provide it.

I'll have to look into this later today if I find time. Thank you for reporting it.

@cowtowncoder
Copy link
Member

Ok so leaving W3CSchema constructor public was probably a mistake as it was not meant as an extension point (even at point when shading was not used).
Same goes for RelaxNGSchema as well.

But be that as it may, I think there are 2 options that could be done:

  1. Add a new factory method or methods in W3CSchemaFactory which could be called to read schema definition from some other intermediate form
  2. Serialize/re-parse W3CSchema, construct InputSource

Of these, (2) feels sort of wrong (but looks like something similar was already done in "loadSchemas()"?) but could be done without changes to Woodstox. Adds more processing overhead.

I would be happy to do (1) if we could figure out what such intermediate form could be.
If so, could release 6.2.0 (minor version upgrade since there's API addition).
But not quite sure what intermediate type could be used... would ideally be something in JDK 6 I think, although at this point in time I could consider requiring JDK8 as baseline.

WDYT?

@dkulp
Copy link
Contributor

dkulp commented Apr 23, 2020

Would a "modified copy" of the W3CMultiSchemaFactory directly into Woodstox work for (1)? Looking at it, there are basically 3 classes. The W3CMultiSchemaFactory itself, the ResolvingGrammarReaderController, and the EmbeddedSchema. The EmbeddedSchema could just be a javax.transform.Source object (and we'd use DOMSource). The ResolvingGrammarReaderController could easily be a private class of W3CMultiSchemaFactory.

@cowtowncoder
Copy link
Member

I'll let @coheigea comment on whether this would work CXF (I certainly hope so).
I have one note on implementation itself, but will add a comment on PR itself.

@coheigea
Copy link
Contributor Author

coheigea commented Apr 24, 2020

@cowtowncoder Yes, it works for CXF. We will probably alter the existing CXF implementation along the lines in the PR, and then use reflection to determine if the current version of Woodstox has the class or not, falling back to the CXF version if not. That way we should be able to work with Woodstox 6.2 as well as versions < Woodstox 6.

Corresponding CXF PR: https://github.com/apache/cxf/pull/588/files

@cowtowncoder
Copy link
Member

Ok sounds good. I also have 5.3 branch and could backport this if it makes sense -- however, it might not given that there are already versions out there without method, meaning that reflection-based approach needed anyway.

@coheigea
Copy link
Contributor Author

Agreed, I think it's not necessary to backport to 5.3.

cowtowncoder pushed a commit that referenced this issue Apr 25, 2020
#106)

Fix #103: add Class to address issue of MSV being shaded making it hard/impossible for other applications that need more complex XMLValidationSchema creation.
cowtowncoder added a commit that referenced this issue Apr 25, 2020
@cowtowncoder cowtowncoder added this to the 6.2.0 milestone Apr 25, 2020
@cowtowncoder
Copy link
Member

cowtowncoder commented Apr 25, 2020

Merged, released as 6.2.0 along with other 2 fixes. Usually would have given bit more time to test but feeling anxious to get this out and then proceed with Jackson 2.11.0.

@coheigea
Copy link
Contributor Author

Thanks @cowtowncoder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants