[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1634: MINIFICPP-2146 Add support for SMB networking protocol

["szaszm (via GitHub)" <gi...@apache.org>]

szaszm commented on code in PR #1634:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1634#discussion_r1322871552


##########
extensions/smb/PutSmb.cpp:
##########
@@ -0,0 +1,94 @@
+/**
+ *
+ * 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"
+#include "core/Resource.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);
+  smb_connection_controller_service_ = SmbConnectionControllerService::getFromProperty(*context, PutSmb::ConnectionControllerService);
+  create_missing_dirs_ = context->getProperty<bool>(PutSmb::CreateMissingDirectories).value_or(true);
+  conflict_resolution_strategy_ = utils::parseEnumProperty<FileExistsResolutionStrategy>(*context, ConflictResolution);

Review Comment:
   Did this start throwing on invalid values, instead of proceeding with a warning? Would it make sense to warn and fallback to a sensible default in these cases?



##########
libminifi/include/utils/file/FileUtils.h:
##########
@@ -570,4 +574,23 @@ inline std::optional<std::filesystem::path> get_relative_path(const std::filesys
   return std::filesystem::relative(path, base_path);
 }
 
+inline size_t countNumberOfFiles(const std::filesystem::path& path) {
+  size_t counter = 0;
+  for (const auto& entry : std::filesystem::directory_iterator(path)) {
+    if (entry.is_regular_file())
+      ++counter;
+  }
+  return counter;
+}

Review Comment:
   This should be a `std::count_if`



##########
extensions/smb/FetchSmb.cpp:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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);
+  smb_connection_controller_service_ = SmbConnectionControllerService::getFromProperty(*context, FetchSmb::ConnectionControllerService);
+}
+
+namespace {
+std::filesystem::path getTargetRelativePath(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_);
+
+  if (auto connection_error = smb_connection_controller_service_->validateConnection()) {
+    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;
+  }
+
+  try {
+    session->write(flow_file, utils::FileReaderCallback{smb_connection_controller_service_->getPath() / getTargetRelativePath(*context, flow_file)});
+    session->transfer(flow_file, Success);
+  } catch (const utils::FileReaderCallbackIOError& io_error) {
+    flow_file->addAttribute(ErrorCode.name, fmt::format("{}", io_error.error_code));

Review Comment:
   This is a string conversion, not string formatting, in my understanding.
   ```suggestion
       flow_file->addAttribute(ErrorCode.name, std::to_string(io_error.error_code));
   ```



##########
extensions/smb/FetchSmb.cpp:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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);
+  smb_connection_controller_service_ = SmbConnectionControllerService::getFromProperty(*context, FetchSmb::ConnectionControllerService);
+}
+
+namespace {
+std::filesystem::path getTargetRelativePath(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());

Review Comment:
   Why is this just blindly removing a forward slash from the beginning? I thought Windows uses backslashes, but regardless, it's unclear why this is happening.



##########
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." OFF)

Review Comment:
   This could be enabled by default on Windows IMO. It only depends on WinAPI, so there's no extra maintenance requirements coming from managing third party libs.



##########
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");
+  }

Review Comment:
   You have a point, let's just mention this in the processor description and leave the interface as is.



-- 
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