Skip to content

Ensure published querying parity between V13 and V17#22622

Merged
kjac merged 10 commits into
mainfrom
v17/bugfix/v13-v17-published-querying-partity
May 1, 2026
Merged

Ensure published querying parity between V13 and V17#22622
kjac merged 10 commits into
mainfrom
v17/bugfix/v13-v17-published-querying-partity

Conversation

@kjac

@kjac kjac commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Prerequisites

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

Description

I have noticed a few differences between V13 and V17 when querying published content in a culture variant context.

This PR ensures the two versions align in behavior.

Consider this structure:

image

All content is published in English and Danish, except "Child 2", which is only in published in Danish.

The rendered output differs between V13 and V17 in the English language. Since all content is published in Danish, the renderings are the same for Danish between V13 and V17.

The template used for testing can be found in the bottom of this PR.

Root, English

image

"Grandchild 2" is rendered in V17, despite its parent being unpublished in English.

Root, English, in preview node

image

In V13, "Child 2" and "Grandchild 2" are rendered as expected when viewing the root in preview. In V17, "Grandchild 2" continues to be rendered without its parent.

Grandchild 2, English, requested by ID/key

image

While it might seem unexpected, both V13 and V17 allows for rendering published content, even if an ancestor is unpublished, as long as it is permitted to resolve content by ID/key. This could potentially qualify as a bug, but that's for another PR.

The empty bullets in the V13 rendering represent "Child 2" in English. Since it's not published, it won't render out a name, but all the same it appears both in the ancestors collection, and as the parent of "Grandchild 2".

Grandchild 2, English, in preview

image

As expected, V13 renders out the name of "Child 2" in preview. V17, however, continues to disregard "Child 2" entirely.

Introducing obsolete code

This PR introduces the Unfiltered() method on IPublishedStatusFilteringService to avoid breaking the signature of a whole lot of published content extensions. This is a temporary means to an end, and the method has been marked as obsolete.

I will create a follow-up task to remove Unfiltered() for V19 (or later) if it is merged in. This does require a bit of planning, as it will break certain published content extensions.

Testing this PR

Use the provided test template (or something similar) to test various permutations of culture variance and published structure querying. The scenarios above can be used as inspiration.

Test template

Here's the template I have used for testing:

@using Umbraco.Cms.Web.Common.PublishedModels;
@inherits Umbraco.Cms.Web.Common.Views.UmbracoViewPage
@{
	Layout = null;
	var test = Model.Descendants().ToArray();
	var debug = true;
}
<html>
<body>
<fieldset>
<legend>Axis: Ancestors</legend>
<ul>
@foreach(var content in Model.Ancestors()) {
	<li>@content.Name</li>
}
</ul>
</fieldset>
<fieldset>
<legend>Axis: Parent</legend>
@if(Model.Parent is not null) {
<ul>
	<li>@Model.Parent.Name</li>
</ul>
}
</fieldset>
<fieldset>
<legend>Axis: Siblings</legend>
<ul>
@foreach(var content in Model.Siblings()) {
	<li>@content.Name</li>
}
</ul>
</fieldset>
<fieldset>
<legend>Axis: Children</legend>
<ul>
@foreach(var content in Model.Children()) {
	<li>@content.Name</li>
}
</ul>
</fieldset>
<fieldset>
<legend>Axis: Descendants</legend>
<ul>
@foreach(var content in Model.Descendants()) {
	<li>@content.Name</li>
}
</ul>
</fieldset>
</body>
</html>

@kjac kjac marked this pull request as ready for review April 28, 2026 05:18
@claude

claude Bot commented Apr 28, 2026

Copy link
Copy Markdown

Claude finished @kjac's task in 4m 55s —— View job


PR Review

Target: origin/main · Based on commit: b3a9948 · Skipped: 0 noise files out of 7 total

Fixes culture-specific ancestor path filtering in FilterAvailable (was checking any-culture instead of the requested culture) and switches parent/ancestor resolution to bypass publish filtering, aligning V17 behavior with V13 for culture-variant content trees.

  • Modified public API: IPublishStatusQueryService (new overload HasPublishedAncestorPath(Guid, string)); IPublishedStatusFilteringService (new internal Unfiltered method)
  • Affected implementations (outside this PR): DocumentCacheService, ContentTypeIndexingNotificationHandler, DeferredSearchReindexService — all use the old HasPublishedAncestorPath(Guid) overload (culture-agnostic), unchanged and unaffected
  • Other changes: Parent and Ancestors() extension methods now return unpublished items (bypassing publish/culture filtering), matching V13 behavior — callers relying on these yielding only published-in-culture content will see different results

Critical

  • src/Umbraco.Core/Services/PublishStatus/IPublishStatusQueryService.cs:36: HasPublishedAncestorPath(Guid, string) is added to a public interface with no default implementation — any plugin/package implementing IPublishStatusQueryService will fail to compile. Per Pattern 3 in the repo conventions, a default implementation is required. A safe fallback delegates to the existing culture-agnostic overload:
    // TODO (V18): Remove the default implementation and require explicit implementation.
    bool HasPublishedAncestorPath(Guid documentKey, string culture)
        => HasPublishedAncestorPath(documentKey);
    (False-positive risk at worst — ancestor treated as published when only checked culture-agnostically — rather than a compile break.)

Important

  • src/Umbraco.Core/Services/PublishStatus/IPublishedStatusFilteringService.cs:23: [Obsolete] message has two issues: "Scheduled for removed" should be "Scheduled for removal", and "V20" should be "Umbraco 19" (current major 17 + 2 = 19, per the repo's [Obsolete] format convention).

Request Changes

The missing default implementation on IPublishStatusQueryService.HasPublishedAncestorPath(Guid, string) is a binary breaking change and must be addressed before merge. The [Obsolete] wording fix is a quick cleanup that can go in the same commit.

The behavior fix itself (culture-specific ancestor path checking, Unfiltered for parent/ancestor resolution) is well-reasoned, clearly explained, and the new unit tests cover the relevant cases cleanly.

Comment thread src/Umbraco.Core/Services/PublishStatus/IPublishStatusQueryService.cs Outdated
Comment thread src/Umbraco.Core/Services/PublishStatus/IPublishedStatusFilteringService.cs Outdated
@claude claude Bot added the area/backend label Apr 28, 2026

@AndyButland AndyButland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code-wise looks good @kjac, and a very smart way to handle avoiding breaking changes. I'll give it a test out later this morning, but first just sharing feedback on the code.

Comment thread src/Umbraco.Core/Services/PublishStatus/IPublishedStatusFilteringService.cs Outdated
Comment thread src/Umbraco.Core/Services/PublishStatus/PublishedContentStatusFilteringService.cs Outdated
Comment thread src/Umbraco.Core/Extensions/PublishedContentExtensions.cs
Comment thread src/Umbraco.Core/Services/PublishStatus/PublishStatusService.cs
@AndyButland

Copy link
Copy Markdown
Contributor

Here's what I found in testing @kjac - please look carefully to make sure it aligns with what you expect.

With a structure of parent, child, grandchild, with each document in two languages, I've verified that:

  • With a child unpublished in a language, the child is removed from Children() rendering and the child and grandchildren under that child are removed from Descendants().
  • In preview mode, the unpublished child is removed from Children() and Descendants() but the grandchildren are shown in Descendants().
  • A grandchild with an unpublished parent in that language doesn't render when requested by path.
  • A grandchild with an published parent in that language does render.
  • A grandchild with an unpublished parent in that language does render when requested by GUID (e.g. /f5717b0a-fe44-45e7-8f9b-1aead2b9cf43?culture=it. The unpublished parent/ancestor is shown with an empty name.
  • A grandchild with an unpublished parent in that language does render in preview, and the unpublished parent/ancestor is shown with an empty name.

@kjac

kjac commented Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

In preview mode, the unpublished child is removed from Children() and Descendants() but the grandchildren are shown in Descendants().

This is certainly not expected, nor is it what I saw when testing. The unpublished child should show in preview mode. I will re-test.

@kjac

kjac commented Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

So... this is what I see. Yes, I spiced up the template with fancy links 😛

publish-parity.webm

@AndyButland

AndyButland commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

See if you can see what I'm missing then. Here's my result previewing the home page in Italian:

image

This is my structure. Everything is published apart from "Child 2" in Italian:

image

And this is my template:

@using Umbraco.Cms.Web.Common.PublishedModels;
@inherits Umbraco.Cms.Web.Common.Views.UmbracoViewPage<ContentModels.MultiLanguageTestPage>
@using ContentModels = Umbraco.Cms.Web.Common.PublishedModels;
@{
	Layout = null;
}

<h4>Published Content Tests</h4>

<fieldset>
    <legend>Axis: Ancestors</legend>
    <ul>
        @foreach (var content2 in Model.Ancestors())
        {
            <li>@content2.Name</li>
        }
    </ul>
</fieldset>
<fieldset>
    <legend>Axis: Parent</legend>
    @if (Model.Parent is not null)
    {
        <ul>
            <li>@Model.Parent.Name</li>
        </ul>
    }
</fieldset>
<fieldset>
    <legend>Axis: Siblings</legend>
    <ul>
        @foreach (var content3 in Model.Siblings())
        {
            <li>@content3.Name</li>
        }
    </ul>
</fieldset>
<fieldset>
    <legend>Axis: Children</legend>
    <ul>
        @foreach (var content4 in Model.Children())
        {
            <li>@content4.Name</li>
        }
    </ul>
</fieldset>
<fieldset>
    <legend>Axis: Descendants</legend>
    <ul>
        @foreach (var content5 in Model.Descendants())
        {
            <li>@content5.Name</li>
        }
    </ul>
</fieldset>

@kjac

kjac commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

Found it - it's resolved in #22662 - which then becomes a prerequisite for this PR. I will flip this PR to draft for now.

@kjac kjac marked this pull request as draft April 30, 2026 12:27
@kjac kjac marked this pull request as ready for review May 1, 2026 04:16
@claude

claude Bot commented May 1, 2026

Copy link
Copy Markdown

Claude finished @kjac's task in 4m 0s —— View job


PR Review

Target: origin/main · Based on commit: d5412758 · Skipped: 0 noise files out of 8 total

Restores culture-specific ancestor path filtering in FilterAvailable (previously used the culture-agnostic overload, which incorrectly allowed content through when only a sibling culture was published) and introduces Unfiltered() on IPublishedStatusFilteringService as a temporary bypass for Parent/Ancestors() navigation, aligning V17 behavior with V13 for culture-variant content trees.

  • Modified public API: IPublishStatusQueryService — new HasPublishedAncestorPath(Guid, string) with culture-agnostic default; IPublishedStatusFilteringService — new obsolete Unfiltered(IEnumerable<Guid>) with throw new NotImplementedException() default
  • Affected implementations (outside this PR): DocumentCacheService, ContentTypeIndexingNotificationHandler, DeferredSearchReindexService — all call the old single-argument HasPublishedAncestorPath(Guid), unchanged and unaffected
  • Other changes: Parent and Ancestors() extension methods now bypass publish/culture filtering and return items regardless of culture publish state, matching V13 behavior — callers relying on these yielding only published-in-culture items will see different results

Important

  • src/Umbraco.Core/Services/PublishStatus/IPublishedStatusFilteringService.cs:23: [Obsolete] message says "Scheduled for removal in V19." — per the repo's [Obsolete] convention (CLAUDE.md §5.4), the required format is "Scheduled for removal in Umbraco 19." This inconsistency makes automated tooling and release notes unreliable.

    Corrected:

    [Obsolete("This is an intermediate solution to avoid breaking changes. Use the IPublishedContentCache to get published content by key. Scheduled for removal in Umbraco 19.")]

Suggestions

  • src/Umbraco.Core/Services/PublishStatus/IPublishStatusQueryService.cs:36: The TODO comment // TODO (V18): Remove the default implementation. is missing the "when" clause from the convention template: // TODO (V{next-major}): Remove the default implementation when {condition}. Adding context (e.g., "when all callers pass culture explicitly") helps future maintainers understand the trigger for removal.

  • src/Umbraco.Core/Services/PublishStatus/IPublishedStatusFilteringService.cs:21: XML doc <param> text reads "The collection of content keys return." — grammatically incomplete. Should be "to return".

  • tests/Umbraco.Tests.UnitTests/.../PublishedContentStatusFilteringServiceTests.cs (new test FilterAvailable_Variant_ForNonPreview_YieldsOnlyItemsWithPublishedAncestorPath): The inline comment "none register as having a published ancestor path" is slightly misleading — ID 3 (at dictionary index 3) satisfies the index > 2 predicate and IS assigned a published ancestor path; it's excluded solely because it's unpublished (odd ID). The comment could read "none pass both the published and ancestor-path checks" to be accurate.


Approved with Suggestions

The core logic is sound and well-tested. The [Obsolete] wording fix (Important) is a one-liner and should be addressed; the suggestions are optional cleanup.

@kjac

kjac commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

#22662 is merged. I have verified it fixes what you're seeing, @AndyButland ... ready for re-testing when you have time 👍

@AndyButland AndyButland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me now @kjac. I've verified the preview issue we were discussing earlier and now see the expected result.

@kjac kjac merged commit 1edf7e9 into main May 1, 2026
27 of 28 checks passed
@kjac kjac deleted the v17/bugfix/v13-v17-published-querying-partity branch May 1, 2026 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants