dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.34k stars 9.98k forks source link

Asp.Net Core does not properly handle the error with polymorphic deserialization from STJ when type discriminator is not specified #54037

Open ilya-scale opened 8 months ago

ilya-scale commented 8 months ago

Description

When type discriminator is not specified in the request, the error is not handled and 500 is returned instead of 400.

Example request 1:

{
    "type": "unknown",
    "someOtherProperty": "value"
}

Example request 2:

{
    "someOtherProperty": "value"
}

Controller action:

[HttpPost("test")]
public void Test(Base request)
{

}

Models:

[JsonPolymorphic(TypeDiscriminatorPropertyName = "type")]
[JsonDerivedType(typeof(Derived), "derived")]
public abstract record Base
{
}

public record Derived : Base
{
}

Reproduction Steps

  1. Create a controller with the action and models from Description
  2. Do a call with the example request 1, response is 400
{
    "type": "https://tools.ietf.org/html/rfc9110#section-15.5.1",
    "title": "One or more validation errors occurred.",
    "status": 400,
    "errors": {
        "$": [
            "Read unrecognized type discriminator id 'unknown'. Path: $ | LineNumber: 2 | BytePositionInLine: 21."
        ],
        "request": [
            "The request field is required."
        ]
    },
    "traceId": "00-a4e0c60070a4c53222d0622acad8d198-e18068d6fe5d5773-00"
}
  1. Do a call with the example request 2, response is 500
    System.NotSupportedException: Deserialization of types without a parameterless constructor, a singular parameterized constructor, or a parameterized constructor annotated with 'JsonConstructorAttribute' is not supported.

Expected behavior

Ideally I expect the 400 error that will say clearly that the "type" TypeDiscriminator has to be provided (since the base class is abstract it is clear it cannot be constructed).

At the very least some form of 400 with json that tells that json cannot be deserialized instead of 500 (there was nothing "wrong" with the server, but client provided a bad request which is clearly a 400).

Actual behavior

A 500 is returned and the exception is not handled by asp.net core (unlike example 1 request)

Regression?

No response

Known Workarounds

One can probably make class not abstract and do a validation oneself that the "base" class is not supported.

Configuration

.Net 8 MacOS ARM

Other information

It is somewhat related to the case when type discriminator is not the first field. If it is not the first - it will be the same error I think, which should also be handled ideally, but this can be fixed in .Net 9 hopefully.

ghost commented 8 months ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details
### Description When type discriminator is not specified in the request, the error is not handled and 500 is returned instead of 400. Example request 1: ```json { "type": "unknown", "someOtherProperty": "value" } ``` Example request 2: ```json { "someOtherProperty": "value" } ``` Controller action: ```csharp [HttpPost("test")] public void Test(Base request) { } ``` Models: ```csharp [JsonPolymorphic(TypeDiscriminatorPropertyName = "type")] [JsonDerivedType(typeof(Derived), "derived")] public abstract record Base { } public record Derived : Base { } ``` ### Reproduction Steps 1. Create a controller with the action and models from Description 2. Do a call with the example request 1, response is 400 ```json { "type": "https://tools.ietf.org/html/rfc9110#section-15.5.1", "title": "One or more validation errors occurred.", "status": 400, "errors": { "$": [ "Read unrecognized type discriminator id 'unknown'. Path: $ | LineNumber: 2 | BytePositionInLine: 21." ], "request": [ "The request field is required." ] }, "traceId": "00-a4e0c60070a4c53222d0622acad8d198-e18068d6fe5d5773-00" } ``` 3. Do a call with the example request 2, response is 500 ``` System.NotSupportedException: Deserialization of types without a parameterless constructor, a singular parameterized constructor, or a parameterized constructor annotated with 'JsonConstructorAttribute' is not supported. ``` ### Expected behavior Ideally I expect the 400 error that will say clearly that the "type" TypeDiscriminator has to be provided (since the base class is abstract it is clear it cannot be constructed). At the very least some form of 400 with json that tells that json cannot be deserialized instead of 500 (there was nothing "wrong" with the server, but client provided a bad request which is clearly a 400). ### Actual behavior A 500 is returned and the exception is not handled by asp.net core (unlike example 1 request) ### Regression? _No response_ ### Known Workarounds One can probably make class not abstract and do a validation oneself that the "base" class is not supported. ### Configuration .Net 8 MacOS ARM ### Other information _No response_
Author: ilya-scale
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
eiriktsarpalis commented 8 months ago

This has been addressed by https://github.com/dotnet/runtime/pull/97474/ and should become available with .NET 9 Preview 1 to be launched today.

ilya-scale commented 8 months ago

@eiriktsarpalis , this is not directly connected to the type discriminator being out of order. In this case the type discriminator is missing completely. Will that issue fix this case as well? (Because it seems it is asp.net core issue, and not directly STJ)

eiriktsarpalis commented 8 months ago

this is not directly connected to the type discriminator being out of order.

The particular PR also improved the error message in the particular use case that you are citing, cf. https://github.com/dotnet/runtime/pull/97474/commits/4b6b478efa3e297d5c91ce882dd20091ef7904a1

At the very least some form of 400 with json that tells that json cannot be deserialized instead of 500 (there was nothing "wrong" with the server, but client provided a bad request which is clearly a 400).

Regarding this concern, it is difficult to map deserialization failure modes to validation errors (there are too many of those, serialization fails on the first occurrence, serialization can fail at arbitrary locations in the object graph). Arguably, every deserialization error might be regarded as a validation error so if you wish, you might want to write an exception filter that intercepts JsonExceptions.

ilya-scale commented 8 months ago

Well, the validation does handle the wrong discriminator type somehow in a nice way, would not it be nice that it will handle all the JsonExceptions to return as 400 (if not with all proper fields at least with some generic message)?

eiriktsarpalis commented 8 months ago

I defer to the @dotnet/aspnet-team on whether such a mapping would be desirable, but my impression is that the current behavior is very much intentional: deserialization != validation.

gregsdennis commented 8 months ago

@ilya-scale you might also consider validating with a JSON Schema prior to deserialization. That will provide more thorough error messaging.

Mine is not the only library for this, but you may enjoy it: https://docs.json-everything.net/schema/basics/

danroth27 commented 8 months ago

Moving to the aspnetcore repo for triage.

ilya-scale commented 8 months ago

Yes, deserialization is of course not the same as validation, but deserialization is a part of handling the http input, and since that input is wrong (does not conform to the expected model) so that we cannot deserialize, I expect to have a 400 the same way as if the whole json is not properly formatted, etc. Failing with 500 seems a bit unnecessary since we know for fact this was a bad request: json that cannot be deserialized.