From 0773cfb0385c141a5e001af690c20e1c826d5e2e Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Fri, 29 Apr 2022 08:37:43 -0700 Subject: [PATCH] deduplicate the concrete paths intersected with the other wildcard paths in server (#17719) --- src/app/InteractionModelEngine.cpp | 59 +++++++++ src/app/InteractionModelEngine.h | 4 + src/app/ReadHandler.cpp | 1 + src/app/tests/TestInteractionModelEngine.cpp | 128 +++++++++++++++++++ src/app/util/attribute-storage.cpp | 4 + src/app/util/mock/attribute-storage.cpp | 5 + src/darwin/Framework/CHIP/CHIPIMDispatch.mm | 5 + 7 files changed, 206 insertions(+) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 78c17ef2ccfabb..060016da49e4f6 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -24,10 +24,14 @@ */ #include "InteractionModelEngine.h" + #include #include +extern bool emberAfContainsAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId, + bool asServer); + namespace chip { namespace app { InteractionModelEngine sInteractionModelEngine; @@ -852,6 +856,61 @@ CHIP_ERROR InteractionModelEngine::PushFrontAttributePathList(ObjectList *& aAttributePaths) +{ + ObjectList * prev = nullptr; + auto * path1 = aAttributePaths; + + while (path1 != nullptr) + { + bool duplicate = false; + // skip all wildcard paths and invalid concrete attribute + if (path1->mValue.HasAttributeWildcard() || + !emberAfContainsAttribute(path1->mValue.mEndpointId, path1->mValue.mClusterId, path1->mValue.mAttributeId, true)) + { + prev = path1; + path1 = path1->mpNext; + continue; + } + + // Check whether a wildcard path expands to something that includes this concrete path. + for (auto * path2 = aAttributePaths; path2 != nullptr; path2 = path2->mpNext) + { + if (path2 == path1) + { + continue; + } + + if (path2->mValue.HasAttributeWildcard() && path2->mValue.IsAttributePathSupersetOf(path1->mValue)) + { + duplicate = true; + break; + } + } + + // if path1 duplicates something from wildcard expansion, discard path1 + if (!duplicate) + { + prev = path1; + path1 = path1->mpNext; + continue; + } + + if (path1 == aAttributePaths) + { + aAttributePaths = path1->mpNext; + mAttributePathPool.ReleaseObject(path1); + path1 = aAttributePaths; + } + else + { + prev->mpNext = path1->mpNext; + mAttributePathPool.ReleaseObject(path1); + path1 = prev->mpNext; + } + } +} + void InteractionModelEngine::ReleaseEventPathList(ObjectList *& aEventPathList) { ReleasePool(aEventPathList, mEventPathPool); diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index bb7072fa75586c..9344248a6f2995 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -157,6 +157,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, CHIP_ERROR PushFrontAttributePathList(ObjectList *& aAttributePathList, AttributePathParams & aAttributePath); + // If a concrete path indicates an attribute that is also referenced by a wildcard path in the request, + // the path SHALL be removed from the list. + void RemoveDuplicateConcreteAttributePath(ObjectList *& aAttributePaths); + void ReleaseEventPathList(ObjectList *& aEventPathList); CHIP_ERROR PushFrontEventPathParamsList(ObjectList *& aEventPathList, EventPathParams & aEventPath); diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 9c4d38748faf30..50ec6d08de5a43 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -473,6 +473,7 @@ CHIP_ERROR ReadHandler::ProcessAttributePathList(AttributePathIBs::Parser & aAtt // if we have exhausted this container if (CHIP_END_OF_TLV == err) { + InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(mpAttributePathList); mAttributePathExpandIterator = AttributePathExpandIterator(mpAttributePathList); err = CHIP_NO_ERROR; } diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index 1d3f377b90b558..3c2e5cb075073d 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -24,6 +24,8 @@ #include #include +#include +#include #include #include #include @@ -44,6 +46,7 @@ class TestInteractionModelEngine { public: static void TestAttributePathParamsPushRelease(nlTestSuite * apSuite, void * apContext); + static void TestRemoveDuplicateConcreteAttribute(nlTestSuite * apSuite, void * apContext); static int GetAttributePathListLength(ObjectList * apattributePathParamsList); }; @@ -95,6 +98,130 @@ void TestInteractionModelEngine::TestAttributePathParamsPushRelease(nlTestSuite InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList); NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 0); } + +void TestInteractionModelEngine::TestRemoveDuplicateConcreteAttribute(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + err = InteractionModelEngine::GetInstance()->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable()); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + ObjectList * attributePathParamsList = nullptr; + AttributePathParams attributePathParams1; + AttributePathParams attributePathParams2; + AttributePathParams attributePathParams3; + + // Three concrete paths, no duplicates + attributePathParams1.mEndpointId = Test::kMockEndpoint3; + attributePathParams1.mClusterId = Test::MockClusterId(2); + attributePathParams1.mAttributeId = Test::MockAttributeId(1); + + attributePathParams2.mEndpointId = Test::kMockEndpoint3; + attributePathParams2.mClusterId = Test::MockClusterId(2); + attributePathParams2.mAttributeId = Test::MockAttributeId(2); + + attributePathParams3.mEndpointId = Test::kMockEndpoint3; + attributePathParams3.mClusterId = Test::MockClusterId(2); + attributePathParams3.mAttributeId = Test::MockAttributeId(3); + + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3); + InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList); + NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 3); + InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList); + + attributePathParams1.mEndpointId = kInvalidEndpointId; + attributePathParams1.mClusterId = kInvalidClusterId; + attributePathParams1.mAttributeId = kInvalidAttributeId; + + attributePathParams2.mEndpointId = Test::kMockEndpoint3; + attributePathParams2.mClusterId = Test::MockClusterId(2); + attributePathParams2.mAttributeId = Test::MockAttributeId(2); + + attributePathParams3.mEndpointId = Test::kMockEndpoint3; + attributePathParams3.mClusterId = Test::MockClusterId(2); + attributePathParams3.mAttributeId = Test::MockAttributeId(3); + + // 1st path is wildcard endpoint, 2nd, 3rd paths are concrete paths, the concrete ones would be removed. + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3); + InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList); + NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 1); + InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList); + + // 2nd path is wildcard endpoint, 1st, 3rd paths are concrete paths, the latter two would be removed. + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3); + InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList); + NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 1); + InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList); + + // 3nd path is wildcard endpoint, 1st, 2nd paths are concrete paths, the latter two would be removed. + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1); + InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList); + NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 1); + InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList); + + attributePathParams1.mEndpointId = Test::kMockEndpoint3; + attributePathParams1.mClusterId = Test::MockClusterId(2); + attributePathParams1.mAttributeId = kInvalidAttributeId; + + attributePathParams2.mEndpointId = Test::kMockEndpoint2; + attributePathParams2.mClusterId = Test::MockClusterId(2); + attributePathParams2.mAttributeId = Test::MockAttributeId(2); + + attributePathParams3.mEndpointId = Test::kMockEndpoint2; + attributePathParams3.mClusterId = Test::MockClusterId(2); + attributePathParams3.mAttributeId = Test::MockAttributeId(3); + + // 1st is wildcard one, but not intersect with the latter two concrete paths, so the paths in total are 3 finally + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3); + InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList); + NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 3); + InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList); + + attributePathParams1.mEndpointId = kInvalidEndpointId; + attributePathParams1.mClusterId = kInvalidClusterId; + attributePathParams1.mAttributeId = kInvalidAttributeId; + + attributePathParams2.mEndpointId = Test::kMockEndpoint3; + attributePathParams2.mClusterId = kInvalidClusterId; + attributePathParams2.mAttributeId = kInvalidAttributeId; + + attributePathParams3.mEndpointId = kInvalidEndpointId; + attributePathParams3.mClusterId = kInvalidClusterId; + attributePathParams3.mAttributeId = Test::MockAttributeId(3); + + // Wildcards cannot be deduplicated. + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams3); + InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList); + NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 3); + InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList); + + attributePathParams1.mEndpointId = kInvalidEndpointId; + attributePathParams1.mClusterId = Test::MockClusterId(2); + attributePathParams1.mAttributeId = Test::MockAttributeId(10); + + attributePathParams2.mEndpointId = Test::kMockEndpoint3; + attributePathParams2.mClusterId = Test::MockClusterId(2); + attributePathParams2.mAttributeId = Test::MockAttributeId(10); + + // 1st path is wildcard endpoint, 2nd path is invalid attribute + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams1); + InteractionModelEngine::GetInstance()->PushFrontAttributePathList(attributePathParamsList, attributePathParams2); + InteractionModelEngine::GetInstance()->RemoveDuplicateConcreteAttributePath(attributePathParamsList); + NL_TEST_ASSERT(apSuite, GetAttributePathListLength(attributePathParamsList) == 2); + InteractionModelEngine::GetInstance()->ReleaseAttributePathList(attributePathParamsList); +} + } // namespace app } // namespace chip @@ -104,6 +231,7 @@ namespace { const nlTest sTests[] = { NL_TEST_DEF("TestAttributePathParamsPushRelease", chip::app::TestInteractionModelEngine::TestAttributePathParamsPushRelease), + NL_TEST_DEF("TestRemoveDuplicateConcreteAttribute", chip::app::TestInteractionModelEngine::TestRemoveDuplicateConcreteAttribute), NL_TEST_SENTINEL() }; // clang-format on diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index ec2c0f12bc823f..88c2e2dc575fcd 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -650,6 +650,10 @@ EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord, // Dynamic endpoints are external and don't factor into storage size if (!isDynamicEndpoint) { + if (emAfEndpoints[ep].endpointType == nullptr) + { + return EMBER_ZCL_STATUS_UNSUPPORTED_ATTRIBUTE; + } attributeOffsetIndex = static_cast(attributeOffsetIndex + emAfEndpoints[ep].endpointType->endpointSize); } } diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp index 57c339504f3840..93c4dde20e76aa 100644 --- a/src/app/util/mock/attribute-storage.cpp +++ b/src/app/util/mock/attribute-storage.cpp @@ -164,6 +164,11 @@ uint16_t emberAfGetServerAttributeIndexByAttributeId(chip::EndpointId endpoint, return UINT16_MAX; } +bool emberAfContainsAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId, bool asServer) +{ + return !(emberAfGetServerAttributeIndexByAttributeId(endpoint, clusterId, attributeId) == UINT16_MAX); +} + chip::EndpointId emberAfEndpointFromIndex(uint16_t index) { VerifyOrDie(index < ArraySize(endpoints)); diff --git a/src/darwin/Framework/CHIP/CHIPIMDispatch.mm b/src/darwin/Framework/CHIP/CHIPIMDispatch.mm index b6abe8a34985a2..8be1bb2834d121 100644 --- a/src/darwin/Framework/CHIP/CHIPIMDispatch.mm +++ b/src/darwin/Framework/CHIP/CHIPIMDispatch.mm @@ -272,6 +272,11 @@ uint16_t emberAfGetServerAttributeIndexByAttributeId(EndpointId endpoint, Cluste return UINT16_MAX; } +bool emberAfContainsAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId, bool asServer) +{ + return false; +} + uint8_t emberAfClusterCount(EndpointId endpoint, bool server) { if (endpoint == kSupportedEndpoint && server) {