umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.49k stars 2.69k forks source link

MultinodeTreePicker XPath Query fails after upgrade from V8 to V13.1 when it contains the double // notation #15647

Closed marcemarc closed 9 months ago

marcemarc commented 9 months ago

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

13.1.0

Bug summary

The upgraded site has a MultinodeTreePicker it sets its start node using XPATH

$site//newsCategoryGroups

I didn't spot this when initially upgraded to V12, but maybe I missed it?, anyway the // notification means find the document type anywhere in the 'XML'. (it's what people use when they can't work out the XPATH properly :-P)

After upgrading to V13, an error is thrown 'XPathValue must be an XPathNavigator or string'.

Stack Trace:

at Umbraco.Cms.Core.Xml.XPath.NavigableNavigator.MoveToFirstChildProperty() at Umbraco.Cms.Core.Xml.XPath.NavigableNavigator.MoveToFirstChild() at MS.Internal.Xml.XPath.DescendantQuery.Advance() at MS.Internal.Xml.XPath.DocumentOrderQuery.Evaluate(XPathNodeIterator context) at System.Xml.XPath.XPathNavigator.Evaluate(XPathExpression expr, XPathNodeIterator context) at System.Xml.XPath.XPathNavigator.Select(XPathExpression expr) at Umbraco.Extensions.XPathNavigatorExtensions.Select(XPathNavigator navigator, String expression, XPathVariable[] variables) at Umbraco.Cms.Infrastructure.PublishedCache.ContentCache.GetSingleByXPath(Boolean preview, String xpath, XPathVariable[] vars) at Umbraco.Cms.Core.PublishedCache.PublishedCacheBase.GetSingleByXPath(String xpath, XPathVariable[] vars) at Umbraco.Cms.Infrastructure.PublishedContentQuery.ItemByXPath(String xpath, XPathVariable[] vars, IPublishedCache cache) at Umbraco.Cms.Infrastructure.PublishedContentQuery.ContentSingleAtXPath(String xpath, XPathVariable[] vars) at Umbraco.Cms.Web.BackOffice.Controllers.EntityController.GetByXPath(String query, Int32 nodeContextId, Nullable`1 parentId, UmbracoEntityTypes type) at lambda_method3220(Closure, Object, Object[]) at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeActionMethodAsync() at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAsync() --- End of stack trace from previous location --- at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context) at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted) at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync() --- End of stack trace from previous location --- at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|26_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)

Workaround

If I update the XPATH to follow the structure of the site eg

$site/data/newsCategoryGroups

then it's just fine and dandy, no errors, works as expected.

And of course, why on earth are you using XPATH, when you have the lovely Dynamic Root Whizzy Thing - can confirm if I build the query using the Dynamic Root wizard then it's all fine.

And I've confirmed it's not an issue with how the DataType configuration is stored,if I switch back to using XPath, and compare the saved value in the database for the datatype to before it is the same.

So my hunch is it is just the use of // which is throwing things off...

Still there are only 43 instances of MultiNodeTreePickers using // on this project, ho hum!

Specifics

No response

Steps to reproduce

Add a Multinode Tree Picker to a site, use XPATH to set the start node and use the // in the XPATH notation

It should find the first instance matching the // query but instead it throws an error when you view a page in the backoffice using the property editor

switch it to use the single path notation XPATH query all is fine.

Expected result / actual result

It would be great if it just worked with // but if not just add it as a breaking change in the docs, so people can take account of it when migrating.

github-actions[bot] commented 9 months ago

Hi there @marcemarc!

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 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:

Migaroez commented 9 months ago

Hey Marc, this is a doozie. I tried to reproduce this on a fresh install and it seems to work as expected.

https://github.com/umbraco/Umbraco-CMS/assets/6025797/1a58acc8-62a7-404c-9975-030668736c87

marcemarc commented 9 months ago

@Migaroez that is super weird, I just added a new MNTP to the site, in case it was something to do with the existing editors and their migration and I get the same issue, but with your test, it must be something to do with the site?

I mean the workaround fix for me is to just migrate these to have better XPATH or use Dynamic Path, so I think we can close, and if someone else has this issue in future and googles the error they should find this issue and see the workaround or add any further information?

Migaroez commented 9 months ago

Sounds like a plan 👍 Just out of curiosity, what happens if you do it in a fresh install? Have you also cleared out your bin folder completely and done a rebuild? Might be some old dll causing havoc?

marcemarc commented 9 months ago

@Migaroez it is a fresh install, just pointing at the db, I'm just assessing the size of the migration task for a report, I had it up to V12 before XMAS, and then the report got put on the back burner, so have spun up a fresh 13.1 and pointed at the db... so yeah, no old DLLs, just weirdness, the only thing that is strange about this site, is it has a Global Node at the top of the tree for settings and then a homepage beneath that, and then the pages of the site, but then I'd expect it to not work with the single slash direct XPath too... I have written in the report that is a great opportunity to migrate to dynamic path :-P