[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #784: MINIFICPP-1206 - Rework and test ExecutePythonProcessor, add in-place script support
arpadboda commented on a change in pull request #784:
URL: https://github.com/apache/nifi-minifi-cpp/pull/784#discussion_r444794441
##
File path: libminifi/test/script-tests/ExecutePythonProcessorTests.cpp
##
@@ -0,0 +1,220 @@
+/**
+ *
+ * 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.
+ */
+
+#define CATCH_CONFIG_MAIN
+
+#include
+#include
+#include
+
+#include "../TestBase.h"
+
+#include "processors/GetFile.h"
+#include "python/ExecutePythonProcessor.h"
+#include "processors/LogAttribute.h"
+#include "processors/PutFile.h"
+#include "utils/file/FileUtils.h"
+#include "utils/TestUtils.h"
+
+namespace {
+using org::apache::nifi::minifi::utils::createTempDir;
+using org::apache::nifi::minifi::utils::putFileToDir;
+using org::apache::nifi::minifi::utils::createTempDirWithFile;
+using org::apache::nifi::minifi::utils::getFileContent;
+
+class ExecutePythonProcessorTestBase {
+ public:
+ ExecutePythonProcessorTestBase() :
+logTestController_(LogTestController::getInstance()),
+
logger_(logging::LoggerFactory::getLogger())
{
+reInitialize();
+ }
+ virtual ~ExecutePythonProcessorTestBase() {
+logTestController_.reset();
+logTestController_.setDebug();
+
logTestController_.setDebug();
+logTestController_.setDebug();
+logTestController_.setDebug();
+ }
+
+ protected:
+ void reInitialize() {
+testController_.reset(new TestController());
+plan_ = testController_->createPlan();
+ }
+
+ std::string getScriptFullPath(const std::string& script_file_name) {
+return SCRIPT_FILES_DIRECTORY + utils::file::FileUtils::get_separator() +
script_file_name;
+ }
+
+ static const std::string TEST_FILE_NAME;
+ static const std::string TEST_FILE_CONTENT;
+ static const std::string SCRIPT_FILES_DIRECTORY;
+
+ std::unique_ptr testController_;
+ std::shared_ptr plan_;
+ LogTestController& logTestController_;
+ std::shared_ptr logger_;
+};
+
+const std::string ExecutePythonProcessorTestBase::TEST_FILE_NAME{
"test_file.txt" };
+const std::string ExecutePythonProcessorTestBase::TEST_FILE_CONTENT{ "Test
text\n" };
+const std::string ExecutePythonProcessorTestBase::SCRIPT_FILES_DIRECTORY{
"test_scripts" };
+
+class SimplePythonFlowFileTransferTest : public ExecutePythonProcessorTestBase
{
+ public:
+ enum class Expectation {
+OUTPUT_FILE_MATCHES_INPUT,
+RUNTIME_RELATIONSHIP_EXCEPTION,
+PROCESSOR_INITIALIZATION_EXCEPTION
+ };
+ SimplePythonFlowFileTransferTest() : ExecutePythonProcessorTestBase{} {}
+
+ protected:
+ void testSimpleFilePassthrough(const Expectation expectation, const
core::Relationship& execute_python_out_conn, const std::string&
used_as_script_file, const std::string& used_as_script_body) {
+reInitialize();
+const std::string input_dir = createTempDirWithFile(testController_.get(),
TEST_FILE_NAME, TEST_FILE_CONTENT);
+const std::string output_dir = createTempDir(testController_.get());
+
+addGetFileProcessorToPlan(input_dir);
+if (Expectation::PROCESSOR_INITIALIZATION_EXCEPTION == expectation) {
+ REQUIRE_THROWS(addExecutePythonProcessorToPlan(used_as_script_file,
used_as_script_body));
+ return;
+}
+REQUIRE_NOTHROW(addExecutePythonProcessorToPlan(used_as_script_file,
used_as_script_body));
+addPutFileProcessorToPlan(execute_python_out_conn, output_dir);
+
+plan_->runNextProcessor(); // GetFile
+if (Expectation::RUNTIME_RELATIONSHIP_EXCEPTION == expectation) {
+ REQUIRE_THROWS(plan_->runNextProcessor()); // ExecutePythonProcessor
+ return;
+}
+REQUIRE_NOTHROW(plan_->runNextProcessor()); // ExecutePythonProcessor
+plan_->runNextProcessor(); // PutFile
+
+const std::string output_file_path = output_dir +
utils::file::FileUtils::get_separator() + TEST_FILE_NAME;
+
+if (Expectation::OUTPUT_FILE_MATCHES_INPUT == expectation) {
+ const std::string output_file_content{ getFileContent(output_file_path)
};
+ REQUIRE(TEST_FILE_CONTENT == output_file_content);
+}
+ }
+ void testsStatefulProcessor() {
+reInitialize();
+const std::string output_dir = createTempDir(testController_.get());
+
+auto executePythonProcessor =
plan_->addProcessor("ExecutePythonProcessor",
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #784: MINIFICPP-1206 - Rework and test ExecutePythonProcessor, add in-place script support
arpadboda commented on a change in pull request #784: URL: https://github.com/apache/nifi-minifi-cpp/pull/784#discussion_r444189007 ## File path: extensions/script/python/ExecutePythonProcessor.h ## @@ -18,12 +18,16 @@ * limitations under the License. */ -#ifndef NIFI_MINIFI_CPP_EXECUTEPYPROC_H -#define NIFI_MINIFI_CPP_EXECUTEPYPROC_H +#ifndef EXTENSIONS_SCRIPT_PYTHON_EXECUTEPYTHONPROCESSOR_H_ +#define EXTENSIONS_SCRIPT_PYTHON_EXECUTEPYTHONPROCESSOR_H_ -#include -#include -#include +#include +#include +#include + +#include // NOLINT +#include // NOLINT Review comment: I would simply disable that error, makes no sense to me. 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: us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #784: MINIFICPP-1206 - Rework and test ExecutePythonProcessor, add in-place script support
arpadboda commented on a change in pull request #784:
URL: https://github.com/apache/nifi-minifi-cpp/pull/784#discussion_r442041040
##
File path: extensions/script/python/ExecutePythonProcessor.cpp
##
@@ -35,155 +35,184 @@ namespace python {
namespace processors {
core::Property ExecutePythonProcessor::ScriptFile("Script File", // NOLINT
-R"(Path to script file to execute)", "");
+R"(Path to script file to execute. Only one of Script File or Script Body
may be used)", "");
+core::Property ExecutePythonProcessor::ScriptBody("Script Body", // NOLINT
Review comment:
Using property builder (withdescription, blah blah) can help breaking
these lines without linter errors. And improve readability as well.
You can also specify some to be required.
##
File path: extensions/script/python/ExecutePythonProcessor.cpp
##
@@ -35,155 +35,184 @@ namespace python {
namespace processors {
core::Property ExecutePythonProcessor::ScriptFile("Script File", // NOLINT
-R"(Path to script file to execute)", "");
+R"(Path to script file to execute. Only one of Script File or Script Body
may be used)", "");
+core::Property ExecutePythonProcessor::ScriptBody("Script Body", // NOLINT
+R"(Script to execute. Only one of Script File or Script Body may be
used)", "");
core::Property ExecutePythonProcessor::ModuleDirectory("Module Directory", //
NOLINT
-R"(Comma-separated list of paths to files and/or directories which
- contain modules required by
the script)", "");
+R"(Comma-separated list of paths to files and/or directories which contain
modules required by the script)", "");
core::Relationship ExecutePythonProcessor::Success("success", "Script
successes"); // NOLINT
core::Relationship ExecutePythonProcessor::Failure("failure", "Script
failures"); // NOLINT
void ExecutePythonProcessor::initialize() {
// initialization requires that we do a little leg work prior to onSchedule
// so that we can provide manifest our processor identity
- std::set properties;
-
- std::string prop;
- getProperty(ScriptFile.getName(), prop);
-
- properties.insert(ScriptFile);
- properties.insert(ModuleDirectory);
- setSupportedProperties(properties);
-
- std::set relationships;
- relationships.insert(Success);
- relationships.insert(Failure);
- setSupportedRelationships(std::move(relationships));
- setAcceptAllProperties();
- if (!prop.empty()) {
-setProperty(ScriptFile, prop);
-std::shared_ptr engine;
-python_logger_ =
logging::LoggerFactory::getAliasedLogger(getName());
+ if (getProperties().empty()) {
+setSupportedProperties({
+ ScriptFile,
+ ScriptBody,
+ ModuleDirectory
+});
+setAcceptAllProperties();
+setSupportedRelationships({
+ Success,
+ Failure
+});
+valid_init_ = false;
+return;
+ }
-engine = createEngine();
+ python_logger_ =
logging::LoggerFactory::getAliasedLogger(getName());
-if (engine == nullptr) {
- throw std::runtime_error("No script engine available");
-}
+ getProperty(ModuleDirectory.getName(), module_directory_);
-try {
- engine->evalFile(prop);
- auto me = shared_from_this();
- triggerDescribe(engine, me);
- triggerInitialize(engine, me);
+ valid_init_ = false;
+ appendPathForImportModules();
+ loadScript();
+ try {
+if (script_to_exec_.size()) {
+ std::shared_ptr engine = getScriptEngine();
+ engine->eval(script_to_exec_);
+ auto shared_this = shared_from_this();
+ engine->describe(shared_this);
+ engine->onInitialize(shared_this);
+ handleEngineNoLongerInUse(std::move(engine));
valid_init_ = true;
-} catch (std::exception ) {
- logger_->log_error("Caught Exception %s", exception.what());
- engine = nullptr;
- std::rethrow_exception(std::current_exception());
- valid_init_ = false;
-} catch (...) {
- logger_->log_error("Caught Exception");
- engine = nullptr;
- std::rethrow_exception(std::current_exception());
- valid_init_ = false;
}
-
+ }
+ catch (const std::exception& exception) {
+logger_->log_error("Caught Exception: %s", exception.what());
+std::rethrow_exception(std::current_exception());
+ }
+ catch (...) {
+logger_->log_error("Caught Exception");
+std::rethrow_exception(std::current_exception());
}
}
void ExecutePythonProcessor::onSchedule(const
std::shared_ptr , const
std::shared_ptr ) {
if (!valid_init_) {
-throw std::runtime_error("Could not correctly in initialize " + getName());
- }
- context->getProperty(ScriptFile.getName(), script_file_);
- context->getProperty(ModuleDirectory.getName(), module_directory_);
- if (script_file_.empty() && script_engine_.empty()) {
-logger_->log_error("Script File must be defined");
-return;
+throw std::runtime_error("Could not correctly initialize " +
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #784: MINIFICPP-1206 - Rework and test ExecutePythonProcessor, add in-place script support
arpadboda commented on a change in pull request #784:
URL: https://github.com/apache/nifi-minifi-cpp/pull/784#discussion_r435407683
##
File path: extensions/script/python/ExecutePythonProcessor.cpp
##
@@ -35,7 +35,11 @@ namespace python {
namespace processors {
core::Property ExecutePythonProcessor::ScriptFile("Script File", // NOLINT
-R"(Path to script file to execute)", "");
+R"(Path to script file to execute.
+Only one of Script File or Script
Body may be used)", "");
+core::Property ExecutePythonProcessor::ScriptBody("Script Body", // NOLINT
+R"(Script to execute.
+Only one of Script File or Script
Body may be used)", "");
Review comment:
Yes, please!
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #784: MINIFICPP-1206 - Rework and test ExecutePythonProcessor, add in-place script support
arpadboda commented on a change in pull request #784:
URL: https://github.com/apache/nifi-minifi-cpp/pull/784#discussion_r435341599
##
File path: libminifi/include/utils/TestUtils.h
##
@@ -0,0 +1,80 @@
+/**
+ *
+ * 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.
+ */
+
+#include
+
+#include "../../test/TestBase.h"
+
+#ifdef WIN32
+#include
+#define GetCurrentDir _getcwd
+#else
+#include
+#define GetCurrentDir getcwd
+#endif
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+std::string getCurrentWorkingDir(void) {
+ char buff[FILENAME_MAX];
+ GetCurrentDir(buff, FILENAME_MAX);
+ std::string current_working_dir(buff);
+ return current_working_dir;
+}
+
+std::string createTempDir(TestController* testController) {
Review comment:
Can it fail anyhow?
I feel that in this case we verify our own testutils, which seems like a bit
overprotective.
A lot of tests are going to fail in case we can't create tempdirs.
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #784: MINIFICPP-1206 - Rework and test ExecutePythonProcessor, add in-place script support
arpadboda commented on a change in pull request #784:
URL: https://github.com/apache/nifi-minifi-cpp/pull/784#discussion_r435340201
##
File path: extensions/script/python/ExecutePythonProcessor.cpp
##
@@ -35,7 +35,11 @@ namespace python {
namespace processors {
core::Property ExecutePythonProcessor::ScriptFile("Script File", // NOLINT
-R"(Path to script file to execute)", "");
+R"(Path to script file to execute.
+Only one of Script File or Script
Body may be used)", "");
+core::Property ExecutePythonProcessor::ScriptBody("Script Body", // NOLINT
+R"(Script to execute.
+Only one of Script File or Script
Body may be used)", "");
Review comment:
I don't think it's helpful at all, no other processors has this weird
line breaks in descriptions.
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #784: MINIFICPP-1206 - Rework and test ExecutePythonProcessor, add in-place script support
arpadboda commented on a change in pull request #784:
URL: https://github.com/apache/nifi-minifi-cpp/pull/784#discussion_r435339793
##
File path: libminifi/include/utils/TestUtils.h
##
@@ -0,0 +1,80 @@
+/**
+ *
+ * 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.
+ */
+
+#include
+
+#include "../../test/TestBase.h"
+
+#ifdef WIN32
+#include
+#define GetCurrentDir _getcwd
+#else
+#include
+#define GetCurrentDir getcwd
+#endif
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+std::string getCurrentWorkingDir(void) {
Review comment:
Misguided you a bit, sorry!
It's called:
```
utils::Environment::getCurrentWorkingDirectory()
```
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:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #784: MINIFICPP-1206 - Rework and test ExecutePythonProcessor, add in-place script support
arpadboda commented on a change in pull request #784:
URL: https://github.com/apache/nifi-minifi-cpp/pull/784#discussion_r430401028
##
File path: extensions/script/python/ExecutePythonProcessor.cpp
##
@@ -35,7 +35,11 @@ namespace python {
namespace processors {
core::Property ExecutePythonProcessor::ScriptFile("Script File", // NOLINT
-R"(Path to script file to execute)", "");
+R"(Path to script file to execute.
+Only one of Script File or Script
Body may be used)", "");
+core::Property ExecutePythonProcessor::ScriptBody("Script Body", // NOLINT
+R"(Script to execute.
+Only one of Script File or Script
Body may be used)", "");
Review comment:
This looks bad, why split to two lines?
##
File path: libminifi/include/utils/TestUtils.h
##
@@ -0,0 +1,80 @@
+/**
+ *
+ * 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.
+ */
+
+#include
+
+#include "../../test/TestBase.h"
+
+#ifdef WIN32
+#include
+#define GetCurrentDir _getcwd
+#else
+#include
+#define GetCurrentDir getcwd
+#endif
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+std::string getCurrentWorkingDir(void) {
Review comment:
This exists in FileUtils or pathutils
##
File path: libminifi/include/utils/TestUtils.h
##
@@ -0,0 +1,80 @@
+/**
+ *
+ * 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.
+ */
+
+#include
+
+#include "../../test/TestBase.h"
+
+#ifdef WIN32
+#include
+#define GetCurrentDir _getcwd
+#else
+#include
+#define GetCurrentDir getcwd
+#endif
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+std::string getCurrentWorkingDir(void) {
+ char buff[FILENAME_MAX];
+ GetCurrentDir(buff, FILENAME_MAX);
+ std::string current_working_dir(buff);
+ return current_working_dir;
+}
+
+std::string createTempDir(TestController* testController) {
Review comment:
Why do we need this wrapper? Especially why do we do with asserts.
##
File path: libminifi/include/utils/TestUtils.h
##
@@ -0,0 +1,80 @@
+/**
+ *
+ * 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.
+ */
+
+#include
+
+#include "../../test/TestBase.h"
+
+#ifdef WIN32
+#include
+#define GetCurrentDir _getcwd
+#else
+#include
+#define GetCurrentDir getcwd
+#endif
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+std::string getCurrentWorkingDir(void) {
+ char buff[FILENAME_MAX];
+ GetCurrentDir(buff, FILENAME_MAX);
+ std::string current_working_dir(buff);
+ return current_working_dir;
+}
+
+std::string
