lordgamez commented on code in PR #1634:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1634#discussion_r1302948771
##########
cmake/MiNiFiOptions.cmake:
##########
@@ -85,6 +85,7 @@ if (WIN32)
add_minifi_option(MSI_REDISTRIBUTE_UCRT_NONASL "Redistribute Universal C
Runtime DLLs with the MSI generated by CPack. The resulting MSI is not
distributable under Apache 2.0." OFF)
add_minifi_option(ENABLE_WEL "Enables the suite of Windows Event Log
extensions." OFF)
add_minifi_option(ENABLE_PDH "Enables PDH support." OFF)
+ add_minifi_option(ENABLE_SMB "Enables SMB support." ON)
Review Comment:
What's the reason this is enabled by default? Should we add an option to
disable it in the win_build_vs.bat file?
##########
extensions/smb/FetchSmb.cpp:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 "FetchSmb.h"
+#include "core/Resource.h"
+#include "utils/file/FileReaderCallback.h"
+
+namespace org::apache::nifi::minifi::extensions::smb {
+
+void FetchSmb::initialize() {
+ setSupportedProperties(Properties);
+ setSupportedRelationships(Relationships);
+}
+
+void FetchSmb::onSchedule(const std::shared_ptr<core::ProcessContext>&
context, const std::shared_ptr<core::ProcessSessionFactory>&) {
+ gsl_Expects(context);
+ if (auto connection_controller_name =
context->getProperty(FetchSmb::ConnectionControllerService)) {
+ smb_connection_controller_service_ =
std::dynamic_pointer_cast<SmbConnectionControllerService>(context->getControllerService(*connection_controller_name));
+ }
+ if (!smb_connection_controller_service_) {
+ throw minifi::Exception(ExceptionType::PROCESS_SCHEDULE_EXCEPTION,
"Missing SMB Connection Controller Service");
+ }
+}
+
+namespace {
+std::filesystem::path getPath(core::ProcessContext& context, const
std::shared_ptr<core::FlowFile>& flow_file) {
+ auto remote_file = context.getProperty(FetchSmb::RemoteFile, flow_file);
+ if (remote_file && !remote_file->empty()) {
+ if (remote_file->starts_with('/'))
+ remote_file->erase(remote_file->begin());
+ return *remote_file;
+ }
+ std::filesystem::path path =
flow_file->getAttribute(core::SpecialFlowAttribute::PATH).value_or("");
+ std::filesystem::path filename =
flow_file->getAttribute(core::SpecialFlowAttribute::FILENAME).value_or("");
+ return path / filename;
+}
+} // namespace
+
+void FetchSmb::onTrigger(const std::shared_ptr<core::ProcessContext>& context,
const std::shared_ptr<core::ProcessSession>& session) {
+ gsl_Expects(context && session && smb_connection_controller_service_);
+
+ auto connection_error =
smb_connection_controller_service_->validateConnection();
Review Comment:
Shouldn't we validate the connection still in the onSchedule phase? Same for
the other processors.
##########
extensions/smb/FetchSmb.cpp:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 "FetchSmb.h"
+#include "core/Resource.h"
+#include "utils/file/FileReaderCallback.h"
+
+namespace org::apache::nifi::minifi::extensions::smb {
+
+void FetchSmb::initialize() {
+ setSupportedProperties(Properties);
+ setSupportedRelationships(Relationships);
+}
+
+void FetchSmb::onSchedule(const std::shared_ptr<core::ProcessContext>&
context, const std::shared_ptr<core::ProcessSessionFactory>&) {
+ gsl_Expects(context);
+ if (auto connection_controller_name =
context->getProperty(FetchSmb::ConnectionControllerService)) {
+ smb_connection_controller_service_ =
std::dynamic_pointer_cast<SmbConnectionControllerService>(context->getControllerService(*connection_controller_name));
+ }
+ if (!smb_connection_controller_service_) {
+ throw minifi::Exception(ExceptionType::PROCESS_SCHEDULE_EXCEPTION,
"Missing SMB Connection Controller Service");
+ }
+}
+
+namespace {
+std::filesystem::path getPath(core::ProcessContext& context, const
std::shared_ptr<core::FlowFile>& flow_file) {
Review Comment:
nitpick, but `getPath` seems to be a bit too generic function name for me
##########
extensions/smb/FetchSmb.cpp:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 "FetchSmb.h"
+#include "core/Resource.h"
+#include "utils/file/FileReaderCallback.h"
+
+namespace org::apache::nifi::minifi::extensions::smb {
+
+void FetchSmb::initialize() {
+ setSupportedProperties(Properties);
+ setSupportedRelationships(Relationships);
+}
+
+void FetchSmb::onSchedule(const std::shared_ptr<core::ProcessContext>&
context, const std::shared_ptr<core::ProcessSessionFactory>&) {
+ gsl_Expects(context);
+ if (auto connection_controller_name =
context->getProperty(FetchSmb::ConnectionControllerService)) {
+ smb_connection_controller_service_ =
std::dynamic_pointer_cast<SmbConnectionControllerService>(context->getControllerService(*connection_controller_name));
+ }
+ if (!smb_connection_controller_service_) {
+ throw minifi::Exception(ExceptionType::PROCESS_SCHEDULE_EXCEPTION,
"Missing SMB Connection Controller Service");
+ }
+}
+
+namespace {
+std::filesystem::path getPath(core::ProcessContext& context, const
std::shared_ptr<core::FlowFile>& flow_file) {
+ auto remote_file = context.getProperty(FetchSmb::RemoteFile, flow_file);
+ if (remote_file && !remote_file->empty()) {
+ if (remote_file->starts_with('/'))
+ remote_file->erase(remote_file->begin());
+ return *remote_file;
+ }
+ std::filesystem::path path =
flow_file->getAttribute(core::SpecialFlowAttribute::PATH).value_or("");
+ std::filesystem::path filename =
flow_file->getAttribute(core::SpecialFlowAttribute::FILENAME).value_or("");
+ return path / filename;
+}
+} // namespace
+
+void FetchSmb::onTrigger(const std::shared_ptr<core::ProcessContext>& context,
const std::shared_ptr<core::ProcessSession>& session) {
+ gsl_Expects(context && session && smb_connection_controller_service_);
+
+ auto connection_error =
smb_connection_controller_service_->validateConnection();
+ if (connection_error) {
Review Comment:
These 2 lines could be merged. Same for the other processors.
##########
extensions/smb/FetchSmb.cpp:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 "FetchSmb.h"
+#include "core/Resource.h"
+#include "utils/file/FileReaderCallback.h"
+
+namespace org::apache::nifi::minifi::extensions::smb {
+
+void FetchSmb::initialize() {
+ setSupportedProperties(Properties);
+ setSupportedRelationships(Relationships);
+}
+
+void FetchSmb::onSchedule(const std::shared_ptr<core::ProcessContext>&
context, const std::shared_ptr<core::ProcessSessionFactory>&) {
+ gsl_Expects(context);
+ if (auto connection_controller_name =
context->getProperty(FetchSmb::ConnectionControllerService)) {
+ smb_connection_controller_service_ =
std::dynamic_pointer_cast<SmbConnectionControllerService>(context->getControllerService(*connection_controller_name));
+ }
+ if (!smb_connection_controller_service_) {
+ throw minifi::Exception(ExceptionType::PROCESS_SCHEDULE_EXCEPTION,
"Missing SMB Connection Controller Service");
+ }
+}
+
+namespace {
+std::filesystem::path getPath(core::ProcessContext& context, const
std::shared_ptr<core::FlowFile>& flow_file) {
+ auto remote_file = context.getProperty(FetchSmb::RemoteFile, flow_file);
+ if (remote_file && !remote_file->empty()) {
+ if (remote_file->starts_with('/'))
+ remote_file->erase(remote_file->begin());
+ return *remote_file;
+ }
+ std::filesystem::path path =
flow_file->getAttribute(core::SpecialFlowAttribute::PATH).value_or("");
+ std::filesystem::path filename =
flow_file->getAttribute(core::SpecialFlowAttribute::FILENAME).value_or("");
+ return path / filename;
+}
+} // namespace
+
+void FetchSmb::onTrigger(const std::shared_ptr<core::ProcessContext>& context,
const std::shared_ptr<core::ProcessSession>& session) {
+ gsl_Expects(context && session && smb_connection_controller_service_);
+
+ auto connection_error =
smb_connection_controller_service_->validateConnection();
+ if (connection_error) {
+ logger_->log_error("Couldn't establish connection to the specified network
location due to %s", connection_error.message());
+ context->yield();
+ return;
+ }
+
+ auto flow_file = session->get();
+ if (!flow_file) {
+ context->yield();
+ return;
+ }
+
+ auto path = getPath(*context, flow_file);
+
+ try {
+ session->write(flow_file,
utils::FileReaderCallback{smb_connection_controller_service_->getPath() /
path});
+ session->transfer(flow_file, Success);
+ } catch (const utils::FileReaderCallbackIOError& io_error) {
+ flow_file->addAttribute(ErrorCode.name, fmt::format("{}",
io_error.error_code));
+ flow_file->addAttribute(ErrorMessage.name, io_error.what());
+ session->transfer(flow_file, Failure);
+ return;
Review Comment:
This line is not needed
##########
extensions/smb/SmbConnectionControllerService.cpp:
##########
@@ -0,0 +1,107 @@
+/**
+ *
+ * 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 "SmbConnectionControllerService.h"
+#include "core/Resource.h"
+#include "utils/OsUtils.h"
+#include "utils/expected.h"
+
+namespace org::apache::nifi::minifi::extensions::smb {
+
+void SmbConnectionControllerService::initialize() {
+ setSupportedProperties(Properties);
+}
+
+void SmbConnectionControllerService::onEnable() {
+ std::string hostname;
+ std::string share;
+
+ if (!getProperty(Hostname, hostname))
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Missing hostname");
+
+ if (!getProperty(Share, share))
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Missing share");
+
+
+ server_path_ = "\\\\" + hostname + "\\" + share;
+
+ auto password = getProperty(Password);
+ auto username = getProperty(Username);
+
+ if (password.has_value() != username.has_value())
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Either a username and
password must be provided, or neither of them should be provided.");
+
+ if (username.has_value())
+ credentials_.emplace(Credentials{.username = *username, .password =
*password});
+ else
+ credentials_.reset();
+
+ ZeroMemory(&net_resource_, sizeof(net_resource_));
+ net_resource_.dwType = RESOURCETYPE_DISK;
+ net_resource_.lpLocalName = nullptr;
+ net_resource_.lpRemoteName = server_path_.data();
+ net_resource_.lpProvider = nullptr;
+}
+
+void SmbConnectionControllerService::notifyStop() {
+ auto disconnection_result = disconnect();
+ if (!disconnection_result)
+ logger_->log_error("Error while disconnecting from SMB: %s",
disconnection_result.error().message());
+}
+
+nonstd::expected<void, std::error_code>
SmbConnectionControllerService::connect() {
+ auto connection_result = WNetAddConnection2A(&net_resource_,
+ credentials_ ? credentials_->password.c_str() : nullptr,
+ credentials_ ? credentials_->username.c_str() : nullptr,
+ CONNECT_TEMPORARY);
+ if (connection_result == NO_ERROR)
+ return {};
+
+ return
nonstd::make_unexpected(utils::OsUtils::windowsErrorToErrorCode(connection_result));
+}
+
+nonstd::expected<void, std::error_code>
SmbConnectionControllerService::disconnect() {
+ auto disconnection_result = WNetCancelConnection2A(server_path_.c_str(), 0,
true);
+ if (disconnection_result == NO_ERROR)
+ return {};
+
+ return
nonstd::make_unexpected(utils::OsUtils::windowsErrorToErrorCode(disconnection_result));
+}
+
+bool SmbConnectionControllerService::isConnected() {
+ std::error_code error_code;
+ auto exists = std::filesystem::exists(server_path_, error_code);
+ if (error_code)
+ return false;
Review Comment:
We could log the error message here
##########
extensions/smb/SmbConnectionControllerService.cpp:
##########
@@ -0,0 +1,107 @@
+/**
+ *
+ * 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 "SmbConnectionControllerService.h"
+#include "core/Resource.h"
+#include "utils/OsUtils.h"
+#include "utils/expected.h"
+
+namespace org::apache::nifi::minifi::extensions::smb {
+
+void SmbConnectionControllerService::initialize() {
+ setSupportedProperties(Properties);
+}
+
+void SmbConnectionControllerService::onEnable() {
+ std::string hostname;
+ std::string share;
+
+ if (!getProperty(Hostname, hostname))
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Missing hostname");
+
+ if (!getProperty(Share, share))
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Missing share");
+
+
Review Comment:
nitpick: unneeded newline
##########
extensions/standard-processors/processors/PutFile.h:
##########
@@ -1,7 +1,5 @@
+
Review Comment:
Accidental newline
##########
extensions/smb/PutSmb.cpp:
##########
@@ -0,0 +1,99 @@
+/**
+ *
+ * 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 "PutSmb.h"
+#include "utils/gsl.h"
+#include "utils/ProcessorConfigUtils.h"
+#include "utils/OsUtils.h"
+#include "utils/file/FileWriterCallback.h"
+
+namespace org::apache::nifi::minifi::extensions::smb {
+
+void PutSmb::initialize() {
+ setSupportedProperties(Properties);
+ setSupportedRelationships(Relationships);
+}
+
+
+void PutSmb::onSchedule(core::ProcessContext* context,
core::ProcessSessionFactory*) {
+ gsl_Expects(context);
+ if (auto connection_controller_name =
context->getProperty(PutSmb::ConnectionControllerService)) {
Review Comment:
Can we extract a utility function or have it in a common smb processor base
class to retrieve the smb controller service in all smb processors to remove
duplication?
##########
extensions/smb/tests/ListSmbTests.cpp:
##########
Review Comment:
Can we add a test case for ignoring/not ignoring hidden files?
##########
extensions/smb/PutSmb.cpp:
##########
@@ -0,0 +1,99 @@
+/**
+ *
+ * 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 "PutSmb.h"
+#include "utils/gsl.h"
+#include "utils/ProcessorConfigUtils.h"
+#include "utils/OsUtils.h"
+#include "utils/file/FileWriterCallback.h"
+
+namespace org::apache::nifi::minifi::extensions::smb {
+
+void PutSmb::initialize() {
+ setSupportedProperties(Properties);
+ setSupportedRelationships(Relationships);
+}
+
+
Review Comment:
nitpick, unneeded newline
##########
extensions/smb/SmbConnectionControllerService.cpp:
##########
@@ -0,0 +1,107 @@
+/**
+ *
+ * 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 "SmbConnectionControllerService.h"
+#include "core/Resource.h"
+#include "utils/OsUtils.h"
+#include "utils/expected.h"
+
+namespace org::apache::nifi::minifi::extensions::smb {
+
+void SmbConnectionControllerService::initialize() {
+ setSupportedProperties(Properties);
+}
+
+void SmbConnectionControllerService::onEnable() {
+ std::string hostname;
+ std::string share;
+
+ if (!getProperty(Hostname, hostname))
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Missing hostname");
+
+ if (!getProperty(Share, share))
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Missing share");
+
+
+ server_path_ = "\\\\" + hostname + "\\" + share;
+
+ auto password = getProperty(Password);
+ auto username = getProperty(Username);
+
+ if (password.has_value() != username.has_value())
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Either a username and
password must be provided, or neither of them should be provided.");
Review Comment:
I would rephrase it like "Either both a username and a password, or neither
of them should be provided."
##########
run_clang_tidy.sh:
##########
@@ -4,7 +4,7 @@ set -uo pipefail
FILE=$1
-EXCLUDED_DIRECTORY=("extensions/pdh" "extensions/windows-event-log" "nanofi")
+EXCLUDED_DIRECTORY=("extensions/pdh" "extensions/windows-event-log"
"extensions/smb" "nanofi")
Review Comment:
Why is smb excluded from clang-tidy?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
