YSOD When populating a nested Ditto POCO in v0.7.0 about umbraco-ditto HOT 18 CLOSED

Thanks @jamiepollock. The gist is super useful, we can make a unit-tested based on this and see what is happening internally.

from umbraco-ditto.

@leekelleher Awesome, I look forward to hearing the results.

from umbraco-ditto.

@leekelleher there was actually an error with the gist, that's what I get for modifying actual code into gist form. That constructor should be public :) I've modified it now in rev 3.

from umbraco-ditto.

I don't understand why you are using Ditto to create the BasicViewModel like that with multiple nested As(). Surely use As() on SEO and pass that as a constructor parameter?

from umbraco-ditto.

I've tried this out with a unit-test and can reproduce the issue.

The problem comes when Ditto is checking the Seo property against a potential value from Umbraco node. Because the node doesn't have a corresponding property, it returns a null. Then regardless of what the BasicPageViewModel did in its constructor, the Seo property is set as null.

(I hope this makes sense so far?)

I think the resolution would be for Ditto to check if the property already has a value set, and if the potential value (from the Umbraco node) is null, then we don't set the property value.

See line 368 in PublishedContentExtensions.cs, instead we could null check the result value?

object propertyValue = GetRawValue(content, culture, propertyInfo, instance);
var result = GetTypedValue(content, culture, propertyInfo, propertyValue, instance);

if (result != null)
{
    propertyInfo.SetValue(instance, result, null);
}

or maybe we need to check the current value of the model's property?

e.g. var currentValue = propertyInfo.GetValue(instance, null)

then figure out if we want to proceed with getting the GetRawValue() or not?

@JimBobSquarePants - I can see why @jamiepollock has taken this approach. I think his BasicViewModel would have many more properties. Passing them in via the constructor may be a lot more code. (But I'm making an assumption here - any comment @jamiepollock?)

As a cleaner way forwards, I'm wondering if there is a better way to populate a model's property with the same IPublishedContent? Do any of the existing built-in TypeConverters do this? (or maybe it's a job for a new custom ValueResolver?

from umbraco-ditto.

@jamiepollock Until we get a fix (or proposed fix) in place, then you may have to rework your model like this...

public class BasicPageViewModel
{
    private IPublishedContent _content;

    public BasicPageViewModel(IPublishedContent content)
    {
        _content = content;
    }

    private SeoViewModel _seo;

    public SeoViewModel Seo
    {
        get
        {
            if (_seo == null)
            {
                _seo = _content.As<SeoViewModel>();
            }

            return _seo;
        }
    }
}

Maybe not ideal.. but hey, it would give you a pseudo-lazy-loading 😼

from umbraco-ditto.

Given he doesn't want Ditto to set the propperty direct, shouldn't he be using [DittoIgnore] attribute on the SEO property. This should then bypass Ditto doing anything with that property?

from umbraco-ditto.

PS I think the current logic is correct personally, in that, unless you tell dito to explicitly ignore a property, it will assume it needs setting, thus if you want to control setting it yourself, use [DittoIgnore] and ditto will ignore your property then you can control setting it yourself. I don't think ditto should be first checking for a current value.

from umbraco-ditto.

from umbraco-ditto.

Ahh [DittoIgnore]! I'll wire it up on Monday to see it working.

I've also thought nested POCOs were wired up via the constructor and as. That way you could query the children of a IPublishedContent in the same way and populate a property of the page.

This'll break a few of our existing projects but it's my fault for being lazy and not adding the correct attribute to non-bindable properties.

from umbraco-ditto.

Hey all,
I can confirm adding [DittoIgnore] will work.

Just a note, the Seo properties needs to be populated via the constructor like so:

protected BasicPageViewModel(IPublishedContent content) {
   Seo = content.As<SeoViewModel>();
}

Simply leaving it out and applying [DittoIgnore] won't populate the nested POCOs via model.As<BasicPageViewModel>().

I believe this issue is closed unless then there is any further discussion about how nested POCOs should be treated. (e.g. try populating [DittoIgnore]'d POCOs automagically if they have any attributes. Personally I'm happy to populate them via the constructor.

Thanks,
Jamie

EDIT: I've updated the gist with the working solution. Added a note that pre rev4 this didn't work.

from umbraco-ditto.

@jamiepollock I'll close this issue off soon... I'm interested in discussing ways that Ditto can "re-apply itself" to its properties. Meaning that you wouldn't need to try to do it within the constructor.

e.g.

public class BasicPageViewModel
{
    [DittoReapplyContent]
    public SeoViewModel Seo { get; set; }
}

Obviously DittoReapplyContent is an absolutely terrible name... (the idea is still boiling)

from umbraco-ditto.

@leekelleher Seems like a sensible choice to mark a nested POCO property as something the initial As() should also automagically propagate.

A few possibles

[DittoNested]
[DittoProperty]
[DittoObject]
[DittoContent]

from umbraco-ditto.

Could this simply be a [NestedValueResolver]?

from umbraco-ditto.

I'm struggling with the word "nested", can't see what is nested about a POCO property?

Shall we close this, and open a new issue for this?

from umbraco-ditto.

I agree. By itself a POCO should work in isolation, this is entirely to do with the context that POCO is being used in. In this case it should be populated as a property by the parent POCO when called using As().

from umbraco-ditto.

Lets move it to a new issue

from umbraco-ditto.

Great! Now I need to figure out how to explain it (again) ;-)

from umbraco-ditto.