Copilot commented on code in PR #2096: URL: https://github.com/apache/nifi-minifi-cpp/pull/2096#discussion_r2773864439
########## libminifi/include/utils/CControllerService.h: ########## @@ -0,0 +1,108 @@ +/** +* 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. + */ + +#pragma once + +#pragma once + +#include <string> +#include <vector> + +#include "minifi-c/minifi-c.h" +#include "minifi-cpp/Exception.h" +#include "minifi-cpp/core/ControllerServiceMetadata.h" +#include "minifi-cpp/core/Property.h" +#include "minifi-cpp/core/controller/ControllerServiceApi.h" + +namespace org::apache::nifi::minifi::utils { +class CControllerService; + + +struct CControllerServiceClassDescription { + std::string group_name; + std::string name; + std::string version; + + std::vector<core::Property> class_properties; + + MinifiControllerServiceCallbacks callbacks; +}; + +class CControllerService final : public core::controller::ControllerServiceApi, public core::controller::ControllerServiceInterface { + public: + CControllerService(CControllerServiceClassDescription class_description, core::ControllerServiceMetadata metadata) + : class_description_(std::move(class_description)), + metadata_(std::move(metadata)) { + MinifiControllerServiceMetadata c_metadata; + auto uuid_str = metadata.uuid.to_string(); + c_metadata.uuid = MinifiStringView{.data = uuid_str.data(), .length = uuid_str.length()}; + c_metadata.name = MinifiStringView{.data = metadata.name.data(), .length = metadata.name.length()}; Review Comment: Potential dangling pointer issue: c_metadata.name holds a MinifiStringView pointing to metadata.name.data(). Since metadata is moved in the member initializer list (line 49), the behavior is undefined. The metadata object is moved-from before this line executes, potentially leaving c_metadata.name pointing to invalid memory. Consider using metadata_ (the member variable) instead of metadata, or avoid moving metadata until after creating c_metadata. ```suggestion auto uuid_str = metadata_.uuid.to_string(); c_metadata.uuid = MinifiStringView{.data = uuid_str.data(), .length = uuid_str.length()}; c_metadata.name = MinifiStringView{.data = metadata_.name.data(), .length = metadata_.name.length()}; ``` ########## libminifi/include/utils/CControllerService.h: ########## @@ -0,0 +1,108 @@ +/** +* 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. + */ + +#pragma once + +#pragma once + Review Comment: Duplicate pragma once directives detected. The header guard appears twice on lines 18 and 20. Remove the duplicate on line 20. ```suggestion ``` ########## libminifi/include/utils/CControllerService.h: ########## @@ -0,0 +1,108 @@ +/** +* 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. + */ + +#pragma once + +#pragma once + +#include <string> +#include <vector> + +#include "minifi-c/minifi-c.h" +#include "minifi-cpp/Exception.h" +#include "minifi-cpp/core/ControllerServiceMetadata.h" +#include "minifi-cpp/core/Property.h" +#include "minifi-cpp/core/controller/ControllerServiceApi.h" + +namespace org::apache::nifi::minifi::utils { +class CControllerService; + + +struct CControllerServiceClassDescription { + std::string group_name; + std::string name; + std::string version; + + std::vector<core::Property> class_properties; + + MinifiControllerServiceCallbacks callbacks; +}; + +class CControllerService final : public core::controller::ControllerServiceApi, public core::controller::ControllerServiceInterface { + public: + CControllerService(CControllerServiceClassDescription class_description, core::ControllerServiceMetadata metadata) + : class_description_(std::move(class_description)), + metadata_(std::move(metadata)) { + MinifiControllerServiceMetadata c_metadata; + auto uuid_str = metadata.uuid.to_string(); + c_metadata.uuid = MinifiStringView{.data = uuid_str.data(), .length = uuid_str.length()}; + c_metadata.name = MinifiStringView{.data = metadata.name.data(), .length = metadata.name.length()}; + c_metadata.logger = reinterpret_cast<MinifiLogger*>(&metadata_.logger); + impl_ = class_description_.callbacks.create(c_metadata); Review Comment: Potential dangling pointer issue: the local variable uuid_str will go out of scope after the constructor executes, but c_metadata.uuid holds a MinifiStringView pointing to its data. This MinifiStringView is then passed to the create callback which may store it. Since uuid_str is destroyed at the end of the constructor, this creates a dangling pointer. Consider storing uuid_str as a member variable or converting it within the create callback itself. ########## minifi-api/include/minifi-c/minifi-c.h: ########## @@ -172,13 +187,25 @@ typedef struct MinifiProcessorClassDefinition { MinifiProcessorCallbacks callbacks; } MinifiProcessorClassDefinition; +typedef struct MinifiControllerServiceClassDefinition { + MinifiStringView full_name; // '::'-delimited fully qualified name e.g. 'org::apache::nifi::minifi::extensions::gcp::GCPCredentialsControllerService Review Comment: Missing closing single quote in the comment example. The comment should end with a closing single quote after 'GCPCredentialsControllerService'. ```suggestion MinifiStringView full_name; // '::'-delimited fully qualified name e.g. 'org::apache::nifi::minifi::extensions::gcp::GCPCredentialsControllerService' ``` ########## Extensions.md: ########## @@ -47,7 +47,10 @@ extern "C" MinifiExtension* InitExtension(MinifiConfig* /*config*/) { .deinit = nullptr, .user_data = nullptr, .processors_count = 0, - .processors_ptr = nullptr + .processors_ptr = nullptr, + .controller_services_count = 0, + .controller_services_ptr = nullptr, + Review Comment: Trailing whitespace on line 53. Remove the trailing whitespace for consistency. ########## libminifi/src/minifi-c.cpp: ########## @@ -440,8 +512,55 @@ void MinifiFlowFileGetAttributes(MinifiProcessSession* session, MinifiFlowFile* gsl_Assert(session != MINIFI_NULL); gsl_Assert(flowfile != MINIFI_NULL); for (auto& [key, value] : (*reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile))->getAttributes()) { - cb(user_ctx, MinifiStringView{.data = key.data(), .length = key.size()}, MinifiStringView{.data = value.data(), .length = value.size()}); + cb(user_ctx, minifiStringView(key), minifiStringView(value)); } } +MinifiStatus MinifiControllerServiceContextGetProperty(MinifiControllerServiceContext* context, MinifiStringView property_name, + void (*result_cb)(void* user_ctx, MinifiStringView result), void* user_ctx) { + gsl_Assert(context != MINIFI_NULL); + auto result = reinterpret_cast<minifi::core::controller::ControllerServiceContext*>(context)->getProperty(toStringView(property_name)); + if (result) { + result_cb(user_ctx, minifiStringView(result.value())); + return MINIFI_STATUS_SUCCESS; + } + switch (static_cast<minifi::core::PropertyErrorCode>(result.error().value())) { + case minifi::core::PropertyErrorCode::NotSupportedProperty: return MINIFI_STATUS_NOT_SUPPORTED_PROPERTY; + case minifi::core::PropertyErrorCode::DynamicPropertiesNotSupported: return MINIFI_STATUS_DYNAMIC_PROPERTIES_NOT_SUPPORTED; + case minifi::core::PropertyErrorCode::PropertyNotSet: return MINIFI_STATUS_PROPERTY_NOT_SET; + case minifi::core::PropertyErrorCode::ValidationFailed: return MINIFI_STATUS_VALIDATION_FAILED; + default: return MINIFI_STATUS_UNKNOWN_ERROR; + } +} + + +MinifiStatus MinifiProcessContextGetControllerService( + MinifiProcessContext* process_context, + MinifiStringView controller_service_name, + void(*cb)( + void* user_ctx, + void* service, + MinifiStringView group_name, + MinifiStringView class_name, Review Comment: Inconsistent parameter naming between declaration and implementation. In the header (line 224), the callback parameters are named 'type', 'group', 'version', but in the implementation (lines 543-545), they are 'group_name', 'class_name', 'version'. Consider renaming for consistency - either use 'type'/'group' or 'class_name'/'group_name' in both places. ```suggestion MinifiStringView type, MinifiStringView group, ``` ########## libminifi/src/minifi-c.cpp: ########## @@ -440,8 +512,55 @@ void MinifiFlowFileGetAttributes(MinifiProcessSession* session, MinifiFlowFile* gsl_Assert(session != MINIFI_NULL); gsl_Assert(flowfile != MINIFI_NULL); for (auto& [key, value] : (*reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile))->getAttributes()) { - cb(user_ctx, MinifiStringView{.data = key.data(), .length = key.size()}, MinifiStringView{.data = value.data(), .length = value.size()}); + cb(user_ctx, minifiStringView(key), minifiStringView(value)); } } +MinifiStatus MinifiControllerServiceContextGetProperty(MinifiControllerServiceContext* context, MinifiStringView property_name, + void (*result_cb)(void* user_ctx, MinifiStringView result), void* user_ctx) { + gsl_Assert(context != MINIFI_NULL); + auto result = reinterpret_cast<minifi::core::controller::ControllerServiceContext*>(context)->getProperty(toStringView(property_name)); + if (result) { + result_cb(user_ctx, minifiStringView(result.value())); + return MINIFI_STATUS_SUCCESS; + } + switch (static_cast<minifi::core::PropertyErrorCode>(result.error().value())) { + case minifi::core::PropertyErrorCode::NotSupportedProperty: return MINIFI_STATUS_NOT_SUPPORTED_PROPERTY; + case minifi::core::PropertyErrorCode::DynamicPropertiesNotSupported: return MINIFI_STATUS_DYNAMIC_PROPERTIES_NOT_SUPPORTED; + case minifi::core::PropertyErrorCode::PropertyNotSet: return MINIFI_STATUS_PROPERTY_NOT_SET; + case minifi::core::PropertyErrorCode::ValidationFailed: return MINIFI_STATUS_VALIDATION_FAILED; + default: return MINIFI_STATUS_UNKNOWN_ERROR; + } +} + + +MinifiStatus MinifiProcessContextGetControllerService( + MinifiProcessContext* process_context, + MinifiStringView controller_service_name, + void(*cb)( + void* user_ctx, + void* service, + MinifiStringView group_name, + MinifiStringView class_name, + MinifiStringView version), + void* user_ctx) { + + gsl_Assert(process_context != MINIFI_NULL); + const auto context = reinterpret_cast<minifi::core::ProcessContext*>(process_context); + const auto name_str = std::string{toStringView(controller_service_name)}; + const auto service_shared_ptr = context->getControllerService(name_str, context->getProcessorInfo().getUUID()); Review Comment: Missing null pointer check: getControllerService can return a null shared_ptr if the controller service is not found. Before dereferencing service_shared_ptr on line 552, check if it's null and return an appropriate error status (e.g., MINIFI_STATUS_UNKNOWN_ERROR or a new status code for controller service not found). ```suggestion const auto service_shared_ptr = context->getControllerService(name_str, context->getProcessorInfo().getUUID()); if (!service_shared_ptr) { return MINIFI_STATUS_UNKNOWN_ERROR; } ``` -- 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]
