Copilot commented on code in PR #2224:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2224#discussion_r3711075953
##########
minifi_rust/minifi_native/src/lib.rs:
##########
@@ -94,6 +94,7 @@ macro_rules! declare_minifi_extension {
let extension_definition =
minifi_native::sys::minifi_extension_definition {
name: env!("CARGO_PKG_NAME").as_minifi_c_type(),
version: env!("CARGO_PKG_VERSION").as_minifi_c_type(),
+ group_name: env!("CARGO_PKG_NAME").as_minifi_c_type(),
deinit: None,
user_data: std::ptr::null_mut(),
};
Review Comment:
`group_name` is currently set to `CARGO_PKG_NAME`, which is the
artifact/name, not a Maven-style group. This will keep producing incorrect
bundle group names in manifests for Rust extensions. Prefer a dedicated
build-time group (e.g. `MINIFI_EXTENSION_GROUP_NAME`) with a sensible default.
##########
minifi-api/include/minifi-api.h:
##########
@@ -208,6 +208,7 @@ struct minifi_controller_service_definition {
struct minifi_extension_definition {
struct minifi_string_view name;
struct minifi_string_view version;
+ struct minifi_string_view group_name;
void (*deinit)(void* user_data);
void* user_data;
};
Review Comment:
Adding `group_name` to `minifi_extension_definition` changes the struct
layout for all C extensions. To preserve binary compatibility,
`MINIFI_API_VERSION` should be bumped (and/or the loader should gate this field
on the reported API version) so older extensions don’t get their fields
misinterpreted at runtime.
##########
minifi-api/include/minifi-cpp/agent/agent_docs.h:
##########
@@ -51,32 +55,119 @@ struct ClassDescription {
bool isSingleThreaded_ = false;
};
-struct Components {
- std::vector<ClassDescription> processors;
- std::vector<ClassDescription> controller_services;
- std::vector<ClassDescription> parameter_providers;
- std::vector<ClassDescription> other_components;
-
- [[nodiscard]] bool empty() const noexcept {
- return processors.empty() && controller_services.empty() &&
parameter_providers.empty() && other_components.empty();
- }
-};
-
struct BundleIdentifier {
std::string name;
+ std::string group_name;
std::string version;
auto operator<=>(const BundleIdentifier& rhs) const = default;
};
+class Components {
+ public:
+ explicit Components(BundleIdentifier bundle_identifier) :
bundle_identifier_(std::move(bundle_identifier)) {
+ }
+ Components(const Components& rhs) = default;
+ Components(Components&& rhs) = default;
+ Components& operator=(const Components& rhs) = default;
+ Components& operator=(Components&& rhs) = default;
+ virtual ~Components() = default;
+
+ void addClassDescription(ClassDescription component, ResourceType
resource_type) {
+ switch (resource_type) {
+ case ResourceType::Processor: {
+ processors_.emplace_back(std::move(component));
+ break;
+ }
+ case ResourceType::ControllerService: {
+ controller_services_.emplace_back(std::move(component));
+ break;
+ }
+ case ResourceType::ParameterProvider: {
+ parameter_providers_.emplace_back(std::move(component));
+ break;
+ }
+ default: {
+ other_components_.emplace_back(std::move(component));
+ break;
+ }
+ }
+ };
+
+ const std::vector<ClassDescription>& getProcessors() const {
+ return processors_;
+ }
+ const std::vector<ClassDescription>& getControllerServices() const {
+ return controller_services_;
+ }
+ const std::vector<ClassDescription>& getParameterProviders() const {
+ return parameter_providers_;
+ }
+ const std::vector<ClassDescription>& getOtherComponents() const {
+ return other_components_;
+ }
+
+ const BundleIdentifier& getBundleIdentifier() const {
+ return bundle_identifier_;
+ }
+
+ [[nodiscard]] bool empty() const noexcept {
+ return processors_.empty() && controller_services_.empty() &&
parameter_providers_.empty() && other_components_.empty();
+ }
+
+ static void sortClassDescription(minifi::ClassDescription&
class_description) {
+ std::ranges::sort(class_description.class_properties_, {},
&minifi::core::Property::getName);
+ std::ranges::sort(class_description.dynamic_properties_, {},
&minifi::core::DynamicProperty::name);
+ std::ranges::sort(class_description.class_relationships_, {},
&minifi::core::Relationship::getName);
+ std::ranges::sort(class_description.output_attributes_, {},
&minifi::core::OutputAttribute::name);
+ std::ranges::sort(class_description.api_implementations, {},
&minifi::core::ControllerServiceType::type);
Review Comment:
This header now uses `std::ranges::sort`, `std::ranges::copy`,
`std::back_inserter`, and `minifi::utils::string::toLower` in inline member
functions, but it does not include the standard/library headers that declare
them (`<algorithm>`, `<iterator>`, and the project header that provides
`utils::string::toLower`, typically `utils/StringUtils.h`). This can break
consumers depending on include order.
##########
core-framework/include/agent/agent_docs.h:
##########
@@ -59,12 +59,11 @@ std::string classNameWithDots() {
} // namespace detail
template<typename Class, ResourceType Type>
-void ClassDescriptionRegistry::createClassDescription(std::string bundle_name,
std::string class_name, std::string version) {
- const BundleIdentifier group_details{.name = std::move(bundle_name),
.version = std::move(version)};
- auto& [processors, controller_services, parameter_providers,
other_components] = getMutableClassDescriptions()[group_details];
-
+void ClassDescriptionRegistry::createClassDescription(const BundleIdentifier&
bundle_identifier, std::string class_name) {
+ auto [it, _success] =
getMutableClassDescriptions().try_emplace(bundle_identifier, bundle_identifier);
+ auto& [id, components] = *it;
if constexpr (Type == ResourceType::Processor) {
Review Comment:
The structured binding introduces two unused variables (`_success` and `id`)
which can trigger -Wunused-variable under common warning settings. Prefer
binding only what’s needed and explicitly discarding the boolean.
##########
minifi-api/include/minifi-cpp/agent/agent_docs.h:
##########
@@ -51,32 +55,119 @@ struct ClassDescription {
bool isSingleThreaded_ = false;
};
-struct Components {
- std::vector<ClassDescription> processors;
- std::vector<ClassDescription> controller_services;
- std::vector<ClassDescription> parameter_providers;
- std::vector<ClassDescription> other_components;
-
- [[nodiscard]] bool empty() const noexcept {
- return processors.empty() && controller_services.empty() &&
parameter_providers.empty() && other_components.empty();
- }
-};
-
struct BundleIdentifier {
std::string name;
+ std::string group_name;
std::string version;
auto operator<=>(const BundleIdentifier& rhs) const = default;
};
+class Components {
+ public:
+ explicit Components(BundleIdentifier bundle_identifier) :
bundle_identifier_(std::move(bundle_identifier)) {
+ }
+ Components(const Components& rhs) = default;
+ Components(Components&& rhs) = default;
+ Components& operator=(const Components& rhs) = default;
+ Components& operator=(Components&& rhs) = default;
+ virtual ~Components() = default;
+
Review Comment:
`Components` is stored by value in `std::map<BundleIdentifier, Components>`
and doesn’t appear to be used polymorphically. A `virtual` destructor makes it
a polymorphic type (adds a vtable), increasing object size and complicating ABI
for a public header without a clear need.
##########
extensions/python/PythonCreator.h:
##########
@@ -174,7 +174,8 @@ class PythonCreator : public
minifi::core::CoreComponentImpl {
.inputRequirement_ = toString(processor->getInputRequirement()),
.isSingleThreaded_ = processor->isSingleThreaded()};
-
minifi::ClassDescriptionRegistry::getMutableClassDescriptions()[details].processors.push_back(description);
+ auto [it, _success] =
ClassDescriptionRegistry::getMutableClassDescriptions().try_emplace(details,
details);
+ it->second.addClassDescription(description, ResourceType::Processor);
Review Comment:
`BundleIdentifier` now includes `group_name`, but this path never sets it,
so Python-loaded bundles will serialize with an empty group in the manifest
(regressing the exact issue this PR is fixing). Please set `details.group_name`
to the appropriate group for Python processors (likely the extension group)
before inserting into the registry.
##########
libminifi/src/minifi-api.cpp:
##########
@@ -328,14 +327,16 @@ minifi_status minifi_register_processor(minifi_extension*
extension, const minif
}
const minifi::BundleIdentifier bundle{
.name = extension_info->name,
+ .group_name = extension_info->group_name,
.version = extension_info->version
};
- auto& bundle_components =
minifi::ClassDescriptionRegistry::getMutableClassDescriptions()[bundle];
+ auto [it, _success] =
minifi::ClassDescriptionRegistry::getMutableClassDescriptions().try_emplace(bundle,
bundle);
+ auto& bundle_components = it->second;
minifi::utils::useCProcessorClassDescription(*processor, [&] (const auto&
description, const auto& c_class_description) {
Review Comment:
The `try_emplace` return value’s boolean (`_success`) is unused here and may
trigger -Wunused-variable. Either discard it explicitly or bind only the
iterator.
This issue also appears on line 356 of the same file.
##########
extensions/standard-processors/tests/unit/ManifestTests.cpp:
##########
@@ -125,7 +126,8 @@ TEST_CASE("Test Relationships", "[rel1]") {
}
TEST_CASE("Test Dependent", "[dependent]") {
- const auto standard_processor_bundle_id = minifi::BundleIdentifier{.name =
"minifi-standard-processors", .version = minifi::AgentBuild::VERSION};
+ const auto standard_processor_bundle_id = minifi::BundleIdentifier{.name =
"minifi-standard-processors", .group_name = "org.apache.nifi.minifi", .version
= minifi::AgentBuild::VERSION};
+ const auto class_desc =
minifi::ClassDescriptionRegistry::getClassDescriptions();
const auto standard_processors_components =
minifi::ClassDescriptionRegistry::getClassDescriptions().at(standard_processor_bundle_id);
Review Comment:
This variable is unused and may trigger -Wunused-variable under common
warning settings. It can be removed.
##########
extensions/standard-processors/tests/unit/ManifestTestHelper.h:
##########
@@ -0,0 +1,127 @@
+/**
+ *
+ * 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
+
+#include <optional>
+#include <string>
+#include <string_view>
+#include <vector>
+
+#include "minifi-cpp/core/state/Value.h"
+#include "range/v3/algorithm/find_if.hpp"
+
+namespace org::apache::nifi::minifi::state::response {
+struct SerializedResponseNode;
+}
+enum ComponentType {
+ kProcessor,
+ kControllerService,
+};
+
+struct AllowedType {
+ std::string type;
+ std::string group;
+ std::string artifact;
+
+ auto operator<=>(const AllowedType&) const = default;
+};
+
+using org::apache::nifi::minifi::state::response::SerializedResponseNode;
+
+const SerializedResponseNode* getBundle(const
std::vector<SerializedResponseNode>& manifest, const std::string_view
bundle_artifact_name) {
Review Comment:
These helper functions are defined in a header with external linkage. If the
header is included by multiple translation units in the same test binary, this
will cause multiple-definition linker errors. Mark header-defined free
functions `inline` (or `static`) to make ODR-safe.
This issue also appears in the following locations of the same file:
- line 58
- line 83
- line 106
##########
extensions/stable-api-testing/ZooProcessor.cpp:
##########
@@ -37,6 +37,10 @@ minifi_status
ZooProcessor::onTriggerImpl(api::core::ProcessContext& process_con
logger_->log_critical("{} has {} legs", num_of_legs_name,
num_legs->numberOfLegs());
}
}
+ if (auto ssl_data = process_context.getSslData(SSLContextService)) {
+ logger_->log_critical("Has ssl_data? {}", ssl_data.has_value());
+ }
Review Comment:
`ProcessContext::getSslData()` returns a `std::expected<...>`. The `if (auto
ssl_data = ...)` branch already guarantees `ssl_data.has_value()` is true, so
the log always prints `true` and doesn’t reflect whether SSL data is actually
present. Check the inner `std::optional` instead.
--
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]
