Prefabs | Introduce sanitation of prefab doms on loading (#1929)

* Update PrefabLoader to sanitize ingested prefabs and have core systems operate with default values

Signed-off-by: sconel <sconel@amazon.com>
Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com>

* Remove IsExplicitDefault implementation to avoid confusion (since the function isn't virtual).
Avoid copying PrefabDoms over in SanitizeLoadTemplate.

Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com>

* Address naming and commenting concerns from PR reviews.

Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com>

* Fix to error detection code

Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com>

* Add support for all uuid formats for zero check.

Signed-off-by: Danilo Aimini <82231674+AMZN-daimini@users.noreply.github.com>

Co-authored-by: sconel <sconel@amazon.com>

Code/Framework/AzCore/AzCore/Math/UuidSerializer.cpp

@@ -23,6 +23,8 @@ namespace AZ
     }
 
     JsonUuidSerializer::JsonUuidSerializer()
+        : m_zeroUuidString(AZ::Uuid::CreateNull().ToString<AZStd::string>())
+        , m_zeroUuidStringNoDashes(AZ::Uuid::CreateNull().ToString<AZStd::string>(true, false))
     {
         m_uuidFormat = AZStd::regex(R"(\{[a-f0-9]{8}-?[a-f0-9]{4}-?[a-f0-9]{4}-?[a-f0-9]{4}-?[a-f0-9]{12}\})",
             AZStd::regex_constants::icase | AZStd::regex_constants::optimize);
@@ -53,7 +55,7 @@ namespace AZ
 
         Uuid* valAsUuid = reinterpret_cast<Uuid*>(outputValue);
 
-        if (IsExplicitDefault(inputValue))
+        if (IsExplicitDefault(inputValue) || (inputValue == m_zeroUuidString.c_str()) || (inputValue == m_zeroUuidStringNoDashes.c_str()))
         {
             *valAsUuid = AZ::Uuid::CreateNull();
             return MessageResult("Uuid value set to default of null.", JSR::ResultCode(JSR::Tasks::ReadField, JSR::Outcomes::DefaultsUsed));

Code/Framework/AzCore/AzCore/Math/UuidSerializer.h

@@ -47,6 +47,8 @@ namespace AZ
             const Uuid& valueTypeId, JsonSerializerContext& context);
 
     private:
+        const AZStd::string m_zeroUuidString;
+        const AZStd::string m_zeroUuidStringNoDashes;
         AZStd::regex m_uuidFormat;
     };
 } // namespace AZ

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.cpp

@@ -47,7 +47,7 @@ namespace AzToolsFramework
                 return valueIterator->value;
             }
 
-            bool StoreInstanceInPrefabDom(const Instance& instance, PrefabDom& prefabDom)
+            bool StoreInstanceInPrefabDom(const Instance& instance, PrefabDom& prefabDom, StoreInstanceFlags flags)
             {
                 InstanceEntityIdMapper entityIdMapper;
                 entityIdMapper.SetStoringInstance(instance);
@@ -58,6 +58,11 @@ namespace AzToolsFramework
                 settings.m_metadata.Add(static_cast<AZ::JsonEntityIdSerializer::JsonEntityIdMapper*>(&entityIdMapper));
                 settings.m_metadata.Add(&entityIdMapper);
 
+                if ((flags & StoreInstanceFlags::StripDefaultValues) != StoreInstanceFlags::StripDefaultValues)
+                {
+                    settings.m_keepDefaults = true;
+                }
+
                 AZ::JsonSerializationResult::ResultCode result =
                     AZ::JsonSerialization::Store(prefabDom, prefabDom.GetAllocator(), instance, settings);
 

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabDomUtils.h

@@ -36,13 +36,25 @@ namespace AzToolsFramework
             PrefabDomValueReference FindPrefabDomValue(PrefabDomValue& parentValue, const char* valueName);
             PrefabDomValueConstReference FindPrefabDomValue(const PrefabDomValue& parentValue, const char* valueName);
 
+            enum class StoreInstanceFlags : uint8_t
+            {
+                //! No flags used during the call to LoadInstanceFromPrefabDom.
+                None = 0,
+
+                //! By default an instance will be stored with default values. In cases where we want to store less json without defaults
+                //! such as saving to disk, this flag will control that behavior.
+                StripDefaultValues = 1 << 0
+            };
+            AZ_DEFINE_ENUM_BITWISE_OPERATORS(StoreInstanceFlags);
+
             /**
             * Stores a valid Prefab Instance within a Prefab Dom. Useful for generating Templates
             * @param instance The instance to store
             * @param prefabDom The prefabDom that will be used to store the Instance data
+            * @param flags Controls behavior such as whether to store default values
             * @return bool on whether the operation succeeded
             */
-            bool StoreInstanceInPrefabDom(const Instance& instance, PrefabDom& prefabDom);
+            bool StoreInstanceInPrefabDom(const Instance& instance, PrefabDom& prefabDom, StoreInstanceFlags flags = StoreInstanceFlags::None);
 
             enum class LoadInstanceFlags : uint8_t
             {
@@ -52,13 +64,13 @@ namespace AzToolsFramework
                 //! unique, e.g. when they are duplicates of live entities, this flag will assign them a random new id.
                 AssignRandomEntityId = 1 << 0
             };
-            AZ_DEFINE_ENUM_BITWISE_OPERATORS(LoadInstanceFlags)
+            AZ_DEFINE_ENUM_BITWISE_OPERATORS(LoadInstanceFlags);
 
             /**
             * Loads a valid Prefab Instance from a Prefab Dom. Useful for generating Instances.
             * @param instance The Instance to load.
             * @param prefabDom The prefabDom that will be used to load the Instance data.
-            * @param shouldClearContainers Whether to clear containers in Instance while loading.
+            * @param flags Controls behavior such as random entity id assignment.
             * @return bool on whether the operation succeeded.
             */
             bool LoadInstanceFromPrefabDom(

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.cpp

@@ -172,7 +172,7 @@ namespace AzToolsFramework
             progressedFilePathsSet.emplace(relativePath);
 
             // Get 'Instances' value from Template.
-            bool isLoadedWithErrors = false;
+            bool isLoadSuccessful = true;
             PrefabDomValueReference instancesReference = newTemplate.GetInstancesValue();
             if (instancesReference.has_value())
             {
@@ -185,7 +185,7 @@ namespace AzToolsFramework
                 {
                     if (!LoadNestedInstance(instanceIterator, newTemplateId, progressedFilePathsSet))
                     {
-                        isLoadedWithErrors = true;
+                        isLoadSuccessful = false;
                         AZ_Error(
                             "Prefab", false,
                             "PrefabLoader::LoadTemplate - "
@@ -196,7 +196,10 @@ namespace AzToolsFramework
                     }
                 }
             }
-            newTemplate.MarkAsLoadedWithErrors(isLoadedWithErrors);
+
+            isLoadSuccessful &= SanitizeLoadedTemplate(newTemplate.GetPrefabDom());
+
+            newTemplate.MarkAsLoadedWithErrors(!isLoadSuccessful);
 
             // Un-mark the file as being in progress.
             progressedFilePathsSet.erase(originPath);
@@ -277,6 +280,63 @@ namespace AzToolsFramework
             return !nestedTemplateReference->get().IsLoadedWithErrors();
         }
 
+        bool PrefabLoader::SanitizeLoadedTemplate(PrefabDomReference loadedTemplateDom)
+        {
+            // Prefabs are stored to disk with default values stripped. However, while in memory, we need those default values to be
+            // present to make patches work consistently. To accomplish this, we'll instantiate the Dom, then serialize the instance
+            // back into a Dom with all of the default values preserved.
+            // Note that this is the default behavior in Prefab serialization, so we don't need to specify StoreInstanceFlags.
+
+            if (!loadedTemplateDom)
+            {
+                return false;
+            }
+
+            Instance loadedPrefabInstance;
+            if (!PrefabDomUtils::LoadInstanceFromPrefabDom(loadedPrefabInstance, loadedTemplateDom->get()))
+            {
+                return false;
+            }
+
+            PrefabDom storedPrefabDom(&loadedTemplateDom->get().GetAllocator());
+            if (!PrefabDomUtils::StoreInstanceInPrefabDom(loadedPrefabInstance, storedPrefabDom))
+            {
+                return false;
+            }
+
+            loadedTemplateDom->get().CopyFrom(storedPrefabDom, loadedTemplateDom->get().GetAllocator());
+            return true;
+        }
+
+        bool PrefabLoader::SanitizeSavingTemplate(PrefabDomReference savingTemplateDom)
+        {
+            // Prefabs are stored in memory with default values spelled out to make patches work consistently. However, when we store them
+            // to disk, we strip those default values to save on file size. To accomplish this, we'll instantiate the Dom, then serialize
+            // the instance back into a Dom with all of the default values stripped.
+
+            if (!savingTemplateDom)
+            {
+                return false;
+            }
+
+            Instance savingPrefabInstance;
+            if (!PrefabDomUtils::LoadInstanceFromPrefabDom(savingPrefabInstance, savingTemplateDom->get()))
+            {
+                return false;
+            }
+
+            PrefabDom storedPrefabDom(&savingTemplateDom->get().GetAllocator());
+            if (!PrefabDomUtils::StoreInstanceInPrefabDom(savingPrefabInstance, storedPrefabDom,
+                PrefabDomUtils::StoreInstanceFlags::StripDefaultValues))
+            {
+                return false;
+            }
+
+            savingTemplateDom->get().CopyFrom(storedPrefabDom, savingTemplateDom->get().GetAllocator());
+
+            return true;
+        }
+
         bool PrefabLoader::SaveTemplate(TemplateId templateId)
         {
             const auto& domAndFilepath = StoreTemplateIntoFileFormat(templateId);
@@ -395,7 +455,7 @@ namespace AzToolsFramework
 
             // Make a copy of a our prefab DOM where nested instances become file references with patch data
             PrefabDom templateDomToSave;
-            if (!templateToSave.CopyTemplateIntoPrefabFileFormat(templateDomToSave))
+            if (!CopyTemplateIntoPrefabFileFormat(templateToSave, templateDomToSave))
             {
                 AZ_Error(
                     "Prefab", false,
@@ -410,6 +470,80 @@ namespace AzToolsFramework
             return { { AZStd::move(templateDomToSave), templateToSave.GetFilePath() } };
         }
 
+        bool PrefabLoader::CopyTemplateIntoPrefabFileFormat(TemplateReference templateRef, PrefabDom& output)
+        {
+            AZ_Assert(
+                templateRef.has_value(),
+                "CopyTemplateIntoPrefabFileFormat called on empty template reference."
+            );
+
+            PrefabDom& prefabDom = templateRef->get().GetPrefabDom();
+
+            // Start by making a copy of our dom
+            output.CopyFrom(prefabDom, prefabDom.GetAllocator());
+
+            SanitizeSavingTemplate(output);
+
+            for (const LinkId& linkId : templateRef->get().GetLinks())
+            {
+                AZStd::optional<AZStd::reference_wrapper<Link>> findLinkResult = m_prefabSystemComponentInterface->FindLink(linkId);
+
+                if (!findLinkResult.has_value())
+                {
+                    AZ_Error(
+                        "Prefab", false,
+                        "Link with id %llu could not be found while attempting to store "
+                        "Prefab Template with source path %s in Prefab File format. "
+                        "Unable to proceed.",
+                        linkId, templateRef->get().GetFilePath().c_str());
+
+                    return false;
+                }
+
+                if (!findLinkResult->get().IsValid())
+                {
+                    AZ_Error(
+                        "Prefab", false,
+                        "Link with id %llu and is invalid during attempt to store "
+                        "Prefab Template with source path %s in Prefab File format. "
+                        "Unable to Proceed.",
+                        linkId, templateRef->get().GetFilePath().c_str());
+
+                    return false;
+                }
+
+                Link& link = findLinkResult->get();
+
+                PrefabDomPath instancePath = link.GetInstancePath();
+                PrefabDom& linkDom = link.GetLinkDom();
+
+                // Get the instance value of the Template copy
+                // This currently stores a fully realized nested Template Dom
+                PrefabDomValue* instanceValue = instancePath.Get(output);
+
+                if (!instanceValue)
+                {
+                    AZ_Error(
+                        "Prefab", false,
+                        "Template::CopyTemplateIntoPrefabFileFormat: Unable to recover nested instance Dom value from link with id %llu "
+                        "while attempting to store a collapsed version of a Prefab Template with source path %s. Unable to proceed.",
+                        linkId, templateRef->get().GetFilePath().c_str());
+
+                    return false;
+                }
+
+                // Copy the contents of the Link to overwrite our Template Dom copies Instance
+                // The instance is now "collapsed" as it contains the file reference and patches from the link
+                instanceValue->CopyFrom(linkDom, prefabDom.GetAllocator());
+            }
+
+            // Remove Source parameter from the dom. It will be added on file load, and should not be stored to disk.
+            PrefabDomPath sourcePath = PrefabDomPath((AZStd::string("/") + PrefabDomUtils::SourceName).c_str());
+            sourcePath.Erase(output);
+
+            return true;
+        }
+
         bool PrefabLoader::IsValidPrefabPath(AZ::IO::PathView path)
         {
             // Check for OS invalid character and paths ending on '/' '\\' separators as final char

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.h

@@ -25,6 +25,8 @@ namespace AzToolsFramework
     namespace Prefab
     {
         class PrefabSystemComponentInterface;
+        class Template;
+        using TemplateReference = AZStd::optional<AZStd::reference_wrapper<Template>>;
 
         /**
         * The Prefab Loader helps saving/loading Prefab files.
@@ -106,6 +108,16 @@ namespace AzToolsFramework
             static bool IsValidPrefabPath(AZ::IO::PathView path);
 
         private:
+            /**
+             * Copies the template dom provided and manipulates it into the proper format to be saved to disk.
+             * @param templateRef The template whose dom we want to transform into the proper format to be saved to disk.
+             * @param[out] output The PrefabDom reference we want to store the result into.
+             * @return True if the operation was completed correctly, false otherwise.
+             */
+            bool CopyTemplateIntoPrefabFileFormat(
+                TemplateReference templateRef,
+                PrefabDom& output
+            );
 
             /**
              * Load Prefab Template from given file path to memory and return the id of loaded Template.
@@ -141,6 +153,20 @@ namespace AzToolsFramework
                 TemplateId targetTemplateId,
                 AZStd::unordered_set<AZ::IO::Path>& progressedFilePathsSet);
 
+            /*
+             * Manipulate the provided PrefabDom into the right format to be stored in memory for editor usage.
+             * @param loadedTemplateDom The template to manipulate. Changes will be applied in place.
+             * @return True if the manipulations where applied correctly, false otherwise.
+             */
+            bool SanitizeLoadedTemplate(PrefabDomReference loadedTemplateDom);
+            
+            /*
+             * Manipulate the provided PrefabDom into the right format to be stored to disk.
+             * @param savingTemplateDom The template to manipulate. Changes will be applied in place.
+             * @return True if the manipulations where applied correctly, false otherwise.
+             */
+            bool SanitizeSavingTemplate(PrefabDomReference savingTemplateDom);
+
             //! Retrieves Dom content and its path from a template id
             AZStd::optional<AZStd::pair<PrefabDom, AZ::IO::Path>> StoreTemplateIntoFileFormat(TemplateId templateId);
 

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabSystemComponent.cpp

@@ -631,13 +631,16 @@ namespace AzToolsFramework
                 //member itself, so we need to move instancesValue to the correct position for the next insert
                 memberFound = instancesValue->get().FindMember(PrefabDomUtils::InstancesName);
                 instancesValue = memberFound->value;
-                instancesValue->get().SetObject();
             }
             else
             {
                 instancesValue = memberFound->value;
             }
 
+            if (!instancesValue->get().IsObject())
+            {
+                instancesValue->get().SetObject();
+            }
             // Only add the instance if it's not there already
             if (instancesValue->get().FindMember(rapidjson::StringRef(instanceAlias.c_str())) == instancesValue->get().MemberEnd())
             {

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Template/Template.cpp

@@ -138,77 +138,6 @@ namespace AzToolsFramework
             return m_prefabDom;
         }
 
-        bool Template::CopyTemplateIntoPrefabFileFormat(PrefabDom& output)
-        {
-            // Start by making a copy of our dom
-            output.CopyFrom(m_prefabDom, m_prefabDom.GetAllocator());
-
-            PrefabSystemComponentInterface* prefabSystemComponentInterface =
-                AZ::Interface<PrefabSystemComponentInterface>::Get();
-
-            AZ_Assert(prefabSystemComponentInterface,
-                "Prefab - Prefab System Component Interface is null while attempting "
-                "to copy Template associated with Prefab file %s into Prefab File format",
-                m_filePath.c_str());
-
-            for (const LinkId& linkId : m_links)
-            {
-                AZStd::optional<AZStd::reference_wrapper<Link>> findLinkResult =
-                    prefabSystemComponentInterface->FindLink(linkId);
-
-                if (!findLinkResult.has_value())
-                {
-                    AZ_Error("Prefab", false,
-                        "Link with id %llu could not be found while attempting to store "
-                        "Prefab Template with source path %s in Prefab File format. "
-                        "Unable to proceed.",
-                        linkId, m_filePath.c_str());
-
-                    return false;
-                }
-
-                if (!findLinkResult->get().IsValid())
-                {
-                    AZ_Error("Prefab", false,
-                        "Link with id %llu and is invalid during attempt to store "
-                        "Prefab Template with source path %s in Prefab File format. "
-                        "Unable to Proceed.",
-                        linkId, m_filePath.c_str());
-
-                    return false;
-                }
-
-                Link& link = findLinkResult->get();
-
-                PrefabDomPath instancePath = link.GetInstancePath();
-                PrefabDom& linkDom = link.GetLinkDom();
-
-                // Get the instance value of the Template copy
-                // This currently stores a fully realized nested Template Dom
-                PrefabDomValue* instanceValue = instancePath.Get(output);
-
-                if (!instanceValue)
-                {
-                    AZ_Error("Prefab", false,
-                        "Template::CopyTemplateIntoPrefabFileFormat: Unable to recover nested instance Dom value from link with id %llu "
-                        "while attempting to store a collapsed version of a Prefab Template with source path %s. Unable to proceed.",
-                        linkId, m_filePath.c_str());
-
-                    return false;
-                }
-
-                // Copy the contents of the Link to overwrite our Template Dom copies Instance
-                // The instance is now "collapsed" as it contains the file reference and patches from the link
-                instanceValue->CopyFrom(linkDom, m_prefabDom.GetAllocator());
-            }
-
-            // Remove Source parameter from the dom. It will be added on file load, and should not be stored to disk.
-            PrefabDomPath sourcePath = PrefabDomPath((AZStd::string("/") + PrefabDomUtils::SourceName).c_str());
-            sourcePath.Erase(output);
-
-            return true;
-        }
-
         PrefabDomValueReference Template::GetInstancesValue()
         {
             if (!IsValid())

Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Template/Template.h

@@ -58,8 +58,6 @@ namespace AzToolsFramework
             PrefabDom& GetPrefabDom();
             const PrefabDom& GetPrefabDom() const;
 
-            bool CopyTemplateIntoPrefabFileFormat(PrefabDom& output);
-
             PrefabDomValueReference GetInstancesValue();
             PrefabDomValueConstReference GetInstancesValue() const;
 

Code/Framework/AzToolsFramework/Tests/Prefab/PrefabUpdateInstancesTests.cpp

@@ -428,8 +428,7 @@ namespace UnitTest
         ASSERT_TRUE(PrefabDomUtils::StoreInstanceInPrefabDom(*newInstance, updatedDom));
         newTemplateDom.CopyFrom(updatedDom, newTemplateDom.GetAllocator());
 
-        // Validate that the prefabTestComponent in the Template's DOM doesn't have a BoolProperty.
-        // Even though we changed the property to false, it won't be serialized out because it's a default value.
+        // Validate that the value of the BoolProperty of the prefabTestComponent in the Template's DOM has changed.
         entityComponents = PrefabTestDomUtils::GetPrefabDomComponents(newTemplateDom, newTemplateEntityAliases.front());
         ASSERT_TRUE(entityComponents != nullptr && entityComponents->IsObject());
         EXPECT_EQ(entityComponents->MemberCount(), 2);
@@ -440,7 +439,7 @@ namespace UnitTest
 
         PrefabDomValueConstReference wheelEntityComponentBoolPropertyValue =
             PrefabDomUtils::FindPrefabDomValue(wheelEntityComponentValue->get(), PrefabTestDomUtils::BoolPropertyName);
-        ASSERT_FALSE(wheelEntityComponentBoolPropertyValue.has_value());
+        ASSERT_TRUE(wheelEntityComponentBoolPropertyValue.has_value() && wheelEntityComponentBoolPropertyValue->get() == false);
 
         // Update Template's Instances and validate if all Instances have no BoolProperty under their prefabTestComponents in entities.
         m_instanceUpdateExecutorInterface->AddTemplateInstancesToQueue(newTemplateId);

Code/Framework/AzToolsFramework/Tests/Prefab/PrefabUpdateTemplateTests.cpp

@@ -145,7 +145,7 @@ namespace UnitTest
         EntityAlias entityAlias = wheelTemplateEntityAliases.front();
         PrefabDomValue* wheelEntityComponents =
             PrefabTestDomUtils::GetPrefabDomComponentsPath(entityAlias).Get(wheelTemplateDom);
-        ASSERT_TRUE(wheelEntityComponents == nullptr);
+        ASSERT_TRUE(wheelEntityComponents->IsArray() && wheelEntityComponents->Size() == 0);
 
         // Create an axle with 0 entities and 1 wheel instance.
         AZStd::unique_ptr<Instance> wheel1UnderAxle = m_prefabSystemComponent->InstantiatePrefab(wheelTemplateId);
@@ -340,7 +340,7 @@ namespace UnitTest
 
         // Validate that the wheel entity does not have a component under it.
         wheelEntityComponents = PrefabTestDomUtils::GetPrefabDomComponentsPath(entityAlias).Get(wheelTemplateDom);
-        ASSERT_TRUE(wheelEntityComponents == nullptr);
+        ASSERT_TRUE(wheelEntityComponents->IsArray() && wheelEntityComponents->Size() == 0);
 
         // Validate that the wheels under the axle have the same DOM as the wheel template.
         PrefabTestDomUtils::ValidatePrefabDomInstances(wheelInstanceAliasesUnderAxle, axleTemplateDom, wheelTemplateDom);
@@ -399,15 +399,14 @@ namespace UnitTest
         m_prefabSystemComponent->UpdatePrefabTemplate(wheelTemplateId, updatedWheelInstanceDom);
         m_instanceUpdateExecutorInterface->UpdateTemplateInstancesInQueue();
 
-        // Validate that the prefabTestComponent in the wheel template DOM doesn't have a BoolProperty.
-        // Even though we changed the property to false, it won't be serialized out because it's a default value.
+        // Validate that the BoolProperty of the prefabTestComponent in the wheel template DOM is set to false.
         wheelEntityComponents = PrefabTestDomUtils::GetPrefabDomComponentsPath(entityAlias).Get(wheelTemplateDom);
         ASSERT_TRUE(wheelEntityComponents != nullptr && wheelEntityComponents->IsObject());
         EXPECT_EQ(wheelEntityComponents->MemberCount(), 1);
 
         PrefabDomValueReference wheelEntityComponentBoolPropertyValue =
             PrefabDomUtils::FindPrefabDomValue(wheelEntityComponents->MemberBegin()->value, PrefabTestDomUtils::BoolPropertyName);
-        ASSERT_FALSE(wheelEntityComponentBoolPropertyValue.has_value());
+        ASSERT_TRUE(wheelEntityComponentBoolPropertyValue.has_value() && wheelEntityComponentBoolPropertyValue->get() == false);
 
         // Validate that the wheels under the axle have the same DOM as the wheel template.
         PrefabTestDomUtils::ValidatePrefabDomInstances(wheelInstanceAliasesUnderAxle, axleTemplateDom, wheelTemplateDom);