[GitHub] [camel] JiriOndrusek opened a new pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] JiriOndrusek opened a new pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox

JiriOndrusek opened a new pull request #5299:
URL: https://github.com/apache/camel/pull/5299


   Issue: https://issues.apache.org/jira/browse/CAMEL-16459
   
   <!-- Uncomment and fill this section if your PR is not trivial
   - [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it).  Trivial changes like typos do not require a JIRA issue.  Your pull request should address just this issue, without pulling in other changes.
   - [ ] Each commit in the pull request should have a meaningful subject line and body.
   - [ ] If you're unsure, you can format the pull request title like `[CAMEL-XXX] Fixes bug in camel-file component`, where you replace `CAMEL-XXX` with the appropriate JIRA issue.
   - [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
   - [ ] Run `mvn clean install -Psourcecheck` in your module with source check enabled to make sure basic checks pass and there are no checkstyle violations. A more thorough check will be performed on your pull request automatically.
   Below are the contribution guidelines:
   https://github.com/apache/camel/blob/master/CONTRIBUTING.md
   -->
   


--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] davsclaus commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox

davsclaus commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r607784012



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       I would like to see if we can find a solution without catching a runtime exception and then try again with stream mode.
   
   Instead we should probably check the exchange body and find the right read method to use from the start.
   
   Any processing that causes an exception to be swallowed is a bad smell and also a performance overhead.




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] JiriOndrusek commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

JiriOndrusek commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r607785064



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       @davsclaus It makes sense, I'll rework it to decide correctly before an exception is thrown. Thanks for the suggestion.




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] JiriOndrusek commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

JiriOndrusek commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r607810643



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       @davclaus I reworked fix to detect `ertx.Buffer` to convert buffer to `InputStream` and handle as an `InputStream.`.
   
   The other option I see is to detect `vertx.Buffer` and use method with InputStream, which uses automatic conversion - which works too.
   
   But current solution would avoid automatic types convertion and should have a sligfhtly better performance.
   
   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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] JiriOndrusek commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

JiriOndrusek commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r607812580



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       The only possible problem could be addition of  dependency `<artifactId>camel-vertx-common</artifactId>`. But I don't see another option how to detect `vertx.Buffer` without it. (so it would be present in both options)
   
   I haven't check karaf feature yet, it has to possible fixed for the current change.




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] JiriOndrusek commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

JiriOndrusek commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r607812580



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       The only possible problem could be addition of  dependency `<artifactId>camel-vertx-common</artifactId>`. But I don't see another option how to detect `vertx.Buffer` without it. (so it would be present in both options)
   
   I haven't checked karaf feature yet, it could require a fix too (because of a new dependency).




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] JiriOndrusek commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

JiriOndrusek commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r607810643



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       @davsclaus I reworked fix to detect `ertx.Buffer` to convert buffer to `InputStream` and handle as an `InputStream.`.
   
   The other option I see is to detect `vertx.Buffer` and use method with InputStream, which uses automatic conversion - which works too.
   
   But current solution would avoid automatic types convertion and should have a sligfhtly better performance.
   
   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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] davsclaus commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

davsclaus commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r607853233



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       We cannot make camel-jsonpath dependent on vert.x. Instead the automatic type converters via Camel is fine to use, to convert the payload to inputstream and use that.




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] JiriOndrusek commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

JiriOndrusek commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r607878542



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       ok, thanks for explaining. I'll use automatic conversion.




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] JiriOndrusek commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

JiriOndrusek commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r607934362



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       @davsclaus
   There is no way, how to detect that adapter conversion fails in such case (`vertx.Buffer` in this case). Unfortunately default adapter in case of `vertx.Buffer` does not return `null` (which means - can not convert), but return map{"bytes": ... "bytesBuff": ...}, which seems to be a valid conversion with TRACE message.
   > JacksonJsonAdapter converted object from: {} to: java.util.Map"
   
   Execution of jsonpath fails afterwards, but without an option to detect why.
   
   The only way of detecting "unknown" content (`vertx.Buffer` in this case) is to try to convert content into `InputStream`. If conversion is successful, we can go with the read method for the input stream.
   
   But there is a small **back-compatibility issue**.
   
   Let say, that there is an adapter, which maps  InputStream into String "InputStream:...text...".
   Current state (before this PR) would return "InpuStream:...text..."
   Fix with automatic conversion would change result to "...text...", because automatic conversion would end successfully and would be used before the adapter.
   
   I agree that catching exception is a wrong way, which could cause more problems, then I originally thought of.
   
   So my opinion is:
    1 - try conversion of exchange into inputStream
    2 - if it succeeds, use  inputStream for jsonpath
    3 - if not, use adapter approach as a fallback
   This solution brings a slight change in behavior (described above), nevertheless  I think that this should be the correct approach and the difference could be mentioned in upgrade doc.
   
   If user really needs to use older order (adpter and fallback to inputStream), there are 2 options:
   1 - user could remove converter from registry
   2 - we can provide a property, which could return component to older behavior
   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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] davsclaus commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

davsclaus commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r608067380



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       Your option is fine. The "problem with the adapter" is that not only introduced more recently with the switch to jackson as json library. The adapter was only something added to make it easier to switch to use jackson instead of that json-smart that it was using as default. But since jackson is now the default, then the adapter is not really to be used.
   
   Just add a note in the 3.10 upgrade guide in the docs about this potential change.
   
   And yes IMHO reading as InputStream first is the best choice - InputStream is used by many components.




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] JiriOndrusek commented on a change in pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

JiriOndrusek commented on a change in pull request #5299:
URL: https://github.com/apache/camel/pull/5299#discussion_r608425425



##########
File path: components/camel-jsonpath/src/main/java/org/apache/camel/jsonpath/JsonPathEngine.java
##########
@@ -182,14 +182,30 @@ private Object doRead(String path, Exchange exchange) throws IOException, CamelE
             return JsonPath.using(configuration).parse(list).read(path);
         } else {
             // can we find an adapter which can read the message body/header
-            Object answer = readWithAdapter(path, exchange);
+            Object answer = null;

Review comment:
       done




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [camel] davsclaus merged pull request #5299: CAMEL-16459 jsonpath and platform-http-vertx causes PathNotFoundException

GitBox
In reply to this post by GitBox

davsclaus merged pull request #5299:
URL: https://github.com/apache/camel/pull/5299


   


--
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:
[hidden email]