Skip to content

Conversation

AndyButland
Copy link
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

Resolves: #19784

Description

This PR adds an option to log rather than throw when encountering an exception deserializating block values, that can be used to avoid an exception when retrieving references.

We have had cases where content is being migrated to a new data type via uSync or Deploy, where the parsed content is in a format for nested content rather than block list, and this then blows up, preventing the migration. It happens when retrieving references, which isn't essential to the operation, so it's reasonable to to just skip values that can't be parsed into an excepted format, and from where references can be obtained.

Testing

Testing is tricky, so it maybe that visual inspection is enough here. But I've been able to trigger the problem from a clean install with this setup, and verified that after this update the migration succeeds:

  • Install Umbraco.Deploy.OnPrem.
  • Create standard doctype A and element type B.
  • On doctype A, create a nested content property which allows adding elements of type B.
  • Create an instance of A with some nested content.
  • Make a change to the content item and save without publishing.
  • Export the content instance along with the related schema to a zip file using the Deploy export feature.
  • Delete the content, nested content data type and document types.
  • Configure the Deploy migrator for nested content to block list with:
using Umbraco.Cms.Core.Composing;
using Umbraco.Deploy.Infrastructure.Migrators;

internal class DeployImportMigrationComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.DeployArtifactMigrators()
            .Append<ReplaceNestedContentDataTypeArtifactMigrator>();

        builder.DeployPropertyTypeMigrators()
            .Append<NestedContentPropertyTypeMigrator>();
    }
}
  • Import the content and schema.

@Copilot Copilot AI review requested due to automatic review settings September 4, 2025 12:37
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds error handling to block editor value deserialization to avoid throwing exceptions when retrieving references during content migration. The change allows the system to gracefully handle malformed data when migrating from nested content to block list formats.

Key changes:

  • Added optional throwOnError parameter to block editor deserialization methods
  • Wrapped deserialization in try-catch to log warnings instead of throwing exceptions when throwOnError is false
  • Modified reference retrieval to use non-throwing mode to allow partial parsing of valid data

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Umbraco.Infrastructure/PropertyEditors/BlockEditorValues.cs Added error handling with optional logging to deserialization method
src/Umbraco.Infrastructure/PropertyEditors/BlockEditorPropertyValueEditor.cs Modified reference retrieval to use non-throwing mode for graceful migration handling

Copy link
Contributor

@ronaldbarendse ronaldbarendse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and would indeed make sure the property data can be updated/migrated (see umbraco/Umbraco.Deploy.Issues#270).

However, updating the relations is still done in ContentRepositoryBase.PersistRelations() that is called on each and every save operation (for documents, media and members): this should be moved into a notification handler (inheriting from IDistributedNotificationHandler to ensure it's also handled after Deploy operations).

Besides the performance overhead of parsing all property values, fetching related entities and updating the relations when an item is saved multiple times in a single scope/transaction, the timing is also not optimal. If a content item is transferred to another environment using Deploy, any missing media will be transferred as well, but if the content is saved before the media was created, the relations won't be created. This can be fixed by re-saving the content again after the deployment is completed, but that just exaggerates the performance issue. Because notifications (published on the scope) are handled after the scope/transaction is completed, this also ensures all related items exists. I'll create a separate issue for this, so this fix can be released as soon as possible (as it blocks customers from migrating to later majors).

@kjac
Copy link
Contributor

kjac commented Sep 16, 2025

@AndyButland @ronaldbarendse I have moved the exception handling to the consumer side, so the changes remain isolated inside BlockEditorPropertyValueEditor - introducing a SafeParseBlockEditorData() method to safely parse editor values. This also replaces similar logic in BlockEditorPropertyValueEditor.FromEditor() and BlockEditorPropertyValueEditor.ToEditor().

@AndyButland
Copy link
Contributor Author

Looks fine to me - and better if we align other "try/catch" checks already there for the editor value transformations.

@AndyButland AndyButland enabled auto-merge (squash) September 16, 2025 09:56
@AndyButland AndyButland merged commit e119186 into v13/dev Sep 16, 2025
19 checks passed
@AndyButland AndyButland deleted the v13/bugfix/avoid-exception-on-get-references-after-migration branch September 16, 2025 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BlockEditorValues.DeserializeAndClean errors not handled when PersistRelations is called
3 participants