U4-7318 - Add Core Property Value Converters for all Property Type's that require them by default - but careful of breaking changes

Created by Shannon Deminick 27 Oct 2015, 17:21:35 Updated by Shannon Deminick 29 May 2017, 18:15:14

Tags: Up For Grabs PR Community Contrib

Relates to: U4-9582

Relates to: U4-9611

Subtask of: UAASSCRUM-810

For example, we should definitely be shipping with a Property Value Converter for the media picker, i'm sure there are plenty of other ones that we should ship with too.

The problem is that this is a breaking change because the output of the property value converters will change what people currently have. There are various solutions to this problem. The easiest is:

  • Document this as a breaking change
  • Document the work around for upgrading (i.e. a simple application event handler that removes these new property value converters on startup)

Comments

James Coxhead 07 Apr 2016, 22:30:45

I'm happy to take this on for non-picker data types (as these have already been implemented in Jeavon's core property value converters package). I'd implement converters for the following data types (based on output from the models builder):

  • Checkbox list => array/list of strings
  • Colour picker => string
  • Dropdown list => string
  • Dropdown list, publishing keys => integer
  • Dropdown list multiple => array/list of strings
  • Dropdown list multiple, publishing keys => array/list of integers
  • Email address => string
  • Member group picker => string
  • Member picker => IPublishedContent
  • Radio button list => int (returns the prevalue ID)
  • Slider => ?? (could be a single value, could be a pair of values)
  • Tags => array/list of strings (assuming this won't break tag queries)
  • User picker => integer


Shannon Deminick 08 Apr 2016, 09:25:08

Hi James, not sure about many of these items above... pretty much anything that is a simple type: string, int doesn't really need a converter right? the value should already be that value already. For anything that is array/list, be sure that it's actually IEnumerable (readonly) you don't want a mutable collection result.


Shannon Deminick 08 Apr 2016, 09:26:14

Also, for things that are in Jeavon's package - we should really consider putting some (all) in the core instead of having it as a package for all of the ones that makes sense.


James Coxhead 10 Apr 2016, 21:23:22

The simple types do seem to work without a value converter (I've just tried GetPropertyValue<string> on an email address) - however the models builder creates properties of type object for these property types so we would need a value converter to get these working correctly.

For the converters in Jeavon's package - maybe that's a conversation to be had between you guys at HQ and Jeavon. I'm not one for reinventing the wheel (and I'm definitely not one for copying somebody else's work!)


James Coxhead 10 Apr 2016, 21:27:55

On a related but unrelated note, the converters in the core are currently split between the Umbraco.Core.PropertyEditors.ValueConverters and Umbraco.Web.PropertyEditors.ValueConverters namespaces. For v8 Would it make sense to have these all in a single place?


Matt Brailsford 10 May 2016, 20:21:40

Just as a note, regarding changing pickers returned values, if this does happen be sure to update the built in redirects logic (umbracoRedirect etc) as these will all break.


Jeavon Leopold 10 May 2016, 20:26:43

I'll happily create a PR for the Converters in my package and we can work from there. You know originally the converters were created for Core (v6.0) but then we realised that they were too breaking and so created the package at a Hackathon many moons ago. Things have dramatically moved on and I think the approach @Shandem took with the ImageCropDataSet converter and type converter is the model to follow to minimise breakingness.


Jeavon Leopold 10 May 2016, 20:30:56

@matt yep, have those here https://github.com/Jeavon/Umbraco-Core-Property-Value-Converters/blob/v2/Our.Umbraco.PropertyConverters/ContentPickerPropertyConverter.cs#L34


James Coxhead 01 Jun 2016, 21:22:01

This has taken me a bit longer than anticipated with work and other commitments, but I should have something ready to submit in the next week (hopefully sooner).

@crumpled_jeavon - that sounds good. Do you need anything from me for this?


Jeremy Pyne 13 Jun 2016, 18:02:48

If this is added to the Core then i would request that the PartialViewMacroModelExtensions.GetParameterValue() also use the same logic. The issue here being that the macro parameters aren't actually properties so I don't know if theres a fancy way to tie the to the PropertyValueConverter's to them to do this automagically.

In short if I add a Content Picker on a macro I should also get a IPublishedContent.


Shannon Deminick 20 Jun 2016, 07:48:36

PR: https://github.com/umbraco/Umbraco-CMS/pull/1311


Jeavon Leopold 20 Jun 2016, 08:35:30

@Shandem PR is WIP targeted to the temp-u4-7318 branch, it only contains the initial 5 converters from my package plus some unbreaking stuff I worked on with @zpqrtbnk at the retreat. It is a good basis for others such as @jcoxhead to collaborate with the further converters once available to fork from the temp-u4-7318 branch. I think @zpqrtbnk is going to review...


Shannon Deminick 20 Jun 2016, 09:34:56

Yup sounds good! This is scheduled for 7.6 so we'll start having a look at all of this once 7.5 is out


Chris Houston 19 Sep 2016, 16:30:02

Hi All,

We recently had an issue with one of Jeavon's value converters that caused a lot of logging and performance issues, so it's something you might want to consider in this issue. This is probably a reasonable edge case, but easily re-produced prior to version 7.5.3 ( @zpqrtbnk changed the logging so it's not so obvious now, but it's still happening )

There is a related issue here: http://issues.umbraco.org/issue/U4-7823

I had an off line chat with @zpqrtbnk about this ticket, the issue turned out to be the Property Value Converter, to reproduce the issue we did the following:

  1. Our site had 7000+ news articles, each with a multiple picker to associate them with items in another area of the site.

  2. Our client would then delete one of the related nodes, hence all these picked items would then be pointing to an item that no longer existed.

  3. The PVC checks to see if the ID exists as a Content node, then a member node and finally a media node and if it is not found as a media node, it logs the error that it is failing back to using GetMedia ( which of course still would return nothing in this scenario )

All the above would probably be fine if you only had a couple of relationships, but some of these pages did this kind of failed look up 20+ times. Hence 20 logged items in the log file and a much slower page render.

If this could somehow be improved by a converter only checking Content OR Members OR Media that would be great.

Cheers,

Chris


Lars-Erik Aabech 29 Oct 2016, 07:48:21

Hi guys, I added one for the Upload field. We need simple converters for intristic types so ModelsBuilder generates the right type. Upload for instance shouldn't be object since it's harder to do "null-or-empty" checks on it. Makes for uglier code. Can do the rest if interesting. (Or let @jcoxhead carry on ;) )


James Coxhead 03 Nov 2016, 00:03:14

I've just submitted a pull request with my converters (better late than never!). See https://github.com/umbraco/Umbraco-CMS/pull/1570. I've implemented these converters:

  • Email address
  • Checkbox list
  • Radio button list
  • Dropdown list
  • Dropdown list (publishing keys)
  • Dropdown list multiple
  • Dropdown list multiple (publishing keys)
  • Member group picker
  • User picker
  • Member picker

...which leaves the tags and slider data types up for grabs.


Jeavon Leopold 03 Nov 2016, 09:11:20

@jcoxhead awesome work! I am going to be working on this today at the UK fest hackathon which is where my package was born many years ago! :)


James Coxhead 03 Nov 2016, 11:35:16

Nice one - I'll be there tomorrow if you want to talk through any of it.


Jeavon Leopold 03 Nov 2016, 17:27:06

I have merged what we currently have (from @jcoxhead and @lars-erik) into a temp branch https://github.com/umbraco/Umbraco-CMS/tree/temp-u4-7318 and also added a umbraco setting (EnablePropertyValueConverters) to enable/disable all converters that are breaking (mainly for existing sites that use dynamic published content).

I will review the converters from @jcoxhead and see if we can add any type converters for upgrade support.

We still need a pre-value caching solution for the multiple media picker (U4-8862) as I have removed the method used in my current package and also as @jcoxhead says we need converters for Tags & Slider.


Jeavon Leopold 05 Dec 2016, 09:53:34

@jcoxhead I'm wondering if the converters should actually return Umbraco.Core.Models.PreValue objects for DropDownList Keys and DropDownListMultiple Keys (again we will need PreValue caching for this U4-8862)?


Jeavon Leopold 03 Jan 2017, 09:50:51

FYI Temp branch to dev-v7.6 branch PR is https://github.com/umbraco/Umbraco-CMS/pull/1640


Shannon Deminick 08 Mar 2017, 13:30:45

There were several things I've fixed up in the PR, we have a few more tasks to complete for further review and refactoring but for the time being this has been merged in!!! thanks everyone :)


Jeavon Leopold 17 Mar 2017, 14:00:03

@Shandem @zpqrtbnk is U4-8862 going to included in v7.6 RTM as currently the prevalues are not cached in the value conveters e.g. https://github.com/umbraco/Umbraco-CMS/blob/dev-v7.6/src/Umbraco.Web/PropertyEditors/ValueConverters/MultipleMediaPickerPropertyConverter.cs#L226


Sebastiaan Janssen 17 Mar 2017, 14:30:14

@crumpled_jeavon I'm voting a big YES on that one, I've added it to be prioritized in our next sprint. Thanks for the reminder!


Shannon Deminick 17 Mar 2017, 23:14:29

Iirc they are cached at the service level already


Robin Neal 12 Apr 2017, 16:40:09

Looks like the changes in 7.6 are causing issues with the existing media property converters - they are expecting to receive an int, but instead receiving a UDI. Supporting the new UDI just requires changing int.Parse to Udi.parse, however this isn't backwards compatible and requires all images to be removed and re-added - so not very practical for a larger site.


Sebastiaan Janssen 13 Apr 2017, 11:35:49

@SudoCat We're trying to parse both UDI and int so it should be backwards compatible. Can you explain (in excruciating detail please!) what exactly is going wrong for you?

So.. what exact datatype, how is it added on your document type, it stores an int, how do you query it, etc?


Robin Neal 13 Apr 2017, 15:44:51

Sure thing. We have managed to implement a fix for now by rewriting our property converter to utilise the Udi.Parse method instead, then replacing all of our old images. It definitely looks like the existing property converter included in 7.6 for MultipleMediaPickers does not parse Udi ids, and can only handle int ids.

{cut Project/Bug Background}

-All media pickers across the site were affected by the issue. The result was that every page containing media picker where the data had been changed after the update had been installed produced an error (I believe the error was Invalid Input String), due to trying to apply int.Parse to the Udi. Any media pickers which had not been changed still worked fine.

-The media pickers were implemented using the MultipleMediaPicker property editor, split across 4 data types (Multiple Media, Single Media, Multiple Images, Single Image). These were then applied onto a variety of document types, some using compositions, and some inside of nested content. All were affected the same way.

-We use the Umbraco Models Builder (Core version, not plugin) to generate strict models for all of our document types to pass to the views. I'm not 100% sure on the implementation details as it was setup by the backend team, however I know it produces the .cs files when run within VS, which are then manually compiled into the project.

-We also use the Property Converter with the models builder to produce models with real data - so Model.Image is returns an IPublishedContent of the media item rather than a string/int of the ID. We then use custom property converters ontop of this to convert certain data types to more specific models.

  • Our custom property converter for MultipleMediaPicker inherits from the base MultipleMediaPickerConverter class.
  • The errors lead me back to the property converter. Inside of the ConvertSourceToObject method of our MultipleMediaPickerConverter, we call the base.ConvertSourceToObject method to receive a IPublishedContent or IEnumerable<IPublishedContent> which we then apply further transformations on. Instead, the ConvertSourceToObject method was receiving null as the source argument for any of the media pickers which had their value changed since the update.

  • We did not have a custom ConvertDataToSource method, so I had no idea why the source was null. Upon checking the [https://github.com/umbraco/Umbraco-CMS/blob/dev-v7.6/src/Umbraco.Web/PropertyEditors/ValueConverters/MultipleMediaPickerPropertyConverter.cs github branch for 7.6] (Not 100% I found the right one though) it seemed that the ConvertDataToSource was only parsing int.

  • I created a custom ConvertDataToSource method, replacing int.Parse with Udi.Parse, which fixed all of the ones which had been updated, but broke all of the old ones which had not had their values updated.

tl;dr -We're using Models Builder with the Property Converter to generate strict models. -After the update, errors were being thrown on the front end when you changed any media data types. -The errors were caused by our custom MultipleMediaPickerConverter ConvertSourceToObject method expecting to receive an object ID and instead receiving null as the source argument for any MutlipleMediaPickers where the value had been changed following the upgrade. -Upon closer inspection, it seems that the MultipleMediaPickerConverter ConvertDataToSource method in the core only runs int.Parse, and just returns null for anything else (such as a UDI)

Anyway, I think I'm just starting to repeat myself - not sure what more information I can provide. It definitely looks to me like the core converter just doesn't support the UDIs right now. I hope that helps and isn't just waffle!


Jeavon Leopold 13 Apr 2017, 16:38:35

Strange, MultipleMediaPickerConverter does not handle Udis because the MultipleMediaPicker does not store Udis. Only MediaPicker2 stores Udis and it's [https://github.com/umbraco/Umbraco-CMS/blob/dev-v7.6/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerPropertyConverter.cs converter] handles them, so the question is how have you managed to get Udi's into the MultipleMediaPicker. I did see something similar once but I was unable to replicate when I tried....


Shannon Deminick 18 Apr 2017, 03:01:28

I'll re-open just to verify that something hasn't gone awry


Robin Neal 18 Apr 2017, 08:22:00

Oh damn, what on earth have we managed to do? Well, we have it working now through a custom property converter, and deadlines are calling so we're going to have to press on, however we'll monitor this and see if we can provide any more information. Let me know if there's anything I can do to help.


Shannon Deminick 19 Apr 2017, 04:07:32

Thanks a ton for this testing feedback, you have in fact discovered a bug. The problem is that the legacy media picker is misconfigured to store UDIs instead of INTs. So when you re-save, the INTs are saved as UDIs which the converter is not built for. So the problem is at the property editor level. I'll fix now.


Shannon Deminick 19 Apr 2017, 04:40:33

THis is all fixed up now @SudoCat when the final release comes out, you would need to re-pick all of the media items in your media pickers that now have stored UDIs - OR, you could continue to use your work around for now and inherit from MultipleMediaPickerConverter to detect UDIs that have already been persisted.


Robin Neal 19 Apr 2017, 08:38:57

@Shandem Ah-ha! Well it wasn't just us breaking things, that's a nice change! Glad I could be of some help, and thanks for the update, we'll probably translate everything over to the new media picker to avoid any further issues.


Jeavon Leopold 19 Apr 2017, 08:41:45

@SudoCat I think you could also switch your data types to use MediaPicker2 then you would continue with UDIs


Dan Booth 20 Apr 2017, 11:21:48

Sorry if this is the wrong place for asking, but is it (or could it be) possible to register multiple Property Value Converters for one Property Editor, so long as they return a different type? So, for example, if a "picker" datatype stored an integer or UDI that relates to a node, you could do:

int value = Model.GetPropertyValue<int>("picker");

IPublishedContent value = Model.GetPropertyValue<IPublishedContent>("picker");

UDI value = Mode.GetPropertyValue<UDI>("picker");

And all would work? It would require three property value converters for each different type. Is this viable? It would help with backward compatibility and also allow people to get raw values when needed.


Jeavon Leopold 20 Apr 2017, 11:55:17

@DanDiplo we have a type converter for https://github.com/umbraco/Umbraco-CMS/blob/dev-v7.6/src/Umbraco.Core/Models/PublishedContent/PublishedContentTypeConverter.cs and have added ToString() override to PublishedContentExtended to allow https://github.com/umbraco/Umbraco-CMS/blob/dev-v7.6/src/Umbraco.Core/Models/PublishedContent/PublishedContentExtended.cs#L191

These should both work regardless of if the property editor is storing int's or UDIs (I haven't tested UDIs)

A UDI TypeConverter is an idea, although I'm not sure what you would need it for?


Dan Booth 20 Apr 2017, 12:20:28

@crumpled_jeavon Sometimes it's useful to use the raw value for things like a value in searches.

Imagine you had a list of content-managed categories stored as nodes. And you have some news items that have a picker to associate them with categories. Now say you want to perform a search where you pass in a category ID and get back all news items that have that ID. It's more efficient to use the raw IDs for matching than converting to published content first, just to then compare against the IDs.

I just think the general principle of people being able to write their own specific converters against a property editor that may already have one could be useful.


Jeavon Leopold 20 Apr 2017, 12:50:20

@DanDiplo you can always get the raw value from GetProperty("alias").DataValue. You can also create as many TypeConverters as you like, e.g. you could create a IPublishedContent to UDI type converter.


Dan Booth 20 Apr 2017, 12:55:12

@crumpled_jeavon Yeah, I currently use DataValue to get the raw value, but you need to cast it from object etc.. But if this changes from an int to UDI at some point it will break the association. How would upgrades work etc? It would be nice if there was strongly typed methods that were guaranteed to always return the integer value of the node (or whatever).


Jeavon Leopold 20 Apr 2017, 12:57:42

@DanDiplo yes, GetPropertyValue("alias") would do this, but I just noticed that the TypeConverter is internal, need to recheck it works


Dan Booth 20 Apr 2017, 14:03:20

@crumpled_jeavon That would be good. But it would be also nice to do GetPropertyValue<IEnumerable<int>>("alias") for properties that stored a list of integers (say as CSV or JSON) alongside being able to do GetPropertyValue<IEnumerable<IPublishedContent>>("alias"). It gives you a choice.

Or how about a picker that can potentially have multiple items but is set to store only one. Would be good if you could do GetPropertyValue<int>("alias") even if it stores CSV, but just gets the first value.


Shannon Deminick 21 Apr 2017, 01:24:00

You cannot have multiple Value Converters target a single property editor - well, sort of ;)

In the core we have 'Default' Property value converters, we have a special attribute that is used so that if non-core value converters are added for this property editor, the custom one will 'override' the default ones.

You have the ability to manipulate the Property Value Converters resolver/collection on startup via code in the ApplicationStarting event using the PropertyValueConvertersResolver. So if you know a package has a custom value converter for a property editor you would like to replace, you could create a custom value converter which may inherit from the original one, then you can add your logic to return values based on different types, then on startup you can remove the value converter from the collection from the package (your own will be automatically added by assembly scanning)


Priority: Up for grabs

Type: Feature (request)

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: False

Fix Submitted:

Affected versions:

Due in version: 7.6.0

Sprint: Sprint 57

Story Points: 3

Cycle: