Refactor CommandHandler to be more unittestable (#32467)

* Refactor CommandHandler

* Restyle and quick fix

* Self review

* Add tests

* Restyled by whitespace

* Restyled by clang-format

* Resolve conflict

* Restyled by whitespace

* Fix CI

* Rename CommandResponder to CommandResponseSender for easier reviewing

* Restyled by clang-format

* Address PR comments

* Restyled by whitespace

* Restyled by clang-format

* Address PR comment

* Restyled by clang-format

* Address PR comments

* Restyled by clang-format

* nit fix

* Address PR comments

---------

Co-authored-by: Restyled.io <commits@restyled.io>

Author: tehampson

.github/workflows/lint.yml

@@ -94,6 +94,7 @@ jobs:
                      --known-failure app/AttributeAccessToken.h \
                      --known-failure app/CommandHandler.h \
                      --known-failure app/CommandHandlerInterface.h \
+                     --known-failure app/CommandResponseSender.h \
                      --known-failure app/CommandSenderLegacyCallback.h \
                      --known-failure app/ReadHandler.h \
                      --known-failure app/reporting/reporting.cpp \

src/app/BUILD.gn

@@ -281,9 +281,9 @@ static_library("app") {
     "ChunkedWriteCallback.cpp",
     "ChunkedWriteCallback.h",
     "CommandHandler.cpp",
+    "CommandHandlerExchangeInterface.h",
     "CommandResponseHelper.h",
     "CommandResponseSender.cpp",
-    "CommandResponseSender.h",
     "DefaultAttributePersistenceProvider.cpp",
     "DefaultAttributePersistenceProvider.h",
     "DeferredAttributePersistenceProvider.cpp",
@@ -304,6 +304,7 @@ static_library("app") {
     # TODO: the following items cannot be included due to interaction-model circularity
     #       (app depending on im and im including these headers):
     #       Name with _ so that linter does not recognize it
+    # "CommandResponseSender._h"
     # "CommandHandler._h"
     # "ReadHandler._h",
     # "WriteHandler._h"

src/app/CommandHandler.cpp

@@ -0,0 +1,108 @@
+/*
+ *    Copyright (c) 2024 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed 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 <access/SubjectDescriptor.h>
+#include <lib/core/DataModelTypes.h>
+#include <lib/core/GroupId.h>
+#include <lib/core/Optional.h>
+#include <messaging/ExchangeContext.h>
+#include <protocols/interaction_model/StatusCode.h>
+#include <system/SystemPacketBuffer.h>
+
+namespace chip {
+namespace app {
+
+/**
+ * Interface for sending InvokeResponseMessage(s).
+ *
+ * Provides information about the associated exchange context.
+ *
+ * Design Rationale: This interface enhances unit testability and allows applications to
+ * customize InvokeResponse behavior. For example, a bridge application might locally execute
+ * a command using cluster APIs without intending to sending a response on an exchange.
+ * These cluster APIs require providing an instance of CommandHandler where a status response
+ * is added (see https://github.com/project-chip/connectedhomeip/issues/32030).
+ */
+class CommandHandlerExchangeInterface
+{
+public:
+    virtual ~CommandHandlerExchangeInterface() = default;
+
+    /**
+     * Get a non-owning pointer to the exchange context the InvokeRequestMessage was
+     * delivered on.
+     *
+     * @return The exchange context. Might be nullptr if no exchange context has been
+     *         assigned or the context has been released. For example, the exchange
+     *         context might not be assigned in unit tests, or if an application wishes
+     *         to locally execute cluster APIs and still receive response data without
+     *         sending it on an exchange.
+     */
+    virtual Messaging::ExchangeContext * GetExchangeContext() const = 0;
+
+    // TODO(#30453): Follow up refactor. It should be possible to remove
+    // GetSubjectDescriptor and GetAccessingFabricIndex, as CommandHandler can get these
+    // values from ExchangeContext.
+
+    /**
+     * Gets subject descriptor of the exchange.
+     *
+     * WARNING: This method should only be called when the caller is certain the
+     * session has not been evicted.
+     */
+    virtual Access::SubjectDescriptor GetSubjectDescriptor() const = 0;
+
+    /**
+     * Gets accessing fabic index of the exchange.
+     *
+     * WARNING: This method should only be called when the caller is certain the
+     * session has not been evicted.
+     */
+    virtual FabricIndex GetAccessingFabricIndex() const = 0;
+    /**
+     * If session for the exchange is a group session, returns its group ID. Otherwise,
+     * returns a null optional.
+     */
+    virtual Optional<GroupId> GetGroupId() const = 0;
+
+    /**
+     * @brief Called to indicate a slow command is being processed.
+     *
+     * Enables the exchange to send whatever transport-level acks might be needed without waiting
+     * for command processing to complete.
+     */
+    virtual void HandlingSlowCommand() = 0;
+
+    /**
+     * @brief Adds a completed InvokeResponseMessage to the queue for sending to requester.
+     *
+     * Called by CommandHandler.
+     */
+    virtual void AddInvokeResponseToSend(System::PacketBufferHandle && aPacket) = 0;
+
+    /**
+     * @brief Called to indicate that an InvokeResponse was dropped.
+     *
+     * Called by CommandHandler to relay this information to the requester.
+     */
+    virtual void ResponseDropped() = 0;
+};
+
+} // namespace app
+} // namespace chip

src/app/CommandHandler.h

@@ -38,12 +38,10 @@ CHIP_ERROR CommandResponseSender::OnMessageReceived(Messaging::ExchangeContext *
         err = statusError;
         VerifyOrExit(err == CHIP_NO_ERROR, failureStatusToSend.SetValue(Status::InvalidAction));
 
-        // If SendCommandResponse() fails, we are responsible for closing the exchange,
-        // as stipulated by the API contract. We fulfill this obligation by not sending
-        // a message expecting a response on the exchange. Since we are in the middle
-        // of processing an incoming message, the exchange will close itself once we are
-        // done processing it, if there is no response to wait for at that point.
         err = SendCommandResponse();
+        // If SendCommandResponse() fails, we must close the exchange. We signal the failure to the
+        // requester with a StatusResponse ('Failure'). Since we're in the middle of processing an
+        // incoming message, we close the exchange by indicating that we don't expect a further response.
         VerifyOrExit(err == CHIP_NO_ERROR, failureStatusToSend.SetValue(Status::Failure));
 
         bool moreToSend = !mChunks.IsNull();
@@ -81,13 +79,28 @@ void CommandResponseSender::OnResponseTimeout(Messaging::ExchangeContext * apExc
     Close();
 }
 
-CHIP_ERROR CommandResponseSender::StartSendingCommandResponses()
+void CommandResponseSender::StartSendingCommandResponses()
 {
-    VerifyOrReturnError(mState == State::ReadyForInvokeResponses, CHIP_ERROR_INCORRECT_STATE);
-    // If SendCommandResponse() fails, we are obligated to close the exchange as per the API
-    // contract. However, this method's contract also stipulates that in the event of our
-    // failure, the caller bears the responsibility of closing the exchange.
-    ReturnErrorOnFailure(SendCommandResponse());
+    VerifyOrDie(mState == State::ReadyForInvokeResponses);
+    CHIP_ERROR err = SendCommandResponse();
+    if (err != CHIP_NO_ERROR)
+    {
+        ChipLogError(DataManagement, "Failed to send InvokeResponseMessage");
+        // TODO(#30453): It should be our responsibility to send a Failure StatusResponse to the requestor
+        // if there is a SessionHandle, but legacy unit tests explicitly check the behavior where
+        // we do not send any message. Changing this behavior should be done in a standalone
+        // PR where only that specific change is made. Here is a possible solution that could
+        // be used that fulfills our responsibility to send a Failure StatusResponse. This causes unit
+        // tests to start failing.
+        //   ```
+        //   if (mExchangeCtx && mExchangeCtx->HasSessionHandle())
+        //   {
+        //       SendStatusResponse(Status::Failure);
+        //   }
+        //   ```
+        Close();
+        return;
+    }
 
     if (HasMoreToSend())
     {
@@ -98,7 +111,31 @@ CHIP_ERROR CommandResponseSender::StartSendingCommandResponses()
     {
         Close();
     }
-    return CHIP_NO_ERROR;
+}
+
+void CommandResponseSender::OnDone(CommandHandler & apCommandObj)
+{
+    if (mState == State::ErrorSentDelayCloseUntilOnDone)
+    {
+        // We have already sent a message to the client indicating that we are not expecting
+        // a response.
+        Close();
+        return;
+    }
+    StartSendingCommandResponses();
+}
+
+void CommandResponseSender::DispatchCommand(CommandHandler & apCommandObj, const ConcreteCommandPath & aCommandPath,
+                                            TLV::TLVReader & apPayload)
+{
+    VerifyOrReturn(mpCommandHandlerCallback);
+    mpCommandHandlerCallback->DispatchCommand(apCommandObj, aCommandPath, apPayload);
+}
+
+Status CommandResponseSender::CommandExists(const ConcreteCommandPath & aCommandPath)
+{
+    VerifyOrReturnValue(mpCommandHandlerCallback, Protocols::InteractionModel::Status::UnsupportedCommand);
+    return mpCommandHandlerCallback->CommandExists(aCommandPath);
 }
 
 CHIP_ERROR CommandResponseSender::SendCommandResponse()
@@ -139,6 +176,9 @@ const char * CommandResponseSender::GetStateStr() const
 
     case State::AllInvokeResponsesSent:
         return "AllInvokeResponsesSent";
+
+    case State::ErrorSentDelayCloseUntilOnDone:
+        return "ErrorSentDelayCloseUntilOnDone";
     }
 #endif // CHIP_DETAIL_LOGGING
     return "N/A";
@@ -157,12 +197,52 @@ void CommandResponseSender::MoveToState(const State aTargetState)
 void CommandResponseSender::Close()
 {
     MoveToState(State::AllInvokeResponsesSent);
-    mCloseCalled = true;
-    if (mResponseSenderDoneCallback)
+    mpCallback->OnDone(*this);
+}
+
+void CommandResponseSender::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, System::PacketBufferHandle && payload,
+                                                   bool isTimedInvoke)
+{
+    VerifyOrDieWithMsg(ec != nullptr, DataManagement, "Incoming exchange context should not be null");
+    VerifyOrDieWithMsg(mState == State::ReadyForInvokeResponses, DataManagement, "state should be ReadyForInvokeResponses");
+
+    // NOTE: we already know this is an InvokeRequestMessage because we explicitly registered with the
+    // Exchange Manager for unsolicited InvokeRequestMessages.
+    mExchangeCtx.Grab(ec);
+    mExchangeCtx->WillSendMessage();
+
+    // Grabbing Handle to prevent mCommandHandler from calling OnDone before OnInvokeCommandRequest returns.
+    // This allows us to send a StatusResponse error instead of any potentially queued up InvokeResponseMessages.
+    CommandHandler::Handle workHandle(&mCommandHandler);
+    Status status = mCommandHandler.OnInvokeCommandRequest(*this, std::move(payload), isTimedInvoke);
+    if (status != Status::Success)
     {
-        mResponseSenderDoneCallback->mCall(mResponseSenderDoneCallback->mContext);
+        VerifyOrDie(mState == State::ReadyForInvokeResponses);
+        SendStatusResponse(status);
+        // The API contract of OnInvokeCommandRequest requires the CommandResponder instance to outlive
+        // the CommandHandler. Therefore, we cannot safely call Close() here, even though we have
+        // finished sending data. Closing must be deferred until the CommandHandler::OnDone callback.
+        MoveToState(State::ErrorSentDelayCloseUntilOnDone);
     }
 }
 
+#if CHIP_WITH_NLFAULTINJECTION
+
+void CommandResponseSender::TestOnlyInvokeCommandRequestWithFaultsInjected(Messaging::ExchangeContext * ec,
+                                                                           System::PacketBufferHandle && payload,
+                                                                           bool isTimedInvoke,
+                                                                           CommandHandler::NlFaultInjectionType faultType)
+{
+    VerifyOrDieWithMsg(ec != nullptr, DataManagement, "TH Failure: Incoming exchange context should not be null");
+    VerifyOrDieWithMsg(mState == State::ReadyForInvokeResponses, DataManagement,
+                       "TH Failure: state should be ReadyForInvokeResponses, issue with TH");
+
+    mExchangeCtx.Grab(ec);
+    mExchangeCtx->WillSendMessage();
+
+    mCommandHandler.TestOnlyInvokeCommandRequestWithFaultsInjected(*this, std::move(payload), isTimedInvoke, faultType);
+}
+#endif // CHIP_WITH_NLFAULTINJECTION
+
 } // namespace app
 } // namespace chip