[NT-0] fix: Removing an asset collection assert (#841)

A CSP client app developer recently reported a crash when attempting to enter a space. Turns out it was due to there being an asset collection/prototype in the space that did not conform to the types CSP expects, which caused CSP to fire an assert.

To fix, we follow the same pattern as with Asset.cpp and emit an Error log message instead of an assert.

Author: MAG-SamBirley

Library/src/Systems/Assets/AssetCollection.cpp

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 #include "CSP/Systems/Assets/AssetCollection.h"
+#include "Debug/Logging.h"
 
 #include "CSP/Common/Systems/Log/LogSystem.h"
 #include "Services/ApiBase/ApiBase.h"
@@ -31,8 +32,6 @@ csp::systems::EAssetCollectionType ConvertDTOPrototypeType(const csp::common::St
 {
     if (DTOPrototypeType == "Default")
         return csp::systems::EAssetCollectionType::DEFAULT;
-    else if (DTOPrototypeType == "Charity")
-        return csp::systems::EAssetCollectionType::FOUNDATION_INTERNAL;
     else if (DTOPrototypeType == "FoundationInternal")
         return csp::systems::EAssetCollectionType::FOUNDATION_INTERNAL;
     else if (DTOPrototypeType == "CommentContainer")
@@ -43,7 +42,8 @@ csp::systems::EAssetCollectionType ConvertDTOPrototypeType(const csp::common::St
         return csp::systems::EAssetCollectionType::SPACE_THUMBNAIL;
     else
     {
-        assert(false && "Unsupported Prototype Type!");
+        CSP_LOG_FORMAT(csp::common::LogLevel::Error, "Encountered unknown prototype type whilst processing an asset collection DTO: %s",
+            DTOPrototypeType.c_str());
         return csp::systems::EAssetCollectionType::DEFAULT;
     }
 }
@@ -56,8 +56,15 @@ namespace csp::systems
 // Currently known to be compatible with both chs::PrototypeDto and chs::CopiedPrototypeDto.
 template <class PrototypeDto> void AssetCollectionFromDtoOfType(const PrototypeDto& Dto, csp::systems::AssetCollection& AssetCollection)
 {
-    AssetCollection.Id = Dto.GetId();
-    AssetCollection.Name = Dto.GetName();
+    if (Dto.HasId())
+    {
+        AssetCollection.Id = Dto.GetId();
+    }
+
+    if (Dto.HasName())
+    {
+        AssetCollection.Name = Dto.GetName();
+    }
 
     if (Dto.HasType())
     {
@@ -106,10 +113,25 @@ template <class PrototypeDto> void AssetCollectionFromDtoOfType(const PrototypeD
         }
     }
 
-    AssetCollection.CreatedBy = Dto.GetCreatedBy();
-    AssetCollection.CreatedAt = Dto.GetCreatedAt();
-    AssetCollection.UpdatedBy = Dto.GetUpdatedBy();
-    AssetCollection.UpdatedAt = Dto.GetUpdatedAt();
+    if (Dto.HasCreatedBy())
+    {
+        AssetCollection.CreatedBy = Dto.GetCreatedBy();
+    }
+
+    if (Dto.HasCreatedAt())
+    {
+        AssetCollection.CreatedAt = Dto.GetCreatedAt();
+    }
+
+    if (Dto.HasUpdatedBy())
+    {
+        AssetCollection.UpdatedBy = Dto.GetUpdatedBy();
+    }
+
+    if (Dto.HasUpdatedAt())
+    {
+        AssetCollection.UpdatedAt = Dto.GetUpdatedAt();
+    }
 
     if (Dto.HasHighlander())
     {

Tests/src/PublicAPITests/AssetSystemTests.cpp

@@ -21,6 +21,7 @@
 #include "CSP/Systems/Spaces/SpaceSystem.h"
 #include "CSP/Systems/SystemsManager.h"
 #include "RAIIMockLogger.h"
+#include "Services/PrototypeService/PrototypeServiceApiMock.h"
 #include "SpaceSystemTestHelpers.h"
 #include "Systems/ResultHelpers.h"
 #include "TestHelpers.h"
@@ -36,9 +37,13 @@
 
 #include "gtest/gtest.h"
 #include <filesystem>
+#include <future>
+#include <gmock/gmock.h>
 
 using namespace csp::multiplayer;
 
+namespace chs_prototype = csp::services::generated::prototypeservice;
+
 namespace
 {
 
@@ -2481,4 +2486,79 @@ CSP_PUBLIC_TEST(CSPEngine, AssetSystemTests, GetAssetCollectionCountTest)
     DeleteSpace(SpaceSystem, Space.Id);
 
     LogOut(UserSystem);
-}
+}
+
+class AssetCollectionTypeDtoMock
+    : public PublicTestBaseWithParam<std::tuple<csp::common::String, csp::systems::EAssetCollectionType, csp::common::String>>
+{
+};
+
+TEST_P(AssetCollectionTypeDtoMock, AssetCollectionTypeDtoMockTest)
+{
+    const auto PrototypeServiceMock = std::make_unique<csp::services::generated::prototypeservice::PrototypeApiMock>();
+
+    csp::systems::SystemsManager::Get().GetLogSystem()->SetSystemLevel(csp::common::LogLevel::Error);
+
+    // Test parameters
+    const csp::common::String& DtoTypeString = std::get<0>(GetParam());
+    const csp::systems::EAssetCollectionType ExpectedAssetCollectionType = std::get<1>(GetParam());
+    const csp::common::String& ExpectedLogMessage = std::get<2>(GetParam());
+
+    const csp::common::String& MockAssetCollectionId = "1234";
+
+    // The promise
+    std::promise<csp::systems::AssetCollectionResult> ResultPromise;
+    std::future<csp::systems::AssetCollectionResult> ResultFuture = ResultPromise.get_future();
+
+    EXPECT_CALL(*PrototypeServiceMock, prototypesIdGet)
+        .WillOnce(
+            [DtoTypeString](const chs_prototype::IPrototypeApiBase::prototypesIdGetParams& /*Params*/,
+                csp::services::ApiResponseHandlerBase* ResponseHandler, csp::common::CancellationToken& /*CancellationToken*/)
+            {
+                chs_prototype::PrototypeDto Dto;
+                auto Response = csp::web::HttpResponse();
+                Response.SetResponseCode(csp::web::EResponseCodes::ResponseOK);
+
+                csp::web::HttpPayload Payload;
+
+                const csp::common::String RequestBody = R"(
+                {
+                  "type": ")"
+                    + DtoTypeString + R"("
+                }
+            )";
+
+                Payload.AddHeader(CSP_TEXT("Content-Type"), CSP_TEXT("application/json"));
+                Payload.SetContent(RequestBody);
+
+                Response.GetMutablePayload() = Payload;
+                ResponseHandler->OnHttpResponse(Response);
+            });
+
+    csp::systems::AssetCollectionResultCallback Callback
+        = [&ResultPromise](const csp::systems::AssetCollectionResult& Result) { ResultPromise.set_value(Result); };
+
+    csp::services::ResponseHandlerPtr ResponseHandler = PrototypeServiceMock->CreateHandler<csp::systems::AssetCollectionResultCallback,
+        csp::systems::AssetCollectionResult, void, csp::services::generated::prototypeservice::PrototypeDto>(Callback, nullptr);
+
+    RAIIMockLogger MockLogger {};
+    if (ExpectedLogMessage.IsEmpty() == false)
+    {
+        EXPECT_CALL(MockLogger.MockLogCallback, Call(csp::common::LogLevel::Error, testing::HasSubstr(ExpectedLogMessage))).Times(1);
+    }
+
+    PrototypeServiceMock->prototypesIdGet({ MockAssetCollectionId }, ResponseHandler, csp::common::CancellationToken::Dummy());
+    auto Result = ResultFuture.get();
+
+    EXPECT_EQ(Result.GetHttpResultCode(), static_cast<uint16_t>(csp::web::EResponseCodes::ResponseOK));
+    EXPECT_EQ(Result.GetAssetCollection().Type, ExpectedAssetCollectionType);
+}
+
+INSTANTIATE_TEST_SUITE_P(AssetSystemTests, AssetCollectionTypeDtoMock,
+    testing::Values(std::make_tuple("Default", csp::systems::EAssetCollectionType::DEFAULT, ""),
+        std::make_tuple("FoundationInternal", csp::systems::EAssetCollectionType::FOUNDATION_INTERNAL, ""),
+        std::make_tuple("CommentContainer", csp::systems::EAssetCollectionType::COMMENT_CONTAINER, ""),
+        std::make_tuple("Comment", csp::systems::EAssetCollectionType::COMMENT, ""),
+        std::make_tuple("SpaceThumbnail", csp::systems::EAssetCollectionType::SPACE_THUMBNAIL, ""),
+        std::make_tuple("NotARealType", csp::systems::EAssetCollectionType::DEFAULT,
+            "Encountered unknown prototype type whilst processing an asset collection DTO:")));

Tests/src/PublicTestBase.cpp

@@ -131,4 +131,5 @@ template class PublicTestBaseWithParam<std::tuple<csp::common::RealtimeEngineTyp
 template class PublicTestBaseWithParam<csp::common::RealtimeEngineType>;
 template class PublicTestBaseWithParam<std::tuple<csp::common::RealtimeEngineType, bool>>;
 template class PublicTestBaseWithParam<std::tuple<csp::systems::AvatarType, csp::common::String, bool>>;
-template class PublicTestBaseWithParam<std::tuple<csp::systems::EResultCode, csp::web::EResponseCodes, csp::common::String, bool>>;
+template class PublicTestBaseWithParam<std::tuple<csp::systems::EResultCode, csp::web::EResponseCodes, csp::common::String, bool>>;
+template class PublicTestBaseWithParam<std::tuple<csp::common::String, csp::systems::EAssetCollectionType, csp::common::String>>;

Tests/src/PublicTestBase.h

@@ -17,6 +17,7 @@
 
 #include "CSP/Common/Interfaces/IRealtimeEngine.h"
 #include "CSP/Common/SharedEnums.h"
+#include "CSP/Systems/Assets/AssetCollection.h"
 #include "CSP/Systems/Settings/SettingsCollection.h"
 #include "CSP/Systems/Spaces/Space.h"
 #include "Mocks/SignalRConnectionMock.h"
@@ -75,4 +76,5 @@ extern template class PublicTestBaseWithParam<std::tuple<csp::common::RealtimeEn
 extern template class PublicTestBaseWithParam<csp::common::RealtimeEngineType>;
 extern template class PublicTestBaseWithParam<std::tuple<csp::common::RealtimeEngineType, bool>>;
 extern template class PublicTestBaseWithParam<std::tuple<csp::systems::AvatarType, csp::common::String, bool>>;
-extern template class PublicTestBaseWithParam<std::tuple<csp::systems::EResultCode, csp::web::EResponseCodes, csp::common::String, bool>>;
+extern template class PublicTestBaseWithParam<std::tuple<csp::systems::EResultCode, csp::web::EResponseCodes, csp::common::String, bool>>;
+extern template class PublicTestBaseWithParam<std::tuple<csp::common::String, csp::systems::EAssetCollectionType, csp::common::String>>;