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 2021/05/07 09:20:56 UTC
[GitHub] [camel-quarkus] JiriOndrusek opened a new pull request #2560: Custom TypeConverter is not automatically registered #2260
JiriOndrusek opened a new pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560
fixes https://github.com/apache/camel-quarkus/issues/2260
@ppalaga @jamesnetherton I'm not sure about one thing, which causes user application to fail.
As you can see in `CamelProcessor`, I'm getting a class from its name (snippet is lower in this comment) and `StaticAnnotationTypeConverterLoader` iterates over the methods of this class. Therefore converter class has to be registered for reflection to work in native mode. Which is done by `CamelProcess`.
However if I try to use this feature from an app (not from a test - which is working). I can see:
- my `System.out.println` showing, that class is registered:
.. registering: [org.apache.camel.converter.NIOConverter, org.apache.camel.converter.CamelConverter, org.apache.camel.converter.CollectionConverter, org.apache.camel.converter.SQLConverter, org.apache.camel.converter.UriTypeConverter, org.apache.camel.converter.stream.StreamCacheConverter, org.apache.camel.converter.IOConverter, **org.acme.MyPairConverter**, org.apache.camel.converter.DurationConverter, org.apache.camel.converter.ObjectConverter, org.apache.camel.converter.DateTimeConverter]
- followed by an error:
````
Caused by: java.lang.ClassNotFoundException: org.acme.MyPairConverter
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581)
at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass(QuarkusClassLoader.java:421)
at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass(QuarkusClassLoader.java:371)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:315)
at org.apache.camel.quarkus.core.CamelRecorder.lambda$loadAnnotatedConverters$0(CamelRecorder.java:66)
````
I'm not sure why I can not create a class from its name if the class is registered for reflection (code from `CamelRecorder`)
````
public void loadAnnotatedConverters(RuntimeValue<TypeConverterRegistry> registry, Set<String> classNames) {
StaticAnnotationTypeConverterLoader.load(registry.getValue(), classNames.stream().map(className -> {
try {
return Class.forName(className);
} catch (ClassNotFoundException e) {
throw new RuntimeException(e);
}
}).collect(Collectors.toSet()));
}
````
The error from app is happening in **JVM** mode.
Do you know why does it happen?
<!-- 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek commented on a change in pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek commented on a change in pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#discussion_r628240625
##########
File path: extensions-core/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/CamelProcessor.java
##########
@@ -248,6 +251,14 @@ CamelTypeConverterRegistryBuildItem typeConverterRegistry(
}
}
+ Set<Class> convertersClasses = index
+ .getAnnotations(DotName.createSimple(Converter.class.getName()))
Review comment:
This make sense, I'll try to be more strict during filtering.
Just an idea: The other option is to remove loaders of internal converters and use this approach.
##########
File path: extensions-core/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/CamelProcessor.java
##########
@@ -248,6 +251,14 @@ CamelTypeConverterRegistryBuildItem typeConverterRegistry(
}
}
+ Set<Class> convertersClasses = index
+ .getAnnotations(DotName.createSimple(Converter.class.getName()))
Review comment:
This makes sense, I'll try to be more strict during filtering.
Just an idea: The other option is to remove loaders of internal converters and use this approach.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek edited a comment on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek edited a comment on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-834338942
@jamesnetherton thanks for the suggestion, that was the case.
This change also fixes https://github.com/apache/camel-quarkus/issues/1896, because it disables `AnnotationTypeConverterLoader` and replaces it with `StaticAnnotationTypeConverterLoader`, which does not use classpath discovery (it searches Jandex index instead)
~~Auto-discovering of `@Converter` classes (**not loaders**) has to be allowed by camel property `camel.main.loadTypeConverters=true` (see https://camel.apache.org/manual/latest/type-converter.html#_discovering_camel_2_x_based_type_converters_not_loader for more details)~~
~~If this fix is merged, I'll put a comment to https://github.com/apache/camel-quarkus/issues/2260, mentioning the requirement of `camel.main.loadTypeConverters=true`~~
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
ppalaga commented on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-836697597
Let me have a look later today
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] ppalaga merged pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
ppalaga merged pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek commented on a change in pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek commented on a change in pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#discussion_r629107053
##########
File path: extensions-core/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/CamelProcessor.java
##########
@@ -248,6 +251,14 @@ CamelTypeConverterRegistryBuildItem typeConverterRegistry(
}
}
+ Set<Class> convertersClasses = index
+ .getAnnotations(DotName.createSimple(Converter.class.getName()))
Review comment:
I filtered out all converters, which belong to `org.apache.camel:camel-*`. This also fixes all the failed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek commented on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek commented on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-836666024
PR is ready for review/merge
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] jamesnetherton commented on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
jamesnetherton commented on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-834215058
I'm guessing it's a `ClassLoader` issue. Have you tried passing a specific `ClassLoader` to `Class.forName`? E.g `Class.forName("foo", true, Thread.currentThread().getContextClassLoader())`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek commented on a change in pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek commented on a change in pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#discussion_r628271433
##########
File path: extensions-core/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/CamelProcessor.java
##########
@@ -248,6 +251,14 @@ CamelTypeConverterRegistryBuildItem typeConverterRegistry(
}
}
+ Set<Class> convertersClasses = index
+ .getAnnotations(DotName.createSimple(Converter.class.getName()))
Review comment:
@jamesnetherton I've found a way of filtering out classes from artifacts identified by GroupId and artifactId. I'll filter out internal libraries of camel (which contains converters). From my POV it is OK, what do you think?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek commented on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek commented on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-834216637
@jamesnetherton thanks for the suggestion, I'll try it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek commented on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek commented on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-834207287
There is a compilation problem which does not have impact on my question/
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek commented on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek commented on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-834354577
PR is ready for review/merge
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek edited a comment on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek edited a comment on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-834340894
~~Both cases (camel.main.loadTypeConverters=true|false) are tested.~~
~~PR is ready for review/merge.~~
EDIT I'll change the behavior regarding 'camel.main.loadTypeConverters=true' as is described here https://camel.apache.org/components/latest/others/cdi.html#_auto_configured_type_converters
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] jamesnetherton commented on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
jamesnetherton commented on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-836674520
@lburgazzoli @ppalaga 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] jamesnetherton commented on a change in pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
jamesnetherton commented on a change in pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#discussion_r628195026
##########
File path: extensions-core/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/CamelProcessor.java
##########
@@ -248,6 +251,14 @@ CamelTypeConverterRegistryBuildItem typeConverterRegistry(
}
}
+ Set<Class> convertersClasses = index
+ .getAnnotations(DotName.createSimple(Converter.class.getName()))
Review comment:
We probably need to do some more filtering here. I think we only really want to scan converters added by the user. Otherwise it'll pick up the internal camel converters which already have their own loader classes 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek commented on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek commented on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-834338942
@jamesnetherton thanks for the suggestion, that was the case.
This change also fixes https://github.com/apache/camel-quarkus/issues/1896, because it disables `AnnotationTypeConverterLoader` and replaces it with `StaticAnnotationTypeConverterLoader`, which does not use classpath discovery (it searches Jandex index instead)
Auto-discovering of `@Converter` classes (**not loaders**) has to be allowed by camel property `camel.main.loadTypeConverters=true` (see https://camel.apache.org/manual/latest/type-converter.html#_discovering_camel_2_x_based_type_converters_not_loader for more details)
If this fix is merged, I'll put a comment to https://github.com/apache/camel-quarkus/issues/2260, mentioning the requirement of `camel.main.loadTypeConverters=true`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek commented on a change in pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek commented on a change in pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#discussion_r628244230
##########
File path: extensions-core/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/CamelProcessor.java
##########
@@ -248,6 +251,14 @@ CamelTypeConverterRegistryBuildItem typeConverterRegistry(
}
}
+ Set<Class> convertersClasses = index
+ .getAnnotations(DotName.createSimple(Converter.class.getName()))
Review comment:
To mimic the functionality in camel, we should filter out all `org.apache.camel.core.*` as is used [here](https://github.com/Talend/apache-camel/blob/master/camel-core/src/main/java/org/apache/camel/impl/converter/AnnotationTypeConverterLoader.java#L104)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] ppalaga commented on a change in pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
ppalaga commented on a change in pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#discussion_r629430440
##########
File path: extensions-core/core/runtime/src/main/java/org/apache/camel/quarkus/core/StaticAnnotationTypeConverterLoader.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.quarkus.core;
+
+import java.util.Set;
+
+import org.apache.camel.impl.converter.AnnotationTypeConverterLoader;
+import org.apache.camel.spi.TypeConverterRegistry;
+
+public class StaticAnnotationTypeConverterLoader extends AnnotationTypeConverterLoader {
+
+ private static StaticAnnotationTypeConverterLoader INSTANCE = new StaticAnnotationTypeConverterLoader();
+
+ public static StaticAnnotationTypeConverterLoader getInstance() {
+ return INSTANCE;
+ }
Review comment:
Another nitpick: Do we really need the singleton? It seems to be used only once. I mean we could replace `StaticAnnotationTypeConverterLoader.getInstance()` with simply `new StaticAnnotationTypeConverterLoader()`. It would be less code doing the same thing.
##########
File path: extensions-core/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/CamelProcessor.java
##########
@@ -248,6 +251,28 @@ CamelTypeConverterRegistryBuildItem typeConverterRegistry(
}
}
+ Set<String> internalConverters = new HashSet<>();
+ //ignore all @converters from org.apache.camel:camel-* dependencies
+ for (ApplicationArchive archive : applicationArchives.getAllApplicationArchives()) {
+ if (archive.getArtifactKey() != null && "org.apache.camel".equals(archive.getArtifactKey().getGroupId())
+ && archive.getArtifactKey().getArtifactId().startsWith("camel-")) {
+ internalConverters.addAll(archive.getIndex().getAnnotations(DotName.createSimple(Converter.class.getName()))
+ .stream().filter(a -> a.target().kind() == AnnotationTarget.Kind.CLASS)
+ .map(a -> a.target().asClass().name().toString())
+ .collect(Collectors.toSet()));
+ }
+ }
+
+ Set<Class> convertersClasses = index
Review comment:
```suggestion
Set<Class<?>> convertersClasses = index
```
would eliminate some compiler warnings
##########
File path: extensions-core/core/deployment/src/main/java/org/apache/camel/quarkus/core/deployment/CamelProcessor.java
##########
@@ -248,6 +251,28 @@ CamelTypeConverterRegistryBuildItem typeConverterRegistry(
}
}
+ Set<String> internalConverters = new HashSet<>();
+ //ignore all @converters from org.apache.camel:camel-* dependencies
+ for (ApplicationArchive archive : applicationArchives.getAllApplicationArchives()) {
+ if (archive.getArtifactKey() != null && "org.apache.camel".equals(archive.getArtifactKey().getGroupId())
+ && archive.getArtifactKey().getArtifactId().startsWith("camel-")) {
+ internalConverters.addAll(archive.getIndex().getAnnotations(DotName.createSimple(Converter.class.getName()))
+ .stream().filter(a -> a.target().kind() == AnnotationTarget.Kind.CLASS)
+ .map(a -> a.target().asClass().name().toString())
+ .collect(Collectors.toSet()));
Review comment:
Nitpick: this would eliminate creating as many sets as there are app archives:
```suggestion
archive.getIndex().getAnnotations(DotName.createSimple(Converter.class.getName()))
.stream().filter(a -> a.target().kind() == AnnotationTarget.Kind.CLASS)
.map(a -> a.target().asClass().name().toString())
.forEach(internalConverters::add);
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek commented on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek commented on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-834340894
Both cases (camel.main.loadTypeConverters=true|false) are tested.
PR is ready for review/merge.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] JiriOndrusek edited a comment on pull request #2560: Custom TypeConverter is not automatically registered #2260
Posted by GitBox <gi...@apache.org>.
JiriOndrusek edited a comment on pull request #2560:
URL: https://github.com/apache/camel-quarkus/pull/2560#issuecomment-834340894
Both cases (camel.main.loadTypeConverters=true|false) are tested.
PR is ready for review/merge.
EDIT I'll change the behavior regarding 'camel.main.loadTypeConverters=true' as is described here https://camel.apache.org/components/latest/others/cdi.html#_auto_configured_type_converters
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org