Add fabric scoping to ECOINFO cluster attributes (project-chip#35022)

tehampson · restyled-commits · andy31415 · web-flow · commit 96ee61efe249 · 2024-08-19T14:30:25.000-04:00
---------

Co-authored-by: Restyled.io &lt;commits@restyled.io&gt;
Co-authored-by: Andrei Litvin &lt;andy314@gmail.com&gt;
diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp
@@ -128,6 +128,13 @@ EcosystemDeviceStruct::Builder & EcosystemDeviceStruct::Builder::AddUniqueLocati
     return *this;
 }
 
+EcosystemDeviceStruct::Builder & EcosystemDeviceStruct::Builder::SetFabricIndex(FabricIndex aFabricIndex)
+{
+    VerifyOrDie(!mIsAlreadyBuilt);
+    mFabricIndex = aFabricIndex;
+    return *this;
+}
+
 std::unique_ptr<EcosystemDeviceStruct> EcosystemDeviceStruct::Builder::Build()
 {
     VerifyOrReturnValue(!mIsAlreadyBuilt, nullptr, ChipLogError(Zcl, "Build() already called"));
@@ -136,6 +143,8 @@ std::unique_ptr<EcosystemDeviceStruct> EcosystemDeviceStruct::Builder::Build()
     VerifyOrReturnValue(!mDeviceTypes.empty(), nullptr, ChipLogError(Zcl, "No device types added"));
     VerifyOrReturnValue(mUniqueLocationIds.size() <= kUniqueLocationIdsListMaxSize, nullptr,
                         ChipLogError(Zcl, "Too many location ids"));
+    VerifyOrReturnValue(mFabricIndex >= kMinValidFabricIndex, nullptr, ChipLogError(Zcl, "Fabric index is invalid"));
+    VerifyOrReturnValue(mFabricIndex <= kMaxValidFabricIndex, nullptr, ChipLogError(Zcl, "Fabric index is invalid"));
 
     for (auto & locationId : mUniqueLocationIds)
     {
@@ -145,12 +154,12 @@ std::unique_ptr<EcosystemDeviceStruct> EcosystemDeviceStruct::Builder::Build()
     // std::make_unique does not have access to private constructor we workaround with using new
     std::unique_ptr<EcosystemDeviceStruct> ret{ new EcosystemDeviceStruct(
         std::move(mDeviceName), mDeviceNameLastEditEpochUs, mBridgedEndpoint, mOriginalEndpoint, std::move(mDeviceTypes),
-        std::move(mUniqueLocationIds), mUniqueLocationIdsLastEditEpochUs) };
+        std::move(mUniqueLocationIds), mUniqueLocationIdsLastEditEpochUs, mFabricIndex) };
     mIsAlreadyBuilt = true;
     return ret;
 }
 
-CHIP_ERROR EcosystemDeviceStruct::Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const FabricIndex & aFabricIndex)
+CHIP_ERROR EcosystemDeviceStruct::Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder)
 {
     Structs::EcosystemDeviceStruct::Type deviceStruct;
     if (!mDeviceName.empty())
@@ -172,9 +181,7 @@ CHIP_ERROR EcosystemDeviceStruct::Encode(const AttributeValueEncoder::ListEncode
     deviceStruct.uniqueLocationIDs = DataModel::List<CharSpan>(locationIds.data(), locationIds.size());
 
     deviceStruct.uniqueLocationIDsLastEdit = mUniqueLocationIdsLastEditEpochUs;
-
-    // TODO(#33223) this is a hack, use mFabricIndex when it exists.
-    deviceStruct.SetFabricIndex(aFabricIndex);
+    deviceStruct.SetFabricIndex(mFabricIndex);
     return aEncoder.Encode(deviceStruct);
 }
 
@@ -226,12 +233,9 @@ CHIP_ERROR EcosystemLocationStruct::Encode(const AttributeValueEncoder::ListEnco
                                            const std::string & aUniqueLocationId, const FabricIndex & aFabricIndex)
 {
     Structs::EcosystemLocationStruct::Type locationStruct;
-    VerifyOrDie(!aUniqueLocationId.empty());
     locationStruct.uniqueLocationID           = CharSpan(aUniqueLocationId.c_str(), aUniqueLocationId.size());
     locationStruct.locationDescriptor         = GetEncodableLocationDescriptorStruct(mLocationDescriptor);
     locationStruct.locationDescriptorLastEdit = mLocationDescriptorLastEditEpochUs;
-
-    // TODO(#33223) this is a hack, use mFabricIndex when it exists.
     locationStruct.SetFabricIndex(aFabricIndex);
     return aEncoder.Encode(locationStruct);
 }
@@ -266,16 +270,20 @@ CHIP_ERROR EcosystemInformationServer::AddDeviceInfo(EndpointId aEndpoint, std::
 }
 
 CHIP_ERROR EcosystemInformationServer::AddLocationInfo(EndpointId aEndpoint, const std::string & aLocationId,
-                                                       std::unique_ptr<EcosystemLocationStruct> aLocation)
+                                                       FabricIndex aFabricIndex, std::unique_ptr<EcosystemLocationStruct> aLocation)
 {
     VerifyOrReturnError(aLocation, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError((aEndpoint != kRootEndpointId && aEndpoint != kInvalidEndpointId), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(!aLocationId.empty(), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(aFabricIndex >= kMinValidFabricIndex, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(aFabricIndex <= kMaxValidFabricIndex, CHIP_ERROR_INVALID_ARGUMENT);
 
-    auto & deviceInfo = mDevicesMap[aEndpoint];
-    VerifyOrReturnError((deviceInfo.mLocationDirectory.find(aLocationId) == deviceInfo.mLocationDirectory.end()),
+    auto & deviceInfo        = mDevicesMap[aEndpoint];
+    EcosystemLocationKey key = { .mUniqueLocationId = aLocationId, .mFabricIndex = aFabricIndex };
+    VerifyOrReturnError((deviceInfo.mLocationDirectory.find(key) == deviceInfo.mLocationDirectory.end()),
                         CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError((deviceInfo.mLocationDirectory.size() < kLocationDirectoryMaxSize), CHIP_ERROR_NO_MEMORY);
-    deviceInfo.mLocationDirectory[aLocationId] = std::move(aLocation);
+    deviceInfo.mLocationDirectory[key] = std::move(aLocation);
     return CHIP_NO_ERROR;
 }
 
@@ -352,11 +360,10 @@ CHIP_ERROR EcosystemInformationServer::EncodeDeviceDirectoryAttribute(EndpointId
         return aEncoder.EncodeEmptyList();
     }
 
-    FabricIndex fabricIndex = aEncoder.AccessingFabricIndex();
     return aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR {
         for (auto & device : deviceInfo.mDeviceDirectory)
         {
-            ReturnErrorOnFailure(device->Encode(encoder, fabricIndex));
+            ReturnErrorOnFailure(device->Encode(encoder));
         }
         return CHIP_NO_ERROR;
     });
@@ -379,11 +386,10 @@ CHIP_ERROR EcosystemInformationServer::EncodeLocationStructAttribute(EndpointId
         return aEncoder.EncodeEmptyList();
     }
 
-    FabricIndex fabricIndex = aEncoder.AccessingFabricIndex();
     return aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR {
-        for (auto & [id, device] : deviceInfo.mLocationDirectory)
+        for (auto & [key, device] : deviceInfo.mLocationDirectory)
         {
-            ReturnErrorOnFailure(device->Encode(encoder, id, fabricIndex));
+            ReturnErrorOnFailure(device->Encode(encoder, key.mUniqueLocationId, key.mFabricIndex));
         }
         return CHIP_NO_ERROR;
     });
diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h
@@ -49,6 +49,7 @@ class EcosystemDeviceStruct
         Builder & SetOriginalEndpoint(EndpointId aOriginalEndpoint);
         Builder & AddDeviceType(Structs::DeviceTypeStruct::Type aDeviceType);
         Builder & AddUniqueLocationId(std::string aUniqueLocationId, uint64_t aUniqueLocationIdsLastEditEpochUs);
+        Builder & SetFabricIndex(FabricIndex aFabricIndex);
 
         // Upon success this object will have moved all ownership of underlying
         // types to EcosystemDeviceStruct and should not be used afterwards.
@@ -62,21 +63,25 @@ class EcosystemDeviceStruct
         std::vector<Structs::DeviceTypeStruct::Type> mDeviceTypes;
         std::vector<std::string> mUniqueLocationIds;
         uint64_t mUniqueLocationIdsLastEditEpochUs = 0;
+        FabricIndex mFabricIndex                   = kUndefinedFabricIndex;
         bool mIsAlreadyBuilt                       = false;
     };
 
-    CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const FabricIndex & aFabricIndex);
+    CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder);
 
 private:
     // Constructor is intentionally private. This is to ensure that it is only constructed with
     // values that conform to the spec.
     explicit EcosystemDeviceStruct(std::string && aDeviceName, uint64_t aDeviceNameLastEditEpochUs, EndpointId aBridgedEndpoint,
                                    EndpointId aOriginalEndpoint, std::vector<Structs::DeviceTypeStruct::Type> && aDeviceTypes,
-                                   std::vector<std::string> && aUniqueLocationIds, uint64_t aUniqueLocationIdsLastEditEpochUs) :
+                                   std::vector<std::string> && aUniqueLocationIds, uint64_t aUniqueLocationIdsLastEditEpochUs,
+                                   FabricIndex aFabricIndex) :
         mDeviceName(std::move(aDeviceName)),
         mDeviceNameLastEditEpochUs(aDeviceNameLastEditEpochUs), mBridgedEndpoint(aBridgedEndpoint),
         mOriginalEndpoint(aOriginalEndpoint), mDeviceTypes(std::move(aDeviceTypes)),
-        mUniqueLocationIds(std::move(aUniqueLocationIds)), mUniqueLocationIdsLastEditEpochUs(aUniqueLocationIdsLastEditEpochUs)
+        mUniqueLocationIds(std::move(aUniqueLocationIds)), mUniqueLocationIdsLastEditEpochUs(aUniqueLocationIdsLastEditEpochUs),
+        mFabricIndex(aFabricIndex)
+
     {}
 
     const std::string mDeviceName;
@@ -86,10 +91,7 @@ class EcosystemDeviceStruct
     std::vector<Structs::DeviceTypeStruct::Type> mDeviceTypes;
     std::vector<std::string> mUniqueLocationIds;
     uint64_t mUniqueLocationIdsLastEditEpochUs;
-    // TODO(#33223) This structure needs to contain fabric index to be spec compliant.
-    // To keep initial PR smaller, we are going to assume that all entries
-    // here are for any fabric. This will allow follow up PR introducing
-    // fabric scoped to be more throughly reviewed with focus on fabric scoping.
+    FabricIndex mFabricIndex;
 };
 
 struct LocationDescriptorStruct
@@ -134,15 +136,11 @@ class EcosystemLocationStruct
         mLocationDescriptor(aLocationDescriptor), mLocationDescriptorLastEditEpochUs(aLocationDescriptorLastEditEpochUs)
     {}
     // EcosystemLocationStruct is used as a value in a key-value map.
-    // Because UniqueLocationId is manditory when an entry exist, and
-    // it is unique, we use it as a key to the key-value pair and is why it is
+    // Because UniqueLocationId and FabricIndex are mandatory when an entry exist,
+    // and needs to be unique, we use it as a key to the key-value pair and is why it is
     // not explicitly in this struct.
     LocationDescriptorStruct mLocationDescriptor;
     uint64_t mLocationDescriptorLastEditEpochUs;
-    // TODO(#33223) This structure needs to contain fabric index to be spec compliant.
-    // To keep initial PR smaller, we are going to assume that all entries
-    // here are for any fabric. This will allow follow up PR introducing
-    // fabric scoped to be more throughly reviewed with focus on fabric scoping.
 };
 
 class EcosystemInformationServer
@@ -186,7 +184,7 @@ class EcosystemInformationServer
      * @return #CHIP_NO_ERROR on success.
      * @return Other CHIP_ERROR associated with issue.
      */
-    CHIP_ERROR AddLocationInfo(EndpointId aEndpoint, const std::string & aLocationId,
+    CHIP_ERROR AddLocationInfo(EndpointId aEndpoint, const std::string & aLocationId, FabricIndex aFabricIndex,
                                std::unique_ptr<EcosystemLocationStruct> aLocation);
 
     /**
@@ -203,12 +201,22 @@ class EcosystemInformationServer
     CHIP_ERROR ReadAttribute(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder);
 
 private:
+    struct EcosystemLocationKey
+    {
+        bool operator<(const EcosystemLocationKey & other) const
+        {
+            return mUniqueLocationId < other.mUniqueLocationId ||
+                (mUniqueLocationId == other.mUniqueLocationId && mFabricIndex < other.mFabricIndex);
+        }
+        std::string mUniqueLocationId;
+        FabricIndex mFabricIndex;
+    };
+
     struct DeviceInfo
     {
         Optional<uint64_t> mRemovedOn = NullOptional;
         std::vector<std::unique_ptr<EcosystemDeviceStruct>> mDeviceDirectory;
-        // Map key is using the UniqueLocationId
-        std::map<std::string, std::unique_ptr<EcosystemLocationStruct>> mLocationDirectory;
+        std::map<EcosystemLocationKey, std::unique_ptr<EcosystemLocationStruct>> mLocationDirectory;
     };
 
     CHIP_ERROR EncodeRemovedOnAttribute(EndpointId aEndpoint, AttributeValueEncoder & aEncoder);
