Uploaded image for project: 'Camel'
  1. Camel
  2. CAMEL-18612

Inconsistency in JsonPath component causes problems with databinding

Details

    • Bug
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • None
    • 3.18.3, 3.20.0
    • camel-jsonpath
    • None
    • Unknown

    Description

      Inconsistent treatment of jsonpath expression result causes problems with data binding. When jsonpath evaluates to an array element, this piece of code threats it as a single object, making it impossible to bind to an array of objects.

       

      (singleElement && !resultIsCollection) {
          result = ((List) result).get(0);
          ...
      }

       

       

      Steps to reproduce:

      1. Create this Camel route 
        from("file:work/")
                .routeId("file-route")
                .to("direct:transform");
        
        from("direct:transform")
                .routeId("direct-transform")
                .streamCaching()
                .log("Before jsonpath transformation >>> ${body}")
                .setBody().jsonpath("$.d.results", String.class)
                .log("After jsonpath transformation >>> ${body}")
                .process(exchange -> {
                    log.info("Before Jackson deserialization");
        
                    String testResponse = exchange.getMessage().getBody(String.class);
                    objectMapper.readValue(testResponse, TestResultsInfo[].class);
        
                    log.info("After Jackson deserialization");
                })
                .to("mock:test");
      1. Use the single-item-array.json file from the attachment
      2. Try to bind the message body to these classes
        @Data
        @NoArgsConstructor
        @JsonIgnoreProperties(ignoreUnknown = true)
        public static class TestResultsInfo {
        
            String resultText;
            @JsonProperty(value = "AddressInfo")
            TestAddressInfo addressInfo;
        
        }
        
        @Data
        @NoArgsConstructor
        @JsonIgnoreProperties(ignoreUnknown = true)
        public static class TestAddressInfo {
        
            @JsonProperty(value = "City")
            String city;
            @JsonProperty(value = "State")
            String street;
        
        }

      It will fail because it's not possible to bind it to TestResultsInfo[]. If you add a second element to the array (or use multiple-item-array.json file instead), it will work fine.

      Attachments

        1. multiple-item-array.json
          0.3 kB
          Radovan Netuka
        2. single-item-array.json
          0.1 kB
          Radovan Netuka

        Issue Links

          Activity

            davsclaus Claus Ibsen added a comment -

            Why do you convert the result to a String when its an array type? When you convert to String then the JDK will convert the List to a string by calling toString, which does a string dump that happens to look like json but it is not.

            In the PR unit test the result is:

            [

            {category=programming, author=Claus Ibsen,Jonathan Anstey, title=Camel in Action, price=39.99, isbn=978-193-518-236-8}

            ]

            davsclaus Claus Ibsen added a comment - Why do you convert the result to a String when its an array type? When you convert to String then the JDK will convert the List to a string by calling toString, which does a string dump that happens to look like json but it is not. In the PR unit test the result is: [ {category=programming, author=Claus Ibsen,Jonathan Anstey, title=Camel in Action, price=39.99, isbn=978-193-518-236-8} ]
            davsclaus Claus Ibsen added a comment -

            I think the conversion to String is not correct as its not json string, but a JDK toString from a List. So instead you can change test as follows

                    Object books = getMockEndpoint("mock:books").getReceivedExchanges().get(0).getIn().getBody();
            
                    ObjectMapper objectMapper = new ObjectMapper();
                    // convert the result to valid Json string
                    String json = objectMapper.writeValueAsString(books);
                    // and convert json to pojo via jackson
                    Book[] result = objectMapper.readValue(json, Book[].class);
            
                    Book[] expected = new Book[] { new Book("programming", "Claus Ibsen,Jonathan Anstey", "Camel in Action", "39.99", "978-193-518-236-8") };
                    assertArrayEquals(expected, result);
            

            And in the route you do

                  <transform>
                    <jsonpath>$.store.book</jsonpath>
                  </transform>
            

            Then its more correct

            davsclaus Claus Ibsen added a comment - I think the conversion to String is not correct as its not json string, but a JDK toString from a List. So instead you can change test as follows Object books = getMockEndpoint( "mock:books" ).getReceivedExchanges().get(0).getIn().getBody(); ObjectMapper objectMapper = new ObjectMapper(); // convert the result to valid Json string String json = objectMapper.writeValueAsString(books); // and convert json to pojo via jackson Book[] result = objectMapper.readValue(json, Book[].class); Book[] expected = new Book[] { new Book( "programming" , "Claus Ibsen,Jonathan Anstey" , "Camel in Action" , "39.99" , "978-193-518-236-8" ) }; assertArrayEquals(expected, result); And in the route you do <transform> <jsonpath>$.store.book</jsonpath> </transform> Then its more correct
            rnetuka Radovan Netuka added a comment - - edited

            davsclaus "Why do you convert the result to a String when its an array type?" Because that's the use case that's failing. If you use returnType=String to get a Json string a then try to bind it with Jackson-databind.

            Note: if you don't specify any return type, it will work because it will fall into an else branch

             

            With the databind, it's more visible there is a fault, but you can check it even without it:

            assertTrue(books.startsWith("["));

             

            The suggested code doesn't help. The bug is reproducable only when a resultType is specified as String.

             

            When combined with camel-jackson, there is a JacksonTypeConverters that kicks in and returns the json string. Unfortunately, it seems it can't be used in the unit test, since it's an independant component.

             

             

            rnetuka Radovan Netuka added a comment - - edited davsclaus "Why do you convert the result to a String when its an array type?" Because that's the use case that's failing. If you use returnType=String to get a Json string a then try to bind it with Jackson-databind. Note: if you don't specify any return type, it will work because it will fall into an else branch   With the databind, it's more visible there is a fault, but you can check it even without it: assertTrue(books.startsWith("["));   The suggested code doesn't help. The bug is reproducable only when a resultType is specified as String.   When combined with camel-jackson, there is a JacksonTypeConverters that kicks in and returns the json string. Unfortunately, it seems it can't be used in the unit test, since it's an independant component.    
            rnetuka Radovan Netuka added a comment -

            davsclaus Also, I think the following code might also fix the issue:

            boolean resultIsCollection = Collection.class.isAssignableFrom(this.resultType);
            boolean singleElement = result instanceof List && ((List)result).size() == 1;
            if (singleElement && !resultIsCollection && !(resultType==String.class)) {
                result = ((List)result).get(0);
                LOG.trace("Unwrapping result: {} from single element List before converting to: {}", result, this.resultType);
            } 

            Note the added && !(resultType==String.class). Just tell me, if you prefer this solution.

             

            I still think the code above smells. A single-element list is still a list and shouldn't be returned as a member of itself.

            rnetuka Radovan Netuka added a comment - davsclaus Also, I think the following code might also fix the issue: boolean resultIsCollection = Collection. class. isAssignableFrom( this .resultType); boolean singleElement = result instanceof List && ((List)result).size() == 1; if (singleElement && !resultIsCollection && !(resultType== String .class)) { result = ((List)result).get(0); LOG.trace( "Unwrapping result: {} from single element List before converting to: {}" , result, this .resultType); } Note the added  && !(resultType==String.class) . Just tell me, if you prefer this solution.   I still think the code above smells. A single-element list is still a list and shouldn't be returned as a member of itself.
            davsclaus Claus Ibsen added a comment -

            I think the code is used when you use jsonpath to filter, where you then filter and have 1 element as the result List(size 1), and want to grab it as X type instead of List(size 1)<X>.

            davsclaus Claus Ibsen added a comment - I think the code is used when you use jsonpath to filter, where you then filter and have 1 element as the result List(size 1), and want to grab it as X type instead of List(size 1)<X>.
            davsclaus Claus Ibsen added a comment -

            Yeah its a bit of hack with the list unwrap if size == 1.

            I think we should not do this by default, but add a new option on camel-jsonpath, so end users can turn back the old behaviour in case of backwards compatible.
            However to find a good name for such option is "hard".

            unwrapSingleArray=true|false

            davsclaus Claus Ibsen added a comment - Yeah its a bit of hack with the list unwrap if size == 1. I think we should not do this by default, but add a new option on camel-jsonpath, so end users can turn back the old behaviour in case of backwards compatible. However to find a good name for such option is "hard". unwrapSingleArray=true|false
            rnetuka Radovan Netuka added a comment -

            I agree with the new option solution. This will fix the issue while maintaining backwards compatibility (if the option is enabled).

             

            PR: https://github.com/apache/camel/pull/8571

            rnetuka Radovan Netuka added a comment - I agree with the new option solution. This will fix the issue while maintaining backwards compatibility (if the option is enabled).   PR: https://github.com/apache/camel/pull/8571
            davsclaus Claus Ibsen added a comment -

            rnetuka can you send a PR to update the upgrade guide at
            https://github.com/apache/camel/blob/main/docs/user-manual/modules/ROOT/pages/camel-3x-upgrade-guide-3_20.adoc

            We can then take that and also put in the 3.18 guide and mention that this is for Camel 3.18.3 onwards.

            davsclaus Claus Ibsen added a comment - rnetuka can you send a PR to update the upgrade guide at https://github.com/apache/camel/blob/main/docs/user-manual/modules/ROOT/pages/camel-3x-upgrade-guide-3_20.adoc We can then take that and also put in the 3.18 guide and mention that this is for Camel 3.18.3 onwards.
            davsclaus Claus Ibsen added a comment -

            This is the last ticket for 3.18.x, would you be able to send a PR today

            davsclaus Claus Ibsen added a comment - This is the last ticket for 3.18.x, would you be able to send a PR today

            People

              rnetuka Radovan Netuka
              rnetuka Radovan Netuka
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: