JasperFx / alba

Easy integration testing for ASP.NET Core applications
https://jasperfx.github.io/alba
Apache License 2.0
405 stars 39 forks source link

`required` properties will trigger a `AlbaJsonFormatterException` when requesting a nested object #153

Closed jsgoupil closed 8 months ago

jsgoupil commented 8 months ago

Alba 7.4.1. .NET 8 xunit 2.5.3

When using these models:

public class RequiredModel
{
    public required string Id { get; set; }
}

public class OptionalModel
{
    public required string Id { get; set; }
}

public class Group
{
    public required RequiredModel RequiredModel { get; set; }
    public OptionalModel? OptionalModel { get; set; }
}

Calling this API:

[Produces("application/json")]
[HttpGet(Name = "GetWeatherForecast")]
public IActionResult Get()
{
    return Ok(new Group { RequiredModel = new RequiredModel() { Id = "test" } });
}

This code will fail

[Fact]
public async Task Test1()
{
    await using var host = await AlbaHost.For<global::Program>();

    // Arrange
    // Act
    var response = await host.Scenario(_ =>
    {
        _.Get.Url($"/WeatherForecast");
        _.StatusCodeShouldBeOk();
    });

    // Assert
    var result = await response.ReadAsJsonAsync<RequiredModel>();
}

The ReadAsJsonAsync will fail with this exception:

Alba.AlbaJsonFormatterException : The JSON formatter was unable to process the raw JSON:
{"requiredModel":{"id":"test"},"optionalModel":null}

There are 2 workarounds:

  1. Removing the required from the RequiredModel.Id, everything works properly.
  2. Requesting the Group rather than RequiredModel in the Assert:
    var result = await response.ReadAsJsonAsync<Group>();

Expected Using RequiredModel as in the code above should work with or without the required keyword.

Hawxy commented 8 months ago

I'm not sure why you're trying to deserialize a nested entity directly. We don't say we support this anywhere, nor is it an expected convention within .NET. The STJ serializer is throwing an exception within the MVC input formatter because it doesn't support this scenario and has no idea what you're trying to do.

Removing the required from the RequiredModel.Id, everything works properly.

Are you sure? Unless you're running custom converters the deserializer is going to return null (STJ) or give you a default object (Newtonsoft).

jsgoupil commented 8 months ago

I'm inheriting some code here. However, "removing the required", I am 100% sure 😄

I thought I was stuck and found the workaround.

I don't need further support but just wanted to let you know in case this is something that was important that you might want to fix. I thought you might be using the MS libraries, but I saw some custom JSON parsing, so I'm not 100% sure if this is your bug or MS bug.

I have pushed the code here so you can give it a shot: https://github.com/jsgoupil/Alba.Required

You're welcome to close the bug if you don't need to go further. I will use the Group in this case.

Thanks for your support and library 👍🏻

Hawxy commented 8 months ago

However, "removing the required", I am 100% sure 😄

I tested your example, Id returns null instead of "test" when I removed the required modifier, which aligns with my expectations of this scenario not being supported: image

I don't need further support but just wanted to let you know in case this is something that was important that you might want to fix. I thought you might be using the MS libraries, but I saw some custom JSON parsing, so I'm not 100% sure if this is your bug or MS bug.

It's on the Microsoft side, for controllers we use the built-in MVC input/formatters and they in turn call whichever serializer you have configured. Alba does very little custom behavior at the serialization level.

Thanks for your support and library

Enjoy!