daviddotcs / safe-routing

A C# source generator for ASP.NET Core razor pages and MVC controllers which produces strongly-typed identifiers for routes.
MIT License
31 stars 2 forks source link

Asp.Net Core MVC Model reverting to default #1

Closed Indeedornot closed 1 year ago

Indeedornot commented 2 years ago

Hi, I'm using asp.net core 3.1 mvc, so far the package worked amaizingly, however today i run into a problem

    public ActionResult Login(EmailLoginVm emailCred)
    {
        return Routes.Controllers.Email.ShowEmails(emailCred).Redirect(this);
    }

    [HttpGet]
    public async Task<ActionResult> ShowEmails(EmailLoginVm emailCred)
    {
        var emails = await _emailService.ReceiveEmailAsync(emailCred);
        return View(emails);
    }

Upon the redirection my model looses stored values and reverts to the default values

image My Folder architecture

daviddotcs commented 2 years ago

Thanks for writing up the issue. Unfortunately, only simple types are supported at this time. Currently, the body of the Login method above is equivalent to:

return RedirectToAction("ShowEmails", "Email", new RouteValueDictionary
{
  ["emailCred"] = emailCred
});

But correctly binding complex types requires something more like the following where each property in the EmailLoginVm type is a separate entry in the RouteValueDictionary:

return RedirectToAction("ShowEmails", "Email", new RouteValueDictionary
{
  ["emailCred.X"] = emailCred.X,
  ["emailCred.Y"] = emailCred.Y
  // ...
});

I'll investigate adding support for complex types in the next release.

daviddotcs commented 1 year ago

Sorry, I only just found the time to investigate this properly and unfortunately, I don't see how this would work. The standard ASP.NET Core methods for redirects require you to explicitly expand the complex types out into simple types. E.g.; The B() redirect will not work and produces a route with the ToString() representation of Foo, but the C() redirect does work:

public sealed class FooController : Controller
{
  public IActionResult A(Foo? foo)
    => Content($"x: {foo?.X}, y: {foo?.Y}");

  public IActionResult B()
    => RedirectToAction("A", new RouteValueDictionary()
    {
      ["foo"] = new Foo { X = "x value", Y = "y value" }
    });

  public IActionResult C()
    => RedirectToAction("A", new RouteValueDictionary()
    {
      ["foo.X"] = "x value",
      ["foo.Y"] = "y value"
    });
}

public sealed class Foo
{
  public string? X { get; set; }
  public string? Y { get; set; }
}

It's not possible for a source generator to reliably determine whether there's a custom model binder registered for a particular type or whether the ToString() representation is sufficient, so knowing which complex types to expand would require additional attributes to flag them. There'd also need to be consideration of cyclical type references, maximum depth of properties to visit, and the ability to include/exclude particular properties.

I don't plan to implement that at this stage, so I'd recommend sticking to simple types with this generator, or using the inbuilt methods where there are complex types which need to be explicitly expanded.