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

ExtraProperties in nested DTOs are not deserialized in Blazor WebAssembly #9581

Closed
MichalCadecky opened this issue Jul 13, 2021 · 13 comments · Fixed by #9626
Closed

ExtraProperties in nested DTOs are not deserialized in Blazor WebAssembly #9581

MichalCadecky opened this issue Jul 13, 2021 · 13 comments · Fixed by #9626
Assignees
Milestone

Comments

@MichalCadecky
Copy link

  • Your ABP Framework version: 4.3.2
  • Your User Interface type: Blazor WebAssembly
  • Your database provider: EF Core

Hello, consider following objects/DTOs:

public class FooDto : ExtensibleObject
{
    public string Name { get; set; }
    public List<BarDto> Bars { get; set; }
}

public class BarDto : ExtensibleObject
{
    public string Value { get; set; }
}

and service:

public interface IFooAppService : IAppService
{
    Task<FooDto> GetFooAsync(string name);
    Task<BarDto> GetBarAsync(string value);
    Task<ListResultDto<BarDto>> GetBarsAsync(string fooName);
}

Both FooDto and BarDto are ExtensibleObject and FooDto has a list of BarDtos. If I call GetFooAsync in my Blazor page, I will get FooDto with its Name, Bars and ExtraProperties but BarDtos in Bars have only Value but the ExtraProperties are empty. I have verified tha the data are sent from the AppService and have been transfered to the client (received JSON had the ExtraProperties filled).

However calling the GetBarAsync or GetBarsAsync will result in getting the BarDto(s) with ExtraProperties containing all relevant data.

All these observations makes me believe there is an issue with the deserialization of received JSON on the Blazor side. Maybe this is some kind of limitation or misconfiguration of the JsonSerializer.

Any ideas or advices will be much appriciated. I don't really like the idea/workaround to create new method just to receive the dto with ExtraProperties filled which could result in N+1 data retrieval problem.

@maliming maliming self-assigned this Jul 13, 2021
@maliming
Copy link
Member

HI

You can try to custom the converter to handle the FooDto type.

https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-converters-how-to?pivots=dotnet-5-0

@maliming maliming removed their assignment Jul 14, 2021
@MichalCadecky
Copy link
Author

I have implemented my own JsonConverter with ExtraProperties deserialization and that indeed works... But that requires to setup JsonConverter for each respective object which is no-go from my perspective.

The thing is - I have introduced even more nesting, 3 layers, 4 layers, etc... and all objects are deserialized perfectly fine. Just the ExtraPropertyDictionary are never properly deserialized if the object holding it is nested.

I believe there has to be a problem with the AbpHasExtraPropertiesJsonConverter which is working only with the RootElement of the JSON document. I did not have enough time to properly analyze it but my guts are telling me this might be the source of the issue.

@maliming
Copy link
Member

hi @MichalCadecky

Can you share your converters?

@MichalCadecky
Copy link
Author

It is nothing really special. Just raw JSON writing and reading based on read token type. It is ignoring the JsonSerializerOptions but for the matter of the test it was not really necessary.

public class BarDtoConverter : JsonConverter<BarDto>
{
    public override BarDto Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        var dto = new BarDto();

        while (reader.Read())
        {
            switch (reader.TokenType)
            {
                case JsonTokenType.PropertyName:
                    DoProperty(ref reader, dto);
                    break;
                case JsonTokenType.EndObject:
                    return dto;
            }
        }

        return dto;
    }

    private void DoProperty(ref Utf8JsonReader reader, BarDto dto)
    {
        var name = reader.GetString();
        switch (name)
        {
            case nameof(BarDto.Value):
                reader.Read();
                dto.Value = reader.GetString();
                break;
            case nameof(BarDto.ExtraProperties):
                reader.Read();
                while (reader.TokenType != JsonTokenType.EndObject)
                {
                    if (reader.TokenType == JsonTokenType.PropertyName)
                    {
                        var extraName = reader.GetString();
                        reader.Read();
                        var value = reader.GetString();
                        dto.SetProperty(extraName, value, false);
                    }

                    reader.Read();
                }
                break;
        }
    }

    public override void Write(Utf8JsonWriter writer, BarDto value, JsonSerializerOptions options)
    {
        writer.WriteStartObject();
        writer.WriteString(nameof(BarDto.Value), value.Value);

        writer.WritePropertyName(nameof(BarDto.ExtraProperties));
        writer.WriteStartObject();
        foreach (var prop in value.ExtraProperties)
        {
            writer.WriteString(prop.Key, prop.Value.ToString());
        }
        writer.WriteEndObject();

        writer.WriteEndObject();
    }

    public override bool CanConvert(Type typeToConvert)
    {
        return typeToConvert == typeof(BarDto);
    }
}

@maliming maliming self-assigned this Jul 21, 2021
@maliming maliming added this to the 5.0-preview milestone Jul 21, 2021
maliming added a commit that referenced this issue Jul 21, 2021
@maliming
Copy link
Member

See #9626

@MichalCadecky
Copy link
Author

I have tried to replace the original AbpHasExtraPropertiesJsonConverterFactory and AbpHasExtraPropertiesJsonConverter with the implementations in proposed pull request and it does not seems to work.

In addition, I am quite unsure if the ExcludeTypes property should be static. It will cause to "convert" the respective type only once and falling back to default deserialization consequently. Have you tried to run it multiple times in a row?

@maliming
Copy link
Member

I have tried to replace the original AbpHasExtraPropertiesJsonConverterFactory and AbpHasExtraPropertiesJsonConverter with the implementations in proposed pull request and it does not seems to work.

Can you share your steps and code?

In addition, I am quite unsure if the ExcludeTypes property should be static. It will cause to "convert" the respective type only once and falling back to default deserialization consequently. Have you tried to run it multiple times in a row?

You are right, I will update.

@MichalCadecky
Copy link
Author

I have copied both AbpHasExtraPropertiesJsonConverterFactory and AbpHasExtraPropertiesJsonConverter and created AbpHasExtraPropertiesJsonConverterFactory2 and AbpHasExtraPropertiesJsonConverter2 with the code from the changes files.

And replaced the converters like this:

context.Services.Configure<AbpSystemTextJsonSerializerOptions>(options =>
{
    var converter = options.JsonSerializerOptions.Converters.FirstOrDefault(x => x is AbpHasExtraPropertiesJsonConverterFactory);
    if (converter != null)
    {
        options.JsonSerializerOptions.Converters.Remove(converter);
    }

    options.JsonSerializerOptions.Converters.Add(new AbpHasExtraPropertiesJsonConverterFactory2());
});

@MichalCadecky
Copy link
Author

Ok, I understand the issue now.

Creating the new options like this:

var newOptions = JsonSerializerOptionsHelper.Create(options, x =>
    x == this ||
    x.GetType() == typeof(AbpHasExtraPropertiesJsonConverterFactory));

will cause the converter to be removed from the available list of converters and later on, when actual deserialization is called

var extensibleObject = JsonSerializer.Deserialize<T>(rootElement.GetRawText(), newOptions);

it will deserialize the object itself but there will be no AbpHasExtraPropertiesJsonConverter for the nested objects causing the nested deserialization to just fallback to default and not knowing what to do with the ExtraProperties.

@maliming
Copy link
Member

hi

You can try to change the JsonOptions and check this commit.

context.Services.Configure<JsonOptions>(options =>
{
    var converter = options.JsonSerializerOptions.Converters.FirstOrDefault(x => x is AbpHasExtraPropertiesJsonConverterFactory);
    if (converter != null)
    {
        options.JsonSerializerOptions.Converters.Remove(converter);
    }

    options.JsonSerializerOptions.Converters.Add(new AbpHasExtraPropertiesJsonConverterFactory2());
});

@maliming
Copy link
Member

it will deserialize the object itself but there will be no AbpHasExtraPropertiesJsonConverter for the nested objects causing the nested deserialization to just fallback to default and not knowing what to do with the ExtraProperties.

Yes, this is the point. 👍 I will try to do some refactor.

@MichalCadecky
Copy link
Author

MichalCadecky commented Jul 21, 2021

Yes, provided commit has fixed the issue. But...

I have just found that ExtendedObject has protected setter for ExtraProperties, that is why the JsonSerializer is ignoring it. And that is why there has to be special handling for the objects implementing IHasExtraProperties and inheriting from ExtendedObject at the same time. I understand why ExtendedObject has the protected setter for ExtraProperties - it is used for entities where it completely make sense. But for DTOs it is a little bit inconvenient and causing unnecessities like this.

Wouldn't it be easier just to create a new ExtensibleObjectDto which implements IHasExtraProperties and has ExtraProperties with public setter? AbpHasExtraPropertiesJsonConverterFactory and AbpHasExtraPropertiesJsonConverter could be then completely removed and the deserialization will be even more performant.

@maliming maliming modified the milestones: 5.0-preview, 4.4-final Jul 22, 2021
@maliming maliming linked a pull request Jul 22, 2021 that will close this issue
@maliming
Copy link
Member

I have just found that ExtendedObject has protected setter for ExtraProperties, that is why the JsonSerializer is ignoring it.

This is the limit of System.Text.Json. I think it will be supported in the future.

#6095

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants