Copilot commented on code in PR #2176:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2176#discussion_r3248228437
##########
minifi-api/include/minifi-c/minifi-c.h:
##########
@@ -207,11 +208,18 @@ typedef struct MinifiProcessorClassDefinition {
MinifiProcessorCallbacks callbacks;
} MinifiProcessorClassDefinition;
+struct MinifiControllerServiceProvidedAPI {
+ MinifiStringView type;
+ void*(*cast_api)(void*);
+};
+
Review Comment:
The `MinifiControllerServiceProvidedAPI` struct is declared here but never
used anywhere in the codebase. The `provided_interfaces_ptr` field added to
`MinifiControllerServiceClassDefinition` below uses `const MinifiStringView*`,
not this struct. The `cast_api` function pointer concept it carries is
implemented via the unrelated `get_interface` callback on
`MinifiControllerServiceCallbacks`. Either remove this dead struct or refactor
`provided_interfaces_ptr` to use it (which would obviate the global
`get_interface` callback).
##########
libminifi/src/minifi-c.cpp:
##########
@@ -277,15 +277,23 @@ void useCControllerServiceClassDescription(const
MinifiControllerServiceClassDef
auto name_segments =
minifi::utils::string::split(toStringView(class_description.full_name), "::");
gsl_Assert(!name_segments.empty());
- minifi::ClassDescription description{
- .type_ = minifi::ResourceType::ControllerService,
- .short_name_ = name_segments.back(),
- .full_name_ = minifi::utils::string::join(".", name_segments),
- .description_ = toString(class_description.description),
- .class_properties_ = properties,
+ std::vector<core::ControllerServiceType> implements_apis;
+ implements_apis.reserve(class_description.provided_interfaces_count);
+ for (size_t i = 0; i < class_description.provided_interfaces_count; ++i) {
+ auto api_segments =
string::split(toStringView(class_description.provided_interfaces_ptr[i]), "::");
+
implements_apis.push_back(core::ControllerServiceType::minifiSystemControllerServiceType(string::join(".",
api_segments)));
+ }
Review Comment:
All provided interfaces from any C-API extension are recorded with a
hardcoded artifact `"minifi-system"` and group `"org.apache.nifi.minifi"`.
C-API extensions are registered as their own bundles (e.g. via
`register_c_api_extension`), so this loses the actual bundle/artifact identity
of the extension that defines the API. As a result,
`providedApiImplementations` reported in the agent manifest will not correctly
identify which extension/bundle the API interface comes from, which is the main
consumer of `api_implementations`. The artifact/group should be derived from
the registering extension (or passed through the C API) rather than hardcoded.
##########
minifi-api/common/include/minifi-cpp/core/ProvidedControllerServiceInterface.h:
##########
@@ -0,0 +1,34 @@
+/**
+ * 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 <string_view>
+#include "core/ClassName.h"
+
+namespace org::apache::nifi::minifi::core {
Review Comment:
Header `core/ClassName.h` lives in `core-framework/common/include`, while
this file lives in `minifi-api/common/include`. Including it here introduces a
dependency from `minifi-api` on `core-framework`, which appears to invert the
intended layering (other headers under `minifi-cpp/core/` avoid pulling in
`core-framework`). Consider moving `ClassName.h` (or just the `className<>()`
template) into `minifi-api` to keep the dependency direction clean.
##########
extensions/stable-api-sandbox/tests/ZooTests.cpp:
##########
@@ -0,0 +1,71 @@
+/**
+ *
+ * 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 "AnimalControllerServices.h"
+#include "CProcessorTestUtils.h"
+#include "ZooProcessor.h"
+#include "api/core/Resource.h"
+#include "minifi-cpp/core/FlowFile.h"
+#include "unit/Catch.h"
+#include "unit/SingleProcessorTestController.h"
+#include "unit/TestBase.h"
+#include "unit/TestUtils.h"
+#include "utils/CProcessor.h"
+
+namespace org::apache::nifi::minifi::api_sandbox::test {
+TEST_CASE("ZooTest") {
+ minifi::test::SingleProcessorTestController
controller(minifi::test::utils::make_custom_c_processor<ZooProcessor>(
+ core::ProcessorMetadata{utils::Identifier{}, "ZooProcessor",
logging::LoggerFactory<ZooProcessor>::getLogger()}));
+ const auto dog_with_jetpack =
minifi::test::utils::make_custom_c_controller_service<DogController>(core::ControllerServiceMetadata{
+ utils::Identifier{},
+ "DogController",
+ logging::LoggerFactory<DogController>::getLogger()});
+ auto dog_with_jetpack_node =
controller.plan->addController("dog_with_jetpack", dog_with_jetpack);
+ CHECK(dog_with_jetpack->setProperty(DogController::HasJetpack.name, "true"));
+
+ const auto duck =
minifi::test::utils::make_custom_c_controller_service<DuckController>(core::ControllerServiceMetadata{utils::Identifier{},
+ "DuckController",
+ logging::LoggerFactory<DogController>::getLogger()});
Review Comment:
The `DuckController` logger is created via
`LoggerFactory<DogController>::getLogger()` instead of
`LoggerFactory<DuckController>::getLogger()`. This appears to be a copy-paste
bug that will misattribute log output from the duck controller.
##########
extensions/stable-api-sandbox/tests/ZooTests.cpp:
##########
@@ -0,0 +1,71 @@
+/**
+ *
+ * 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 "AnimalControllerServices.h"
+#include "CProcessorTestUtils.h"
+#include "ZooProcessor.h"
+#include "api/core/Resource.h"
+#include "minifi-cpp/core/FlowFile.h"
+#include "unit/Catch.h"
+#include "unit/SingleProcessorTestController.h"
+#include "unit/TestBase.h"
+#include "unit/TestUtils.h"
+#include "utils/CProcessor.h"
+
+namespace org::apache::nifi::minifi::api_sandbox::test {
+TEST_CASE("ZooTest") {
+ minifi::test::SingleProcessorTestController
controller(minifi::test::utils::make_custom_c_processor<ZooProcessor>(
+ core::ProcessorMetadata{utils::Identifier{}, "ZooProcessor",
logging::LoggerFactory<ZooProcessor>::getLogger()}));
+ const auto dog_with_jetpack =
minifi::test::utils::make_custom_c_controller_service<DogController>(core::ControllerServiceMetadata{
+ utils::Identifier{},
+ "DogController",
+ logging::LoggerFactory<DogController>::getLogger()});
+ auto dog_with_jetpack_node =
controller.plan->addController("dog_with_jetpack", dog_with_jetpack);
+ CHECK(dog_with_jetpack->setProperty(DogController::HasJetpack.name, "true"));
+
+ const auto duck =
minifi::test::utils::make_custom_c_controller_service<DuckController>(core::ControllerServiceMetadata{utils::Identifier{},
+ "DuckController",
+ logging::LoggerFactory<DogController>::getLogger()});
+ auto duck_node = controller.plan->addController("duck", duck);
+
+ {
+
CHECK(controller.getProcessor()->setProperty(ZooProcessor::CanFlyService.name,
"dog_with_jetpack"));
+
CHECK(controller.getProcessor()->setProperty(ZooProcessor::NumberOfLegsService.name,
"duck"));
+ const auto& result = controller.trigger();
+
CHECK(LogTestController::getInstance().contains("[org::apache::nifi::minifi::api_sandbox::ZooProcessor]
[critical] Can fly? true"));
+
CHECK(LogTestController::getInstance().contains("[org::apache::nifi::minifi::api_sandbox::ZooProcessor]
[critical] Num of legs? 2"));
+ }
+ {
+ LogTestController::getInstance().clear();
+
CHECK(controller.getProcessor()->setProperty(ZooProcessor::CanFlyService.name,
"duck"));
+
CHECK(controller.getProcessor()->setProperty(ZooProcessor::NumberOfLegsService.name,
"duck"));
+ const auto& result = controller.trigger();
+
CHECK(LogTestController::getInstance().contains("[org::apache::nifi::minifi::api_sandbox::ZooProcessor]
[critical] Can fly? true"));
+
CHECK(LogTestController::getInstance().contains("[org::apache::nifi::minifi::api_sandbox::ZooProcessor]
[critical] Num of legs? 2"));
+ }
+ {
+ LogTestController::getInstance().clear();
+
CHECK(controller.getProcessor()->setProperty(ZooProcessor::CanFlyService.name,
"duck"));
+
CHECK(controller.getProcessor()->setProperty(ZooProcessor::NumberOfLegsService.name,
"invalid"));
+ const auto& result = controller.trigger();
Review Comment:
The `result` returned by `controller.trigger()` is declared but never used
in any of the three test blocks (lines 49, 57, 65). Either assert something on
the trigger result (e.g. that it produced a flow file or routed to a specific
relationship) or drop the binding to keep the test intent clear.
--
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]
