[
https://issues.apache.org/jira/browse/MINIFICPP-1501?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Adam Hunyadi updated MINIFICPP-1501:
------------------------------------
Description:
*Background:*
Processor::onTrigger(..., core::ProcessSession*) calls take sessions by
pointers even though they are never expected to be called with a nullptr
argument. We could check whether this ptr is null in the onTrigger calls, and
use it dereferenced, but this would produce a lot of redundant code. Changing
the onTrigger signature would be a serious API break, so we should not change
it.
Currently session is not checked for validity before derefencing:
{code:bash|title=Session dereferencing}
➜ git --no-pager grep 'session->' | grep -i 'processors/' | cut -d ":" -f1 |
less | sort | uniq | xargs -n1 sh -c 'echo $1 && egrep "if (.*session.*)" $1' sh
extensions/aws/processors/DeleteS3Object.cpp
extensions/aws/processors/FetchS3Object.cpp
extensions/aws/processors/PutS3Object.cpp
extensions/civetweb/processors/ListenHTTP.cpp
extensions/http-curl/processors/InvokeHTTP.cpp
extensions/mqtt/processors/ConsumeMQTT.cpp
extensions/mqtt/processors/ConvertHeartBeat.cpp
extensions/mqtt/processors/ConvertJSONAck.cpp
extensions/mqtt/processors/PublishMQTT.cpp
extensions/openwsman/processors/SourceInitiatedSubscriptionListener.cpp
extensions/sftp/processors/FetchSFTP.cpp
extensions/sftp/processors/ListSFTP.cpp
if (createAndTransferFlowFileFromChild(session, hostname, port,
username, file)) {
if (!createAndTransferFlowFileFromChild(session, hostname, port, username,
updated_entity)) {
extensions/sftp/processors/PutSFTP.cpp
if (!this->processOne(context, session)) {
extensions/standard-processors/processors/AppendHostInfo.cpp
extensions/standard-processors/processors/ExecuteProcess.cpp
extensions/standard-processors/processors/ExtractText.cpp
extensions/standard-processors/processors/GenerateFlowFile.cpp
extensions/standard-processors/processors/GetFile.cpp
extensions/standard-processors/processors/GetTCP.cpp
extensions/standard-processors/processors/HashContent.cpp
extensions/standard-processors/processors/ListenSyslog.cpp
extensions/standard-processors/processors/LogAttribute.cpp
extensions/standard-processors/processors/PutFile.cpp
extensions/standard-processors/processors/RetryFlowFile.cpp
extensions/standard-processors/processors/RouteOnAttribute.cpp
extensions/standard-processors/processors/TailFile.cpp
if (!session->existsFlowFileInRelationship(Success)) {
extensions/standard-processors/processors/UpdateAttribute.cpp
extensions/standard-processors/tests/unit/GetTCPTests.cpp
extensions/standard-processors/tests/unit/ProcessorTests.cpp
{code}
*Proposal:*
If we want to have this added safety of checking pointers before deferencing
them, we can enforce adding a gsl_Expects(session) to all of our
implementations that overload the mentioned signature by adding a static
code-check next to the linter check that enforces the presence at the start of
the function using this overload. We can also provide another overload that
takes session by reference - and by default the original implementation would
do the checks and call into this one, so overloads on this would not require
additional checks. This would mean checking for nullptr in
Processor::onTrigger(shrd_ptr<>, shrd_ptr<>) and Processor::onTrigger(*, *) and
forwarding the call to Processor::onTrigger(&, &) and moving overloads to the
(&, &) version whenever possible.
was:
*Background:*
Processor::onTrigger(..., core::ProcessSession*) calls take sessions by
pointers even though they are never expected to be called with a nullptr
argument. We could check whether this ptr is null in the onTrigger calls, and
use it dereferenced, but this would produce a lot of redundant code. Changing
the onTrigger signature would be a serious API break, so we should not change
it.
Currently session is not checked for validity before derefencing:
{code:bash|title=Session dereferencing}
➜ git --no-pager grep 'session->' | grep -i 'processors/' | cut -d ":" -f1 |
less | sort | uniq | xargs -n1 sh -c 'echo $1 && egrep "if (.*session.*)" $1'
sh extensions/aws/processors/DeleteS3Object.cpp
extensions/aws/processors/FetchS3Object.cpp
extensions/aws/processors/PutS3Object.cpp
extensions/civetweb/processors/ListenHTTP.cpp
extensions/http-curl/processors/InvokeHTTP.cpp
extensions/mqtt/processors/ConsumeMQTT.cpp
extensions/mqtt/processors/ConvertHeartBeat.cpp
extensions/mqtt/processors/ConvertJSONAck.cpp
extensions/mqtt/processors/PublishMQTT.cpp
extensions/openwsman/processors/SourceInitiatedSubscriptionListener.cpp
extensions/sftp/processors/FetchSFTP.cpp
extensions/sftp/processors/ListSFTP.cpp
if (createAndTransferFlowFileFromChild(session, hostname, port,
username, file)) {
if (!createAndTransferFlowFileFromChild(session, hostname, port, username,
updated_entity)) {
extensions/sftp/processors/PutSFTP.cpp
if (!this->processOne(context, session)) {
extensions/standard-processors/processors/AppendHostInfo.cpp
extensions/standard-processors/processors/ExecuteProcess.cpp
extensions/standard-processors/processors/ExtractText.cpp
extensions/standard-processors/processors/GenerateFlowFile.cpp
extensions/standard-processors/processors/GetFile.cpp
extensions/standard-processors/processors/GetTCP.cpp
extensions/standard-processors/processors/HashContent.cpp
extensions/standard-processors/processors/ListenSyslog.cpp
extensions/standard-processors/processors/LogAttribute.cpp
extensions/standard-processors/processors/PutFile.cpp
extensions/standard-processors/processors/RetryFlowFile.cpp
extensions/standard-processors/processors/RouteOnAttribute.cpp
extensions/standard-processors/processors/TailFile.cpp
if (!session->existsFlowFileInRelationship(Success)) {
extensions/standard-processors/processors/UpdateAttribute.cpp
extensions/standard-processors/tests/unit/GetTCPTests.cpp
extensions/standard-processors/tests/unit/ProcessorTests.cpp
{code}
*Proposal:*
If we want to have this added safety of checking pointers before deferencing
them, we can enforce adding a gsl_Expects(session) to all of our
implementations that overload the mentioned signature by adding a static
code-check next to the linter check that enforces the presence at the start of
the function using this overload. We can also provide another overload that
takes session by reference - and by default the original implementation would
do the checks and call into this one, so overloads on this would not require
additional checks. This would mean checking for nullptr in
Processor::onTrigger(shrd_ptr<>, shrd_ptr<>) and Processor::onTrigger(*, *) and
forwarding the call to Processor::onTrigger(&, &) and moving overloads to the
(&, &) version whenever possible.
> Enforce that Processor::onTrigger calls are always invoked with a valid
> session
> -------------------------------------------------------------------------------
>
> Key: MINIFICPP-1501
> URL: https://issues.apache.org/jira/browse/MINIFICPP-1501
> Project: Apache NiFi MiNiFi C++
> Issue Type: Task
> Affects Versions: 0.7.0
> Reporter: Adam Hunyadi
> Assignee: Adam Hunyadi
> Priority: Minor
> Labels: MiNiFi-CPP-Hygiene
> Fix For: 1.0.0
>
>
> *Background:*
> Processor::onTrigger(..., core::ProcessSession*) calls take sessions by
> pointers even though they are never expected to be called with a nullptr
> argument. We could check whether this ptr is null in the onTrigger calls, and
> use it dereferenced, but this would produce a lot of redundant code. Changing
> the onTrigger signature would be a serious API break, so we should not change
> it.
> Currently session is not checked for validity before derefencing:
> {code:bash|title=Session dereferencing}
> ➜ git --no-pager grep 'session->' | grep -i 'processors/' | cut -d ":" -f1 |
> less | sort | uniq | xargs -n1 sh -c 'echo $1 && egrep "if (.*session.*)" $1'
> sh
> extensions/aws/processors/DeleteS3Object.cpp
> extensions/aws/processors/FetchS3Object.cpp
> extensions/aws/processors/PutS3Object.cpp
> extensions/civetweb/processors/ListenHTTP.cpp
> extensions/http-curl/processors/InvokeHTTP.cpp
> extensions/mqtt/processors/ConsumeMQTT.cpp
> extensions/mqtt/processors/ConvertHeartBeat.cpp
> extensions/mqtt/processors/ConvertJSONAck.cpp
> extensions/mqtt/processors/PublishMQTT.cpp
> extensions/openwsman/processors/SourceInitiatedSubscriptionListener.cpp
> extensions/sftp/processors/FetchSFTP.cpp
> extensions/sftp/processors/ListSFTP.cpp
> if (createAndTransferFlowFileFromChild(session, hostname, port,
> username, file)) {
> if (!createAndTransferFlowFileFromChild(session, hostname, port,
> username, updated_entity)) {
> extensions/sftp/processors/PutSFTP.cpp
> if (!this->processOne(context, session)) {
> extensions/standard-processors/processors/AppendHostInfo.cpp
> extensions/standard-processors/processors/ExecuteProcess.cpp
> extensions/standard-processors/processors/ExtractText.cpp
> extensions/standard-processors/processors/GenerateFlowFile.cpp
> extensions/standard-processors/processors/GetFile.cpp
> extensions/standard-processors/processors/GetTCP.cpp
> extensions/standard-processors/processors/HashContent.cpp
> extensions/standard-processors/processors/ListenSyslog.cpp
> extensions/standard-processors/processors/LogAttribute.cpp
> extensions/standard-processors/processors/PutFile.cpp
> extensions/standard-processors/processors/RetryFlowFile.cpp
> extensions/standard-processors/processors/RouteOnAttribute.cpp
> extensions/standard-processors/processors/TailFile.cpp
> if (!session->existsFlowFileInRelationship(Success)) {
> extensions/standard-processors/processors/UpdateAttribute.cpp
> extensions/standard-processors/tests/unit/GetTCPTests.cpp
> extensions/standard-processors/tests/unit/ProcessorTests.cpp
> {code}
> *Proposal:*
> If we want to have this added safety of checking pointers before deferencing
> them, we can enforce adding a gsl_Expects(session) to all of our
> implementations that overload the mentioned signature by adding a static
> code-check next to the linter check that enforces the presence at the start
> of the function using this overload. We can also provide another overload
> that takes session by reference - and by default the original implementation
> would do the checks and call into this one, so overloads on this would not
> require additional checks. This would mean checking for nullptr in
> Processor::onTrigger(shrd_ptr<>, shrd_ptr<>) and Processor::onTrigger(*, *)
> and forwarding the call to Processor::onTrigger(&, &) and moving overloads to
> the (&, &) version whenever possible.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
