-
Notifications
You must be signed in to change notification settings - Fork 33
YSOD When populating a nested Ditto POCO in v0.7.0 #77
Comments
Thanks @jamiepollock. The gist is super useful, we can make a unit-tested based on this and see what is happening internally. |
@leekelleher Awesome, I look forward to hearing the results. |
@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. |
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? |
I've tried this out with a unit-test and can reproduce the issue. The problem comes when Ditto is checking the (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 See line 368 in 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. then figure out if we want to proceed with getting the @JimBobSquarePants - I can see why @jamiepollock has taken this approach. I think his As a cleaner way forwards, I'm wondering if there is a better way to populate a model's property with the same |
@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 😼 |
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? |
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. |
Ah yes, the [DittoIgnore] attribute!
Always good to have a fresh pair of eyes on these things, thanks
@mattbrailsford!
|
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. |
Hey all, 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 I believe this issue is closed unless then there is any further discussion about how nested POCOs should be treated. (e.g. try populating Thanks, EDIT: I've updated the gist with the working solution. Added a note that pre rev4 this didn't work. |
@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 |
@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] |
Could this simply be a [NestedValueResolver]? |
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? |
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 |
Lets move it to a new issue |
Great! Now I need to figure out how to explain it (again) ;-) |
Hey folks,
I'm currently trying out the v0.7.0 release, however when I try populating a POCO from within a POCO it gives an Object not set to reference YSOD when rendering the view. Here's my gist with the code.
On debug, I can see the constructor being called but the Seo object is null. The properties also exist in Umbraco as I've checked they exist.
This code works on v0.6.1 and the properties populate fine.
Thanks,
Jamie
The text was updated successfully, but these errors were encountered: