Setting internal properties?

[leekelleher]

While upgrading one of my projects to Ditto v0.9.0-beta, I hit an error in the new PropertyInfoInvocations class - it said that the setter method passed to BuildGetAccessor is null.

I checked my model and found something like this...

public string MyProperty { get; internal set; }

I'm not entirely sure why I had originally set the property to have an internal setter, (I probably didn't need to do it), however this did work with Ditto v0.8.x, so my question is... Should we correct this?

I'm mixed on the semantics of this, whether internal should be that only the user's code should set the value, or that it means once it's set (by Ditto), it's effectively read-only.

[mattbrailsford]

Hmm, should this be a config option? (default off) If so, then it's an option of setting globally or per mapping?

[leekelleher]

I've been playing with a unit-test for this, it the setter is omitted, then Ditto will ignore it. The internal setter throws an exception, (which is only a null check needed to fix it).

I'm currently swaying towards that we set any marked as internal.

[leekelleher]

I've put my unit-test in PR #173

[JimBobSquarePants]

Did it work with the lazy loading on 0.8x?

I'm not sure Ditto should try to set it. If you're marking something as internal there's usually good reason and we'd be breaking contract rules. We certainly shouldn't throw an exception anyway.

[leekelleher]

I didn't test lazy-loading. I'll give it a try.

Reviewing the rest of my app code, my intention for having an internal setter was to let Ditto populate the value, then let it be read-only in the my app. But I can see that it wasn't the best design decision (given no one is ever going to try to update the value in my app). I've now made it public.

[JimBobSquarePants]

Cool. I tend to avoid internal setters unless they are absolutely necessary. Even then I always feel like I have done something wrong.

[leekelleher]

Just pushed the test I did for the lazy-loading, brace yourselves, weird results.

https://github.com/leekelleher/umbraco-ditto/pull/173/files#diff-a79b95e85e860f5f18ac6cd5331f1462

The non-virtual property with internal setter doesn't set the value, but the virtual property with internal setter does. :confused:

@JimBobSquarePants For the virtual properties, does this use the Proxy code to call the Setter?

[JimBobSquarePants]

@leekelleher Yeah it does because the proxy is intercepting the Get/Set MethodBase. It doesn't actually Get/Set the property on the item, rather it updates an internal dictionary.

You can see that here:

https://github.com/leekelleher/umbraco-ditto/blob/c68d5e633d6e3712861188637424c3221f0b5694/src/Our.Umbraco.Ditto/Proxy/LazyInterceptor.cs#L56

I think the best fix rather than augmenting PropertyInfoInvocations is to add extra sanitising code earlier when we are collecting the properties to parse.

The line here:

https://github.com/leekelleher/umbraco-ditto/blob/c68d5e633d6e3712861188637424c3221f0b5694/src/Our.Umbraco.Ditto/Extensions/PublishedContentExtensions.cs#L318

Should probably read something like.

var properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance)
                                 .Where(x => x.CanWrite && x.GetSetMethod() != null).ToArray();

I think that should do it.

[leekelleher]

Thanks @JimBobSquarePants, good idea! I'll give it a try/test later today.

[leekelleher]

Closing this issue off, updated PR #173 with fixes.

Agreed approach is that if the property's get method is not public, then we don't attempt to set the value.