Umbraco-CMS icon indicating copy to clipboard operation
Umbraco-CMS copied to clipboard

Xpath Query on Multinode Treepicker no longer working for new Nodes.

Open marcloveUSN opened this issue 2 years ago • 11 comments

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

12.1.1

Bug summary

The Xpath Query for the Multinode Treepicker does not work when used on a new node in the content tree. Once the node is saved the Xpath Query will work as expected.

Do not know what version of Umbraco this started to happen with however I can confirm that it was functioning properly with Umbraco 10.3.2 and doesn't work with 11.2.2 and above.

Specifics

Treepicker documentation and how to work with XPath can be found here:

https://docs.umbraco.com/umbraco-cms/fundamentals/backoffice/property-editors/built-in-umbraco-property-editors/multinode-treepicker

Steps to reproduce

You can reproduce by creating a simple Document Type / Content Structure in Umbraco.

Home (Document Type - alias 'home') Page type 1 (Document Type - Page type 1 allowed below Home - alias 'pageType1') Page type 2 (Document Type - Page Type 2 allowed below Home and below Page type 1 - alias 'pageType2')

Create a Multinode Treepicker with the following xpath query:

$current/ancestor-or-self::home/pageType1

This XPath picker should find Home and then first node using pageType1 document type. It should then list all nodes below.

Add this picker to page type 2

In the content Tree add:

Home ---Text page 1 (Page Type 1) ------Sub page 1 ------Sub page 2 ---Text page 2 (Page Type 2) DO NOT PUBLISH

The picker on page type 2 should only show Sub page 1 and Sub page 2 however it will show all nodes in the content tree.

If you save and publish the page and then try the picker again, it will work as expected and only show Sub page 1 and Sub page 2.

The location of the unpublished node is therefore no longer working as it did in previous versions of Umbraco.

Expected result / actual result

No response

marcloveUSN avatar Aug 18 '23 14:08 marcloveUSN

Hi there @marcloveUSN!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

  • We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
  • If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
  • We'll replicate the issue to ensure that the problem is as described.
  • We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

github-actions[bot] avatar Aug 18 '23 14:08 github-actions[bot]

Hey!

Thanks a lot for reporting this issue and for providing clear reproduction steps 😄. I was able to reproduce this issue on versions 12.1.1 and 11.4.2.

When I created the 'Test page 2' with the MultinodePicker that had the specified XPath. I got all of the content nodes in Home. The same thing happened when I tried to save or save and publish. After I refreshed the page, I was able to get 'Sub page 1' and 'Sub page 2' from 'Text page 1'.

When looking at the network calls for 'GetApplicationTrees' before and after saving and reloading there is a difference. Before saving and reloading the GetApplicationTrees looks like this: /GetApplicationTrees?application=content&tree=content&use=dialog&dataTypeKey=a385d5ea-fd95-4559-ae57-c3061b690182. After saving and reloading it looks like this: /GetApplicationTrees?application=content&tree=content&use=dialog&startNodeId=umb%3A%2F%2Fdocument%2Fc48fe31742d043129eff348c10cbc0fb&dataTypeKey=a385d5ea-fd95-4559-ae57-c3061b690182.

We can see that startNodeId=umb%3A%2F%2Fdocument%2Fc48fe31742d043129eff348c10cbc0fb is missing from the first call. the startNodeId is the id of 'Text page 1'. Since we are missing the startNodeId from the first call we end up getting all the content nodes in Home.

So the problem is that the XPath does not have the $current part of the XPath until the 'Text page 2' is reloaded.

I will mark this as an up for grabs!

andr317c avatar Aug 21 '23 10:08 andr317c

Hi @marcloveUSN,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post.

Thanks muchly, from your friendly Umbraco GitHub bot :-)

github-actions[bot] avatar Aug 21 '23 10:08 github-actions[bot]

Good to hear you managed to recreate it. Any guidance on the location of the code that is causing the issue would be appreciated.

marcloveUSN avatar Sep 01 '23 09:09 marcloveUSN

I'm experiencing the same issue, any update on this?

huzzi avatar Nov 01 '23 10:11 huzzi

Also experiencing this issue, any updates?

ojwiya avatar Nov 01 '23 17:11 ojwiya

The issue seems to be with the ParseXPathQuery in Umbraco.Core\Xml\UmbracoXPathPathSyntaxParser.cs in line 140 we have this logic.

    if (nodeContextId.HasValue)
        {
            vars.Add("$current", q =>
            {
                var closestPublishedAncestorId = getClosestPublishedAncestor(getPath(nodeContextId.Value));
                return q.Replace("$current", string.Format(rootXpath, closestPublishedAncestorId));
            });
        }

Since the item is not saved the nodeContextId value 0 and the $current value return from getClosestPublishedAncestor is -1.

@andr317c @marcloveUSN

huzzi avatar Nov 02 '23 19:11 huzzi

This updated code seems to work for the scenario described by @andr317c . I have not tested this in other scenarios.

    public static string ParseXPathQuery(
        string xpathExpression,
        int? nodeContextId,
        int? parentId,
        Func<int, IEnumerable<string>?> getPath,
        Func<int, bool> publishedContentExists)
    {
        // TODO: This should probably support some of the old syntax and token replacements, currently
        // it does not, there is a ticket raised here about it: http://issues.umbraco.org/issue/U4-6364
        // previous tokens were: "$currentPage", "$ancestorOrSelf", "$parentPage" and I believe they were
        // allowed 'inline', not just at the beginning... whether or not we want to support that is up
        // for discussion.
        if (xpathExpression == null)
        {
            throw new ArgumentNullException(nameof(xpathExpression));
        }

        if (string.IsNullOrWhiteSpace(xpathExpression))
        {
            throw new ArgumentException(
                "Value can't be empty or consist only of white-space characters.",
                nameof(xpathExpression));
        }

        if (getPath == null)
        {
            throw new ArgumentNullException(nameof(getPath));
        }

        if (publishedContentExists == null)
        {
            throw new ArgumentNullException(nameof(publishedContentExists));
        }

        // no need to parse it
        if (xpathExpression.StartsWith("$") == false)
        {
            return xpathExpression;
        }

        // get nearest published item
        Func<IEnumerable<string>?, int> getClosestPublishedAncestor = path =>
        {
            if (path is not null)
            {
                foreach (var i in path)
                {
                    if (int.TryParse(i, NumberStyles.Integer, CultureInfo.InvariantCulture, out int idAsInt))
                    {
                        var exists = publishedContentExists(int.Parse(i, CultureInfo.InvariantCulture));
                        if (exists)
                        {
                            return idAsInt;
                        }
                    }
                }
            }

            return -1;
        };

        const string rootXpath = "id({0})";

        // parseable items:
        var vars = new Dictionary<string, Func<string, string>>();

        if (parentId.HasValue)
        {
            vars.Add("$parent", q =>
            {
                var path = getPath(parentId.Value)?.ToArray();
                var closestPublishedAncestorId = getClosestPublishedAncestor(path);
                return q.Replace("$parent", string.Format(rootXpath, closestPublishedAncestorId));
            });

            vars.Add("$site", q =>
            {
                var closestPublishedAncestorId = getClosestPublishedAncestor(getPath(parentId.Value));
                return q.Replace(
                    "$site",
                    string.Format(rootXpath, closestPublishedAncestorId) + "/ancestor-or-self::*[@level = 1]");
            });

            vars.Add("$current", q =>
            {
                var closestPublishedAncestorId = getClosestPublishedAncestor(getPath(parentId.Value));
                return q.Replace("$current", string.Format(rootXpath, closestPublishedAncestorId));
            });
        }
        else if (nodeContextId.HasValue)
        {
            vars.Add("$parent", q =>
            {
                var path = getPath(nodeContextId.Value)?.ToArray();
                if (path?[0] == nodeContextId.ToString())
                {
                    path = path?.Skip(1).ToArray();
                }

                var closestPublishedAncestorId = getClosestPublishedAncestor(path);
                return q.Replace("$parent", string.Format(rootXpath, closestPublishedAncestorId));
            });

            vars.Add("$site", q =>
            {
                var closestPublishedAncestorId = getClosestPublishedAncestor(getPath(nodeContextId.Value));
                return q.Replace(
                    "$site",
                    string.Format(rootXpath, closestPublishedAncestorId) + "/ancestor-or-self::*[@level = 1]");
            });

            vars.Add("$current", q =>
            {
                var closestPublishedAncestorId = getClosestPublishedAncestor(getPath(nodeContextId.Value));
                return q.Replace("$current", string.Format(rootXpath, closestPublishedAncestorId));
            });
        }

        // TODO: This used to just replace $root with string.Empty BUT, that would never work
        // the root is always "/root . Need to confirm with Per why this was string.Empty before!
        vars.Add("$root", q => q.Replace("$root", "/root"));

        foreach (KeyValuePair<string, Func<string, string>> varible in vars)
        {
            if (xpathExpression.StartsWith(varible.Key))
            {
                xpathExpression = varible.Value(xpathExpression);
                break;
            }
        }

        return xpathExpression;
    }

huzzi avatar Nov 02 '23 19:11 huzzi

Pull request created https://github.com/umbraco/Umbraco-CMS/pull/15159

huzzi avatar Nov 07 '23 12:11 huzzi

Just tested in 13.0.1 and the issue is present here also.

marcloveUSN avatar Dec 19 '23 20:12 marcloveUSN

Can we get the PR in please.

RuddyOne avatar Feb 02 '24 12:02 RuddyOne