Skip to content

Adds XML header docs indicating usage of options on NuCacheSerializerType #19555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

AndyButland
Copy link
Contributor

Prerequisites

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

Relates to #19544

Description

We currently offer a legacy configuration option for cache serialization that's no longer supported since the introduction of the hybrid cache implementation, and has some issues with particular property types such as integers.

To make it work changes would be needed to property type converters, e.g.

public override object ConvertSourceToIntermediate(
    IPublishedElement owner,
    IPublishedPropertyType propertyType,
    object? source,
    bool preview)
{
    if (source is JsonElement jsonElementSource && jsonElementSource.ValueKind == JsonValueKind.Number)
    {
        return jsonElementSource.GetInt32();
    }

    return source.TryConvertTo<int>().Result;
}

I considered that as there's no reason to use this option over the faster and more compact MessagePack, we would obsolete the options and schedules the removal in 17.

However we have some integration tests that rely on it for assertions. Possibly we could make those work with MessagePack, but it didn't seem straightforward. Also this got me thinking that the one benefit of this option is that it's more readable, so given that it's worth keeping around for these tests (and potentially others in future).

As such I've limited changes to just providing more information on appropriate usage.

@Copilot Copilot AI review requested due to automatic review settings June 13, 2025 12:03
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 updates the XML documentation for NuCacheSerializerType to clarify its usage and deprecation status for the legacy JSON serializer.

  • Updates XML header docs for both MessagePack and JSON options
  • Clarifies that JSON is maintained only for testing readability purposes
Comments suppressed due to low confidence (2)

src/Umbraco.Core/Configuration/Models/NuCacheSerializerType.cs:7

  • [nitpick] Verify that the updated terminology 'published content cache' is consistent across the codebase to avoid confusion with legacy nomenclature.
///     The serializer type that the published content cache uses to persist documents in the database.

src/Umbraco.Core/Configuration/Models/NuCacheSerializerType.cs:19

  • Consider adding an explicit [Obsolete] attribute to the JSON serializer to signal its planned removal in future releases.
/// JSON = 2,

@AndyButland AndyButland changed the title Adds obsoletion message and XML header docs to NuCacheSerializerType. Adds XML header docs indicating usage of options on NuCacheSerializerType. Jun 13, 2025
@AndyButland AndyButland changed the title Adds XML header docs indicating usage of options on NuCacheSerializerType. Adds XML header docs indicating usage of options on NuCacheSerializerType Jun 13, 2025
Copy link
Contributor

@lauraneto lauraneto left a comment

Choose a reason for hiding this comment

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

Approved!
We should also update the docs with this information.

@AndyButland
Copy link
Contributor Author

PR for docs is here: umbraco/UmbracoDocs#7203

@AndyButland AndyButland merged commit c223d93 into main Jun 30, 2025
26 checks passed
@AndyButland AndyButland deleted the v16/task/obsolete-json-option-for-cache-serialization branch June 30, 2025 05:53
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.

2 participants