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/07/28 15:23:04 UTC
[GitHub] [nifi] NissimShiman opened a new pull request, #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
NissimShiman opened a new pull request, #6254:
URL: https://github.com/apache/nifi/pull/6254
<!-- 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. -->
# Summary
[NIFI-10287](https://issues.apache.org/jira/browse/NIFI-10287)
This resolves a bug that crept in where the ExecuteScript processor used to support using external modules for python, but has lost that functionality. This has been fixed in this ticket.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [ ] Pull Request based on current revision of the `main` branch
- [ ] 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
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 8
- [ ] JDK 11
- [ ] 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] NissimShiman commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r967390285
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java:
##########
@@ -265,7 +265,11 @@ public void setupVariables(final PropertyContext context) {
scriptEngineName = context.getProperty(SCRIPT_ENGINE).getValue();
scriptPath = context.getProperty(ScriptingComponentUtils.SCRIPT_FILE).evaluateAttributeExpressions().getValue();
scriptBody = context.getProperty(ScriptingComponentUtils.SCRIPT_BODY).getValue();
- modules = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources().flattenRecursively();
+ if (scriptEngineName.equals("python")) {
Review Comment:
committed fix that tries to incorporate both of your comments :)
Thank you very much @dan-s1 for your comments!
I agree that some sort of enum/ refactoring to incorporate the enum would be nice although this maybe pushing the scope of this ticket ( To be sure this is a valid area that could use improvement, but this has been around for a while pre-dating this ticket)
--
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] NissimShiman commented on pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6254:
URL: https://github.com/apache/nifi/pull/6254#issuecomment-1301506031
Thank you @mattyb149 !
--
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] mattyb149 closed pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
mattyb149 closed pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
URL: https://github.com/apache/nifi/pull/6254
--
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] dan-s1 commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
dan-s1 commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r961806190
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java:
##########
@@ -265,7 +265,11 @@ public void setupVariables(final PropertyContext context) {
scriptEngineName = context.getProperty(SCRIPT_ENGINE).getValue();
scriptPath = context.getProperty(ScriptingComponentUtils.SCRIPT_FILE).evaluateAttributeExpressions().getValue();
scriptBody = context.getProperty(ScriptingComponentUtils.SCRIPT_BODY).getValue();
- modules = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources().flattenRecursively();
+ if (scriptEngineName.equals("python")) {
Review Comment:
Not sure that is necessary as code in [ScriptRunnerFactory](https://github.com/NissimShiman/nifi/blob/NIFI-10287/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptRunnerFactory.java#L55) uses lowercase. I would though pattern after that class and have if statement read as
`if ("python".equals(scriptEngineName))`
I am just concerned about the proliferation of the hard coded string "python". It is done now in this class, in ScriptRunnerFactory as mentioned, in [JythonScriptRunner](https://github.com/NissimShiman/nifi/blob/NIFI-10287/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/impl/JythonScriptRunner.java), [ScriptedRecordProcessor](https://github.com/NissimShiman/nifi/blob/NIFI-10287/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/ScriptedRecordProcessor.java#L53) and various unit tests under module nifi-scripting-processors. It would have been nice to have some static final strings of all the script names or an enum.
--
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] NissimShiman commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r967390475
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestExecuteJython.java:
##########
@@ -61,6 +61,34 @@ public void testReadFlowFileContentAndStoreInFlowFileAttributeWithScriptBody() t
result.get(0).assertAttributeEquals("from-content", "test content");
}
+ /**
+ * Tests a Jython script that references an outside python module
+ *
+ * @throws Exception Any error encountered while testing
+ */
+ @Test
+ public void testAccessModuleAndStoreInFlowFileAttributeWithScriptBody() throws Exception {
Review Comment:
done. Nice catch!
--
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] mattyb149 commented on pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on PR #6254:
URL: https://github.com/apache/nifi/pull/6254#issuecomment-1301180476
+1 LGTM, thanks for the improvement and sticking with this! Merging to main
--
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] NissimShiman commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r982906854
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestExecuteJython.java:
##########
@@ -61,6 +67,83 @@ public void testReadFlowFileContentAndStoreInFlowFileAttributeWithScriptBody() t
result.get(0).assertAttributeEquals("from-content", "test content");
}
+ /**
+ * Tests a Jython script that references an outside python module
+ *
+ */
+ @Test
+ public void testAccessModuleAndStoreInFlowFileAttributeWithScriptBody() {
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES, "target/test/resources/jython/");
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "from test_external_module import ExternalModule\n"
+ + "externalModule = ExternalModule()\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile, \"key\", externalModule.testHelloWorld())\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals("key", "helloWorld");
+ }
+
+
+ /**
+ * Tests a Jython script when adding comma separated list of all existing directories from PYTHONPATH
+ * as value for {@link ScriptingComponentUtils} MODULES property.
+ *
+ * @throws Exception If PYTHONPATH cannot be retrieved.
+ */
+ @Test
+ @EnabledOnOs(OS.LINUX)
+ public void testAccessPythonPackageModulesAndStoreInFlowFileAttributeWithScriptBody() throws Exception {
+ String attributeName = "key";
+ String attributeValue = "helloWorld";
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES,
+ String.join(",", getExistingPythonPathModuleDirectories()));
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile," + "\"" + attributeName + "\", '" + attributeValue + "')\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals(attributeName, attributeValue);
+ }
+
+ /**
+ * Method which retrieves the PYTHONPATH and builds a java.util.List of existing directories on the PYTHONPATH.
+ * This method uses java.lang.ProcessBuilder and java.lang.Process to execute python to obtain the PYTHONPATH.
+ *
+ * @return java.util.List of existing directories on PYTHONPATH.
+ * @throws Exception If an error occurs when executing a java.lang.Process.
+ */
+ List<String> getExistingPythonPathModuleDirectories() throws Exception{
Review Comment:
@mattyb149 Thank you very much for looking at this!
I hear you ... this is overkill for a unit test, but I *think* it is the only way to mimic the second situation mentioned in the jira ticket (i.e. where a user used the machine's python path , /usr/lib/python3.7/ or whatever) in the Module Directory property).
This ended up recursively sucking in so many files that the the processor couldn't function.
Of course, only external modules are supposed to referenced with this property (as opposed to an entire python installation), so essentially this test is more to protect against user error as opposed to an actual processor flaw.
So ... maybe it is overkill to include this to begin with.as it may be beyond the scope of unit tests to be looking for the set of possible user errors.
So I think either we let this go or we keep it, as I'm not sure there is a middle ground in testing that particular use case
Let me know if we wish to drop it.
Thank You!
--
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 #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r982970539
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestExecuteJython.java:
##########
@@ -61,6 +67,83 @@ public void testReadFlowFileContentAndStoreInFlowFileAttributeWithScriptBody() t
result.get(0).assertAttributeEquals("from-content", "test content");
}
+ /**
+ * Tests a Jython script that references an outside python module
+ *
+ */
+ @Test
+ public void testAccessModuleAndStoreInFlowFileAttributeWithScriptBody() {
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES, "target/test/resources/jython/");
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "from test_external_module import ExternalModule\n"
+ + "externalModule = ExternalModule()\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile, \"key\", externalModule.testHelloWorld())\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals("key", "helloWorld");
+ }
+
+
+ /**
+ * Tests a Jython script when adding comma separated list of all existing directories from PYTHONPATH
+ * as value for {@link ScriptingComponentUtils} MODULES property.
+ *
+ * @throws Exception If PYTHONPATH cannot be retrieved.
+ */
+ @Test
+ @EnabledOnOs(OS.LINUX)
+ public void testAccessPythonPackageModulesAndStoreInFlowFileAttributeWithScriptBody() throws Exception {
+ String attributeName = "key";
+ String attributeValue = "helloWorld";
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES,
+ String.join(",", getExistingPythonPathModuleDirectories()));
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile," + "\"" + attributeName + "\", '" + attributeValue + "')\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals(attributeName, attributeValue);
+ }
+
+ /**
+ * Method which retrieves the PYTHONPATH and builds a java.util.List of existing directories on the PYTHONPATH.
+ * This method uses java.lang.ProcessBuilder and java.lang.Process to execute python to obtain the PYTHONPATH.
+ *
+ * @return java.util.List of existing directories on PYTHONPATH.
+ * @throws Exception If an error occurs when executing a java.lang.Process.
+ */
+ List<String> getExistingPythonPathModuleDirectories() throws Exception{
+ String pathDelimiter = ",";
+ ProcessBuilder processBuilder = new ProcessBuilder();
+ processBuilder.command("python", "-c", "import sys; print('" + pathDelimiter + "'.join(sys.path))");
Review Comment:
This command places a requirement on having the `python` binary available for builds, which is not necessarily a guarantee. It looks like this approach should be changed, or made conditional.
--
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] dan-s1 commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
dan-s1 commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r961806190
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java:
##########
@@ -265,7 +265,11 @@ public void setupVariables(final PropertyContext context) {
scriptEngineName = context.getProperty(SCRIPT_ENGINE).getValue();
scriptPath = context.getProperty(ScriptingComponentUtils.SCRIPT_FILE).evaluateAttributeExpressions().getValue();
scriptBody = context.getProperty(ScriptingComponentUtils.SCRIPT_BODY).getValue();
- modules = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources().flattenRecursively();
+ if (scriptEngineName.equals("python")) {
Review Comment:
Not sure that is necessary as code in [ScriptRunnerFactory](https://github.com/NissimShiman/nifi/blob/NIFI-10287/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptRunnerFactory.java#L55) uses lowercase. I would though pattern after that class and have if statement read as
`if ("python".equals(scriptEngineName))`
I am just concerned though that the hard coded string "python" is done now in this class, in ScriptRunnerFactory as mentioned, in [JythonScriptRunner](https://github.com/NissimShiman/nifi/blob/NIFI-10287/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/impl/JythonScriptRunner.java), [ScriptedRecordProcessor](https://github.com/NissimShiman/nifi/blob/NIFI-10287/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/ScriptedRecordProcessor.java#L53) and various unit tests under module nifi-scripting-processors. It would have been nice to have some static final strings of all the script names or an enum.
--
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] dan-s1 commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
dan-s1 commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r961806190
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java:
##########
@@ -265,7 +265,11 @@ public void setupVariables(final PropertyContext context) {
scriptEngineName = context.getProperty(SCRIPT_ENGINE).getValue();
scriptPath = context.getProperty(ScriptingComponentUtils.SCRIPT_FILE).evaluateAttributeExpressions().getValue();
scriptBody = context.getProperty(ScriptingComponentUtils.SCRIPT_BODY).getValue();
- modules = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources().flattenRecursively();
+ if (scriptEngineName.equals("python")) {
Review Comment:
Not sure that is necessary as code in [ScriptRunnerFactory](https://github.com/NissimShiman/nifi/blob/NIFI-10287/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptRunnerFactory.java#L55) uses lowercase. I would though pattern after that class and have if statement read as
`if ("python".equals(scriptEngineName))`
I am just concerned though that the hard coded string "python" is done now in this class, in ScriptRunnerFactory as mentioned, in [JythonScriptRunner](https://github.com/NissimShiman/nifi/blob/NIFI-10287/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/impl/JythonScriptRunner.java), []ScriptedRecordProcessor(https://github.com/NissimShiman/nifi/blob/NIFI-10287/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/processors/script/ScriptedRecordProcessor.java#L53) and various unit tests under module nifi-scripting-processors. It would have been nice to have some static final strings of all the script names or an enum.
--
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] dan-s1 commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
dan-s1 commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r961927977
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestExecuteJython.java:
##########
@@ -61,6 +61,34 @@ public void testReadFlowFileContentAndStoreInFlowFileAttributeWithScriptBody() t
result.get(0).assertAttributeEquals("from-content", "test content");
}
+ /**
+ * Tests a Jython script that references an outside python module
+ *
+ * @throws Exception Any error encountered while testing
+ */
+ @Test
+ public void testAccessModuleAndStoreInFlowFileAttributeWithScriptBody() throws Exception {
Review Comment:
Intellij says "Exception 'java.lang.Exception' is never thrown in the method" and suggests i should be removed.
--
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] NissimShiman commented on pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6254:
URL: https://github.com/apache/nifi/pull/6254#issuecomment-1265652117
@mattyb149 @exceptionfactory @MikeThomsen I removed the unit test that was concerning so I think we are good. The unit test for the main use case of the ticket remains. Please let me know if there are other holdup related concerns.
--
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] dan-s1 commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
dan-s1 commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r961806190
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java:
##########
@@ -265,7 +265,11 @@ public void setupVariables(final PropertyContext context) {
scriptEngineName = context.getProperty(SCRIPT_ENGINE).getValue();
scriptPath = context.getProperty(ScriptingComponentUtils.SCRIPT_FILE).evaluateAttributeExpressions().getValue();
scriptBody = context.getProperty(ScriptingComponentUtils.SCRIPT_BODY).getValue();
- modules = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources().flattenRecursively();
+ if (scriptEngineName.equals("python")) {
Review Comment:
Not sure that is necessary as code in [ScriptRunnerFactory](https://github.com/NissimShiman/nifi/blob/NIFI-10287/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptRunnerFactory.java#L55) uses lowercase. I would though pattern after that class and have if statement read as
`if ("python".equals(scriptEngineName))`
--
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] dan-s1 commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
dan-s1 commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r961927977
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestExecuteJython.java:
##########
@@ -61,6 +61,34 @@ public void testReadFlowFileContentAndStoreInFlowFileAttributeWithScriptBody() t
result.get(0).assertAttributeEquals("from-content", "test content");
}
+ /**
+ * Tests a Jython script that references an outside python module
+ *
+ * @throws Exception Any error encountered while testing
+ */
+ @Test
+ public void testAccessModuleAndStoreInFlowFileAttributeWithScriptBody() throws Exception {
Review Comment:
Intellij says "Exception 'java.lang.Exception' is never thrown in the method" and suggests it should be removed.
--
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] NissimShiman commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r983903112
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestExecuteJython.java:
##########
@@ -61,6 +67,83 @@ public void testReadFlowFileContentAndStoreInFlowFileAttributeWithScriptBody() t
result.get(0).assertAttributeEquals("from-content", "test content");
}
+ /**
+ * Tests a Jython script that references an outside python module
+ *
+ */
+ @Test
+ public void testAccessModuleAndStoreInFlowFileAttributeWithScriptBody() {
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES, "target/test/resources/jython/");
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "from test_external_module import ExternalModule\n"
+ + "externalModule = ExternalModule()\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile, \"key\", externalModule.testHelloWorld())\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals("key", "helloWorld");
+ }
+
+
+ /**
+ * Tests a Jython script when adding comma separated list of all existing directories from PYTHONPATH
+ * as value for {@link ScriptingComponentUtils} MODULES property.
+ *
+ * @throws Exception If PYTHONPATH cannot be retrieved.
+ */
+ @Test
+ @EnabledOnOs(OS.LINUX)
+ public void testAccessPythonPackageModulesAndStoreInFlowFileAttributeWithScriptBody() throws Exception {
+ String attributeName = "key";
+ String attributeValue = "helloWorld";
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES,
+ String.join(",", getExistingPythonPathModuleDirectories()));
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile," + "\"" + attributeName + "\", '" + attributeValue + "')\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals(attributeName, attributeValue);
+ }
+
+ /**
+ * Method which retrieves the PYTHONPATH and builds a java.util.List of existing directories on the PYTHONPATH.
+ * This method uses java.lang.ProcessBuilder and java.lang.Process to execute python to obtain the PYTHONPATH.
+ *
+ * @return java.util.List of existing directories on PYTHONPATH.
+ * @throws Exception If an error occurs when executing a java.lang.Process.
+ */
+ List<String> getExistingPythonPathModuleDirectories() throws Exception{
+ String pathDelimiter = ",";
+ ProcessBuilder processBuilder = new ProcessBuilder();
+ processBuilder.command("python", "-c", "import sys; print('" + pathDelimiter + "'.join(sys.path))");
Review Comment:
@exceptionfactory This is a very good observation. Just checked in code that should handle this.
One of the ci/cd jobs didn't finish for one the builds (i.e. timed out after 2 hours), but the others finished OK so we should be good there
Thank you very much for looking at this!
--
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] MikeThomsen commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r983935136
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestExecuteJython.java:
##########
@@ -61,6 +67,83 @@ public void testReadFlowFileContentAndStoreInFlowFileAttributeWithScriptBody() t
result.get(0).assertAttributeEquals("from-content", "test content");
}
+ /**
+ * Tests a Jython script that references an outside python module
+ *
+ */
+ @Test
+ public void testAccessModuleAndStoreInFlowFileAttributeWithScriptBody() {
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES, "target/test/resources/jython/");
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "from test_external_module import ExternalModule\n"
+ + "externalModule = ExternalModule()\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile, \"key\", externalModule.testHelloWorld())\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals("key", "helloWorld");
+ }
+
+
+ /**
+ * Tests a Jython script when adding comma separated list of all existing directories from PYTHONPATH
+ * as value for {@link ScriptingComponentUtils} MODULES property.
+ *
+ * @throws Exception If PYTHONPATH cannot be retrieved.
+ */
+ @Test
+ @EnabledOnOs(OS.LINUX)
+ public void testAccessPythonPackageModulesAndStoreInFlowFileAttributeWithScriptBody() throws Exception {
+ String attributeName = "key";
+ String attributeValue = "helloWorld";
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES,
+ String.join(",", getExistingPythonPathModuleDirectories()));
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile," + "\"" + attributeName + "\", '" + attributeValue + "')\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals(attributeName, attributeValue);
+ }
+
+ /**
+ * Method which retrieves the PYTHONPATH and builds a java.util.List of existing directories on the PYTHONPATH.
+ * This method uses java.lang.ProcessBuilder and java.lang.Process to execute python to obtain the PYTHONPATH.
+ *
+ * @return java.util.List of existing directories on PYTHONPATH.
+ * @throws Exception If an error occurs when executing a java.lang.Process.
+ */
+ List<String> getExistingPythonPathModuleDirectories() throws Exception{
Review Comment:
> used the machine's python path , /usr/lib/python3.7/ or whatever
Unless the Python path points 2.7 or older, why would we really need to worry about this?
--
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] NissimShiman commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r961825380
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java:
##########
@@ -265,7 +265,11 @@ public void setupVariables(final PropertyContext context) {
scriptEngineName = context.getProperty(SCRIPT_ENGINE).getValue();
scriptPath = context.getProperty(ScriptingComponentUtils.SCRIPT_FILE).evaluateAttributeExpressions().getValue();
scriptBody = context.getProperty(ScriptingComponentUtils.SCRIPT_BODY).getValue();
- modules = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources().flattenRecursively();
+ if (scriptEngineName.equals("python")) {
+ modules = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources();
Review Comment:
Thank for you very much, @markobean for looking at this!
"flatten recursively" finds all the files in a directory
This doesn't work well with python which prefers including directory names as opposed to the file names.
More specifically, we are using sys.path.append
https://github.com/apache/nifi/blob/rel/nifi-1.17.0/nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/impl/JythonScriptRunner.java#L40
which then looks inside the directory for any needed files
It is looking for a directory name, though, so when it gets a filename (because of the "flatten recursively")
it doesn't handle it well
--
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] NissimShiman commented on pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6254:
URL: https://github.com/apache/nifi/pull/6254#issuecomment-1285633551
@mattyb149 I was wondering if you could take a look at this again. The previous issue you (and another reviewer) mentioned has been resolved.
--
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] NissimShiman commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r983959070
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestExecuteJython.java:
##########
@@ -61,6 +67,83 @@ public void testReadFlowFileContentAndStoreInFlowFileAttributeWithScriptBody() t
result.get(0).assertAttributeEquals("from-content", "test content");
}
+ /**
+ * Tests a Jython script that references an outside python module
+ *
+ */
+ @Test
+ public void testAccessModuleAndStoreInFlowFileAttributeWithScriptBody() {
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES, "target/test/resources/jython/");
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "from test_external_module import ExternalModule\n"
+ + "externalModule = ExternalModule()\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile, \"key\", externalModule.testHelloWorld())\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals("key", "helloWorld");
+ }
+
+
+ /**
+ * Tests a Jython script when adding comma separated list of all existing directories from PYTHONPATH
+ * as value for {@link ScriptingComponentUtils} MODULES property.
+ *
+ * @throws Exception If PYTHONPATH cannot be retrieved.
+ */
+ @Test
+ @EnabledOnOs(OS.LINUX)
+ public void testAccessPythonPackageModulesAndStoreInFlowFileAttributeWithScriptBody() throws Exception {
+ String attributeName = "key";
+ String attributeValue = "helloWorld";
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES,
+ String.join(",", getExistingPythonPathModuleDirectories()));
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile," + "\"" + attributeName + "\", '" + attributeValue + "')\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals(attributeName, attributeValue);
+ }
+
+ /**
+ * Method which retrieves the PYTHONPATH and builds a java.util.List of existing directories on the PYTHONPATH.
+ * This method uses java.lang.ProcessBuilder and java.lang.Process to execute python to obtain the PYTHONPATH.
+ *
+ * @return java.util.List of existing directories on PYTHONPATH.
+ * @throws Exception If an error occurs when executing a java.lang.Process.
+ */
+ List<String> getExistingPythonPathModuleDirectories() throws Exception{
Review Comment:
Thank you @MikeThomsen for adding your input!
Test has been removed
--
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] dan-s1 commented on pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
dan-s1 commented on PR #6254:
URL: https://github.com/apache/nifi/pull/6254#issuecomment-1243987237
LGTM+1
--
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] markobean commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r961720694
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java:
##########
@@ -265,7 +265,11 @@ public void setupVariables(final PropertyContext context) {
scriptEngineName = context.getProperty(SCRIPT_ENGINE).getValue();
scriptPath = context.getProperty(ScriptingComponentUtils.SCRIPT_FILE).evaluateAttributeExpressions().getValue();
scriptBody = context.getProperty(ScriptingComponentUtils.SCRIPT_BODY).getValue();
- modules = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources().flattenRecursively();
+ if (scriptEngineName.equals("python")) {
+ modules = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources();
Review Comment:
Why does python not "flattenRecursively()"?
--
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] markobean commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
markobean commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r961693664
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/main/java/org/apache/nifi/script/ScriptingComponentHelper.java:
##########
@@ -265,7 +265,11 @@ public void setupVariables(final PropertyContext context) {
scriptEngineName = context.getProperty(SCRIPT_ENGINE).getValue();
scriptPath = context.getProperty(ScriptingComponentUtils.SCRIPT_FILE).evaluateAttributeExpressions().getValue();
scriptBody = context.getProperty(ScriptingComponentUtils.SCRIPT_BODY).getValue();
- modules = context.getProperty(ScriptingComponentUtils.MODULES).evaluateAttributeExpressions().asResources().flattenRecursively();
+ if (scriptEngineName.equals("python")) {
Review Comment:
Suggest you make this case-insensitive allowing some future-proofing. I see other engines start with a capital.
--
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] mattyb149 commented on a diff in pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on code in PR #6254:
URL: https://github.com/apache/nifi/pull/6254#discussion_r982805284
##########
nifi-nar-bundles/nifi-scripting-bundle/nifi-scripting-processors/src/test/java/org/apache/nifi/processors/script/TestExecuteJython.java:
##########
@@ -61,6 +67,83 @@ public void testReadFlowFileContentAndStoreInFlowFileAttributeWithScriptBody() t
result.get(0).assertAttributeEquals("from-content", "test content");
}
+ /**
+ * Tests a Jython script that references an outside python module
+ *
+ */
+ @Test
+ public void testAccessModuleAndStoreInFlowFileAttributeWithScriptBody() {
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES, "target/test/resources/jython/");
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "from test_external_module import ExternalModule\n"
+ + "externalModule = ExternalModule()\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile, \"key\", externalModule.testHelloWorld())\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals("key", "helloWorld");
+ }
+
+
+ /**
+ * Tests a Jython script when adding comma separated list of all existing directories from PYTHONPATH
+ * as value for {@link ScriptingComponentUtils} MODULES property.
+ *
+ * @throws Exception If PYTHONPATH cannot be retrieved.
+ */
+ @Test
+ @EnabledOnOs(OS.LINUX)
+ public void testAccessPythonPackageModulesAndStoreInFlowFileAttributeWithScriptBody() throws Exception {
+ String attributeName = "key";
+ String attributeValue = "helloWorld";
+ runner.setValidateExpressionUsage(false);
+ runner.setProperty(scriptingComponent.getScriptingComponentHelper().SCRIPT_ENGINE, "python");
+ runner.setProperty(ScriptingComponentUtils.MODULES,
+ String.join(",", getExistingPythonPathModuleDirectories()));
+ runner.setProperty(ScriptingComponentUtils.SCRIPT_BODY,
+ "from org.apache.nifi.processors.script import ExecuteScript\n"
+ + "flowFile = session.get()\n"
+ + "flowFile = session.putAttribute(flowFile," + "\"" + attributeName + "\", '" + attributeValue + "')\n"
+ + "session.transfer(flowFile, ExecuteScript.REL_SUCCESS)");
+
+ runner.assertValid();
+ runner.enqueue("test content".getBytes(StandardCharsets.UTF_8));
+ runner.run();
+
+ runner.assertAllFlowFilesTransferred(ExecuteScript.REL_SUCCESS, 1);
+ final List<MockFlowFile> result = runner.getFlowFilesForRelationship(ExecuteScript.REL_SUCCESS);
+ result.get(0).assertAttributeEquals(attributeName, attributeValue);
+ }
+
+ /**
+ * Method which retrieves the PYTHONPATH and builds a java.util.List of existing directories on the PYTHONPATH.
+ * This method uses java.lang.ProcessBuilder and java.lang.Process to execute python to obtain the PYTHONPATH.
+ *
+ * @return java.util.List of existing directories on PYTHONPATH.
+ * @throws Exception If an error occurs when executing a java.lang.Process.
+ */
+ List<String> getExistingPythonPathModuleDirectories() throws Exception{
Review Comment:
This seems like overkill for a unit test, can't we just set up a module structure in test/resources/jython? We do that already for the `callbacks` module
--
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] markobean commented on pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
markobean commented on PR #6254:
URL: https://github.com/apache/nifi/pull/6254#issuecomment-1256482258
Unit test looks good. I built the code (including -Pcontrib-check), and all looks good. I tested by instantiating an ExecuteScript processor. It was configured with a module directory with a .py module file similar to the one used in the unit test. Ran data and it worked like a charm. I changed the module code and re-ran a flowfile (without stopping the processor.) The changes were not reflected in the processed flowfile. However, restarting the processor picks up the module update. I believe this is expected behavior since we do not want to reload modules (or even check if they should be reloaded) on every flowfile; I believe that would be overly burdensome.
Overall, looks good to merge to me. Thanks @NissimShiman !
--
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] NissimShiman commented on pull request #6254: NIFI-10287 ExecuteScript - Allow python scripts to use external modules
Posted by GitBox <gi...@apache.org>.
NissimShiman commented on PR #6254:
URL: https://github.com/apache/nifi/pull/6254#issuecomment-1244244831
Thank you very much @dan-s1 for the review and for volunteering the unit test for the use case that was missed!
Squashed commits after approval to make it easier for additional reviewers to see code and to capture attribution for @dan-s1's generous contribution.
--
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