U4-3091 - Xml cache size optymalization

Created by wtct 10 Oct 2013, 12:39:28 Updated by Shannon Deminick 04 Aug 2014, 17:00:29

We don't need to populate empty properties in xml cache (I mean umbraco.config file). The empty simple node in xml is treated like the node which doesn't exists. By this modyfication we can save RAM and time during any dynamic operations on XML.

This modyfication is very simple and I will create pull request.

Comments

Shannon Deminick 19 Dec 2013, 02:12:03

I'm just checking with other core members to ensure that nobody is expecting empty xml properties to exist - hopefully not because I agree that they just take up space.


Shannon Deminick 28 Dec 2013, 04:32:14

https://github.com/umbraco/Umbraco-CMS/pull/162


Rody van Sambeek 14 May 2014, 13:58:17

Hi Shannon, the package "PliableForms" depends on empty nodes in it's xslt macros. It checks for existince of these empty fields. This functionality fails in Umbraco 6.2.0 because of these empty fields.

I agree it is better to strip the empty XML fields, but brea in mind that this is a breaking change and some packages can depend on empty fields in the XML cache.


Shannon Deminick 14 May 2014, 23:22:00

What empty fields does this package require? Are they empty fields that are part of content items for defining PliableForms or are they empty fields that are part of regular user content?

If they are empty fields that are part of the pliable forms definitions then you could work around that by adding a real value to your fields.


Rody van Sambeek 15 May 2014, 11:15:09

It uses an empty "label" field to filter out the Pliable Fields and the other info in the XML. I solved it by using a different (not empty) field. So for now I am ok.


Jeroen Breuer 26 May 2014, 18:05:50

I think this is a breaking change. For example when writing a query: public IEnumerable GetNewsItems(string search) { return ( from n in CurrentPage.Children orderby n.GetPropertyValue("currentDate") descending where n.GetPropertyValue("title").Contains(search) select new NewsItem() { Title = n.GetPropertyValue("title"), Url = n.Url(), Image = n.GetCroppedImage("image", 300, 300), Date = n.GetPropertyValue("currentDate") } ); } This used to work in 6.1.6, but in 6.2 it can break becasue n.GetPropertyValue("title") can return null so .Contains will throw an "object reference not set to an instance of an object" error.


Shannon Deminick 27 May 2014, 23:23:06

Are you certain that this is due to the non existence of empty xml elements? When dealing with strings you should always check for null since a string can be null or empty. Your logic should typically check for null's otherwise you will end up with 'Object reference' errors. Just add a HasValue check:

public IEnumerable<NewsItem> GetNewsItems(string search)
{
	return
	(
		from n in CurrentPage.Children
		orderby n.GetPropertyValue<DateTime>("currentDate") descending
		where n.HasValue("title") && n.GetPropertyValue<string>("title").Contains(search)
		select new NewsItem()
		{
			Title = n.GetPropertyValue<string>("title"),
			Url = n.Url(),
			Image = n.GetCroppedImage("image", 300, 300),
			Date = n.GetPropertyValue<DateTime>("currentDate")
		}
	);
}


Jeroen Breuer 28 May 2014, 07:07:42

I'm pretty sure is due to the non existence of empty xml elements. In 6.1.6 n.GetPropertyValue("title") always returned an empty string because it always existed in the xml cache. I know that checking for nulls or using HasValue will solve the problem, but I never needed to do that before. It's not really a problem, but I still think it's a breaking change.


Shannon Deminick 28 May 2014, 07:16:23

Can you test it by putting the empty element back in your umbraco.config and restarting the app ?

If that is the case then unfortunately this is an unforeseen break but the change is already in production. To un-break we'd have to a specific type check for typeof(string) in the underlying GetValue method and then change null to string.Empty. We are currently using the standard default(T) for empty values and in the case of string it = null. It was never the intention to have GetPropertyValue() return an empty string when the property value is actually null.


Jeroen Breuer 28 May 2014, 07:49:12

I placed the empty element back in the umbraco.config and restarted the app and I still got the exception so it might not be related to this. Maybe something else changed in 6.2 because I still think that n.GetPropertyValue("title") never returned null in 6.1.6 when the property was empty.


Stephan 28 May 2014, 09:57:23

Not related to XML cache.

In 6.2 and 7 GetPropertyValue returns default(typeof(T)) consistently when the value is missing, and for strings we assume that "null or empty" means "missing". And default(string) is null. So if a string property does not have a value, you'll get null.

Returning default(typeof(T)) is what makes most sense, globally. But if you don't like it, you can override the builtin IPropertyValueConverter for strings so it always return an empty string.


Jeroen Breuer 28 May 2014, 10:19:25

Thanks for the info. Is that also reported somewhere else since that's still a breaking change.


Peter Gregory 31 May 2014, 06:55:56

I just hit this issue just now after upgrading a site all queries break :/ Which sux.

EG I had a query that used to say

var posts = Model.Content.DescendantsOrSelf("BlogPost").Where(x => x.GetPropertyValue("tags").ToLower().Contains("someTag");

However this now breaks under the new return default Type scenario because the strings return nulls.. and I cant ToLower a null and I can do a contains on a null.

I really think this was a bit of an oversight and should be repaired.


Benjamin Howarth 04 Aug 2014, 15:15:24

Pete, you can use x => !String.IsNullOrEmpty(x.GetPropertyValue("tags") to work around this.


Shannon Deminick 04 Aug 2014, 17:00:29

Ben: HasProperty method is probably nicer to use than that.


Priority: Normal

Type: Bug

State: Fixed

Assignee: Shannon Deminick

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 4.8.0, 4.9.0, 4.10.0, 4.11.0, 6.0.0, 6.1.0, 7.0.0, 4.9.1, 4.11.1, 4.11.2, 4.11.3, 4.11.4, 6.0.1, 4.11.5, 6.0.2, 4.11.6, 6.0.3, 6.0.4, 4.11.7, 6.1.1, 6.0.6, 4.11.9, 6.0.5, 4.11.8, 6.0.7, 6.1.2, 4.5.0, 4.5.1, 4.8.1, 4.10.1, 4.11.10, 6.1.3, 6.1.4, 6.1.5, 6.1.6, 7.0.1

Due in version: 6.2.0, 7.0.2

Sprint:

Story Points:

Cycle: