Copilot commented on code in PR #2113:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2113#discussion_r2832755394
##########
extension-framework/cpp-extension-lib/src/core/ProcessSession.cpp:
##########
@@ -104,13 +108,17 @@ void ProcessSession::read(FlowFile& flow_file, const
io::InputStreamCallback& ca
}
}
-void ProcessSession::setAttribute(FlowFile& ff, std::string_view key,
std::string value) { // NOLINT(performance-unnecessary-value-param)
- MinifiStringView value_ref = utils::toStringView(value);
- MinifiFlowFileSetAttribute(impl_, ff.get(), utils::toStringView(key),
&value_ref);
+void ProcessSession::setAttribute(FlowFile& ff, const std::string_view key,
std::string value) { // NOLINT(performance-unnecessary-value-param)
Review Comment:
Unnecessary const qualifier: Adding const to std::string_view parameters is
unnecessary since std::string_view is already cheap to copy (essentially two
pointers). The const qualifier provides no benefit here and deviates from the
typical convention for pass-by-value parameters. Consider removing the const
qualifier from the key parameter.
##########
libminifi/src/minifi-c.cpp:
##########
@@ -345,15 +339,25 @@ MINIFI_OWNED MinifiFlowFile*
MinifiProcessSessionCreate(MinifiProcessSession* se
return MINIFI_NULL;
}
-void MinifiProcessSessionTransfer(MinifiProcessSession* session, MINIFI_OWNED
MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
+MinifiStatus MinifiProcessSessionTransfer(MinifiProcessSession* session,
MINIFI_OWNED MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
gsl_Assert(flowfile != MINIFI_NULL);
- reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
- *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ try {
+ reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
+ *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ return MINIFI_STATUS_SUCCESS;
+ } catch (std::exception&) {
+ return MINIFI_STATUS_UNKNOWN_ERROR;
+ }
}
-void MinifiProcessSessionRemove(MinifiProcessSession* session, MINIFI_OWNED
MinifiFlowFile* flowfile) {
+MinifiStatus MinifiProcessSessionRemove(MinifiProcessSession* session,
MINIFI_OWNED MinifiFlowFile* flowfile) {
gsl_Assert(flowfile != MINIFI_NULL);
-
reinterpret_cast<minifi::core::ProcessSession*>(session)->remove(*reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile));
+ try {
+
reinterpret_cast<minifi::core::ProcessSession*>(session)->remove(*reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile));
+ return MINIFI_STATUS_SUCCESS;
+ } catch (std::exception&) {
Review Comment:
Memory leak: The flowfile parameter is marked as MINIFI_OWNED, indicating
this function takes ownership and should delete it. However, the allocated
std::shared_ptr<minifi::core::FlowFile> created on line 327 or 337 is never
deleted. After dereferencing and using the shared_ptr for the remove operation,
you should delete the flowfile pointer. Add 'delete
reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile);' before
returning.
```suggestion
reinterpret_cast<minifi::core::ProcessSession*>(session)->remove(*reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile));
delete
reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile);
return MINIFI_STATUS_SUCCESS;
} catch (std::exception&) {
delete
reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile);
```
##########
libminifi/src/minifi-c.cpp:
##########
@@ -345,15 +339,25 @@ MINIFI_OWNED MinifiFlowFile*
MinifiProcessSessionCreate(MinifiProcessSession* se
return MINIFI_NULL;
}
-void MinifiProcessSessionTransfer(MinifiProcessSession* session, MINIFI_OWNED
MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
+MinifiStatus MinifiProcessSessionTransfer(MinifiProcessSession* session,
MINIFI_OWNED MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
gsl_Assert(flowfile != MINIFI_NULL);
- reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
- *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ try {
+ reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
+ *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ return MINIFI_STATUS_SUCCESS;
+ } catch (std::exception&) {
+ return MINIFI_STATUS_UNKNOWN_ERROR;
+ }
}
Review Comment:
Inconsistent exception handling: This function catches std::exception&,
while similar functions MinifiProcessSessionRead and MinifiProcessSessionWrite
use catch (...) to catch all exceptions. For consistency and robustness (to
catch non-standard exceptions), consider using catch (...) instead.
##########
extension-framework/cpp-extension-lib/src/core/ProcessSession.cpp:
##########
@@ -104,13 +108,17 @@ void ProcessSession::read(FlowFile& flow_file, const
io::InputStreamCallback& ca
}
}
-void ProcessSession::setAttribute(FlowFile& ff, std::string_view key,
std::string value) { // NOLINT(performance-unnecessary-value-param)
- MinifiStringView value_ref = utils::toStringView(value);
- MinifiFlowFileSetAttribute(impl_, ff.get(), utils::toStringView(key),
&value_ref);
+void ProcessSession::setAttribute(FlowFile& ff, const std::string_view key,
std::string value) { // NOLINT(performance-unnecessary-value-param)
+ const MinifiStringView value_ref = utils::toStringView(value);
+ if (MINIFI_STATUS_SUCCESS != MinifiFlowFileSetAttribute(impl_, ff.get(),
utils::toStringView(key), &value_ref)) {
+ throw minifi::Exception(minifi::FILE_OPERATION_EXCEPTION, "Failed to set
attribute");
+ }
}
-void ProcessSession::removeAttribute(FlowFile& ff, std::string_view key) {
- MinifiFlowFileSetAttribute(impl_, ff.get(), utils::toStringView(key),
nullptr);
+void ProcessSession::removeAttribute(FlowFile& ff, const std::string_view key)
{
Review Comment:
Unnecessary const qualifier: Adding const to std::string_view parameters is
unnecessary since std::string_view is already cheap to copy (essentially two
pointers). The const qualifier provides no benefit here and deviates from the
typical convention for pass-by-value parameters. Consider removing the const
qualifier from the key parameter.
##########
libminifi/src/minifi-c.cpp:
##########
@@ -345,15 +339,25 @@ MINIFI_OWNED MinifiFlowFile*
MinifiProcessSessionCreate(MinifiProcessSession* se
return MINIFI_NULL;
}
-void MinifiProcessSessionTransfer(MinifiProcessSession* session, MINIFI_OWNED
MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
+MinifiStatus MinifiProcessSessionTransfer(MinifiProcessSession* session,
MINIFI_OWNED MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
gsl_Assert(flowfile != MINIFI_NULL);
- reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
- *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ try {
+ reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
+ *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ return MINIFI_STATUS_SUCCESS;
+ } catch (std::exception&) {
+ return MINIFI_STATUS_UNKNOWN_ERROR;
+ }
}
Review Comment:
Missing null check for session parameter. All other session-related
functions in this file check that session is not null using gsl_Assert(session
!= MINIFI_NULL), but this function is missing that check. This should be added
for consistency and safety.
##########
libminifi/src/minifi-c.cpp:
##########
@@ -345,15 +339,25 @@ MINIFI_OWNED MinifiFlowFile*
MinifiProcessSessionCreate(MinifiProcessSession* se
return MINIFI_NULL;
}
-void MinifiProcessSessionTransfer(MinifiProcessSession* session, MINIFI_OWNED
MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
+MinifiStatus MinifiProcessSessionTransfer(MinifiProcessSession* session,
MINIFI_OWNED MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
gsl_Assert(flowfile != MINIFI_NULL);
- reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
- *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ try {
+ reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
+ *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ return MINIFI_STATUS_SUCCESS;
+ } catch (std::exception&) {
+ return MINIFI_STATUS_UNKNOWN_ERROR;
+ }
}
-void MinifiProcessSessionRemove(MinifiProcessSession* session, MINIFI_OWNED
MinifiFlowFile* flowfile) {
+MinifiStatus MinifiProcessSessionRemove(MinifiProcessSession* session,
MINIFI_OWNED MinifiFlowFile* flowfile) {
gsl_Assert(flowfile != MINIFI_NULL);
-
reinterpret_cast<minifi::core::ProcessSession*>(session)->remove(*reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile));
+ try {
+
reinterpret_cast<minifi::core::ProcessSession*>(session)->remove(*reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile));
+ return MINIFI_STATUS_SUCCESS;
+ } catch (std::exception&) {
+ return MINIFI_STATUS_UNKNOWN_ERROR;
+ }
}
Review Comment:
Missing null check for session parameter. All other session-related
functions in this file check that session is not null using gsl_Assert(session
!= MINIFI_NULL), but this function is missing that check. This should be added
for consistency and safety.
##########
libminifi/src/minifi-c.cpp:
##########
@@ -345,15 +339,25 @@ MINIFI_OWNED MinifiFlowFile*
MinifiProcessSessionCreate(MinifiProcessSession* se
return MINIFI_NULL;
}
-void MinifiProcessSessionTransfer(MinifiProcessSession* session, MINIFI_OWNED
MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
+MinifiStatus MinifiProcessSessionTransfer(MinifiProcessSession* session,
MINIFI_OWNED MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
gsl_Assert(flowfile != MINIFI_NULL);
- reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
- *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ try {
+ reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
+ *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ return MINIFI_STATUS_SUCCESS;
+ } catch (std::exception&) {
+ return MINIFI_STATUS_UNKNOWN_ERROR;
+ }
}
-void MinifiProcessSessionRemove(MinifiProcessSession* session, MINIFI_OWNED
MinifiFlowFile* flowfile) {
+MinifiStatus MinifiProcessSessionRemove(MinifiProcessSession* session,
MINIFI_OWNED MinifiFlowFile* flowfile) {
gsl_Assert(flowfile != MINIFI_NULL);
-
reinterpret_cast<minifi::core::ProcessSession*>(session)->remove(*reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile));
+ try {
+
reinterpret_cast<minifi::core::ProcessSession*>(session)->remove(*reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile));
+ return MINIFI_STATUS_SUCCESS;
+ } catch (std::exception&) {
+ return MINIFI_STATUS_UNKNOWN_ERROR;
+ }
}
Review Comment:
Inconsistent exception handling: This function catches std::exception&,
while similar functions MinifiProcessSessionRead and MinifiProcessSessionWrite
use catch (...) to catch all exceptions. For consistency and robustness (to
catch non-standard exceptions), consider using catch (...) instead.
##########
libminifi/src/minifi-c.cpp:
##########
@@ -421,6 +425,7 @@ void MinifiFlowFileSetAttribute(MinifiProcessSession*
session, MinifiFlowFile* f
reinterpret_cast<minifi::core::ProcessSession*>(session)->putAttribute(
**reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
toString(attribute_name), toString(*attribute_value));
}
+ return MINIFI_STATUS_SUCCESS;
}
Review Comment:
Missing exception handling: This function now returns MinifiStatus to signal
errors, but lacks try-catch blocks. The removeAttribute and putAttribute calls
on lines 423 and 425-426 can potentially throw exceptions (e.g., from
fmt::format operations within ProcessSessionImpl). For consistency with
MinifiProcessSessionTransfer and MinifiProcessSessionRemove which have
try-catch blocks, this function should wrap the operations in try-catch and
return MINIFI_STATUS_UNKNOWN_ERROR on exception.
##########
libminifi/src/minifi-c.cpp:
##########
@@ -345,15 +339,25 @@ MINIFI_OWNED MinifiFlowFile*
MinifiProcessSessionCreate(MinifiProcessSession* se
return MINIFI_NULL;
}
-void MinifiProcessSessionTransfer(MinifiProcessSession* session, MINIFI_OWNED
MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
+MinifiStatus MinifiProcessSessionTransfer(MinifiProcessSession* session,
MINIFI_OWNED MinifiFlowFile* flowfile, MinifiStringView relationship_name) {
gsl_Assert(flowfile != MINIFI_NULL);
- reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
- *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ try {
+ reinterpret_cast<minifi::core::ProcessSession*>(session)->transfer(
+ *reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile),
minifi::core::Relationship{toString(relationship_name), ""});
+ return MINIFI_STATUS_SUCCESS;
+ } catch (std::exception&) {
+ return MINIFI_STATUS_UNKNOWN_ERROR;
+ }
}
Review Comment:
Memory leak: The flowfile parameter is marked as MINIFI_OWNED, indicating
this function takes ownership and should delete it. However, the allocated
std::shared_ptr<minifi::core::FlowFile> created on line 327 or 337 is never
deleted. After dereferencing and using the shared_ptr for the transfer
operation, you should delete the flowfile pointer. Add 'delete
reinterpret_cast<std::shared_ptr<minifi::core::FlowFile>*>(flowfile);' before
returning.
--
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]
