-
Notifications
You must be signed in to change notification settings - Fork 33
Conversation
Added an XPath processor that can query the Umbraco content cache. Included a supporting unit-test. We do need to review how the mocking objects/services are created, I believe PR #204 may cover this?
…t> was being passed in
# Conflicts: # src/Our.Umbraco.Ditto/Our.Umbraco.Ditto.csproj
.ContentCache | ||
.GetByXPath(xpath) | ||
.OrderBy(x => x.Level) | ||
.ThenBy(x => x.SortOrder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattbrailsford Do you think this processor should be dealing with the sort order?
The reason I ask, is that on my local version of this, I've had to extend the sorting to include the parent sort level/order as well...
return UmbracoContext
.Current
.ContentCache
.GetByXPath(xpath)
.OrderBy(x => x.Parent != null ? x.Parent.Level : 1)
.ThenBy(x => x.Parent != null ? x.Parent.SortOrder : 1)
.ThenBy(x => x.Level)
.ThenBy(x => x.SortOrder);
...I was querying for a bunch of nodes at a certain level deep, across different siblings - so sorting by Level
+ SortOrder
didn't come out as I'd expected. But even with my local fix, I'm wondering about how to order even deeper nodes?
Then it gets me thinking that the XPath processor shouldn't be concerned with sorting, just do the query and pass it on. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to install a heap allocation viewer and inspect that query. Each of those lines will most probably be creating a new collection which will both be memory intensive and slow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern (it's a big one) with this processor is that it allows you to too easily add navigation properties without protecting you from the overheads of doing so.
While on the surface to a consuming developer it might look like we are simply doing the same as the pickers, we are most definitely not. parsing $site
, $root
etc involves document traversal which should be a method call rather than a property since they are so expensive. Sure you can mark it as lazy but who will?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JimBobSquarePants Thanks for the heap allocation advice, (I haven't used anything like that before, I'll take a look), I suspect you're right about it being memory intensive. 👍
My current feeling is to remove the sort ordering from this processor.
I'm not as convinced about it being expensive, the XML content cache is (currently; in v7) in-memory, so the XPath queries should be fast. With the inline variables ($current
, $site
, etc), they're being swapped out with XPath's id()
function, of which the XML document already has them indexed, again should be pretty fast, (in terms of XPath traversing). Though I can't account for how long the XML doc to IPublishedContent conversion takes.
I'm road testing this processor on a couple of websites at the moment, it seems to be performing nicely... though saying that I don't have any benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an extension for Resharper that gives you the absolute fear. I'll be running a full pass of Ditto sometime when I get the chance to look at any performance issues.
As long as it swaps out to id then i'm ok with it then as that should be fast 👍
Removed the sort ordering ( |
Added an XPath processor that can query the Umbraco content cache.
Included a supporting unit-test. We do need to review how the mocking objects/services are created, I believe PR #204 may cover this?