You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by peter-toth <gi...@git.apache.org> on 2018/06/07 10:47:47 UTC
[GitHub] nifi pull request #2768: NIFI-5278: fixes JSON escaping of code parameter in...
GitHub user peter-toth opened a pull request:
https://github.com/apache/nifi/pull/2768
NIFI-5278: fixes JSON escaping of code parameter in Execute Spark Interactive processor
Change-Id: I2cb0e6c658d4a0f2aad9c4aab9201a3334ee54df
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/peter-toth/nifi NIFI-5278
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2768.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2768
----
commit f54792cd58e69bc43323ef9a063cce4c5c956e61
Author: Peter Toth <pt...@...>
Date: 2018-06-07T10:13:21Z
NIFI-5278: fixes JSON escaping of code
Change-Id: I2cb0e6c658d4a0f2aad9c4aab9201a3334ee54df
----
---
[GitHub] nifi pull request #2768: NIFI-5278: fixes JSON escaping of code parameter in...
Posted by peter-toth <gi...@git.apache.org>.
Github user peter-toth commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2768#discussion_r194157588
--- Diff: nifi-nar-bundles/nifi-spark-bundle/nifi-livy-processors/src/test/java/org/apache/nifi/processors/livy/TestExecuteSparkInteractiveSSL.java ---
@@ -109,13 +109,15 @@ private static TestServer createServer() throws IOException {
public void testSslSparkSession() throws Exception {
addHandler(new LivyAPIHandler());
- runner.enqueue("print \"hello world\"");
+ String code = "print \"hello world\" //'?!<>[]{}()$&*=%;.|_-\\";
--- End diff --
Removed.
---
[GitHub] nifi pull request #2768: NIFI-5278: fixes JSON escaping of code parameter in...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2768#discussion_r194108434
--- Diff: nifi-nar-bundles/nifi-spark-bundle/nifi-livy-processors/src/test/java/org/apache/nifi/processors/livy/TestExecuteSparkInteractiveSSL.java ---
@@ -109,13 +109,15 @@ private static TestServer createServer() throws IOException {
public void testSslSparkSession() throws Exception {
addHandler(new LivyAPIHandler());
- runner.enqueue("print \"hello world\"");
+ String code = "print \"hello world\" //'?!<>[]{}()$&*=%;.|_-\\";
--- End diff --
I think that adding a UT for the non-SSL case is enough, isn't it? There is no difference among SSL and non-SSL about escaping and the content IIUC.
---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Posted by peter-toth <gi...@git.apache.org>.
Github user peter-toth commented on the issue:
https://github.com/apache/nifi/pull/2768
@mattyb149 I've rebased this onto latest master.
---
[GitHub] nifi pull request #2768: NIFI-5278: fixes JSON escaping of code parameter in...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2768#discussion_r194106518
--- Diff: nifi-nar-bundles/nifi-spark-bundle/nifi-livy-processors/src/test/java/org/apache/nifi/processors/livy/ExecuteSparkInteractiveTestBase.java ---
@@ -64,33 +66,37 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
}
session1Requests++;
}
-
- response.setContentLength(responseBody.length());
-
- try (PrintWriter writer = response.getWriter()) {
- writer.print(responseBody);
- writer.flush();
- }
-
} else if ("POST".equalsIgnoreCase(request.getMethod())) {
-
- String responseBody = "{}";
- response.setContentType("application/json");
-
- if ("/sessions".equalsIgnoreCase(target)) {
- responseBody = "{\"id\": 1, \"kind\": \"spark\", \"state\": \"idle\"}";
- } else if ("/sessions/1/statements".equalsIgnoreCase(target)) {
- responseBody = "{\"id\": 7}";
+ String requestBody = IOUtils.toString(request.getReader());
+ try {
+ System.out.println("requestBody: " + requestBody);
--- End diff --
I think this is a leftover from your tests and should be removed.
---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2768
@peter-toth can you rebase this against the latest master? Not sure if you'd worked on this and other Livy stuff at the same time, but there are now merge conflicts and I wasn't quite sure what to include from each.
---
[GitHub] nifi pull request #2768: NIFI-5278: fixes JSON escaping of code parameter in...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2768#discussion_r194107658
--- Diff: nifi-nar-bundles/nifi-spark-bundle/nifi-livy-processors/src/test/java/org/apache/nifi/processors/livy/ExecuteSparkInteractiveTestBase.java ---
@@ -64,33 +66,37 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
}
session1Requests++;
}
-
- response.setContentLength(responseBody.length());
-
- try (PrintWriter writer = response.getWriter()) {
- writer.print(responseBody);
- writer.flush();
- }
-
} else if ("POST".equalsIgnoreCase(request.getMethod())) {
-
- String responseBody = "{}";
- response.setContentType("application/json");
-
- if ("/sessions".equalsIgnoreCase(target)) {
- responseBody = "{\"id\": 1, \"kind\": \"spark\", \"state\": \"idle\"}";
- } else if ("/sessions/1/statements".equalsIgnoreCase(target)) {
- responseBody = "{\"id\": 7}";
+ String requestBody = IOUtils.toString(request.getReader());
+ try {
+ System.out.println("requestBody: " + requestBody);
+
+ new ObjectMapper().readTree(requestBody);
--- End diff --
may you please add some comments explaining what and why you are doing this? It is clear since we are in the context of this PR, but for future readers I think a comment would be very helpful.
---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/nifi/pull/2768
Is there a test that should be created or updated for this change?
---
[GitHub] nifi pull request #2768: NIFI-5278: fixes JSON escaping of code parameter in...
Posted by peter-toth <gi...@git.apache.org>.
Github user peter-toth commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2768#discussion_r194157501
--- Diff: nifi-nar-bundles/nifi-spark-bundle/nifi-livy-processors/src/test/java/org/apache/nifi/processors/livy/ExecuteSparkInteractiveTestBase.java ---
@@ -64,33 +66,37 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
}
session1Requests++;
}
-
- response.setContentLength(responseBody.length());
-
- try (PrintWriter writer = response.getWriter()) {
- writer.print(responseBody);
- writer.flush();
- }
-
} else if ("POST".equalsIgnoreCase(request.getMethod())) {
-
- String responseBody = "{}";
- response.setContentType("application/json");
-
- if ("/sessions".equalsIgnoreCase(target)) {
- responseBody = "{\"id\": 1, \"kind\": \"spark\", \"state\": \"idle\"}";
- } else if ("/sessions/1/statements".equalsIgnoreCase(target)) {
- responseBody = "{\"id\": 7}";
+ String requestBody = IOUtils.toString(request.getReader());
+ try {
+ System.out.println("requestBody: " + requestBody);
--- End diff --
Fixed.
---
[GitHub] nifi pull request #2768: NIFI-5278: fixes JSON escaping of code parameter in...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2768#discussion_r194108030
--- Diff: nifi-nar-bundles/nifi-spark-bundle/nifi-livy-processors/src/test/java/org/apache/nifi/processors/livy/TestExecuteSparkInteractive.java ---
@@ -85,16 +85,17 @@ private static TestServer createServer() throws IOException {
@Test
public void testSparkSession() throws Exception {
-
addHandler(new LivyAPIHandler());
- runner.enqueue("print \"hello world\"");
+ String code = "print \"hello world\" //'?!<>[]{}()$&*=%;.|_-\\";
--- End diff --
instead of changing the existing UT, what about creating a new one for this specific case? It is good that every UT has very little scope so that failures can clearly indicate to the developer what he/she broke applying a patch...
---
[GitHub] nifi pull request #2768: NIFI-5278: fixes JSON escaping of code parameter in...
Posted by peter-toth <gi...@git.apache.org>.
Github user peter-toth commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2768#discussion_r194157756
--- Diff: nifi-nar-bundles/nifi-spark-bundle/nifi-livy-processors/src/test/java/org/apache/nifi/processors/livy/TestExecuteSparkInteractive.java ---
@@ -85,16 +85,17 @@ private static TestServer createServer() throws IOException {
@Test
public void testSparkSession() throws Exception {
-
addHandler(new LivyAPIHandler());
- runner.enqueue("print \"hello world\"");
+ String code = "print \"hello world\" //'?!<>[]{}()$&*=%;.|_-\\";
--- End diff --
Thanks for the review @mgaido91. I made a minor refactor to the tests and separated the cases.
---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2768
@peter-toth Since this is adding a new dependency (at least directly and possibly transitively) we'll at least need to make a License and Notice update. Can you please verify what the old dependencies that would be included are for impacted nar(s) and see what the difference is. The new artifacts need to be in the Nar(s) L&N as appropriate. For commons-text at least I suspect we need an entry in the spark-nar and possibly in the nifi-assembly notice as well. Please take a look to identify the gaps so we can get this included.
To the other comment(s) from otto/marco it would be good, if there is already a unit test, to create one which shows this change. If there were not unit or integration tests I dont think you should be held to a higher standard than the original author was while you're trying to fix a bug. We can proceed without if so but we cannot proceed without L&N updates as needed.
---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2768
+1 LGTM, ran build with unit tests and tried a pyspark session with `print('Hi')`, verified it fails before the fix and passes after. Thanks for the fix! Merging to master
---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Posted by peter-toth <gi...@git.apache.org>.
Github user peter-toth commented on the issue:
https://github.com/apache/nifi/pull/2768
@joewitt , thanks for the feedback. I've added Apache Commons Text to NOTICE of the nifi-livy-nar and nifi-assembly as you suggested. I checked that it does not bring in any new transitive dependency and also amended the existing test.
---
[GitHub] nifi pull request #2768: NIFI-5278: fixes JSON escaping of code parameter in...
Posted by peter-toth <gi...@git.apache.org>.
Github user peter-toth commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2768#discussion_r194157547
--- Diff: nifi-nar-bundles/nifi-spark-bundle/nifi-livy-processors/src/test/java/org/apache/nifi/processors/livy/ExecuteSparkInteractiveTestBase.java ---
@@ -64,33 +66,37 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
}
session1Requests++;
}
-
- response.setContentLength(responseBody.length());
-
- try (PrintWriter writer = response.getWriter()) {
- writer.print(responseBody);
- writer.flush();
- }
-
} else if ("POST".equalsIgnoreCase(request.getMethod())) {
-
- String responseBody = "{}";
- response.setContentType("application/json");
-
- if ("/sessions".equalsIgnoreCase(target)) {
- responseBody = "{\"id\": 1, \"kind\": \"spark\", \"state\": \"idle\"}";
- } else if ("/sessions/1/statements".equalsIgnoreCase(target)) {
- responseBody = "{\"id\": 7}";
+ String requestBody = IOUtils.toString(request.getReader());
+ try {
+ System.out.println("requestBody: " + requestBody);
+
+ new ObjectMapper().readTree(requestBody);
--- End diff --
Done.
---
[GitHub] nifi issue #2768: NIFI-5278: fixes JSON escaping of code parameter in Execut...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/nifi/pull/2768
LGTM
---
[GitHub] nifi pull request #2768: NIFI-5278: fixes JSON escaping of code parameter in...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2768
---