Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot map Value or HasValue Property of Nullable<T> #571

Closed
onionhammer opened this issue Jul 17, 2023 · 15 comments
Closed

Cannot map Value or HasValue Property of Nullable<T> #571

onionhammer opened this issue Jul 17, 2023 · 15 comments
Labels
bug Something isn't working

Comments

@onionhammer
Copy link

onionhammer commented Jul 17, 2023

Describe the bug
I upgraded from next-2 to next-3 and now I am receiving a build error

ModelMapper.g.cs(705,78): error CS1061: 'JsonElement' does not contain a definition for 'Value' and no accessible extension method 'Value' accepting a first argument of type 'JsonElement' could be found (are you missing a using directive or an assembly reference?)

It looks like the generated code is calling .Value twice:

// Generated code
        {
            var target = new global::SellerAmend()
            {
                Kind = source.Kind,
                SellerPartnerId = source.SellerPartnerId,
                SellerLoanId = source.SellerLoanId,
                DateCreated = source.DateCreated
            };
            if (source.ListingAmendment != null)
            {
                                                                   // .Value.Value should just be .Value
                target.ListingAmendmentValue = source.ListingAmendment.Value.Value.ToString();
            }

            return target;
        }

@latonz latonz added the bug Something isn't working label Jul 17, 2023
@latonz
Copy link
Contributor

latonz commented Jul 17, 2023

Thanks for the bug report, I'll look into it.

@onionhammer
Copy link
Author

Here's a repro:

[Mapper(PropertyNameMappingStrategy = PropertyNameMappingStrategy.CaseInsensitive)]
public class MapperClass
{
    public static partial Car MapCar(CarDto input);
    public static partial CarDto MapCar(Car input);
}

public record CarDto()
{
    public JsonElement? Amendment { get; set; }
}

public record Car()
{
    [JsonIgnore, System.Runtime.Serialization.IgnoreDataMember]
    public OtherThing? AmendmentMetadata
    {
        get => Amendment?.Deserialize<OtherThing>(JsonExtensions.JsonOptions);
    }

    [JsonIgnore, System.Runtime.Serialization.DataMember]
    public string? AmendmentValue
    {
        get => Amendment?.GetRawText();
        set => Amendment = value != null ? JsonSerializer.Deserialize<JsonElement>(value) : null;
    }
    
    [JsonInclude, System.Runtime.Serialization.IgnoreDataMember]
    public JsonElement? Amendment { get; set; }
}

public class OtherThing
{
    public string? Value { get; set; }
}

@TimothyMakkison
Copy link
Collaborator

TimothyMakkison commented Jul 17, 2023

Away rn, but I'm guessing because Amendment is a Nullable<JsonElement>, mapperly sees an object with a Value field and attempts to map it? Because mapperly automatically accesses the Value field (because it knows its not null), it then attempts to now "access" the value member, and fails because it has already been accessed.

@onionhammer
Copy link
Author

onionhammer commented Jul 17, 2023

@TimothyMakkison I think its because of the Value suffix and some special case with Mapperly for nullables?

If you rename AmendmentValue to AmendmentVal it compiles.

@TimothyMakkison
Copy link
Collaborator

@TimothyMakkison I think its because of the Value suffix and some special case with Mapperly for nullables?

If you rename AmendmentValue to AmendmentVal it compiles.

Yeah I've updated my comment with more speculation. I'm guessing it only affects nullable value types because its an actual type.
Does this not happen with V2?

@onionhammer
Copy link
Author

@TimothyMakkison maybe it was just doing a throw new NotImplementedException on v2

@TimothyMakkison
Copy link
Collaborator

TimothyMakkison commented Jul 17, 2023

It's strange this issue didn't crop up when field mapping was introduced.
Iirc one of my nit pick optimizations changed how nullable types were checked, perhaps that broke it. #555

@latonz
Copy link
Contributor

latonz commented Jul 17, 2023

One bug which I just found but also existed in v2.8.0 is that a nested nullable struct's flattened values cannot be resolved. I created another issue #572 since I'm not sure if it is related to this issue.

@TimothyMakkison
Copy link
Collaborator

I don't think #555 caused it I've been using the following

[Fact]
public void VaueTest()
{
    var source = TestSourceBuilder.MapperWithBodyAndTypes(
        "partial B Map(A src);",
        "public record A(int? P);",
        "public record B { public int PValue { get; set; } }"
    );
    TestHelper
        .GenerateMapper(source)
        .Should()
        .HaveSingleMethodBody(
            """
            var target = new global::B();
            if (src.P != null)
            {
                target.PValue = src.P.Value.Value;
            }

            return target;
            """
        );
}

@latonz
Copy link
Contributor

latonz commented Jul 17, 2023

@TimothyMakkison The test you provided, which should reproduce the bug described by OP, fails on v2.8.0. So this probably exists since a longer time and isn't a regression of v2.9.0-next.3

@TimothyMakkison
Copy link
Collaborator

The test passes when the bug occurs. Does this mean the bug doesn't happen with 2.8?

@latonz
Copy link
Contributor

latonz commented Jul 17, 2023

@TimothyMakkison sorry my bad, I adjusted the test to verify that the bug does not exist. If I use exact your version, it fails on v2.8.0 as well as on v2.9.0-next.3.
The problem is that the target property has a auto-flattened name of ending with Value. When resolving the name, autoflattening resolves it to P.Value (the value property of Nullable<T>). Then when building the mapping, the builder encounters a nullable value type and adds another .Value to access the value inside the null if condition. It relates to #572. If Nullable<T> properties are treated as T while resolving mappable members, the problem shouldn't occur.

@onionhammer
Copy link
Author

Name of this issue makes no sense anymore, should we close as duplicate?

@latonz latonz changed the title Using derived type with 2.9.0-next.3 yields 'CS1061' 'JsonElement does not contain a definition for Value' Cannot map Value or HasValue Property of Nullable<T> Jul 18, 2023
@latonz
Copy link
Contributor

latonz commented Jul 18, 2023

I adjusted the title. I think this is a slightly different issue than #572. Here the name of the target property Value which matches the Value property of Nullable<T>, which is special cased, is a problem. While in #572 the problem is that the member resolution does take Nullable<T> into account, while the user certainly expects T. However, the solution could probably be the same.

@latonz
Copy link
Contributor

latonz commented Jul 18, 2023

Rel. #98

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants