When an item is trashed that is referenced in NC, problems will occur because the item is no longer valid
Shazwazza opened this issue · 10 comments
This is a problem we have with many picker property types and normally it doesn't cause any real problems apart from perhaps images or links missing on the front-end. However when things like Courier is used to transfer these items that are in trash, it converts the INT id to a GUID id and transfers it. On the other end it cannot convert back to an INT so the value in JSON remains as a GUID.
NC doesn't validate the input before it tries to convert this value to string using this code: https://github.com/umco/umbraco-nested-content/blob/develop/src/Our.Umbraco.NestedContent/PropertyEditors/NestedContentPropertyEditor.cs#L122 and at this line this is where it throws: https://github.com/umco/umbraco-nested-content/blob/develop/src/Our.Umbraco.NestedContent/PropertyEditors/NestedContentPropertyEditor.cs#L164
because it's expecting an INT. NC needs to validate the input here to ensure it's the correct data type.
We should also update the NC Courier resolver to take this into account so that when it transfers it doesn't actually include the GUID if it knows the item was in the trash.
Hmm, I'm not sure how we should validate this though as NC is just building up a fake context to pass on the ConvertDbToString call. For us to parse that value would mean we would need to know what the property type is and what it's expected value type is and then cast it which would end up giving us a dependency on the given property editors.
Shouldn't this get handled in the given property editors ConvertDbToString function? or are you saying our code physically throws an exception at the .ToString()? If it's the later I'm not sure why as we are doing a null check.
When you do this:
var prop = new Property(propType, propValues[propKey] == null ? null : propValues[propKey].ToString());
This throws an exception, for example:
System.InvalidOperationException occurred
HResult=0x80131509
Message=Value "3885acdb-3ed7-43bf-ae39-1a46ea94c290" of type "System.String" could not be converted to type "System.Int32" which is expected by property type "image".
Source=Umbraco.Core
StackTrace:
at Umbraco.Core.Models.Property.ThrowTypeException(Object value, Type expected, String alias) in X:\Projects\Umbraco\Umbraco_7.5\src\Umbraco.Core\Models\Property.cs:line 138
at Umbraco.Core.Models.Property.set_Value(Object value) in X:\Projects\Umbraco\Umbraco_7.5\src\Umbraco.Core\Models\Property.cs:line 176
at Umbraco.Core.Models.Property..ctor(PropertyType propertyType, Object value) in X:\Projects\Umbraco\Umbraco_7.5\src\Umbraco.Core\Models\Property.cs:line 35
at Our.Umbraco.NestedContent.PropertyEditors.NestedContentPropertyEditor.NestedContentPropertyValueEditor.ConvertDbToString(Property property, PropertyType propertyType, IDataTypeService dataTypeService)
at Umbraco.Core.PropertyEditors.PropertyValueEditor.ConvertDbToXml(Property property, PropertyType propertyType, IDataTypeService dataTypeService) in X:\Projects\Umbraco\Umbraco_7.5\src\Umbraco.Core\PropertyEditors\PropertyValueEditor.cs:line 350
at Umbraco.Core.Services.EntityXmlSerializer.Serialize(IDataTypeService dataTypeService, Property property) in X:\Projects\Umbraco\Umbraco_7.5\src\Umbraco.Core\Services\EntityXmlSerializer.cs:line 129
at Umbraco.Core.Services.EntityXmlSerializer.Serialize(IDataTypeService dataTypeService, IContentBase contentBase, String nodeName) in X:\Projects\Umbraco\Umbraco_7.5\src\Umbraco.Core\Services\EntityXmlSerializer.cs:line 498
at Umbraco.Core.Services.EntityXmlSerializer.Serialize(IContentService contentService, IDataTypeService dataTypeService, IUserService userService, IContent content, Boolean deep) in X:\Projects\Umbraco\Umbraco_7.5\src\Umbraco.Core\Services\EntityXmlSerializer.cs:line 38
at UmbracoExamine.UmbracoContentIndexer.<PerformIndexAll>b__35_4(IContent c) in X:\Projects\Umbraco\Umbraco_7.5\src\UmbracoExamine\UmbracoContentIndexer.cs:line 526
at UmbracoExamine.UmbracoContentIndexer.<GetSerializedContent>d__37.MoveNext() in X:\Projects\Umbraco\Umbraco_7.5\src\UmbracoExamine\UmbracoContentIndexer.cs:line 659
at Examine.LuceneEngine.Providers.LuceneIndexer.ForceProcessQueueItems(Boolean block)
Except this exception is thrown anytime you visit the node, re-index the node, try to rebuild the data integrity check, basically a lot of things and unfortunately renders much of the site in a broken state.
The value type is validated inside of the ctor of the Property you are creating and the way we validate it is to throw an exception because it is not allowed.
Ok, well all I can think to do is duplicate what's in Property and do an IsPropertyValueValid check and set to NULL if not.
var val = propValues[propKey] == null ? null : propValues[propKey].ToString();
if (!propType.IsPropertyValueValid(val))
{
val = null;
}
var prop = new Property(propType, val);
Would that suffice?
Hmm, I think this would still fail as IsPropertyValueValid allows strings so IsPropertyValueValid isn't picking up the incorrect type either. Though personally, I think that's a more logical place the fix should be made.
Only other thing we could do in NC whilst remaining un-dependant on any prop editors would be to wrap it in a try catch
Hrm, as it turns out stephen has already made changes in that method in 7.6 - for reasons i don't know but I've asked : umbraco/Umbraco-CMS@09526e6
But a try catch might be the best/easiest for now, just so it doesn't stop parts of the site from operating - i think that might be the best win, just catch an InvalidOperationException and you'll know it's because of an invalid value
Will do, thanks for the feedback
Well this is the IsPropertyValueValid now: https://github.com/umbraco/Umbraco-CMS/blob/dev-v7.6/src/Umbraco.Core/Models/PropertyType.cs#L365 and IsPropertyTypeValid is this: https://github.com/umbraco/Umbraco-CMS/blob/dev-v7.6/src/Umbraco.Core/Models/PropertyType.cs#L308
In any case I think try catch is easiest :)
You are very very very fast sir!
I aim to please :)