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.55k stars 10.05k forks source link

`[FromRoute]` does not work when used as model property, not as action parameter. #45392

Closed tvardero closed 1 year ago

tvardero commented 1 year ago

Is there an existing issue for this?

Describe the bug

[FromRoute] does not work when used as model property, not as action parameter (see reproduction).

  1. Value for the property is set from the body. image

  2. Property generates validation error ("field is required"), if omitted on request (but route value is specified). image image

  3. Swagger generates incorrect body schema for the model - [FromRoute] propery shown as a part of the body schema. image

Expected Behavior

  1. Parameter is mapped from route, not from the body.
  2. Parameter does not generate 400 Bad Request result with model validation error, when parameter is missing in body.
  3. Swagger gen schema generated properly - route parameter is not part of the body schema.

Steps To Reproduce

  1. dotnet new webapi
  2. Create new .cs file and paste this code:
    
    using System.ComponentModel.DataAnnotations;
    using Microsoft.AspNetCore.Mvc;

namespace Controllers;

[ApiController, Route("/Companies/{companyId}/[controller]")] public class EmployeesController : ControllerBase { [HttpPost] // [HttpPut] // Same result // [HttpPatch] // Same result public IActionResult CreateEmployee(CreateEmployeeDto dto) { return Ok(dto); } }

public record CreateEmployeeDto { [FromRoute] // [FromRoute(Name = "companyId")] // Same result public string CompanyId { get; init; } = null!;

// [FromBody] // Same result
// [FromForm] // Same result
[Range(1, int.MaxValue)]
public int PhoneNumber { get; init; }

}

3. Open swagger and test the action. Use different values for "companyId" for route value and for body parameter (to distinguish them).
![image](https://user-images.githubusercontent.com/45442278/205068694-39a95ad4-5c4a-422b-8adf-f5c7db00aacb.png)

Program.cs is untouched. You may remove WeatherController as you wish.

### Exceptions (if any)

_No response_

### .NET Version

6.0.403, 7.0.100

### Anything else?

VS Code.

.NET SDK: Version: 7.0.100
Commit: e12b7af219

Runtime Environment:
OS Name: Windows OS Version: 10.0.19044 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\7.0.100\

Host: Version: 7.0.0 Architecture: x64 Commit: d099f075e4

.NET SDKs installed: 6.0.403 [C:\Program Files\dotnet\sdk] 7.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed: Microsoft.AspNetCore.App 6.0.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 7.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 6.0.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 7.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 6.0.11 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 7.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found: x86 [C:\Program Files (x86)\dotnet]

Environment variables: Not set

global.json file: Not found

captainsafia commented 1 year ago

Triage: @brunolins16 will look into this but it is likely that you need to add an attribute to the action parameter to opt-in to serializing the model from individual parameters.

brunolins16 commented 1 year ago

@tvardero Thanks for contacting us. Api Controllers have a mechanism to infer parameters source when not explicitly specified, unfortunately this mechanism does not walk through the type to check if there is attributes specifying the source. Because of that, in your case, your parameter is detected as a complex type and the FromBody source is inferred, ignoring any other explicitly defined source.

In this case, you need to explicitly define a source for your parameter eg. FromQuery or FromForm.

Eg. :

public IActionResult CreateEmployee([FromQuery] CreateEmployeeDto dto)
    {
        return Ok(dto);
    } 

Obs. : The route attribute showing in Swagger is there because you have it in your route template, no because the defined property.

ghost commented 1 year ago

Hi @tvardero. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

tvardero commented 1 year ago

@brunolins16

Results are described as comments inside actions. I have tested today with POST method only.

using Microsoft.AspNetCore.Mvc;

namespace Controllers;

public record Model
{
    public string NoAttributeParameter { get; init; } = null!;

    [FromBody]
    public string FromBodyParameter { get; init; } = null!;

    [FromQuery]
    public string FromQueryParameter { get; init; } = null!;

    [FromForm]
    public string FromFormParameter { get; init; } = null!;

    [FromRoute]
    public string FromRouteParameter { get; init; } = null!;
}

[ApiController, Route("api/[controller]/{fromRouteParameter}/[action]")]
public class TestController : ControllerBase
{
    // * - might be not ASP.NET problem, but Swashbuckle schema generator problem

    [HttpPost]
    public IActionResult WithNoAttribute(Model model)
    {
        // OK - "fromRouteParameter" is present in RouteData and is set correctly
        // Fail - FromRouteParameter of model was mapped from the body, which could contain different data or no data at all
        // Fail - 400 Bad request if FromRouteParameter is missing in the body
        // OK - "?fromQueryParameter" is present in Request.QueryString and is set correctly
        // Fail - FromQueryParameter of model was mapped from the body, which could contain different data or no data at all
        // Fail - 400 Bad request if FromQueryParameter is missing in the body
        // Fail* - FromQueryParameter is not present as query parameter in Swagger OpenAPI definition
        return Ok(model);
    }

    [HttpPost]
    public IActionResult WithFromBodyAttribute([FromBody] Model model)
    {
        // Same results as with WithNoAttribute action
        return Ok(model);
    }

    [HttpPost]
    public IActionResult WithFromFormAttribute([FromForm] Model model)
    {
        // FAIL - Unable to send JSON body: Bad Request 400 "The JSON value could not be converted to System.String. Path: $ | LineNumber: 0 | BytePositionInLine: 1."

        // Results with ConfigureApiBehaviorOptions(o => o.SuppressModelStateInvalidFilter = true):
        // FAIL - FromBodyParameter was not mapped (value is null)
        // FAIL - FromFormParameter was not mapped (value is null)
        // FAIL - NoAttributeParameter was not mapped (value is null)
        // FAIL - Unable to read from Request.Body stream - unable to seek to origin
        // FAIL* - Empty body schema in Swagger OpenAPI definition
        // OK - "fromRouteParameter" is present in RouteData and is set correctly
        // OK - FromRouteParameter of model was mapped from RouteData
        // OK - "?fromQueryParameter" is present in Request.QueryString and is set correctly
        // OK - FromQueryParameter of model was mapped from Request.QueryString

        return Ok(model);
    }

    [HttpPost]
    public IActionResult WithFromQueryAttribute([FromQuery] Model model)
    {
        // Same results as with WithFromFormAttribute action, except:
        // OK - NoAttributeParameter is mapped from query string and present in Swagger OpenAPI definition as query parameter

        return Ok(model);
    }

    [HttpPost]
    public IActionResult WithFromRouteAttribute([FromRoute] Model model)
    {
        // Same results as with WithFromFormAttribute action

        return Ok(model);
    }
}

OpenAPI definition: https://pastebin.com/fgBHJUga

tvardero commented 1 year ago

I know that I can create something like that and it will work 100% fine:

[HttpPost]
public IActionResult SomeAction(
    [FromRoute] RouteModel routeModel,
    [FromQuery] QueryModel queryModel,
    [FromBody] BodyModel bodyModel)
{
    return Ok(new { routeModel, queryModel, bodyModel });
}

But my team lead think another way and requires all things to be put together as one class object. This however does not work:

[HttpPost]
public IActionResult SomeAction(Model model)
{
    return Ok(model);
}

public record Model
{
    [FromRoute]
    public RouteModel RouteModel { get; init; }

    [FromQuery]
    public QueryModel QueryModel { get; init; }

    [FromBody]
    public BodyModel BodyModel { get; init; }
}

This post was EDITED on 11.12.2022 to reduce confusion.

halter73 commented 1 year ago

Do you want the route parameter to override the property that's deserialized from the JSON body? What if the property is in the JSON body but not in the route (e.g. a request to /show given [HttpGet("show/{id?}")])? Should the value from the body be ignored? How would input formatters know to skip [FromRoute] properties?

Does [FromQuery] currently just overwrite model properties that could have been set by the input formatter? It doesn't surprise me that [FromForm] completely breaks things if you're trying to upload a JSON body.

brunolins16 commented 1 year ago

I am a little bit confusing here.

In line with @halter73 questions. Are you QueryAndBodyModel expecting to be bound from body and source at same time?

    [FromQuery]
    QueryAndBodyModel QueryAndBodyModel { get; init; }

Ignoring the fact that you might want to bind the property from different source, your example doesn't work because of the mechanism that infer the binding source (https://github.com/dotnet/aspnetcore/issues/45392#issuecomment-1335368244).

Currently, the most common workarounds are:

That said, I believe this is a bad behavior and the infer mechanism could potentially no infer in this case since it will always get it wrong.

Obs.: I am still needed to review your examples (https://github.com/dotnet/aspnetcore/issues/45392#issuecomment-1336405034) to check what is expected or not. Obs.: I believe it is just a typo but your properties must have a public getter be public to be evaluated by the Model Binding.

ghost commented 1 year ago

Hi @tvardero. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

tvardero commented 1 year ago

@brunolins16 , Sorry, prob I made a lot of confusion with QueryAndBodyModel. I even confused myself. I've updated my latest comment above, there are now query model and body model separate.

- will try.

  • Add an explict binding source: public IActionResult SomeAction([FromQuery]Model model)

- see comment.

tvardero commented 1 year ago

@halter73 What I expect from input formatter is that properties that are marked with [FromRoute] attribute are mapped from RouteData and are not treated as part of JSON body / form.

If property exists in the model, but does not exist in the route (key-value is missing in RouteData), either exception is thrown, or default is assigned, or some sort of ModelState error is returned with code 400 response. If on request the body contained the parameter (so we have data duplication - in route and in json body) - value in the body is ignored.

However, reading the body as stream and deserializing it manually to our model prob will map our route parameter from the body stream, as i'm not sure json serializers (system.text.json or newtonsoft) respect binding source attributes.

brunolins16 commented 1 year ago

@tvardero Sorry for the delay. I finally got to your samples comment

Firstly, applying FromBody to a parameter is expected to not work. https://learn.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-7.0#frombody-attribute

FromForm will only work if the client send the correct content-type (application/x-www-form-urlencoded or multipart/form-data), so, mixing FromBody and FromForm seems weird.

As I mentioned, your example https://github.com/dotnet/aspnetcore/issues/45392#issuecomment-1336406290 should just work if

Also, I will open an issue to have a better inference mechanism.

brunolins16 commented 1 year ago

What I expect from input formatter is that properties that are marked with [FromRoute] attribute are mapped from RouteData and are not treated as part of JSON body / form.

As I mentioned, is expected once we have FromBody in the parameter all property attributes will be ignored https://learn.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-7.0#frombody-attribute

brunolins16 commented 1 year ago

If property exists in the model, but does not exist in the route (key-value is missing in RouteData), either exception is thrown, or default is assigned, or some sort of ModelState error is returned with code 400 response. If on request the body contained the parameter (so we have data duplication - in route and in json body) - value in the body is ignored.

Sorry but can you share a real example of what you are trying to do.

ghost commented 1 year ago

Hi @tvardero. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

ghost commented 1 year ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.

See our Issue Management Policies for more information.

tvardero commented 1 year ago

@brunolins16

once we have FromBody in the parameter all property attributes will be ignored https://learn.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-7.0#frombody-attribute

Thank you for pointing out. But WithNoAttribute (here) behaves the same, does that mean that [FromBody] is applied implicitly for every action parameter? Also I noticed that if the action is HttpGet and parameters don't have explicit FromQuery attribute, then swagger treats those parameters as body schema, which is unusual for GET methods.

Sorry but can you share a real example of what you are trying to do.

Real case scenario was that on out project we have endpoints with route and body parameters. For example we have entity "Company" and it has collection of entities "Employee". We did routing in RESTlike style, so we have like /api/company/{companyId}/employees/{employeeId}. POST methods (for new entity creation) and PUT methods (for updating the entity) have also a body, where parameters for creation/update are provided. DTO is then converted to a command using AutoMapper and sent to handler with MediatR (CQRS).

Since for both DTOs and CQRS we use records models which init-only properties, we need either to use command = command with { CompanyId = companyId }; or to make record's property mutable (give it a setter), and thats feels a little dirty.

The goal to achieve is to go from this:

[HttpPut("company/{companyId}/employees/{employeeId}")]
public IActionResult UpdateEmployeeById([FromRoute] string companyId, [FromRoute] string employeeId, [FromBody] UpdateEmployeeDto dto)
{
   var command = _mapper.Map<UpdateEmployeeByIdCommand>(dto);
   command = command with { EmployeeId = employeeId };
   command = command with { CompanyId = companyId };

   _mediator.Publish(command);

   return NoContent();
}

To this:

[HttpPut("company/{companyId}/employees/{employeeId}")]
public IActionResult UpdateEmployeeById(UpdateEmployeeByIdCommand dto)
{   
   _mediator.Publish(command);

   return NoContent();
}

public record UpdateEmployeeByIdCommand 
{   
   [FromRoute(Name = "companyId")]
   public required string CompanyId { get; init; }

   [FromRoute(Name = "employeeId")]
   public required string EmployeeId { get; init; }

   [FromBody]
   public required string Name { get; init; }

   [FromBody]
   public required string Phone { get; init; }
}
brunolins16 commented 1 year ago

Thank you for pointing out. But WithNoAttribute (here) behaves the same, does that mean that [FromBody] is applied implicitly for every action parameter? Also I noticed that if the action is HttpGet and parameters don't have explicit FromQuery attribute, then swagger treats those parameters as body schema, which is unusual for GET methods.

@tvardero this is the behavior I mentioned before, and I totally agree with you. I just filed a bug for this: https://github.com/dotnet/aspnetcore/issues/46140

brunolins16 commented 1 year ago

Thanks for sharing your scenario.

   [FromBody]
   public required string Name { get; init; }

   [FromBody]
   public required string Phone { get; init; }

I need to double check, but I believe multiple FromBody won't work, however if you do this [FromBody] UpdateEmployeeDto dto it must work if the infer mechanism isn't inferring it wrong.

Let me quickly repro and get back to you.

brunolins16 commented 1 year ago

@tvardero I just tried and multiple FromBody will cause some problem, however something like this works:

        [HttpPut("company/{companyId}/employees/{employeeId}")]
        public IActionResult UpdateEmployeeById(UpdateEmployeeByIdCommand dto)
        {
            return NoContent();
        }

        public record UpdateEmployeeByIdCommand
        {
            [FromRoute(Name = "companyId")]
            public required string CompanyId { get; init; }

            [FromRoute(Name = "employeeId")]
            public required string EmployeeId { get; init; }

            [FromBody]
            public required UpdateEmployeeDto Employee { get; init; }
        }
        public record UpdateEmployeeDto
        {
            public required string Name { get; init; }

            public required string Phone { get; init; }
        }

However, you need to disable the infer mechanism 😢 or wait for bug fix I just mentioned.

builder.Services.Configure<ApiBehaviorOptions>(options => options.SuppressInferBindingSourcesForParameters = true);
ghost commented 1 year ago

Hi @tvardero. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 1 year ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.

See our Issue Management Policies for more information.

tvardero commented 1 year ago

Thank you, I will wait for a fix :) Do I need to provide any additional feedback / use case scenarios?

brunolins16 commented 1 year ago

Closing since we are tracking the issue here #46140