You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2022/08/19 02:16:11 UTC
[GitHub] [camel-quarkus] zhfeng opened a new pull request, #4018: Fix #3904 increase xslt extension test coverage
zhfeng opened a new pull request, #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018
- [x] Add `ref`, `bean`, `http`, `file` schemas which could only work in jvm mode since generating xslt template classes dynamically does not be supported in native mode.
- [ ] Different outputs (string, bytes, dom, file)
- [ ] Using xsl:include to include files
- [x] Add aggergate test
- [x] Add custom URIResolver
- [ ] Caching
- [ ] Being able to raise errors with xsl:message and messages getting into `Exchange.XSLT_ERROR`, `Exchange.XSLT_FATAL_ERROR`, or `Exchange.XSLT_WARNING`
<!-- Uncomment and fill this section if your PR is not trivial
[ ] An issue should be filed for the change unless this is a trivial change (fixing a typo or similar). One issue should ideally be fixed by not more than one commit and the other way round, each commit should fix just one issue, without pulling in other changes.
[ ] Each commit in the pull request should have a meaningful and properly spelled subject line and body. Copying the title of the associated issue is typically enough. Please include the issue number in the commit message prefixed by #.
[ ] The pull request description should explain what the pull request does, how, and why. If the info is available in the associated issue or some other external document, a link is enough.
[ ] Phrases like Fix #<issueNumber> or Fixes #<issueNumber> will auto-close the named issue upon merging the pull request. Using them is typically a good idea.
[ ] Please run mvn process-resources -Pformat (and amend the changes if necessary) before sending the pull request.
[ ] Contributor guide is your good friend: https://camel.apache.org/camel-quarkus/latest/contributor-guide.html
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on a diff in pull request #4018: Fix #3904 increase xslt extension test coverage
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018#discussion_r954455764
##########
integration-tests/xml/pom.xml:
##########
@@ -35,14 +35,30 @@
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-xslt</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-xslt-saxon</artifactId>
+ </dependency>
<dependency>
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-xpath</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-bean</artifactId>
+ </dependency>
<dependency>
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-direct</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-file</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-mock</artifactId>
+ </dependency>
Review Comment:
OK, let's raise a follow up issue to group the xml tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on a diff in pull request #4018: Fix #3904 increase xslt extension test coverage
Posted by GitBox <gi...@apache.org>.
ppalaga commented on code in PR #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018#discussion_r950036555
##########
integration-tests/xml/pom.xml:
##########
@@ -35,14 +35,30 @@
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-xslt</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-xslt-saxon</artifactId>
+ </dependency>
<dependency>
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-xpath</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-bean</artifactId>
+ </dependency>
<dependency>
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-direct</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-file</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-mock</artifactId>
+ </dependency>
Review Comment:
@zhfeng I have some concerns about adding this many new dependencies to the test. I mean it is generally better to test with as little dependencies as possible, so that it does not happen that an extension that is there just for testing some niche feature brings some build steps that become vital for the execution of some basic functionality.
Wouldn't it perhaps be worthwhile to introduce a new XML testing group like we already have with [Azure, AWS an others](https://github.com/apache/camel-quarkus/tree/main/integration-test-groups)? We could have several XML modules there that would test the use cases coverable with their minimal set of deps and ideally still be able to run them all together on the CI (in the grouped module), so that the exec time does not explode. See also https://camel.apache.org/camel-quarkus/2.11.x/contributor-guide/extension-testing.html#_grouping
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on a diff in pull request #4018: Fix #3904 increase xslt extension test coverage
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018#discussion_r952302516
##########
integration-tests/xml/pom.xml:
##########
@@ -35,14 +35,30 @@
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-xslt</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-xslt-saxon</artifactId>
+ </dependency>
<dependency>
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-xpath</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-bean</artifactId>
+ </dependency>
<dependency>
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-direct</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-file</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-mock</artifactId>
+ </dependency>
Review Comment:
@ppalaga `camel-quarkus-bean` is introduce to test with `xslt:bean` schema. And `camel-quarkus-file` and `camel-qurakus-xslt-saxon` are for testing with `Aggregator`.
I think it is good idea to introduce a new XML testing group. But how to group the sub modules? such like `xml-schema` to test all the supported schemas(`classpath`, `ref`, `bean`, `file`, `http`), `xml-aggregate`, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on a diff in pull request #4018: Fix #3904 increase xslt extension test coverage
Posted by GitBox <gi...@apache.org>.
ppalaga commented on code in PR #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018#discussion_r952666665
##########
integration-tests/xml/pom.xml:
##########
@@ -35,14 +35,30 @@
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-xslt</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-xslt-saxon</artifactId>
+ </dependency>
<dependency>
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-xpath</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-bean</artifactId>
+ </dependency>
<dependency>
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-direct</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-file</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-mock</artifactId>
+ </dependency>
Review Comment:
> But how to group the sub modules?
The grouping is given by the required dependencies. I.e. if the `bean:` test protocol requires an additional `camel-quarkus-bean` dependency, then it should be a separate test module with that dependency under the new group.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on a diff in pull request #4018: Fix #3904 increase xslt extension test coverage
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018#discussion_r1002829410
##########
integration-tests/xml/src/main/java/org/apache/camel/quarkus/component/xml/it/XmlRouteBuilder.java:
##########
@@ -18,9 +18,17 @@
import org.w3c.dom.Document;
+import io.quarkus.runtime.annotations.RegisterForReflection;
import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.xslt.saxon.XsltSaxonAggregationStrategy;
import org.apache.camel.support.builder.Namespaces;
+@RegisterForReflection(classNames = {
+ "net.sf.saxon.Configuration",
+ "net.sf.saxon.functions.String_1",
+ "net.sf.saxon.functions.Tokenize_1",
+ "net.sf.saxon.functions.StringJoin",
+ "org.apache.camel.component.xslt.saxon.XsltSaxonBuilder" })
Review Comment:
yeah, I will leave a comment here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng merged pull request #4018: Fix #3904 increase xslt extension test coverage
Posted by GitBox <gi...@apache.org>.
zhfeng merged PR #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on a diff in pull request #4018: Fix #3904 increase xslt extension test coverage
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018#discussion_r1002828444
##########
extensions/xslt/runtime/src/main/doc/configuration.adoc:
##########
@@ -9,7 +9,22 @@ quarkus.camel.xslt.sources = transform.xsl, classpath:path/to/my/file.xsl
Scheme-less URIs are interpreted as `classpath:` URIs.
-Only `classpath:` URIs are supported on Quarkus. `file:`, `http:` and other kinds of URIs do not work by design.
+Only `classpath:` URIs are supported on Quarkus native mode. `file:`, `http:` and other kinds of URIs can be used on JVM mode only.
+
+`xsl:include` and `xsl:messaging` are aslo supported in JVM mode only right now.
Review Comment:
It looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on a diff in pull request #4018: Fix #3904 increase xslt extension test coverage
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018#discussion_r1002828734
##########
extensions/xslt/runtime/src/main/java/org/apache/camel/quarkus/component/xslt/CamelXsltRecorder.java:
##########
@@ -52,6 +55,23 @@ public void addRuntimeUriResolverEntry(RuntimeValue<RuntimeUriResolver.Builder>
builder.getValue().entry(templateUri, transletClassName);
}
+ static class QuarkusXsltUriResolverFactory extends DefaultXsltUriResolverFactory {
+ private final RuntimeUriResolver uriResolver;
+
+ public QuarkusXsltUriResolverFactory(RuntimeUriResolver uriResolver) {
+ this.uriResolver = uriResolver;
+ }
+
+ @Override
+ public URIResolver createUriResolver(CamelContext camelContext, String resourceUri) {
+ if (uriResolver.getTransletClassName(resourceUri) != null) {
+ return uriResolver;
+ } else {
+ return super.createUriResolver(camelContext, resourceUri);
+ }
Review Comment:
yeah, it is right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on pull request #4018: Fix #3904 increase xslt extension test coverage
Posted by GitBox <gi...@apache.org>.
zhfeng commented on PR #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018#issuecomment-1286390781
@ppalaga please can you take a review of these changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on a diff in pull request #4018: Fix #3904 increase xslt extension test coverage
Posted by GitBox <gi...@apache.org>.
ppalaga commented on code in PR #4018:
URL: https://github.com/apache/camel-quarkus/pull/4018#discussion_r1001723760
##########
integration-tests/xml/src/main/java/org/apache/camel/quarkus/component/xml/it/XmlRouteBuilder.java:
##########
@@ -18,9 +18,17 @@
import org.w3c.dom.Document;
+import io.quarkus.runtime.annotations.RegisterForReflection;
import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.xslt.saxon.XsltSaxonAggregationStrategy;
import org.apache.camel.support.builder.Namespaces;
+@RegisterForReflection(classNames = {
+ "net.sf.saxon.Configuration",
+ "net.sf.saxon.functions.String_1",
+ "net.sf.saxon.functions.Tokenize_1",
+ "net.sf.saxon.functions.StringJoin",
+ "org.apache.camel.component.xslt.saxon.XsltSaxonBuilder" })
Review Comment:
I see that camel-quarkus-xslt-saxon is JVM only.
If porting camel-quarkus-xslt-saxon to native would be too much work, then it would be worthwhile to put a comment here saying that `@RegisterForReflection` should be removed with fixing https://github.com/apache/camel-quarkus/issues/1615
In https://github.com/apache/camel-quarkus/issues/1615, we should note that XmlRouteBuilder.java needs some care
##########
extensions/xslt/runtime/src/main/java/org/apache/camel/quarkus/component/xslt/CamelXsltRecorder.java:
##########
@@ -52,6 +55,23 @@ public void addRuntimeUriResolverEntry(RuntimeValue<RuntimeUriResolver.Builder>
builder.getValue().entry(templateUri, transletClassName);
}
+ static class QuarkusXsltUriResolverFactory extends DefaultXsltUriResolverFactory {
+ private final RuntimeUriResolver uriResolver;
+
+ public QuarkusXsltUriResolverFactory(RuntimeUriResolver uriResolver) {
+ this.uriResolver = uriResolver;
+ }
+
+ @Override
+ public URIResolver createUriResolver(CamelContext camelContext, String resourceUri) {
+ if (uriResolver.getTransletClassName(resourceUri) != null) {
+ return uriResolver;
+ } else {
+ return super.createUriResolver(camelContext, resourceUri);
+ }
Review Comment:
A comment would be nice about what these two options are supposed to do. The first one is supposed to catch the cases where we compile the translet at build time, right? Which is for `classpath:` XSLT resources in both JVM and native mode?
The `else` branch is for all other cases, right?
##########
extensions/xslt/runtime/src/main/java/org/apache/camel/quarkus/component/xslt/RuntimeUriResolver.java:
##########
@@ -60,11 +60,7 @@ public Source resolve(String href, String base) throws TransformerException {
* @return the unqualified translet name associated with the given {@code uri}
Review Comment:
Just guessing - please correct me if needed:
```suggestion
* @return the unqualified translet name associated with the given {@code uri} or {@code null} if the given XSLT resource was not compiled to a translet at build time.
```
##########
extensions/xslt/runtime/src/main/doc/configuration.adoc:
##########
@@ -9,7 +9,22 @@ quarkus.camel.xslt.sources = transform.xsl, classpath:path/to/my/file.xsl
Scheme-less URIs are interpreted as `classpath:` URIs.
-Only `classpath:` URIs are supported on Quarkus. `file:`, `http:` and other kinds of URIs do not work by design.
+Only `classpath:` URIs are supported on Quarkus native mode. `file:`, `http:` and other kinds of URIs can be used on JVM mode only.
+
+`xsl:include` and `xsl:messaging` are aslo supported in JVM mode only right now.
Review Comment:
```suggestion
`<xsl:include>` and `<xsl:messaging>` XSLT elements are also supported in JVM mode only right now.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org