Release notes?

[phillip-haydon]

Are there release notes for the changes from 0.17.0 to 0.18.0?

The model binding stopped working when I upgraded and I can't figure out what changed.

[billbogaiv]

No official release notes 😞.

The bulk of changes came from https://github.com/billbogaiv/hybrid-model-binding/pull/45.

This change-set added features to allow class-level bind-ordering (so this doesn't need explicitly defined on each property) and the ability to see how a property was set (by implementing IHybridBoundModel).

Here's the change-set for the README: https://github.com/billbogaiv/hybrid-model-binding/commit/7275737c35d4c547d6281ca902007622930b83ec#diff-04c6e90faac2675aa89e2176d2eec7d8

All changes should be backwards-compatible. I didn't come across anything glaring even though I made several updates to the internal binding-process.

Do you have a small working sample you could submit that I could review with the current release-version?

[bkaid]

0.18.0 also causes issues for me as well. .NET Core 3.1.

public class MyModel
{
    public Guid Prop1 { get; set; }

    public string Prop2 { get; set; }
}

[Route("")]
[ApiController]
public class MyController : ControllerBase
{
   [HttpPost("{Prop1:Guid}/myAction")]
   public async Task<IActionResult> MyAction([FromHybrid] MyModel model, CancellationToken cancellationToken) {}
}

Behavior in 0.17.0: Route parameter Prop1 would be correctly populated when doing a POST to this endpoint Behavior in 0.18.0: Route parameter Prop1 is empty when doing a POST to this endpoint

Let me know if you need a more fully working example. I'll try to go through the diff to see if I can spot something.

[bkaid]

I've created a gist with a test that works on 0.17.0 and doesn't on 0.18.0. It turns out the issue for me was using a Guid for a route value causes it to be all 0's in 0.18.0. The workaround is to use a Guid? or a string.

[phillip-haydon]

Right now for me there’s no workaround as I don’t want to change 200 api endpoint :(

Thank you @bkaid for adding a gist to reproduce the issue. I missed the notification from @billbogaiv

[billbogaiv]

I'll step through code using that gist later today... I'm stumped 🤔.

[billbogaiv]

I've identified the problem, but am not sure there will be a fix...

TL;DR

Explicitly set the binding of the affected properties so Source.Body is not included or push it to the end:

public class Person
{
    [HybridBindProperty(new[] { Source.Form, Source.QueryString, Source.Route })]
    public Guid Id { get; set; }

    public string Name { get; set; }
}

Set fallback-ordering (also found in the README):

public void ConfigureServices(IServiceCollection services)
{
    services
        .AddMvc()
        .AddHybridModelBinder(options =>
        {
            /**
             * This is optional and overrides internal ordering of how binding gets applied to a model that doesn't have explicit binding-rules.
             * Internal ordering is: body => form-values => route-values => querystring-values => header-values
             */
            options.FallbackBindingOrder = new[] { Source.QueryString, Source.Route };
        });
}

Internal "default ordering" is defined by: https://github.com/billbogaiv/hybrid-model-binding/blob/f89615b015d869223061fb774ff7dd41a3641e5d/src/HybridModelBinding/HybridModelBinder.cs#L25

Make a global change so the ability to bind to a property doesn't stop at the first-match:

public void ConfigureServices(IServiceCollection services)
{
    services
        .AddMvc()
        .AddHybridModelBinder(options => options.Passthrough = true);
}

⚠️ This might introduce more unexpected results which is why it's off by-default. This change flips which bind-source "wins" from first to last. However, there are known problems using this option and IHybridBoundModel. 0.18.1-alpha-2 or higher will eventually fix the last-part.

Full-story explanation

The biggest change from 0.17.0 to 0.18.0 was to fix that a property couldn't have bind-ordering where Source.Body wasn't the first-option:

[HybridBindProperty(Source.Route, Source.Query, Source.Body)]

...would internally bind Source.Body first (since IModelBinder comes before IValueProvider).

I suspect most use-cases wanted Source.Body first anyway so it was probably never seen as a "problem" by anyone using the library.

Fast-forward to 0.18.0 and I introduced a wrapper around the result of IModelBinder, called BodyValueProvider, so I could properly allow the bind-ordering specified above (and logging to know how a property was bound). By-default, HybridModelBinding uses the ASP.NET Core implementation of BodyModelBinder. Given the following model and request:

public class Person
{
    public Guid Id { get; set; }
    public string Name { get; set; }
}

POST
{
  "name": "Bill"
}

using BodyModelBinder will create an instance of Person:

{
    "Id": "00000000-0000-0000-0000-000000000000',
    "Name": "Bill"
}

There's just not enough info. coming back from BodyModelBinder.BindModelAsync to know whether Id was bound from the request or is the default-value. I played-around with checking for default(Type), but that creates its own problems since an empty-Guid (or any non-null value-type) could be sent in the request and HybridModelBinding wouldn't know the difference).

I also thought about creating a custom BodyModelBinder to use by-default which would try and do those extra features, but then anyone deciding to use the ASP.NET Core-version might not understand why the behaviors were different... maybe no one even cares to use HybridModelBinding defining their own binders/value-providers, so that could still be an option.

I'm open to other suggestions.

[phillip-haydon]

IMO, if the request has a value at any point in the ordering, set the value.

I mean, if I have a value in the route "/orders/{id:int} and a value in the payload { "id": 123 }

If I have the ordering as: Body, Route.

I don't care what the body set, i want the route to set the value.

If the ordering is Route, Body, then what ever was sent on the payload should override the Route.

The thing is, I don't understand why you would ever bind the body last. It doesn't make any sense and I've been pondering this a few days and I cannot think of a single valid scenario.

Any scenario where you are binding from multiple sources, you want to bind the other sources on top of the body. Because you're taking an id or such from the route/query string. Or you're adding header version.

IMO I would remove the complexity of allowing binding Body last.

Body + [Ordered Route/Query/Headers] (so only allow ordering of whats applied on top of the body)

[phillip-haydon]

I think I'm gonna be stuck on 17 forever :(