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

Support for camel-xml-io #849

Merged
merged 3 commits into from Mar 6, 2020
Merged

Support for camel-xml-io #849

merged 3 commits into from Mar 6, 2020

Conversation

lburgazzoli
Copy link
Contributor

Fixes #847

@lburgazzoli
Copy link
Contributor Author

With this PR, camel-quarkus-support-xml has no dependencies and the only task is to add reflective classes.

I'm not sure if this extension should be kept as it is (i.e. the name is misleading) but I think we need to have a better view about the requirements of other extensions in relation to XML so we may re-factor it when we have a better idea.

@ppalaga
Copy link
Contributor

ppalaga commented Mar 5, 2020

A general question: are all these new camel-xml-* extensions supposed to be used by the app devs directly? If not, they should rather be support extensions, should they not?

@lburgazzoli
Copy link
Contributor Author

lburgazzoli commented Mar 5, 2020

the user needs to decide what xml implementation is needed by its application and yes, we can certainly move them to support but there is the same artefact on camel side so to be consistent with the other extension I didn't put them to support folder

@ppalaga
Copy link
Contributor

ppalaga commented Mar 5, 2020

I am struggling to understand the purpose of camel-xml-jaxp and camel-xml-io (in Camel). camel-xml-jaxp is just an alias for camel-support and and camel-xml-io is just an alias for camel-core-engine. What is the actual reason for their existence?

@oscerd
Copy link
Contributor

oscerd commented Mar 5, 2020

If this is your idea probably you didn't look too deep in the camel code. They are not just alias.

@gnodet
Copy link
Contributor

gnodet commented Mar 5, 2020

@ppalaga we're trying to make camel a bit more modular. In addition to that, we want to have a camel which is lightweight enough in Quarkus. Previsouly, xml routes were supported through jaxb only. Now, we do support loading xml routes either using JAXB (using camel-xml-jaxb support) or using a generated lightweight parser (using camel-xml-io).
In the same idea, all the xml related support (mostly things that do use the JDK XML support) has been moved to camel-xml-jaxp.
The overall goal is that camel quarkus native binaries will be as light as possible, not including the xml parsers (xerces / xalan embedded in the JDK) or jaxb if not needed.

@ppalaga
Copy link
Contributor

ppalaga commented Mar 5, 2020

Ah, sorry, indeed there is actually some code in those modules. I am deformed by Camel Quarkus where we just wrap things.

@ppalaga
Copy link
Contributor

ppalaga commented Mar 5, 2020

OK, I hope I am starting to grasp this.

Here a couple of notes about this PR (that still may reside on my false assumptions; plz correct me in such cases):

  • The naming of the modules that follows the naming in Camel is a good thing.
  • If camel-xml-jaxb support and camel-xml-io are there for parsing routes from XML, their pom.xml descriptions (and yaml metadata) should say so. I do not understand them as a general purpose XML parser and a general purpose JAXB package. They are there for end user apps that require parsing routes from XML. They are not there for other Camel Quarkus extensions to get a general XML support (we may need to add new/adapt existing extensions for that). Right?
  • camel-xml-jaxp looks like a general purpose JAXP package to used by other camel components on the camel side and I assume it should be similar here in Camel Quarkus: camel-quarkus-xml-jaxp should be used by other extensions that need JAXP. End user apps should not depend on it directly. Right? If so, the current camel-quarkus-support-xml should be merged with camel-xml-jaxp. WDYT?

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nitpicks inline

extensions/xml-io/pom.xml Outdated Show resolved Hide resolved
extensions/xml-jaxb/deployment/pom.xml Outdated Show resolved Hide resolved
extensions/xml-jaxp/deployment/pom.xml Outdated Show resolved Hide resolved
@lburgazzoli
Copy link
Contributor Author

* If camel-xml-jaxb support and camel-xml-io are there for parsing routes from XML, their pom.xml descriptions (and yaml metadata) should say so. I do not understand them as a general purpose XML parser and a general purpose JAXB package. They are there for end user apps that require parsing routes from XML. They are not there for other Camel Quarkus extensions to get a general XML support (we may need to add new/adapt existing extensions for that). Right?

Not really, camel-xml-io contains an xml parser implementation as far as i know

* camel-xml-jaxp looks like a general purpose JAXP package to used by other camel components on the camel side and I assume it should be similar here in Camel Quarkus: camel-quarkus-xml-jaxp should be used by other extensions that need JAXP. End user apps should not depend on it directly. Right? If so, the current camel-quarkus-support-xml should be merged with camel-xml-jaxp. WDYT?

The artefact contains as example the XMLTokenizeLanguage so I think it is safe to let the user if/when to include it.

@ppalaga
Copy link
Contributor

ppalaga commented Mar 5, 2020

* If camel-xml-jaxb support and camel-xml-io are there for parsing routes from XML, their pom.xml descriptions (and yaml metadata) should say so. I do not understand them as a general purpose XML parser and a general purpose JAXB package. They are there for end user apps that require parsing routes from XML. They are not there for other Camel Quarkus extensions to get a general XML support (we may need to add new/adapt existing extensions for that). Right?

Not really, camel-xml-io contains an xml parser implementation as far as i know

Yes, there is a parser implementing Camels own interface org.apache.camel.xml.io.XmlPullParser and no other Camel component depends on it except for the core part that parses the XML routes. So, for me this does not look like a general purpose parser ATM. Is it supposed to become one in the future?

* camel-xml-jaxp looks like a general purpose JAXP package to used by other camel components on the camel side and I assume it should be similar here in Camel Quarkus: camel-quarkus-xml-jaxp should be used by other extensions that need JAXP. End user apps should not depend on it directly. Right? If so, the current camel-quarkus-support-xml should be merged with camel-xml-jaxp. WDYT?

The artefact contains as example the XMLTokenizeLanguage so I think it is safe to let the user if/when to include it.

OK, so we need a separate support JAXP extension for the native quirks - so camel-quarkus-support-xml could be renamed to camel-quarkus-support-jaxp. WDYT?

@lburgazzoli
Copy link
Contributor Author

* If camel-xml-jaxb support and camel-xml-io are there for parsing routes from XML, their pom.xml descriptions (and yaml metadata) should say so. I do not understand them as a general purpose XML parser and a general purpose JAXB package. They are there for end user apps that require parsing routes from XML. They are not there for other Camel Quarkus extensions to get a general XML support (we may need to add new/adapt existing extensions for that). Right?

Not really, camel-xml-io contains an xml parser implementation as far as i know

Yes, there is a parser implementing Camels own interface org.apache.camel.xml.io.XmlPullParser and no other Camel component depends on it except for the core part that parses the XML routes. So, for me this does not look like a general purpose parser ATM. Is it supposed to become one in the future?

That is a question for @gnodet

* camel-xml-jaxp looks like a general purpose JAXP package to used by other camel components on the camel side and I assume it should be similar here in Camel Quarkus: camel-quarkus-xml-jaxp should be used by other extensions that need JAXP. End user apps should not depend on it directly. Right? If so, the current camel-quarkus-support-xml should be merged with camel-xml-jaxp. WDYT?

The artefact contains as example the XMLTokenizeLanguage so I think it is safe to let the user if/when to include it.

OK, so we need a separate support JAXP extension for the native quirks - so camel-quarkus-support-xml could be renamed to camel-quarkus-support-jaxp. WDYT?

Yes that is something I was thinking about

@ppalaga
Copy link
Contributor

ppalaga commented Mar 5, 2020

we need a separate support JAXP extension for the native quirks - so camel-quarkus-support-xml could be renamed to camel-quarkus-support-jaxp. WDYT?

Yes that is something I was thinking about

Perfect. Please file a followup unless you want to amend now.

lburgazzoli added a commit to lburgazzoli/apache-camel-quarkus that referenced this pull request Mar 5, 2020
@lburgazzoli
Copy link
Contributor Author

we need a separate support JAXP extension for the native quirks - so camel-quarkus-support-xml could be renamed to camel-quarkus-support-jaxp. WDYT?

Yes that is something I was thinking about

Perfect. Please file a followup unless you want to amend now.

Will open a follow up issue as soon this get merged so I have the right pointers

lburgazzoli added a commit to lburgazzoli/apache-camel-quarkus that referenced this pull request Mar 5, 2020
@ppalaga
Copy link
Contributor

ppalaga commented Mar 5, 2020

The last open issue from my PoV is to document the purpose of the new extensions in their runtime/pom.xml <description>s and yaml metadata. Users should get an idea in which situations they need the dependency.

@lburgazzoli
Copy link
Contributor Author

should we fix it at camel side ? those extensions here are mainly wrapper

@ppalaga
Copy link
Contributor

ppalaga commented Mar 5, 2020

should we fix it at camel side ?

Ideally both.

@lburgazzoli
Copy link
Contributor Author

I think we should avoid to duplicate the effort and document how the xml thing wotk on camel side, we can link the page here but I’d avoid duplication

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A proposal inline how to improve the descriptions and the yaml metadata.

@gnodet
Copy link
Contributor

gnodet commented Mar 6, 2020

* If camel-xml-jaxb support and camel-xml-io are there for parsing routes from XML, their pom.xml descriptions (and yaml metadata) should say so. I do not understand them as a general purpose XML parser and a general purpose JAXB package. They are there for end user apps that require parsing routes from XML. They are not there for other Camel Quarkus extensions to get a general XML support (we may need to add new/adapt existing extensions for that). Right?

Not really, camel-xml-io contains an xml parser implementation as far as i know

Yes, there is a parser implementing Camels own interface org.apache.camel.xml.io.XmlPullParser and no other Camel component depends on it except for the core part that parses the XML routes. So, for me this does not look like a general purpose parser ATM. Is it supposed to become one in the future?

The goal is not to make the parser public. If other parts/components need a lightweight parser, why not, but it should be part of a public API for users.
The only goal was to avoid having the dependency on the whole JAXP stack, but if users actually need to process XML data, it's safer to get the usual public API/ implementations.

@ppalaga ppalaga merged commit 0ee6af8 into apache:master Mar 6, 2020
@lburgazzoli lburgazzoli deleted the github-847 branch March 6, 2020 09:43
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 this pull request may close these issues.

Support for camel-xml-io
4 participants