Copilot commented on code in PR #2219:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2219#discussion_r3721304430
##########
minifi-api/include/minifi-cpp/core/ProcessorApi.h:
##########
@@ -50,7 +50,6 @@ class ProcessorApi {
virtual void initialize(ProcessorDescriptor& descriptor) = 0;
virtual bool isSingleThreaded() const = 0;
- virtual std::string getProcessorType() const = 0;
virtual void onTrigger(ProcessContext&, ProcessSession&) = 0;
virtual void onSchedule(ProcessContext&, ProcessSessionFactory&) = 0;
Review Comment:
Removing a virtual method from a public interface in `minifi-api` is a
source/ABI breaking change (vtable layout changes), which will break
out-of-tree processors built against older headers/binaries. If backward
compatibility is required, keep `getProcessorType()` in the interface for at
least one deprecation cycle (potentially as a non-pure virtual with a default
implementation) or version the interface/type-erasure boundary so older
extensions can still load.
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -986,7 +986,7 @@ void StructuredConfiguration::parseFunnels(const Node&
node, core::ProcessGroup*
throw Exception(ExceptionType::GENERAL_EXCEPTION, "Incorrect funnel UUID
format.");
});
- auto funnel = std::make_unique<core::Processor>(name, uuid.value(),
std::make_unique<minifi::Funnel>(name, uuid.value()));
+ auto funnel = std::make_unique<core::Processor>("Funnel", name,
uuid.value(), std::make_unique<minifi::Funnel>(name, uuid.value()));
Review Comment:
These introduce hard-coded processor type strings. This is brittle
(renames/typos won’t be caught, and different call sites may choose different
spellings), and it scatters the authoritative type name across the codebase.
Prefer a single source of truth (e.g., a `static constexpr std::string_view
TypeName` on the wrapped implementation or a small helper that derives the
short type from `className<T>()`) and use that here.
##########
libminifi/test/libtest/unit/ProcessorUtils.h:
##########
@@ -32,15 +33,15 @@ std::unique_ptr<core::Processor>
make_processor(std::string_view name, std::opti
.name = std::string{name},
.logger =
minifi::core::logging::LoggerFactory<T>::getLogger(uuid.value())
});
- return std::make_unique<core::Processor>(name, uuid.value(),
std::move(processor_impl));
+ return
std::make_unique<core::Processor>(minifi::utils::string::partAfterLastOccurrenceOf(core::className<T>(),
':'), name, uuid.value(), std::move(processor_impl));
Review Comment:
The logic for deriving the short processor type from a fully-qualified class
name is now duplicated across multiple call sites. Consider centralizing it
(e.g., a dedicated helper like `core::getShortTypeName<T>()`) to ensure
consistent behavior everywhere and reduce the chance of subtle inconsistencies.
Also, extracting by the character `':'` is non-obvious given C++ scopes are
`\"::\"`; using an explicit scope delimiter (if supported by the helper) or
documenting the intent would make this easier to maintain.
##########
libminifi/src/core/flow/StructuredConfiguration.cpp:
##########
@@ -741,7 +741,7 @@ void StructuredConfiguration::parseRPGPort(const Node&
port_node, core::ProcessG
auto port_impl = std::make_unique<minifi::RemoteProcessGroupPort>(
nameStr, parent->getURL(), this->configuration_, uuid, direction);
auto* port = port_impl.get();
- auto port_wrapper = std::make_unique<core::Processor>(nameStr, uuid,
std::move(port_impl));
+ auto port_wrapper =
std::make_unique<core::Processor>("RemoteProcessGroupPort", nameStr, uuid,
std::move(port_impl));
Review Comment:
These introduce hard-coded processor type strings. This is brittle
(renames/typos won’t be caught, and different call sites may choose different
spellings), and it scatters the authoritative type name across the codebase.
Prefer a single source of truth (e.g., a `static constexpr std::string_view
TypeName` on the wrapped implementation or a small helper that derives the
short type from `className<T>()`) and use that here.
##########
libminifi/test/libtest/unit/ProcessorUtils.h:
##########
@@ -32,15 +33,15 @@ std::unique_ptr<core::Processor>
make_processor(std::string_view name, std::opti
.name = std::string{name},
.logger =
minifi::core::logging::LoggerFactory<T>::getLogger(uuid.value())
});
- return std::make_unique<core::Processor>(name, uuid.value(),
std::move(processor_impl));
+ return
std::make_unique<core::Processor>(minifi::utils::string::partAfterLastOccurrenceOf(core::className<T>(),
':'), name, uuid.value(), std::move(processor_impl));
}
template<typename T, typename ...Args>
std::unique_ptr<core::Processor> make_custom_processor(Args&&... args) {
auto processor_impl = std::make_unique<T>(std::forward<Args>(args)...);
auto name = processor_impl->getName();
auto uuid = processor_impl->getUUID();
- return std::make_unique<core::Processor>(name, uuid,
std::move(processor_impl));
+ return
std::make_unique<core::Processor>(minifi::utils::string::partAfterLastOccurrenceOf(core::className<T>(),
':'), name, uuid, std::move(processor_impl));
Review Comment:
The logic for deriving the short processor type from a fully-qualified class
name is now duplicated across multiple call sites. Consider centralizing it
(e.g., a dedicated helper like `core::getShortTypeName<T>()`) to ensure
consistent behavior everywhere and reduce the chance of subtle inconsistencies.
Also, extracting by the character `':'` is non-obvious given C++ scopes are
`\"::\"`; using an explicit scope delimiter (if supported by the helper) or
documenting the intent would make this easier to maintain.
##########
libminifi/include/Port.h:
##########
@@ -50,9 +50,9 @@ class PortImpl final : public ForwardingNode {
PortType port_type_;
};
-class Port : public core::Processor {
+class Port final : public core::Processor {
public:
- Port(std::string_view name, const utils::Identifier& uuid,
std::unique_ptr<PortImpl> impl): Processor(name, uuid, std::move(impl)) {}
+ Port(std::string_view name, const utils::Identifier& uuid,
std::unique_ptr<PortImpl> impl): Processor("Port", name, uuid, std::move(impl))
{}
Review Comment:
Marking `Port` as `final` is an API-breaking change for any downstream code
that subclasses `Port`. If there isn’t a strong reason to forbid inheritance,
consider reverting `final` or documenting the rationale in the header so
downstream embedders/extensions aren’t surprised by this restriction.
--
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]
