U4-8345 - Wrapped property editors report the wrong type in the core

Created by Kevin Giszewski 18 Apr 2016, 14:50:52 Updated by Shannon Deminick 04 May 2016, 21:28:46

When using the core Tags property editor inside of an Archetype, the value passed into the core is of JArray to this core method: https://github.com/umbraco/Umbraco-CMS/blob/fc7cfea4ada8ccc510c8e3c1180e27360a2e7d34/src/Umbraco.Core/Models/PropertyType.cs#L373-L426

This causes an exception in the log:

2016-04-18 10:45:44,691 [P16132/D8/T65] ERROR Archetype.PropertyEditors.ArchetypePropertyEditor+ArchetypePropertyValueEditor - Type validation failed. The value type: 'JArray' does not match the DataType in PropertyType with alias: 'tag'
System.Exception: Type validation failed. The value type: 'JArray' does not match the DataType in PropertyType with alias: 'tag'
   at Umbraco.Core.Models.Property.set_Value(Object value)
   at Archetype.PropertyEditors.ArchetypePropertyEditor.ArchetypePropertyValueEditor.ConvertDbToEditor(Property property, PropertyType propertyType, IDataTypeService dataTypeService)

I'm not entirely sure what this property editor check does, but in order to get past it; a modification to the core is needed (I think) to notice that JArrays and JObjects are just strings. I don't think modifying the property editor itself will solve this but I'd be open to hearing a solution if that exists.

There is also a JS error with the Tags property when wrapped inside an Archetype, but that will be fixed in Archetype itself.

I have a PR for this issue.

1 Attachments

Comments

Kevin Giszewski 18 Apr 2016, 14:53:05

PR is located here: https://github.com/umbraco/Umbraco-CMS/pull/1233


Shannon Deminick 20 Apr 2016, 08:08:31

Hrm, i think I need a better understanding of the issue and what is actually happening here. Your PR might 'fix' the issue but i think it's just band-aiding something else.

Inside of Property.Value (setter) it calls IsPropertyTypeValid - this is because we have 5 types of data types that get stored in the database in the correct column types: Int, Decimal, DateTime, NVarchar (string), NText (string). The values must be validated before attempting to be stored in these columns otherwise you'll get SQL exceptions.

I'm not sure what ArchetypePropertyValueEditor.ConvertDbToEditor is doing, but in theory only one of these 5 types can be passed into Property.Value - this is used for persisting values. So I would think that Archetype needs to take this into account, it needs to figure out what type to pass to this method otherwise you might end up saving values in the wrong db column.


Kevin Giszewski 02 May 2016, 12:03:45

@Shandem, For whatever reason it appears the Tags property type doesn't return a string but rather a JArray. I'll try to move my PR into Archetype code, but this code be an issue for others (Nested Content, Grid). The real problem might occur with the Tags Property Editor.

Closing.


Shannon Deminick 02 May 2016, 13:21:19

Hey Kev,

What i need to understand what is happening when this is called: ArchetypePropertyValueEditor.ConvertDbToEditor. It might shed some more light on how this is used in the core if you lookup references of Umbraco.Core.Models.Property.set_Value(Object value)

The ConvertDbToEditor shouldn't really be setting a property value since that is normally used for persisting values, the documentation message for this method is: A method used to format the database value to a value that can be used by the editor


Kevin Giszewski 02 May 2016, 13:58:23

@Shandem

So the code is here: https://github.com/imulus/Archetype/blob/a081d9754609f2bfe1f2b670e0238c58c642502d/app/Umbraco/Umbraco.Archetype/PropertyEditors/ArcheTypePropertyEditor.cs#L124-L151

As you can see here: https://github.com/imulus/Archetype/blob/a081d9754609f2bfe1f2b670e0238c58c642502d/app/Umbraco/Umbraco.Archetype/PropertyEditors/ArcheTypePropertyEditor.cs#L137-L141

We're not setting any values from the editors, we're passing what we get through.

It's possible that when we deserialize the data it's interpreting it as JArray/JObject: https://github.com/imulus/Archetype/blob/a081d9754609f2bfe1f2b670e0238c58c642502d/app/Umbraco/Umbraco.Archetype/PropertyEditors/ArcheTypePropertyEditor.cs#L129

I'll try to tinker with this because as you mention it could just be how we're handling this. The odd part is this is the only property type (Tags) that does this. Other JSON-driven property editors do not exhibit this.


Kevin Giszewski 02 May 2016, 18:05:07

@Shandem

I've made a PR for Archetype that looks like it'll avoid this issue altogether: https://github.com/imulus/Archetype/pull/359/commits/3a30c7ba40c5f10912d4e5cf6294977c3b8c24da

Just weird that only the Tags type has ever done this :)

Thanks for taking the time to reply #h5yr


Kevin Giszewski 04 May 2016, 18:04:29

Just a quick follow-up. I was unable to figure out a solution to get Archetype to handle this nicely. If this affects anyone greatly, the only solution I know works is the closed PR I sent.

I spent a day or so on this an can't quite figure out a solution. Worst case scenario will be log spam which is why I'm going to just leave this issue as-is :) Time is becoming more limited.


Shannon Deminick 04 May 2016, 21:28:46

I saw your note on your github issue - I guess we need to look at the ConvertEditorToDb method and it's result, where its used and what is the intended result. It's been a while since I've been in that codebase so don't recall without digging into it.


Priority: Normal

Type: Bug

State: Closed

Assignee:

Difficulty: Normal

Category: Editor

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.4.0, 7.3.1, 7.3.2, 7.3.3, 7.3.4, 7.4.1, 7.3.5, 7.3.6, 7.3.7, 7.3.8, 7.4.2, 7.4.3

Due in version:

Sprint:

Story Points:

Cycle: