Skip to content

Blazor SSR traditional form post handling #48760

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

Closed
SteveSandersonMS opened this issue Jun 13, 2023 · 11 comments
Closed

Blazor SSR traditional form post handling #48760

SteveSandersonMS opened this issue Jun 13, 2023 · 11 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-full-stack-web-ui Full stack web UI with Blazor
Milestone

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jun 13, 2023

While we have now implemented support for EditForm usage in particular patterns, this issue is about enabling support for more basic, unopinionated form post handling. Capabilities to enable:

  • Receive an arbitrary HTTP POST to a component endpoint
    • access HttpRequest and thereby access Form, Headers, Cookies, etc.
      • ... and manually perform model binding procedurally against that incoming Form data
    • ability to disable enforcing antiforgery validation at the per-endpoint level
  • Submit a plain <form> via POST to the page you're on
    • receive the values via [SupplyParameterFromForm]
      • partly already done, but the API may need some tweaks to enable the following
      • they are available by the time OnInitialized runs
      • optionally set a form field name, [SupplyParameterFromForm("acceptsTerms")], otherwise defaults to property name
        • either way, it automatically respects the cascading name prefix, if any
      • optionally set a handler name, [SupplyParameterFromForm(Handler = "myform")], otherwise accepts any POST
    • parts of the form can be split into a child component (with developer managing field name prefixes manually)
      • Should be covered automatically by the above features
    • can emit an antiforgery token
      • Should be already covered by Blazor antiforgery #46987, unless we decide also to emit this automatically during prerendering based on the action being an internal URL
    • can specify a handler name (already done, but needs some tweaks to enable all the following)
      • make it optional - if not set, defaults to some fixed string such as empty, which is a valid handler name
        • TBD: should we actually make nonempty handler name a prerequisite if you have method="post"?
      • nonhierarchical - it's enough to say you must pick a unique enough name
      • if set, automatically emitted as a hidden form field during SSR, to avoid interfering with URLs
      • probably should call this a "form name" or "form handler name" not just a "handler name" for clarity
    • triggers any @onsubmit handler once the page reaches quiescence
  • Optionally use Input* components
    • TODO: Should @bind-Value=lambda really generate a name by default? This only works in basic cases and we don't have a design proposal to scale this up to forms split into child components or if using custom edit components.
    • If we do continue generating field names automatically, have optional mechanism for cascading a name prefix (e.g., CascadingModelBinder, but change it not to be templated to avoid context clash)
  • Optionally use EditForm
    • During SSR, if we are in a POST matching the handler name:
      • automatically attaches any modelbinding errors as a ValidationMessageStore (ideally this should be something you can do procedurally to an EditContext too, rather than being exclusive to an EditForm)
      • supports the same @onsubmit triggering as a plain <form>, thereby triggering validation and OnValidSubmit etc

Earlier version of this issue description, all of which should be covered above

Expand
  • Receiving an arbitrary POST request that populates [SupplyParameterFromForm] parameters
  • Receiving a plain HTML <form> post that does that
  • Having a plain HTML <form> trigger an @onsubmit handler
  • Ensuring that [SupplyParameterFromForm] can work with simple scalar values, with the field name taken from the property name (no form name or prefix required)
  • Ensuring that [SupplyParameterFromForm] can work with complex objects given appropriate name prefixes
  • Ensuring the error experience around ambiguous form posts is clear (e.g., form name is required if and only if a form has both method=post and nonempty @onsubmit)
  • Supporting patterns for one-form-multiple-actions and multiple-forms-one-action
  • Where possible simplifying the developer experience, e.g., making sure CascadingModelBinder isn't required for basic scenarios, and perhaps streamlining functionality by reducing the number of concepts or parts of implementation required
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jun 13, 2023
@SteveSandersonMS SteveSandersonMS added the feature-full-stack-web-ui Full stack web UI with Blazor label Jun 13, 2023
@SteveSandersonMS SteveSandersonMS self-assigned this Jun 13, 2023
@SteveSandersonMS SteveSandersonMS added this to the 8.0-preview7 milestone Jun 13, 2023
@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Jun 30, 2023

I've updated the description above and would love to check this is what people are expecting before I start on any implementation. I think the description here merges the latest stuff from this issue, #47804, and the "form scenarios" exercise I put together a few weeks ago.

The biggest open question for me is whether or not we should really be generating form field name attribute values automatically based on @bind lambdas. It is genuinely helpful for basic forms, but it runs off a cliff if people try to split part of a form out into a child component or have a custom edit component, and I'm not aware we have a suggestion for how people then proceed. Consider a scenario like:

MyForm.razor

<EditForm Model="@Person">
    <AddressEditor Title="Home address" Address="@Person.HomeAddress" />
    <AddressEditor Title="Work address" Address="@Person.WorkAddress" />
</EditForm>

AddressEditor.razor:

<h2>@Title</h2>
<InputText @bind-Value="Address.Line1" />
<InputText @bind-Value="Address.City" />
<InputText @bind-Value="Address.PostCode" />

@code {
    [Parameter] public AddressData Address { get; set; }
}

This will work perfectly as an interactive form. But it won't generate the correct field names for SSR, because AddressEditor.razor doesn't know about the context it's in. So my first expectation is we'd have something like:

MyForm.razor

<EditForm Model="@Person">
    <FormContext For="@(() => Person.HomeAddress)" context="formContext">
        <AddressEditor Title="Home address" Address="@formContext" />
    </FormContext>
    <FormContext For="@(() => Person.WorkAddress)" context="formContext">
        <AddressEditor Title="Work address" Address="@formContext" />
    </FormContext>
</EditForm>

AddressEditor.razor:

<h2>@Title</h2>
<InputText @bind-Value="Address.Line1" />
<InputText @bind-Value="Address.City" />
<InputText @bind-Value="Address.PostCode" />

@code {
    [Parameter] public AddressData Address { get; set; }
}

... but that still doesn't work because even if InputText knows to apply the prefix cascaded into it, how would it know not to prefix the field names with Address? It seems like the developer would have to do something like:

@* Explicitly remove the wrong part of the field name *@
<FieldNameContext For="@(() => Address)">
    <InputText @bind-Value="Address.Line1" />
    ...
</FieldNameContext>

@* Or, explicitly remove it on the InputText *@
<InputText @bind-Value="Address.Line1" SkipPrefix="@(() => Address)" />

@* Or, explicitly state just the part after the cascaded prefix (probably the most intelligible choice of these) *@
<InputText Name="Line1" @bind-Value="Address.Line1" />

@* Or we bake in some hidden magic that tries to guess what you're doing based on whether some prefix
   of your lambda seems to match with a [Parameter] property (not [SupplyParameterFromForm]), and we just
   hope people don't try to do anything more custom in terms of how they pass values to child components,
   which will be very leaky *@

Altogether none of these seem great, so I'm unclear on whether we're doing people a service by trying to generate names or a disservice because as soon as it goes wrong, there's no reasonable solution besides overriding it manually anyway (and the way in which it goes wrong would be nonobvious and seem like a framework bug).

I would be OK with just not trying to solve this problem and expecting people to manage names manually until or if we can think of a nonleaky solution. Or possibly have something like <Input @bind-Value="@(() => Address.Line1)" AutoName="true" /> for people who actually know their lambda will correspond to a model-bindable field name.

Opinions?

@SteveSandersonMS
Copy link
Member Author

cc @dotnet/aspnet-blazor-eng for feedback here

@SteveSandersonMS
Copy link
Member Author

@dotnet/aspnet-blazor-eng Last call for design feedback! I'll start implementing this tomorrow.

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Jul 3, 2023

One other possible solution I thought of for the name generation logic would be that if you have something like:

    <FormContext For="@(() => Person.HomeAddress)" context="formContext">
        <AddressEditor Title="Home address" Address="@formContext" />
    </FormContext>

and

<h2>@Title</h2>
<InputText @bind-Value="Address.Line1" />
<InputText @bind-Value="Address.City" />
<InputText @bind-Value="Address.PostCode" />

@code {
    [Parameter] public AddressData Address { get; set; }
}

... then the formatting logic could check whether the first token in the lambda has a value reference-equal with the "form context" value and if so, skip it. That is, it would evaluate Address and see that it's reference-equal with the current "context" object, and so skip it.

Realistically I don't think we'd do that since it involves compiling a whole new lambda expression, which (1) could be harmful for perf and I'm unsure if it could even be cached based on what we know and (2) would likely never align with AOT plans. It also doesn't work in more general cases like if you have a custom editor that receives two objects and edits them both.

So altogether I don't have a real solution for this and am really questioning what we are going to tell people who try to use forms in nontrivial cases.

@MackinnonBuck
Copy link
Member

Given the potential for customers to misunderstand the mechanics of field name generation, I would definitely be fine with solving that problem at a later point and requiring manual field name management. Or if there was some way to enable it for basic scenarios while making the error experience clear for advanced scenarios (like nested components), I think that would be acceptable too.

@javiercn
Copy link
Member

javiercn commented Jul 4, 2023

  • Receive an arbitrary HTTP POST to a component endpoint
    • access HttpRequest and thereby access Form, Headers, Cookies, etc. Possible through IHttpContextAccessor
      • ... and manually perform model binding procedurally against that incoming Form data Its a matter of exposing a Map API in ModelBindingContext for example.
    • ability to disable enforcing antiforgery validation at the per-endpoint level. Covered in the PR mentioned
  • Submit a plain <form> via POST to the page you're on
    • receive the values via [SupplyParameterFromForm]
      • partly already done, but the API may need some tweaks to enable the following
      • they are available by the time OnInitialized runs Already the case
      • optionally set a form field name, [SupplyParameterFromForm("acceptsTerms")], otherwise defaults to property name
      • optionally set a handler name, [SupplyParameterFromForm(Handler = "myform")], otherwise accepts any POST The Name attribute (shorthand for HandlerName) is currently used for this
    • parts of the form can be split into a child component (with developer managing field name prefixes manually) Possible but requires specifying the field names manually. This was a scoping the decision at the time, but we can implement it if we changed our minds
      • Should be covered automatically by the above features
    • can emit an antiforgery token
      • Should be already covered by Blazor antiforgery #46987, unless we decide also to emit this automatically during prerendering based on the action being an internal URL
    • can specify a handler name (already done, but needs some tweaks to enable all the following)
      • make it optional - if not set, defaults to some fixed string such as empty, which is a valid handler name
        • TBD: should we actually make nonempty handler name a prerequisite if you have method="post"? If we don't like specifying the handler name, there is an alternative, we can observe the same Razor Pages convention OnPost and OnPostHandlerName on the method to define the outcome.
      • nonhierarchical - it's enough to say you must pick a unique enough name How does it work when you render the same component in two different parts of the app at the same time
      • if set, automatically emitted as a hidden form field during SSR, to avoid interfering with URLs We should make this more general and just allow it to be either on the URL (as a route value), query string, or form body in that order. I'm fine if we decide it to emit it as a hidden field.
      • probably should call this a "form name" or "form handler name" not just a "handler name" for clarity. The choice of "handler" is deliberate to help with migration scenarios from Razor Pages. At the very least, we should accept it as an alias.
    • triggers any @onsubmit handler once the page reaches quiescence
      • if all matching handlers (based on optional handler name) are for the same (target, MethodInfo) pairs, invoke it. Even if you are able to dispatch to the method, there is still ambiguity on where to report errors in the UI if for example, validation fails. You don't want the errors to appear on all the forms, and you don't know from the N forms with the same handler, which one is the actual target. It's also troublesome to do this, as there are many valid constructs that won't work with this (like lambdas, and for which it will fail in an obvious way). and might preclude compiler changes/optimizations in the future
      • otherwise throw ambiguous match error
      • handles @onsubmit event exceptions in an ErrorBoundary-aware way (Blazor SSR form post handling: integrate errors with ErrorBoundary #47903)
  • Optionally use Input* components
    • TODO: Should @bind-Value=lambda really generate a name by default? This only works in basic cases and we don't have a design proposal to scale this up to forms split into child components or if using custom edit components.
    • If we do continue generating field names automatically, have optional mechanism for cascading a name prefix (e.g., CascadingModelBinder, but change it not to be templated to avoid context clash)
      Sample implementation: e48c31d
  • Optionally use EditForm
    • During SSR, if we are in a POST matching the handler name:
      • automatically attaches any modelbinding errors as a ValidationMessageStore (ideally this should be something you can do procedurally to an EditContext too, rather than being exclusive to an EditForm)
      • supports the same @onsubmit triggering as a plain <form>, thereby triggering validation and OnValidSubmit etc

@javiercn
Copy link
Member

javiercn commented Jul 4, 2023

More feedback, based on the scenarios you raised a while ago:

Scenario 1 Reading the form data procedurally

@inject IHttpContextAccessor HttpContextAccessor
@if (Request.HasFormContentType)
{
    foreach (var kvp in Request.Form)
    {
        <p>@kvp.Key: @kvp.Value</p>
    }
}

@code {
    HttpRequest Request => HttpContextAccessor.HttpContext!.Request;
}

... then curl -v https://localhost:7247/handlepost -d "key1=value1&key2=value2"

Error: No named event handler was captured for ''.

Workaround: Add this fake form, then it works: <form @onsubmit="@(() => {})">@{ __builder.SetEventHandlerName(""); }

Preferred outcome: It is not an error, and we just let you access the form post data.

Why? This seems like a pit of failure. If you don't have a form, this opens up to processing data in unexpected ways. Razor Pages will 404 if you don't have an OnPost handler defined, it doesn't re-render the page instead or anything like that.

This will also never work because you need antiforgery

Also, what is the scenario here, you typically want to take an action when you receive data, how do you do that. OnInitializedAsync? How is that better than handling it inside the submit handler?

Actual code

@inject IHttpContextAccessor HttpContextAccessor

<PageTitle>Forms</PageTitle>

<form method="post" @onsubmit="OnPost">
    ...fields...
    <AntiforgeryToken />
    <input type="submit" value="send" />
</form>

@if (Request.HasFormContentType)
{
    foreach (var kvp in Request.Form)
    {
        <p>@kvp.Key: @kvp.Value</p>
    }
}

@code {
    public void OnPost() => _submitted = true;
}

Scenario 2 Model binding scalar values

<p>Name: @Name</p>
<p>Age: @Age</p>

@code {
    [SupplyParameterFromForm] public string? Name { get; set; }
    [SupplyParameterFromForm] public int Age { get; set; }
}

This again needs antiforgery, and needs to go inside a form.

<form method="post" @onsubmit="OnPost">
    <input name="Name" value="@name" />
    <input name="Age" value="@age" />
    <AntiforgeryToken />
    <input type="submit" value="send" />
</form>

@if(_submitted)
{
  <p>Name: @Name</p>
  <p>Age: @Age</p>
}

@code {
    public void OnPost() => _submitted = true;
}

** Scenario 3 Model Binding scalar values**

<form method="post">
    <input name="Name" value="Bert" />
    <input name="Age" value="123" />
    <button type="submit">Submit</button>
</form>

<p>Name: @Name</p>
<p>Age: @Age</p>

@code {
    [SupplyParameterFromForm] public string? Name { get; set; }
    [SupplyParameterFromForm] public int Age { get; set; }
}

Note that this should not require any handler name or @onsubmit to be specified.

It is unclear how this scenario works or how it scales up to Server/Webassembly scenarios.

<form method="post" @onsubmit="OnPost">
    <input name="Name" value="Bert" />
    <input name="Age" value="123" />
    <AntiforgeryToken />
    <button type="submit">Submit</button>
</form>

if(_submitted)
{
  <p>Name: @Name</p>
  <p>Age: @Age</p>
}

@code {
    [SupplyParameterFromForm] public string? Name { get; set; }
    [SupplyParameterFromForm] public int Age { get; set; }

    public void OnPost() => _submitted = true; // Or update the DB, or whatever you want to do.
}

Scenario 4 Model binding complex values

<form method="post" @onsubmit="OnPost">
    <input name="Person.Name" value="Bert" />
    <input name="Person.Age" value="123" />
    <AntiforgeryToken />
    <button type="submit">Submit</button>
</form>

@if (Person is not null)
{
    <p>Name: @Person.Name</p>
    <p>Age: @Person.Age</p>
}

@code {
    [SupplyParameterFromForm] public PersonDto? Person { get; set; }

    public class PersonDto
    {
        public string? Name { get; set; }
        public int Age { get; set; }
    }

    override OnInitializedAsync() => Person ??= await //Load customer

    private void OnPost()
    {
        // Update customer
    }
}

Scenario 5 Constructing names manually

  • this works, it's covered by E2E tests

Generating field names automatically

e48c31d

Scenario 6 Plain form submit events

<form name="login" method="post" @onsubmit="OnSubmit">
    <input name="username">
    <input name="password">
    <input type="submit" value="Log in" />
</form>

@code{
    public void OnSubmit(SubmitEventArgs args)
    {
        Console.WriteLine(args.FormData.Read<string>("UserName"));
        Console.WriteLine(args.FormData.Read<string>("Password"));

        // Or:
        var loginData = args.FormData.Read<LoginData>();

        // Or have [SupplyValueFromForm(Name = "")] public LoginData Data { get; set; }

        // Or if you also want to capture validation errors
        var loginData = args.FormData.Read<LoginData>(editContext);
    }
}

We don't have the SubmitEventArgs, but it can work just fine with the OnPost convention in the form.

<form name="login" method="post" @onsubmit="OnPost">
    <input name="username">
    <input name="password">
    <AntiforgeryToken />
    <input type="submit" value="Log in" />
</form>

@code{
    public void OnPost(SubmitEventArgs args)
    {
        Console.WriteLine(args.FormData.Read<string>("UserName"));
        Console.WriteLine(args.FormData.Read<string>("Password"));

        // Or:
        var loginData = args.FormData.Read<LoginData>();

        // Or have [SupplyValueFromForm(Name = "")] public LoginData Data { get; set; }

        // Or if you also want to capture validation errors
        var loginData = args.FormData.Read<LoginData>(editContext);
    }
}

Scenario 7

@foreach (var thing in things)
{
    <fieldset>
        <legend>Thing: @thing</legend>
        <form name="addToCart" method="post" @onsubmit="AddToCart">
            <input type="number" name="quantity" value="1" />
            <button name="id" value="@thing" type="submit">Add to cart: @thing</button>
        </form>
    </fieldset>
}

<h3>Cart</h3>
<ul>
    @foreach (var item in _cart)
    {
        <li>@item</li>
    }
</ul>

@code{
    private static List<string> _cart = new(); // TODO: Should use Session or Cookies or similar
    private string[] things = new string[] { "Alpha", "Beta", "Gamma", "Delta" };

    public void AddToCart(SubmitEventArgs eventArgs)
    {
        var form = eventArgs.FormData;
        var quantity = form.Read<int>("quantity");
        var thing = form.Read<string>("id");
        // ... or alternatively have a DTO and use [SupplyParameterFromForm(Name = "")] public AddToCartInfo AddToCart { get; set; }
    }
}

Challenge here is that you end up with N forms on the page. There is state associated with those forms, like validation, etc, that you can't simply ignore.

  • For example, if there are binding errors, how do you apply those to the right fields.
  • If you are binding values, that are displayed as part of the form, how do you prevent all fields from getting the posted value bound.
@foreach (var thing in things)
{
    <fieldset>
        <legend>Thing: @thing</legend>
        <form name="addToCart" method="post" @onsubmit="AddToCart" @onsubmit:name="AddToCart(@thing.Id)">
            <input type="number" name="quantity" value="1" />
            <input type="hidden" name="handler" value="AddToCart(@thing.Id)" />
	    </AntiforgeryToken />
            <button name="id" value="@thing" type="submit">Add to cart: @thing</button>
        </form>
    </fieldset>
}

<h3>Cart</h3>
<ul>
    @foreach (var item in _cart)
    {
        <li>@item</li>
    }
</ul>


@code{
    private static List<string> _cart = new(); // TODO: Should use Session or Cookies or similar
    private string[] things = new string[] { "Alpha", "Beta", "Gamma", "Delta" };

    public void AddToCart(SubmitEventArgs eventArgs)
    {
        var form = eventArgs.FormData;
        var quantity = form.Read<int>("quantity");
        var thing = form.Read<string>("id");
        // ... or alternatively have a DTO and use [SupplyParameterFromForm(Name = "")] public AddToCartInfo AddToCart { get; set; }
    }
}

Alternatively, we disambiguate based on the handler + key, and we require you to setup a key that is IFormattable. That disambiguates which of the N forms we are targeting.

@foreach (var thing in things)
{
    <fieldset>
        <legend>Thing: @thing</legend>
        <form @key="@thing.Id" name="addToCart" method="post" @onsubmit="AddToCart" @onsubmit:name="AddToCart(@thing.Id)">
            <input type="number" name="quantity" value="1" />
            <input type="hidden" name="handler" value="AddToCart(@thing.Id)" />
	    <input type="hidden" name="key" value="@thing.Id" />
	    </AntiforgeryToken />
            <button name="id" value="@thing" type="submit">Add to cart: @thing</button>
        </form>
    </fieldset>
}

<h3>Cart</h3>
<ul>
    @foreach (var item in _cart)
    {
        <li>@item</li>
    }
</ul>


@code{
    private static List<string> _cart = new(); // TODO: Should use Session or Cookies or similar
    private string[] things = new string[] { "Alpha", "Beta", "Gamma", "Delta" };

    public void AddToCart(SubmitEventArgs eventArgs)
    {
        var form = eventArgs.FormData;
        var quantity = form.Read<int>("quantity");
        var thing = form.Read<string>("id");
        // ... or alternatively have a DTO and use [SupplyParameterFromForm(Name = "")] public AddToCartInfo AddToCart { get; set; }
    }
}

Scenario 8

<form name="updateTodos" method="post" @onsubmit="HandleSubmit">
    @foreach (var (id, item) in Items)
    {
        <p>
            <input type="checkbox" name="items[@id].IsDone" />
            <input name="items[@id].Text" value="@item.Text" />
            <button name="deleteItem" value="@id" type="submit">Delete @item</button>
        </p>
    }

    <button type="submit">Update items</button>

    <p>
        Add item:
        <input name="newItemText" placeholder="Type here..." />
        <button name="addItem" type="submit">Add</button>
    </p>
</form>

@code {
    public class TodoItem
    {
        public bool IsDone { get; set; }
        public string? Text { get; set; }
    }

    [SupplyParameterFromForm] public Dictionary<Guid, TodoItem> Items { get; set; } = new();
    [SupplyParameterFromForm] public Guid? DeleteItem { get; set; }
    [SupplyParameterFromForm] public string? AddItem { get; set; }
    [SupplyParameterFromForm] public string? NewItemText { get; set; }

    public void HandleSubmit()
    {
        if (DeleteItem.HasValue)
        {
            Items.Remove(DeleteItem.Value);
        }

        if (!string.IsNullOrEmpty(AddItem) && !string.IsNullOrEmpty(NewItemText))
        {
            Items.Add(Guid.NewGuid(), new TodoItem { Text = NewItemText });
        }
    }
}

This should work.

Scenario 9 Edit form and validation

This should work with the exception of splitting the form into multiple components, which is covered by

e48c31d

Scenario 10: Isomorphic forms

This works

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Jul 4, 2023

Thanks for the feedback. Regarding the "incoming POST without any form or submit handler" cases, those are not mainstream scenarios; they were more indicative of how we could untangle the web of concepts and allow each thing to work independently as basic machinery.

It's not a mainstream scenario and so I'm fine with imposing a rule like "we only accept a POST if it comes with a handler name that matches something we can find on the page", which implies you must have a form with a name. It doesn't strictly imply you must have @onsubmit but it does mean you must have @onsubmit:name.

Quite a few of the desired experience improvements are in flight already, such as taking [SupplyValueFromForm] name from the associated property name by default, so it seems like we're already heading the right way.

Following feedback, I think these are the updates I'm planning to do, hopefully as much as possible this week:

  • Implement support for cascading values from below the root component via DI. Somewhat independent, but having done this, it will be possible to decouple the model binding pieces from RouteView and possibly EditForm.
  • Improve usability of handler names
    • Ensure a plain <form> can be submitted and hits its @onsubmit handler without having to insert a handler name into the form (e.g., update prerendering logic to emit the hidden field containing the handler name)
    • If an incoming POST arrives with no handler name, give an error like The incoming POST request did not specify a form name. Update the form by adding an @onsubmit:name attribute with a unique name.
    • Require @onsubmit:name values to be nonempty, as @onsubmit:name="" is strange and makes debugging really difficult (e.g., "no handler was registered for ''")
    • We could optionally use the id attribute on a form as its handler name if there's no @onsubmit:name if we want, but arguably that could be a future enhancement
  • Improve correctness and flexibility of handler name->delegate lookups
    • Make event name tracking more robust by not separately tracking it each time the form is rendered (since if it renders multiple times, this can lead to wrong behavior as we'd be tracking the wrong delegate). Instead, perform the lookup at event dispatch time. Probably this means having the renderer subclass able to track a dictionary of event handlers by event name.
    • Support many-forms-one-handler patterns by only requiring the handler name to match a consistent (target, methodinfo) pair, not a single delegate [1]
  • Mechanism for accessing parts of HttpContext (perhaps [SupplyFromHttpContext] public HttpRequest Request { get; set; }), because we keep hearing how undesirable it is to recommend IHttpContextAccessor. Arguably this could be a future enhancement, so might be more of an RC1 thing.
  • Expose a Map API in ModelBindingContext

[1] This is about allowing basic old-school machinery like "using a form post to trigger an @onsubmit handler with some data (to perform some operation like add to cart, delete item, or launch missile)" without imposing undue high-level patterns. It doesn't require people to be using their form as a stateful edit form. No values have to be bound to any form here. If the triggered code wants to surface an error message like "Error: item 123 is no longer available" then the developer writes their own logic to show it wherever they want.

nonhierarchical - it's enough to say you must pick a unique enough name
How does it work when you render the same component in two different parts of the app at the same time

There does have to be an option to provide a unique name (like dozens of other web dev scenarios where you have to give a unique-enough name, like for cookies, headers, route templates, parameter names, etc.) but I don't think it has to be hierarchical, as in combining arbitrarily many levels. A single optional cascaded name could be combined with any form name specified within a component.

@javiercn
Copy link
Member

javiercn commented Jul 4, 2023

  • Implement support for cascading values from below the root component via DI. Somewhat independent, but having done this, it will be possible to decouple the model binding pieces from RouteView and possibly EditForm.
    • I understand what you mean by RouteView, but what exactly do you want to decouple from EditForm?
  • Improve usability of handler names
    • Ensure a plain <form> can be submitted and hits its @onsubmit handler without having to insert a handler name into the form (e.g., update prerendering logic to emit the hidden field containing the handler name)
      • What's the value of hiding this inside the renderer? It seems like unnecessary coupling. Why not give the user control to put the handler wherever they want in the request? (url, query, body).
    • If an incoming POST arrives with no handler name, give an error like The incoming POST request did not specify a form name. Update the form by adding an @onsubmit:name attribute with a unique name.
      • This sounds great.
    • Require @onsubmit:name values to be nonempty, as @onsubmit:name="" is strange and makes debugging really difficult (e.g., "no handler was registered for ''")
      • I don't think the particular value is a problem here, just the error message, which we can improve. The default was empty so that it would not require us to change the action parameter on the form.
    • We could optionally use the id attribute on a form as its handler name if there's no @onsubmit:name if we want, but arguably that could be a future enhancement
      • I'd be cautious about this, as it can make something in the page inadvertently bindable. For example when you add an id for interacting with it in JS or for E2E testing purposes.
  • Improve correctness and flexibility of handler name->delegate lookups
    • Make event name tracking more robust by not separately tracking it each time the form is rendered (since if it renders multiple times, this can lead to wrong behavior as we'd be tracking the wrong delegate). Instead, perform the lookup at event dispatch time. Probably this means having the renderer subclass able to track a dictionary of event handlers by event name.
      • I am not sure why this is desirable. The current implementation actively protects against this and imposes the requirement that since the app starts rendering to the time it gets to dispatch the form, only one element can register a given submit name. This is done to avoid situations where we could potentially end-up dispatching to the wrong form handler because they both registered the same name at different times.
        • Can you provide a concrete case that you are trying to fix with this?
        • See the logic here for details.
    • Support many-forms-one-handler patterns by only requiring the handler name to match a consistent (target, methodinfo) pair, not a single delegate [1]
      Are there any other ways we can accomplish this that do not rely on the implementation of choice
  • Mechanism for accessing parts of HttpContext (perhaps [SupplyFromHttpContext] public HttpRequest Request { get; set; }), because we keep hearing how undesirable it is to recommend IHttpContextAccessor. Arguably this could be a future enhancement, so might be more of an RC1 thing.
    • Sounds good.
  • Expose a Map API in ModelBindingContext
    • Sounds good.

[1] This is about allowing basic old-school machinery like "using a form post to trigger an @onsubmit handler with some data (to perform some operation like add to cart, delete item, or launch missile)" without imposing undue high-level patterns. It doesn't require people to be using their form as a stateful edit form. No values have to be bound to any form here. If the triggered code wants to surface an error message like "Error: item 123 is no longer available" then the developer writes their own logic to show it wherever they want.

How do they map the event handler to the form to show the information in context? For example, if you have a grid, you might want to highlight the row with the error. It has nothing to do with edit form or anything "higher level", but the triggered action should be associated with a unique element, just as in HTML.

This is about allowing basic old-school machinery like "using a form post to trigger an @onsubmit handler with some data (to perform some operation like add to cart, delete item, or launch missile)" without imposing undue high-level patterns.

nonhierarchical - it's enough to say you must pick a unique enough name
How does it work when you render the same component in two different parts of the app at the same time

There does have to be an option to provide a unique name (like dozens of other web dev scenarios where you have to give a unique-enough name, like for cookies, headers, route templates, parameter names, etc.) but I don't think it has to be hierarchical, as in combining arbitrarily many levels. A single optional cascaded name could be combined with any form name specified within a component.

Page.razor

<h3>Table component</h3>

<div>
    <table>
        <thead>
            <tr>
                <th>Id</th>
                <th>Name</th>
                <th>Price</th>
                <th>Remaining Units</th>
                <th></th>
                <th></th>
            </tr>
        </thead>
        <tbody>
            @foreach (var product in _catalog.Products)
            {
                <tr @key="product">
                    <td>@product.Id</td>
                    <td>@product.Name</td>
                    <td>@product.Price</td>
                    <td>@product.RemainingUnits</td>
                    <td>
                        <CascadingModelBinder Name="DeleteProduct(product.Id)">
                            <DeleteProduct Product="@product" OnDeleteProduct="DeleteProduct" />
                        </CascadingModelBinder>
                    </td>
                    <td>
                        <CascadingModelBinder Name="UpdateRemainingItems(product.Id)">
                            <UpdateRemaingItems Product="@product"
                                                OnUpdateRemainingItems="(remaining => UpdateRemainingItems(remaining.Product, remaining.Ammount))" />
                        </CascadingModelBinder>
                    </td>
                </tr>
            }
        </tbody>
    </table>
</div>

@code {
    static Catalog _catalog = Catalog.Instance;
    
    // This is just because we don't have string interpolation for component attributes.
    public string DeleteProduct(int id) => $"DeleteProduct({id})";
    public string UpdateRemainingItems(int id) => $"UpdateRemainingItems({id})";

    public void DeleteProduct(Product product)
    {
        _catalog.Products.RemoveAll(p => p == product);
        StateHasChanged();
    }

    public void UpdateRemainingItems(Product product, int ammount)
    {
        var remaining = _catalog.Products.SingleOrDefault(p => p == product);
        remaining!.RemainingUnits -= ammount;
        StateHasChanged();
    }
}

DeleteProduct.razor

<form method="post" @onsubmit="() => OnDeleteProduct.InvokeAsync(Product)" @onsubmit:name="@BindingContext.Name" >
    <AntiforgeryToken />
    <input type="hidden" name="handler" value="@BindingContext.Name" />
    <input type="submit" value="Delete" />
</form>

@code {

    [CascadingParameter] public ModelBindingContext BindingContext { get; set; }

    [Parameter] public Product Product { get; set; } = null!;

    [Parameter] public EventCallback<Product> OnDeleteProduct { get; set; }
}

UpdateRemainingItems.razor

<form
    method="post"
    @onsubmit="() => OnUpdateRemainingItems.InvokeAsync(new(Product, Ammount))"
    @onsubmit:name="@BindingContext.Name">
        <AntiforgeryToken />
        <input type="hidden" name="handler" value="@BindingContext.Name" />
        <input type="text" name="Ammount" value="1" />
    @{
        var errors = BindingContext.GetAllErrors();
    }
    @if (errors?.Any() == true)
    {
        <input type="submit" value="Update remaining items" />
    }
    else
    {
        <input type="submit" value="Update remaining items" />
    }
</form>

@code {
    [CascadingParameter] public ModelBindingContext BindingContext { get; set; }

    [Parameter] public Product Product { get; set; } = null!;

    [SupplyParameterFromForm] public int Ammount { get; set; } = 1;

    [Parameter] public EventCallback<RemainingItems> OnUpdateRemainingItems { get; set; }

    public record RemainingItems(Product Product, int Ammount);
}

The individual instances of UpdateRemainingItems need a way to disambiguate. They could take in a handler parameter that is passed in by the parent and use that in the form @onsubmit:name, but there is no way to apply that into the [SupplyParameterFromForm(Handler ="<<>>") parameter.

Then the other part of it is that you might be building and shipping more complex components, so the context must be able to flow deep into the hierarchy without requiring users to do so.

The above example shows that and some other things:

  • The only place where ids are involved is when we provide scope (via the CascadingModelBinder Name="..."
  • The DeleteProduct and UpdateRemainingItems components don't have to deal with a name themselves, they can just use the one in the binding context. Same for adding the handler in the body.
  • The mapping between strings <=> memory is done within the handlers. All the operations in the components are expressed in terms of entities/instances.

@SteveSandersonMS
Copy link
Member Author

I understand what you mean by RouteView, but what exactly do you want to decouple from EditForm?

Only the extra layer of model binding context that it creates. It might be necessary but also maybe there could be an opportunity to make the model binding context more explicit and EditForm more consistent with <form>. This is just something I'll investigate and see if it could work without breaking scenarios.

What's the value of hiding this inside the renderer? It seems like unnecessary coupling

It just simplifies things a lot for developers so they don't have to create the hidden field manually (or even care what magic name we use for the parameter). I'm totally fine with letting people optionally supply the handler name in other ways, such as querystring, but an automatic hidden field will likely do exactly what people want in 99% of cases.

I'd be cautious about this, as it can make something in the page inadvertently bindable.

Fair. I was only going to do this for forms that have both method="post" and @onsubmit=... as that seemed to be a clear indication of intent to receive submit actions. However I'm not in a rush to do this - we could always add it in the future if we see indications that people are struggling to understand @onsubmit:name (which probably won't happen).

(Name tracking) Can you provide a concrete case that you are trying to fix with this?

Event handler delegates can be added, updated, or removed arbitrarily with normal interactive rendering and it will be hard to understand (and seem like a framework bug) if that's not the case with SSR. For example:

<form method="POST" @onsubmit="@submitAction" @onsubmit:name="myform">
    <AntiforgeryToken />
    <input type="hidden" name="handler" value="myform" />
    <input type="submit" value="Send" />
</form>

@code {
    private Action? submitAction;

    public void HandleFormBasic() { ... }

    public void HandleFormBlueCheck() { ... }

    protected override async Task OnInitializedAsync()
    {
        var user = await MyData.LoadUser();
        submitAction = user.Name == "Elon" ? HandleFormBlueCheck : HandleFormBasic;
    }
}

If I'm understand the implementation correctly, then if LoadUser was synchronous then this would work, whereas if it's async then there will be an exception when trying to submit the form because the handler isn't tracked. That would be very hard to understand.

There's already a whole system for tracking how events get added/updated/removed so ideally we'd follow a similar pattern or perhaps just take a more basic approach of lookup during dispatch so we always get the correct delegate. I totally accept it's possible there's no reasonable solution to this but since it's a core, low-level feature, I at least want to look into it.

Are there any other ways we can accomplish this that do not rely on the implementation of choice

Not certain what's meant by "the implementation of choice". Are you distinguishing between @onsubmit=lambda and @onsubmit=method?

I'm only interested in doing the many-forms-one-handler thing if it follows easily and naturally from an updated event name tracking mechanism. I definitely won't do huge amounts of work/change for this if it doesn't follow naturally. AFAIK users can achieve the same outcome by wrapping their forms in a <CascadingModelBinder>.

The individual instances of UpdateRemainingItems need a way to disambiguate. They could take in a handler parameter that is passed in by the parent and use that in the form @onsubmit:name, but there is no way to apply that into the [SupplyParameterFromForm(Handler ="<<>>") parameter.

Perhaps I was unclear in how I phrased things before, but I was trying to be explicit that I'm not disputing the usefulness of adding a <CascadingModelBinder>, since that's the only way of configuring a [SupplyParameterFromForm] dynamically. I know that's required for cases like the one you posted (at least for the UpdateRemainingItems part anyway; it's not required for DeleteProduct since that has no [SupplyParameterFromForm] and can define its own handler name dynamically just as easily as the parent component could).

All I am questioning is the relevance of it being fully hierarchical, as in allowing for arbitrarily many levels. AFAICT it is sufficient to just look at the closest CascadingModelBinder and require that to have a sufficiently unique name (which developers can always organize themselves, including if that means taking in a parameter to the place that renders the CascadingModelBinder). If we can make that simplification it will reduce the sophistication of what's going on, will be easier to reason about, will give people less reason to get confused into creating deep hierarchies thinking it makes a difference, etc.

@SteveSandersonMS
Copy link
Member Author

Done in #49340 and #49261

@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jul 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-full-stack-web-ui Full stack web UI with Blazor
Projects
None yet
Development

No branches or pull requests

4 participants