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

#3503 Add xslt.features support #3526

Merged
merged 1 commit into from Feb 8, 2022

Conversation

DenisIstomin
Copy link
Member

Related to #3503

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.

Thanks, @DenisIstomin this looks quite good!

Except for the inline suggestions, could you please add some test coverage to the test module under integration-tests/xml? Ideally a new XSLT file. If the functions implementations really have to be in a separate jar file, the you can create a new module under /integration-tests-support.

@DenisIstomin DenisIstomin force-pushed the 3503-xslt-extention-functions branch 2 times, most recently from b378dc3 to 2e686e6 Compare February 8, 2022 10:03
@DenisIstomin
Copy link
Member Author

Thanks @ppalaga, added an integration test for the extension functions.

@ppalaga
Copy link
Contributor

ppalaga commented Feb 8, 2022

Great work, thanks @DenisIstomin !

I am checking locally, whether I can make loading the functions from the current project work.
FWIW, it works in native mode because the classloader there is flat.

I have sent #3533 to unblock building this and other PRs

@ppalaga
Copy link
Contributor

ppalaga commented Feb 8, 2022

I am checking locally, whether I can make loading the functions from the current project work.
FWIW, it works in native mode because the classloader there is flat.

I think this is caused by the fact that TransformerFactory class is resolved using a wrong class loader around here in Camel https://github.com/apache/camel/blob/camel-3.15.0/components/camel-xslt/src/main/java/org/apache/camel/component/xslt/XsltEndpoint.java#L372

I'd say this PR is good to go like this with the known limitation.

I have filed a followup issue #3534 for the class loading problem.

@ppalaga
Copy link
Contributor

ppalaga commented Feb 8, 2022

The CI build should work now. Could you please squash your two commits, rebase on top of the recent master and force-push? - That should re-trigger the CI for this PR.

@DenisIstomin DenisIstomin force-pushed the 3503-xslt-extention-functions branch 2 times, most recently from 5927609 to ea1eed9 Compare February 8, 2022 17:11
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.

Thanks again, @DenisIstomin !

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.

None yet

2 participants