You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/11/19 20:53:06 UTC

[GitHub] [nifi] ChrisSamo632 opened a new pull request, #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors

ChrisSamo632 opened a new pull request, #6687:
URL: https://github.com/apache/nifi/pull/6687

   # Summary
   
   [NIFI-10844](https://issues.apache.org/jira/browse/NIFI-10844) allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors
   
   # Tracking
   
   Please complete the following tracking steps prior to pull request creation.
   
   ### Issue Tracking
   
   - [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
   
   ### Pull Request Tracking
   
   - [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
   - [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
   
   ### Pull Request Formatting
   
   - [x] Pull Request based on current revision of the `main` branch
   - [x] Pull Request refers to a feature branch with one commit containing changes
   
   # Verification
   
   Please indicate the verification steps performed prior to pull request creation.
   
   ### Build
   
   - [x] Build completed using `mvn clean install -P contrib-check`
     - [ ] JDK 8
     - [ ] JDK 11
     - [x] JDK 17
   
   ### Licensing
   
   - ~[ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)~
   - ~[ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files~
   
   ### Documentation
   
   - ~[ ] Documentation formatting appears as expected in rendered files~
   


-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors

Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6687:
URL: https://github.com/apache/nifi/pull/6687#discussion_r1055894635


##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/test/java/org/apache/nifi/elasticsearch/integration/ElasticSearchClientService_IT.java:
##########
@@ -205,19 +206,29 @@ void testVerifyFailedApiKeyAuth() {
 
     @Test
     void testBasicSearch() throws Exception {
+        assertBasicSearch(null);
+    }
+
+    @Test
+    void testBasicSearchRequestParameters() throws Exception {
+        assertBasicSearch(createParameters("preference", "_local"));
+    }
+
+    private void assertBasicSearch(final Map<String, String> requestParameters) throws JsonProcessingException {
         final Map<String, Object> temp = new MapBuilder()
-            .of("size", 10, "query", new MapBuilder().of("match_all", new HashMap<>()).build(),
-                    "aggs", new MapBuilder()
-                            .of("term_counts", new MapBuilder()
-                                    .of("terms", new MapBuilder()
-                                            .of("field", "msg", "size", 5)
-                                            .build())
-                                    .build())
-                            .build())
+                .of("size", 10, "query", new MapBuilder().of("match_all", new HashMap<>()).build(),

Review Comment:
   I don't think this is required, the use of `HashMap` here (and throughout all uses of the `MapBuilder` class in the tests) has been to create an empty Map (i.e. JSON Object) for the `match_all` part of the Elasticsearch Query DSL request body
   
   I have, however, changed the `MapBuilder` to use a `LinkedHashMap` internally instead of a `HashMap`, which might be relevant for keeping things in order (although that shouldn't matter for the submission to Elasticsearch)



-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] ChrisSamo632 commented on pull request #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors

Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on PR #6687:
URL: https://github.com/apache/nifi/pull/6687#issuecomment-1358286729

   > Thanks for the contribution @ChrisSamo632, this looks like a useful improvement.
   > 
   > One implementation note, rather defining instances of `AllowableValue`, what do you think about creating two new `enum` definitions, named `SearchResultFormat` and `AggregationResultFormat`, both of which could implement `DescribedValue`. This approach supports using the `enum` name as the property value, and makes value handling and comparison a bit more straightforward.
   
   @exceptionfactory sounds sensible. I think `DescribedValue` is something (comparatively) new that I've not used before but have seen you/others mention in recent PRs - I'll look at how they're used and how to rework for this submission


-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors

Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6687:
URL: https://github.com/apache/nifi/pull/6687#discussion_r1056252416


##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/api/PaginationType.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.nifi.processors.elasticsearch.api;
+
+import org.apache.nifi.components.DescribedValue;
+
+public enum PaginationType implements DescribedValue {
+    SCROLL("Use Elasticsearch \"scroll\" to page results."),

Review Comment:
   Need to make the backwards-compatible for the existing `AllowableValues` they're replacing in the processors



-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] ChrisSamo632 commented on pull request #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors

Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on PR #6687:
URL: https://github.com/apache/nifi/pull/6687#issuecomment-1363395679

   @exceptionfactory refactored to use `DescribedValue` enums for the new and other existing properties in these processors - led to me uncovering the fact that I'd mistakenly not added the new result formatting properties to the Paginated processors, which has now been corrected


-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors

Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6687:
URL: https://github.com/apache/nifi/pull/6687#discussion_r1056535336


##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/api/ResultOutputStrategy.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.nifi.processors.elasticsearch.api;
+
+import org.apache.nifi.components.DescribedValue;
+
+public enum ResultOutputStrategy implements DescribedValue {
+    PER_HIT("Flowfile per hit."),

Review Comment:
   Used the existing property `value`s for the new `enum`s to maintain backwards compatibility



##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/api/PaginationType.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.nifi.processors.elasticsearch.api;
+
+import org.apache.nifi.components.DescribedValue;
+
+public enum PaginationType implements DescribedValue {
+    SCROLL("Use Elasticsearch \"scroll\" to page results."),

Review Comment:
   Used the existing property `value`s for the new `enum`s to maintain backwards compatibility



-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors

Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6687:
URL: https://github.com/apache/nifi/pull/6687#discussion_r1052613032


##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/test/java/org/apache/nifi/elasticsearch/integration/ElasticSearchClientService_IT.java:
##########
@@ -205,19 +206,29 @@ void testVerifyFailedApiKeyAuth() {
 
     @Test
     void testBasicSearch() throws Exception {
+        assertBasicSearch(null);
+    }
+
+    @Test
+    void testBasicSearchRequestParameters() throws Exception {
+        assertBasicSearch(createParameters("preference", "_local"));
+    }
+
+    private void assertBasicSearch(final Map<String, String> requestParameters) throws JsonProcessingException {
         final Map<String, Object> temp = new MapBuilder()
-            .of("size", 10, "query", new MapBuilder().of("match_all", new HashMap<>()).build(),
-                    "aggs", new MapBuilder()
-                            .of("term_counts", new MapBuilder()
-                                    .of("terms", new MapBuilder()
-                                            .of("field", "msg", "size", 5)
-                                            .build())
-                                    .build())
-                            .build())
+                .of("size", 10, "query", new MapBuilder().of("match_all", new HashMap<>()).build(),

Review Comment:
   Recommend using `LinkedHashMap`:
   ```suggestion
                   .of("size", 10, "query", new MapBuilder().of("match_all", new LinkedHashMap<>()).build(),
   ```



-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors

Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6687:
URL: https://github.com/apache/nifi/pull/6687#discussion_r1056252276


##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/api/ResultOutputStrategy.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.nifi.processors.elasticsearch.api;
+
+import org.apache.nifi.components.DescribedValue;
+
+public enum ResultOutputStrategy implements DescribedValue {
+    PER_HIT("Flowfile per hit."),

Review Comment:
   Need to make the backwards-compatible for the existing `AllowableValues` they're replacing in the processors



-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors

Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6687:
URL: https://github.com/apache/nifi/pull/6687#discussion_r1052613646


##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/AbstractJsonQueryElasticsearch.java:
##########
@@ -281,8 +342,25 @@ private void handleAggregations(final Map<String, Object> aggregations, final Pr
         }
     }
 
+    @SuppressWarnings("unchecked")
+    private Map<String, Object> formatAggregations(final Map<String, Object> aggregations) {
+        final Map<String, Object> formattedAggregations;
+
+        if (AGGS_METADATA_ONLY.getValue().equals(aggregationFormat)) {
+            formattedAggregations = new HashMap<>(aggregations);

Review Comment:
   Recommend using `LinkedHashMap` for more deterministic results.
   ```suggestion
               formattedAggregations = new LinkedHashMap<>(aggregations);
   ```



-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] exceptionfactory closed pull request #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors

Posted by GitBox <gi...@apache.org>.
exceptionfactory closed pull request #6687: NIFI-10844 allow _source only output for GetElasticsearch and JsonQueryElasticsearch processors
URL: https://github.com/apache/nifi/pull/6687


-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org