Make Uuid / AssetId ToFixedString calls more consistent and less error prone. (#9020)

* add ToFixedString() methods to Uuid and AssetId

Using the same ToString<>() methods with the Uuid MaxStringBuffer leads
to an easy to fallint trap and error, that frequently only reveals
itself when some error condition with var args is hit. This probably
won't happen during initial testing. This should give an easy to find
safer option.

Signed-off-by: carlitosan <[email protected]>

* consolidate ToFixedString functionality for Uuid and AssetId

Both AssetId and Uuid have template functionality to allow AZStd::fixed_string as
the template instatation of their ToString calls. However, this leaves an
easy trap to fall into, especially in string format calls. It is easy to
use the Uuid::MaxStringBuffer when calling the AssetId to fixed string calls,
which leads to errors since the AssetId requires extra characters for the subId.

This change adds ToFixedString member functions to both classes, and updates
all direct references in the codebase to Uuid::MaxStringBuffer to use AssetId.

Signed-off-by: carlitosan <[email protected]>

Code/Framework/AzCore/AzCore/Asset/AssetCommon.cpp

@@ -92,6 +92,13 @@ namespace AZ::Data
         }
     }
 
+    AssetId::FixedString AssetId::ToFixedString() const
+    {
+        FixedString result;
+        result = FixedString::format("%s:%08x", m_guid.ToFixedString().c_str(), m_subId);
+        return result;
+    }
+
     namespace AssetInternal
     {
         Asset<AssetData> FindOrCreateAsset(const AssetId& id, const AssetType& type, AssetLoadBehavior assetReferenceLoadBehavior)

Code/Framework/AzCore/AzCore/Asset/AssetCommon.h

@@ -8,18 +8,19 @@
 #pragma once
 
 #include <AzCore/EBus/EBus.h>
-#include <AzCore/std/parallel/atomic.h>
-#include <AzCore/std/parallel/mutex.h>
-#include <AzCore/std/function/function_fwd.h>
-#include <AzCore/RTTI/RTTI.h>
-#include <AzCore/Memory/SystemAllocator.h>
+#include <AzCore/IO/IStreamerTypes.h>
 #include <AzCore/Math/Uuid.h>
+#include <AzCore/Memory/SystemAllocator.h>
 #include <AzCore/Preprocessor/Enum.h>
+#include <AzCore/RTTI/RTTI.h>
 #include <AzCore/std/containers/bitset.h>
+#include <AzCore/std/function/function_fwd.h>
+#include <AzCore/std/parallel/atomic.h>
+#include <AzCore/std/parallel/mutex.h>
+#include <AzCore/std/string/fixed_string.h>
 #include <AzCore/std/string/string.h>
 #include <AzCore/std/string/string_view.h>
 #include <AzCore/std/typetraits/is_base_of.h>
-#include <AzCore/IO/IStreamerTypes.h>
 
 namespace AZ
 {
@@ -82,6 +83,10 @@ namespace AZ
             static AssetId CreateString(AZStd::string_view input);
             static void Reflect(ReflectContext* context);
 
+            static constexpr size_t MaxStringBuffer = AZ::Uuid::MaxStringBuffer + 9; /// UUid size (includes terminal) + ":" + hex, subId
+            using FixedString = AZStd::fixed_string<MaxStringBuffer>;
+            FixedString ToFixedString() const;
+
             Uuid m_guid;
             u32  m_subId;   ///< To allow easier and more consistent asset guid, we can provide asset sub ID. (i.e. Guid is a cubemap texture, subId is the index of the side)
             // Explicitly define and clear the set of pad bytes in this struct.
@@ -1037,13 +1042,11 @@ namespace AZ
             if (assetData && !assetData->RTTI_IsTypeOf(AzTypeInfo<T>::Uuid()))
             {
 #ifdef AZ_ENABLE_TRACING
-                char assetDataIdGUIDStr[Uuid::MaxStringBuffer];
-                char assetTypeIdGUIDStr[Uuid::MaxStringBuffer];
-                assetData->GetId().m_guid.ToString(assetDataIdGUIDStr, AZ_ARRAY_SIZE(assetDataIdGUIDStr));
-                AzTypeInfo<T>::Uuid().ToString(assetTypeIdGUIDStr, AZ_ARRAY_SIZE(assetTypeIdGUIDStr));
-                AZ_Error("AssetDatabase", false, "Asset of type %s:%x (%s) is not related to %s (%s)!",
-                    assetData->GetType().ToString<AZStd::string>().c_str(), assetData->GetId().m_subId, assetDataIdGUIDStr,
-                    AzTypeInfo<T>::Name(), assetTypeIdGUIDStr);
+                AZ_Error("AssetDatabase", false, "Asset: %s TypeId: %s, is not related to Type: %s (%s)!"
+                    , assetData->GetId().ToFixedString().c_str()
+                    , assetData->GetType().ToFixedString().c_str()
+                    , AzTypeInfo<T>::Name()
+                    , AzTypeInfo<T>::Uuid().ToFixedString().c_str());
 #endif // AZ_ENABLE_TRACING
                 m_assetId = AssetId();
                 m_assetType = azrtti_typeid<T>();

Code/Framework/AzCore/AzCore/Math/Uuid.cpp

@@ -419,6 +419,11 @@ namespace AZ
         return CreateData(&mergedData, AZ_ARRAY_SIZE(mergedData));
     }
 
+    Uuid::FixedString Uuid::ToFixedString(bool isBrackets, bool isDashes) const
+    {
+        return ToString<FixedString>(isBrackets, isDashes);
+    }
+
 #if AZ_TRAIT_UUID_SUPPORTS_GUID_CONVERSION
     //=========================================================================
     // Uuid

Code/Framework/AzCore/AzCore/Math/Uuid.h

@@ -8,9 +8,10 @@
 
 #pragma once
 
+#include <AzCore/Math/MathUtils.h>
 #include <AzCore/base.h>
 #include <AzCore/std/hash.h>
-#include <AzCore/Math/MathUtils.h>
+#include <AzCore/std/string/fixed_string.h>
 
 #if AZ_TRAIT_UUID_SUPPORTS_GUID_CONVERSION
 struct  _GUID;
@@ -43,8 +44,8 @@ namespace AZ
         };
 
         static constexpr int ValidUuidStringLength = 32; /// Number of characters (data only, no extra formatting) in a valid UUID string
-        static const size_t MaxStringBuffer = 39; /// 32 Uuid + 4 dashes + 2 brackets + 1 terminate
-        
+        static constexpr size_t MaxStringBuffer = 39; /// 32 Uuid + 4 dashes + 2 brackets + 1 terminate
+        using FixedString = AZStd::fixed_string<MaxStringBuffer>;
         Uuid() = default;
         Uuid(const char* string, size_t stringLength = 0) { *this = CreateString(string, stringLength); }
 
@@ -120,6 +121,8 @@ namespace AZ
             ToString(&result[0], static_cast<int>(result.size()) + 1, isBrackets, isDashes);
         }
 
+        FixedString ToFixedString(bool isBrackets = true, bool isDashes = true) const;
+
         AZ_MATH_INLINE bool operator==(const Uuid& rhs) const
         {
             const AZ::u64* lhs64 = reinterpret_cast<const AZ::u64*>(data);

Code/Framework/AzCore/AzCore/RTTI/BehaviorContext.h

@@ -2948,11 +2948,7 @@ namespace AZ
         {
             if (classTypeIt != m_typeToClassMap.end())
             {
-                // class already reflected, display name and uuid
-                char uuidName[AZ::Uuid::MaxStringBuffer];
-                classTypeIt->first.ToString(uuidName, AZ::Uuid::MaxStringBuffer);
-
-                AZ_Error("Reflection", false, "Class '%s' is already registered using Uuid: %s!", name, uuidName);
+                AZ_Error("Reflection", false, "Class '%s' is already registered using Uuid: %s!", name, classTypeIt->first.ToFixedString().c_str());
                 return ClassBuilder<T>(this, static_cast<BehaviorClass*>(nullptr));
             }
 

Code/Framework/AzCore/AzCore/Serialization/Json/JsonSerializer.cpp

@@ -197,7 +197,7 @@ namespace AZ
         {
             return context.Report(Tasks::RetrieveInfo, Outcomes::Unknown,
                 AZStd::string::format("Failed to retrieve serialization information for type %s.",
-                    classElement.m_typeId.ToString<AZStd::fixed_string<AZ::Uuid::MaxStringBuffer>>().c_str()));
+                    classElement.m_typeId.ToFixedString().c_str()));
         }
         if (!elementClassData->m_azRtti)
         {

Code/Framework/AzCore/AzCore/Serialization/Json/PathSerializer.cpp

@@ -43,11 +43,10 @@ namespace AZ::JsonPathSerializerInternal
                 *pathValue = PathType(AZStd::string_view(inputValue.GetString(), pathLength)).LexicallyNormal();
                 return context.Report(JSR::Tasks::ReadField, JSR::Outcomes::Success, "Successfully read path.");
             }
-            using UuidString = AZStd::fixed_string<AZ::Uuid::MaxStringBuffer>;
             using ErrorString = AZStd::fixed_string<256>;
             return context.Report(JsonSerializationResult::Tasks::ReadField, JSR::Outcomes::Invalid,
                 ErrorString::format("Json string value is too large to fit within path type %s. It needs to be less than %zu code points",
-                    azrtti_typeid<PathType>().template ToString<UuidString>().c_str(), pathValue->Native().max_size()));
+                    azrtti_typeid<PathType>().ToFixedString().c_str(), pathValue->Native().max_size()));
         }
         default:
             return context.Report(JSR::Tasks::ReadField, JSR::Outcomes::Unknown, "Unknown json type encountered for string value.");
@@ -88,8 +87,7 @@ namespace AZ
                 context);
         }
 
-        using UuidString = AZStd::fixed_string<AZ::Uuid::MaxStringBuffer>;
-        auto errorTypeIdString = outputValueTypeId.ToString<UuidString>();
+        auto errorTypeIdString = outputValueTypeId.ToFixedString();
         AZ_Assert(false, "Unable to serialize json string"
             " to a path of type %s", errorTypeIdString.c_str());
 
@@ -114,14 +112,10 @@ namespace AZ
                 reinterpret_cast<const AZ::IO::FixedMaxPath*>(defaultValue), context);
         }
 
-        using UuidString = AZStd::fixed_string<AZ::Uuid::MaxStringBuffer>;
-        auto errorTypeIdString = valueTypeId.ToString<UuidString>();
-        AZ_Assert(false, "Unable to serialize path type %s to a json string",
-            errorTypeIdString.c_str());
-
+        AZ_Assert(false, "Unable to serialize path type %s to a json string", valueTypeId.ToFixedString().c_str());
         using ErrorString = AZStd::fixed_string<256>;
         return context.Report(JsonSerializationResult::Tasks::WriteValue, JsonSerializationResult::Outcomes::TypeMismatch,
-            ErrorString::format("Input value type ID %s is not a valid Path type", errorTypeIdString.c_str()));
+            ErrorString::format("Input value type ID %s is not a valid Path type", valueTypeId.ToFixedString().c_str()));
     }
 
 } // namespace AZ

Code/Framework/AzCore/AzCore/Serialization/ObjectStream.cpp

@@ -1651,7 +1651,7 @@ namespace AZ
                         // So use the classTypeId instead
                         AZ_UNUSED(classTypeId);
                         AZ_Error("Serialize", false, "CloseElement is attempted to be called without a corresponding WriteElement when writing class %s",
-                            classTypeId.ToString<AZStd::fixed_string<AZ::TypeId::MaxStringBuffer>>().c_str());
+                            classTypeId.ToFixedString().c_str());
                         return true;
                     }
                     if (m_writeElementResultStack.back())

Code/Framework/AzCore/AzCore/Serialization/ObjectStream.h

@@ -236,9 +236,8 @@ namespace AZ
             classData = genericClassInfo ? genericClassInfo->GetClassData() : nullptr;
             if (classData)
             {
-                char uuidStr[Uuid::MaxStringBuffer];
-                SerializeGenericTypeInfo<T>::GetClassTypeId().ToString(uuidStr, Uuid::MaxStringBuffer, false);
-                AZ_Error("Serializer", false, "Serialization of generic type (%s,%s) or a derivative as root element is not supported!!", classData->m_name, uuidStr);
+                AZ_Error("Serializer", false, "Serialization of generic type (%s,%s) or a derivative as root element is not supported!!"
+                    , classData->m_name, SerializeGenericTypeInfo<T>::GetClassTypeId().ToFixedString().c_str());
             }
             else
             {

Code/Framework/AzCore/Tests/Asset/AssetCommon.cpp

@@ -73,6 +73,22 @@ namespace UnitTest
         ASSERT_FALSE(left < right);
     }
 
+    TEST_F(AssetIdTest, ToFixedString_ResultIsAccurate_Succeeds)
+    {
+        AssetId id("{A9F596D7-9913-4BA4-AD4E-7E477FB9B542}", 0xFEDC1234);
+        const AZStd::string dynamic = id.ToString<AZStd::string>(AZ::Data::AssetId::SubIdDisplayType::Hex);
+        const AssetId::FixedString fixed = id.ToFixedString();
+        EXPECT_STREQ(dynamic.c_str(), fixed.c_str());
+    }
+
+    TEST_F(AssetIdTest, ToFixedString_FormatSpecifier_Succeeds)
+    {
+        AssetId source("{A9F596D7-9913-4BA4-AD4E-7E477FB9B542}", 0xFEDC1234);
+        const AZStd::string dynamic = AZStd::string::format("%s", source.ToString<AZStd::string>().c_str());
+        const AZStd::string fixed = AZStd::string::format("%s", source.ToFixedString().c_str());
+        EXPECT_EQ(dynamic, fixed);
+    }
+
     using AssetTest = AllocatorsFixture;
 
     TEST_F(AssetTest, AssetPreserveHintTest_Const_Copy)

Code/Framework/AzCore/Tests/UUIDTests.cpp

@@ -242,7 +242,7 @@ namespace UnitTest
     {
         Uuid left = Uuid::CreateNull();
 
-        // The below check should just give an empty uuid due to the g
+        // The below check should just give an empty uuid due to the 'g' 
         const char permissiveStr1[] = "{CCF8AB1E- gA04A-43D1-AD8A-70725BC3392E}";
         Uuid right = Uuid::CreateStringPermissive(permissiveStr1);
         EXPECT_EQ(left, right);
@@ -270,4 +270,42 @@ namespace UnitTest
         Uuid right = Uuid::CreateStringPermissive(permissiveStr);
         EXPECT_EQ(left, right);
     }
+
+    TEST_F(UuidTests, ToFixedString_ResultIsAccurate_Succeeds)
+    {
+        {
+            const char uuidStr[] = "{34D44249-E599-4B30-811F-4215C2DEA269}";
+            const Uuid source = Uuid::CreateString(uuidStr);
+            const AZStd::string dynamic = source.ToString<AZStd::string>();
+            const Uuid::FixedString fixed = source.ToFixedString();
+            EXPECT_STREQ(dynamic.c_str(), fixed.c_str());
+        }
+
+        {
+            const char uuidStr[] = "{678EFGBA-E599-4B30-811F-77775555AAFF}";
+            const Uuid source = Uuid::CreateString(uuidStr);
+            const AZStd::string dynamic = source.ToString<AZStd::string>();
+            const Uuid::FixedString fixed = source.ToFixedString();
+            EXPECT_STREQ(dynamic.c_str(), fixed.c_str());
+        }
+    }
+
+    TEST_F(UuidTests, ToFixedString_FormatSpecifier_Succeeds)
+    {
+        {
+            const char uuidStr[] = "{34D44249-E599-4B30-811F-4215C2DEA269}";
+            const Uuid source = Uuid::CreateString(uuidStr);
+            const AZStd::string dynamic = AZStd::string::format("%s", source.ToString<AZStd::string>().c_str());
+            const AZStd::string fixed = AZStd::string::format("%s", source.ToFixedString().c_str());
+            EXPECT_EQ(dynamic, fixed);
+        }
+
+        {
+            const char uuidStr[] = "{678EFGBA-E599-4B30-811F-77775555AAFF}";
+            const Uuid source = Uuid::CreateString(uuidStr);
+            const AZStd::string dynamic = AZStd::string::format("%s", source.ToString<AZStd::string>().c_str());
+            const AZStd::string fixed = AZStd::string::format("%s", source.ToFixedString().c_str());
+            EXPECT_EQ(dynamic, fixed);
+        }
+    }
 }

Code/Framework/AzTest/AzTest/Platform/Windows/ScopedAutoTempDirectory_Windows.cpp

@@ -18,7 +18,6 @@ namespace AZ::Test
 {
     ScopedAutoTempDirectory::ScopedAutoTempDirectory()
     {
-        using UuidString = AZStd::fixed_string<AZ::Uuid::MaxStringBuffer>;
         constexpr DWORD bufferSize = static_cast<DWORD>(AZ::IO::MaxPathLength);
 
         wchar_t tempDirW[AZ::IO::MaxPathLength]{};
@@ -32,7 +31,7 @@ namespace AZ::Test
         {
             AZ::IO::FixedMaxPath testPath = tempDirectoryRoot /
                 AZ::IO::FixedMaxPathString::format("UnitTest-%s",
-                    AZ::Uuid::CreateRandom().ToString<UuidString>().c_str());
+                    AZ::Uuid::CreateRandom().ToFixedString().c_str());
             // Try to create the temp directory if it doesn't exist
             if (!AZ::IO::SystemFile::Exists(testPath.c_str()) && AZ::IO::SystemFile::CreateDir(testPath.c_str()))
             {

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Spawnable/InMemorySpawnableAssetContainer.cpp

@@ -260,8 +260,8 @@ namespace AzToolsFramework::Prefab::PrefabConversionUtils
                     {
                         AZ_Error(
                             "Prefab", false, "Failed to queue asset '%s' (%s) of type '%s' for loading while entering game mode.",
-                            asset->GetHint().c_str(), asset->GetId().ToString<AZStd::fixed_string<AZ::Uuid::MaxStringBuffer>>().c_str(),
-                            asset->GetType().ToString<AZStd::fixed_string<AZ::Uuid::MaxStringBuffer>>().c_str());
+                            asset->GetHint().c_str(), asset->GetId().ToFixedString().c_str(),
+                            asset->GetType().ToFixedString().c_str());
                         return false;
                     }
 
@@ -285,8 +285,8 @@ namespace AzToolsFramework::Prefab::PrefabConversionUtils
             {
                 AZ_Error(
                     "Prefab", false, "Asset '%s' (%s) of type '%s' failed to preload while entering game mode", asset->GetHint().c_str(),
-                    asset->GetId().ToString<AZStd::fixed_string<AZ::Uuid::MaxStringBuffer>>().c_str(),
-                    asset->GetType().ToString<AZStd::fixed_string<AZ::Uuid::MaxStringBuffer>>().c_str());
+                    asset->GetId().ToFixedString().c_str(),
+                    asset->GetType().ToFixedString().c_str());
 
                 continue;
             }

Code/Tools/AssetProcessor/native/utilities/PlatformConfiguration.cpp

@@ -1666,9 +1666,7 @@ namespace AssetProcessor
                 QString gemAbsolutePath = QString::fromUtf8(absoluteSourcePath.c_str(), aznumeric_cast<int>(absoluteSourcePath.Native().size())); // this is an absolute path!
                 // Append the index of the source path array element to make a unique portable key is created for each path of a gem
                 AZ::Uuid gemNameUuid = AZ::Uuid::CreateName((gemElement.m_gemName + AZStd::to_string(sourcePathIndex)).c_str());
-                char gemNameToUuidBuffer[AZ::Uuid::MaxStringBuffer];
-                gemNameUuid.ToString(gemNameToUuidBuffer);
-                QString gemNameAsUuid(gemNameToUuidBuffer);
+                QString gemNameAsUuid(gemNameUuid.ToFixedString().c_str());
 
                 QDir gemDir(gemAbsolutePath);
 

Gems/EditorPythonBindings/Code/Tests/PythonLogSymbolsComponentTests.cpp

@@ -167,19 +167,12 @@ namespace UnitTest
             return AZStd::string::format(AZ_STRING_FORMAT, AZ_STRING_ARG(s));
         };
 
-        auto uuidHelper = [](const AZ::Uuid& uuid)
-        {
-            char buffer[AZ::Uuid::MaxStringBuffer];
-            uuid.ToString(buffer, AZ::Uuid::MaxStringBuffer, true, true);
-            return AZStd::string(buffer);
-        };
-
         for (auto& typeInfo : typesToTest)
         {
             AZStd::string_view result = pythonLogSymbolsComponent.FetchPythonTypeAndTraitsWrapper(AZStd::get<0>(typeInfo), AZStd::get<1>(typeInfo));
             EXPECT_EQ(result, AZStd::get<2>(typeInfo))
                 << "Expected '" << stringViewHelper(AZStd::get<2>(typeInfo)).c_str()
-                << "' when converting type with id " << uuidHelper(AZStd::get<0>(typeInfo)).c_str()
+                << "' when converting type with id " << AZStd::get<0>(typeInfo).ToFixedString().c_str()
                 << " but got '" << stringViewHelper(result).c_str() << "'.";
         }
     }

Gems/Maestro/Code/Source/Cinematics/AnimComponentNode.cpp

@@ -578,11 +578,8 @@ void CAnimComponentNode::Serialize(XmlNodeRef& xmlNode, bool bLoading, bool bLoa
     else
     {
         // saving
-        char uuidStringBuf[AZ::Uuid::MaxStringBuffer] = { 0 };
-
         xmlNode->setAttr("ComponentId", m_componentId);
-        m_componentTypeId.ToString(uuidStringBuf, AZ::Uuid::MaxStringBuffer);
-        xmlNode->setAttr("ComponentTypeId", uuidStringBuf);
+        xmlNode->setAttr("ComponentTypeId", m_componentTypeId.ToFixedString().c_str());
     }
 }