tpalfy commented on a change in pull request #3646: NIFI-6546 - Add JsonPath 
set value support
URL: https://github.com/apache/nifi/pull/3646#discussion_r317560942
 
 

 ##########
 File path: 
nifi-commons/nifi-expression-language/src/test/java/org/apache/nifi/attribute/expression/language/TestQuery.java
 ##########
 @@ -327,26 +336,47 @@ public void testJsonPath() throws IOException {
         }
     }
 
+    private void verifyCommonAddressBookAttributes(Map<String,String> 
attributes, String skipCheck) {
 
 Review comment:
   Checked and the same "see if the same as in original unless should skip" 
logic can be applied to the phone numbers as well. In fact in one of the tests 
it does set skipCheck as one of the phone numbers, but it's not handled here - 
even though it could be.
   
   However this whole logic I think could be simplified by providing the 
original addressBook, like this:
   ```java
       private void verifyCommonAddressBookAttributes(String 
originalAddressBook, Map<String,String> attributes, String changedAttribute) {
           List<String> checkedAttributes = Arrays.asList(
                   ADDRESS_BOOK_JSON_PATH_FIRST_NAME,
                   ADDRESS_BOOK_JSON_PATH_LAST_NAME,
                   ADDRESS_BOOK_JSON_PATH_AGE,
                   ADDRESS_BOOK_JSON_PATH_VOTER,
                   ADDRESS_BOOK_JSON_PATH_ADDRESS_POSTAL_CODE,
                   ADDRESS_BOOK_JSON_PATH_PHONE_NUMBERS_TYPE_HOME_NUMBER,
                   ADDRESS_BOOK_JSON_PATH_PHONE_NUMBERS_TYPE_OFFICE_NUMBER
           );
   
           Map<String, String> originalAttributes = new HashMap<>(){{
               put("json", originalAddressBook);
           }};
   
           checkedAttributes.stream()
                   .filter(checkedAttribute -> 
!checkedAttribute.equals(changedAttribute))
                   .forEach(checkedAttribute -> {
                               String expected = 
Query.evaluateExpressions(checkedAttribute, originalAttributes, null, null, 
ParameterLookup.EMPTY);
                               verifyEquals(checkedAttribute, attributes, 
expected);
                           }
                   );
       }
   ```
   
   And an example test with this change:
   
   ```java
       @Test
       public void testJsonPathDeleteHomePhoneNumber() throws IOException {
           final Map<String, String> attributes = new HashMap<>();
           String addressBook = getResourceAsString("/json/address-book.json");
           attributes.put("json", addressBook);
   
           verifyEquals(ADDRESS_BOOK_JSON_PATH_PHONE_NUMBERS_TYPE_HOME_NUMBER, 
attributes, "212 555-1234");
   
           String addressBookAfterDelete = 
Query.evaluateExpressions("${json:jsonPathDelete(\"$.phoneNumbers[?(@.type=='home')]\")}",
 attributes, ParameterLookup.EMPTY);
   
           attributes.clear();
           attributes.put("json", addressBookAfterDelete);
   
           verifyCommonAddressBookAttributes(addressBook, attributes, 
ADDRESS_BOOK_JSON_PATH_PHONE_NUMBERS_TYPE_HOME_NUMBER);
           verifyEquals(ADDRESS_BOOK_JSON_PATH_PHONE_NUMBERS_TYPE_HOME_NUMBER, 
attributes, "[]");
       }
   ```
   
   You could go even further and use an enum instead of the constants but that 
might be a bit overkill.

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


With regards,
Apache Git Services

Reply via email to