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

Binding error passes silently #55

Open
neistow opened this issue Jul 30, 2021 · 6 comments
Open

Binding error passes silently #55

neistow opened this issue Jul 30, 2021 · 6 comments
Assignees
Labels

Comments

@neistow
Copy link

neistow commented Jul 30, 2021

I've stumbled upon an odd behaviour with hybrid model binding. Consider this example:

Model:

public class AnotherDto
{
    public string SomeString { get; set; }
    public int  SomeInt { get; set: }
}

public class MyDto
{
    public string SomeString { get; set; }
    public DateTime SomeDateTime { get; set; }
    public AnotherDto SomeNestedDto { get; set; }
}

Controller endpoint:

public IActionResult SomeEndpoint([FromHybrid] MyDto dto)
{
    // code here...
}

Request:

{
    "someString" : "str",
    "someDateTime": "*imagine this is a valid datetime*"
    "someNestedDto": { "someProperty": 123, "anotherProperty": "..." }
}

In the request property "someNestedDto" has INTENTIONALLY a wrong type.

Behaviour: dto that comes to my controller method has empty fields with default values.

Expected behaviour: parse error. If this happens with [FromBody] attribute applied to MyDto, then a parse exception will be raised.

As for me such errors shouldn't pass silently.

@billbogaiv
Copy link
Owner

I can't duplicate the exception-scenario mentioned in "Expected behaviour". May you post a sample project as a ZIP-file or the stack trace from the exception? When I use [FromBody] and submit Request, dto.someNestedDto is initialized with default-values for all properties. When I don't pass the property in the request, dto.someNestedDto is null.

@neistow
Copy link
Author

neistow commented Oct 6, 2021

Hi @billbogaiv,

Was a bit busy this month but today we had again an issue with Hybird model binding attribute so I decided that today is the day to come back to old problems so I've made an example repo where you can reproduce issue.

Check the following screenshot which illustrates the problem:
image

[PUT] https://localhost:5001/api/example/

{
    "test": [1,2,3],
    "model": 10
}

Also there is a weird and confusing behavior with FluentValidaton, for example if we have the following validator defined for ExampleModel and we pass the wrong type e.g int instead of object the response will have a misleading error that regards another property which is valid.

    public class ExampleModelValidator : AbstractValidator<ExampleModel>
    {
        public ExampleModelValidator()
        {
            RuleFor(c => c.Test).NotEmpty();
            RuleFor(c => c.Model.Value).GreaterThanOrEqualTo(1).When(c => c.Model != null);
        }
    }

image

The behavior I get when I'm using [FromBody] and which is the correct behavior in my opinion:
image

@billbogaiv billbogaiv added the bug label Oct 7, 2021
@billbogaiv billbogaiv self-assigned this Oct 7, 2021
@billbogaiv
Copy link
Owner

Thanks for the sample repo. I've identified a couple issues and should be able to get an update pushed soon.

  • New behavior will return failures created by ASP.NET's BodyModelBinder.
  • Non-generic enumerable properties caused an internal exception to be thrown. New behavior will correctly handle types like int[].

Regarding the null in the response, using [FromHybrid] on its own won't work. You still need to register the service in Startup:

public void ConfigureServices(IServiceCollection services)
{
    services
        .AddControllers()
        .AddHybridModelBinder();
}

AddHybridModelBinder works with any IMvcBuilder-type.

@neistow
Copy link
Author

neistow commented Oct 7, 2021

Registering HybridModelBinder in the services won't help. I've updated sample repo so you can test it and ensure that even with HybridModelBinder registered in services there is still null when passing a wrong type. While [FromBody] return a parse exception.

@billbogaiv
Copy link
Owner

The forthcoming update will fix those problems, but without registering the service, even submitting a request like the following won't trigger the expected binding:

{
    "model": { "value": 1 }
}

Using attributes like [FromHybrid] and [FromBody] only specifies a binding-source to use. There still needs to be a registered binding-provider which uses that source. ASP.NET won't throw an error if the binder isn't registered and you'll continue to see nulls in the response.

services.AddControllers() registers the necessary binders to use something like [FromBody].

@mtpultz
Copy link

mtpultz commented Jan 5, 2022

Hi @billbogaiv any news on this update? We've run into a similar issue and wanted to know if this update was on the horizon. It's a new year and everyone is busy so I get it if this isn't happening anytime soon. Only asking to inform our decisions going forward.

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

No branches or pull requests

3 participants