umco / umbraco-ditto

Ditto - the friendly view-model mapper for Umbraco
http://our.umbraco.org/projects/developer-tools/ditto
MIT License
79 stars 33 forks source link

Problem with overridden properties that depend on other properties #177

Closed Slowacki closed 8 years ago

Slowacki commented 8 years ago

I have a following-ish setup in my project.

An abstract class with some virtual properties:

[JsonObject(MemberSerialization.OptIn)]
    public abstract class Base: IInterface1, IInterface2
    {
        [JsonProperty(PropertyName = "id")]
        public virtual string IdString { get { return IdInt.ToString(); } }

        [JsonProperty]
        public virtual int IdInt { get; set; }
    }

And a class that inherits it:

 [JsonObject(MemberSerialization.OptIn)]
    public class NewClass : Base
    {    
        [JsonProperty(PropertyName = "id2")]
        public override string IdString { get { return IdInt.ToString(); } }

        [JsonProperty("id1")]
        [TypeConverter(typeof(IdContentPickerConverter))]
        [UmbracoProperty("ItemLink")]
        public override int IdInt { get; set; }
    }

After mapping IPublishedContent to NewClass, what I would expect is to have IdInt and IdString output as the same value, but one as an int and the other as a string. However, unfortunately that is not what happens. The output looks like that:

{
id2: "0",
id1: 1613
}

If I instead create a new property instead of IdString in the NewClass or change the modifier from override to new (which I obviously don't want to do), the IdString value is output correctly.

I'm using Ditto v0.8.3 and Umbraco v7.3.7.

leekelleher commented 8 years ago

@Slowacki Thanks for raising this. Ditto shouldn't be attempting to do anything with the IdString property, since it doesn't have a setter method. But I'll set up a unit-test to try out this scenario.

leekelleher commented 8 years ago

@Slowacki OK, I can reproduce the error, it seems to be related to using the virtual properties within the abstract Base class.

We'll probably need @JimBobSquarePants to take a look at this too. Here's a gist of the unit-test I made (branched off from "0.8.4" tag)...

https://gist.github.com/leekelleher/5985a13e6716d77c2b5d0790b7db21d2

JimBobSquarePants commented 8 years ago

@leekelleher What does v0.9.0 do in this scenario?

leekelleher commented 8 years ago

@JimBobSquarePants I haven't tried it yet... will do (between CG16 sessions) :metal:

leekelleher commented 8 years ago

@JimBobSquarePants Tried it... it's the same issue on v0.9.0. (No rush on this though, we're at CG16 after all :smirk_cat:)

mattbrailsford commented 8 years ago

@JimBobSquarePants you had chance to look at this yet?

leekelleher commented 8 years ago

Shall I include the failing unit-test I made, (see gist) in a PR? Then we can assess it further

leekelleher commented 8 years ago

I've finally added the unit-test in its own branch/PR; for further review.

JimBobSquarePants commented 8 years ago

Ah great. Still think this setup is weird though.

leekelleher commented 8 years ago

@JimBobSquarePants I know what you mean, it's not a typical set-up that I'd use myself either. However when I was setting up the unit-test, I set the assertions to be what I'd expect the results should be...

https://github.com/leekelleher/umbraco-ditto/blob/d9bd32178c86a6b654e2802de3fb0c2cca638d9a/tests/Our.Umbraco.Ditto.Tests/Issue177Tests.cs#L48

...and those expectations failed. Whether that's the right or wrong way, we can discuss over on the PR.

JimBobSquarePants commented 8 years ago

@leekelleher I'll see what I can do asap.

mattbrailsford commented 8 years ago

Should we close this ticket down, as this should be solved with PR #191 right?

leekelleher commented 8 years ago

I agree, closing this ticket. Any discussion can be carried on PR #191.