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

Add support for argument list surrogates in minimal APIs #40712

Closed
1 task done
davidfowl opened this issue Mar 15, 2022 · 11 comments · Fixed by #41325
Closed
1 task done

Add support for argument list surrogates in minimal APIs #40712

davidfowl opened this issue Mar 15, 2022 · 11 comments · Fixed by #41325
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks Docs This issue tracks updating documentation feature-minimal-actions Controller-like actions for endpoint routing
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Mar 15, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

The idea here is to support using a type that can act like a surrogate for the argument list in a minimal API. The motivation is about refactoring a minimal API that takes a large set of parameters into one that takes a single object with top level properties that represent what were once arguments. This is a common pattern seen with CQRS frameworks where a request object is made to bind all inputs.

Describe the solution you'd like

app.MapGet("/products/{id}", (int id, int pageSize, int page, ILogger<Program> logger, MyDb db) =>
{
    logger.LogInformation("Getting products for page {Page}, page);
    
    return db.Products.Skip((page - 1) * pageSize).Take(pageSize);
});

The user wishes to refactor this to:

app.MapGet("/products/{id}", ([Parameters]ProductRequest req) =>
{
     req.Logger.LogInformation("Getting products for page {Page}, req.Page);
    
     return req.Db.Products.Skip((req.Page - 1) * req.PageSize).Take(req.PageSize);
});

public record ProductRequest(int Id, int PageSize, int Page, ILogger<Program> Logger, MyDb Db);

The parameter or type needs to be marked explicitly to identify that this is not from the body or elsewhere (query etc). These properties should also support binding attributes:

public class ProductRequest
{
  [FromRoute]
  public int Id { get; set; } 

  [FromQuery]
  public int PageSize { get; set; }

  [FromQuery]
  public int Page { get; set; } 

  [FromServices]
  public ILogger<Program> Logger { get; set; } 

  [FromServices]
  public MyDb Db {get; set; }
}

This would only work for top level properties in this object.

Additional context

Open questions:

  • Do we want to support multiple of these? No 😄
@ghost
Copy link

ghost commented Mar 15, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 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.

@Acerby
Copy link
Contributor

Acerby commented Apr 4, 2022

I suspect its fairly common for a number of API method to support a common set/subset of options/properties. Being able to do something akin to:
app.MapGet("/chairs/{cid}", (int cid, [Parameters]CommonProps) => {...}
app.MapGet("/seats/{sid}", (int sid, [Parameters]CommonProps) => {...}
would seem to provide a nice way to allow these to be declared/managed/used in a consistent way.
Additionally common pre-processing operations could be encapsulated by methods/extension-methods on the above CommonProps class.

@Acerby
Copy link
Contributor

Acerby commented Apr 4, 2022

It feels like there is an overlap between this functionality and that supported by the IModelBinder interface. The primary difference being the class is based declarative automatic generation of the fields based as opposed to being generated via bespoke code. I guess one potential approach to implementation would be to use the [Properties] attribute on the class as a trigger to the complier to auto generate the code to add a default implementation of the IModelBinder interface on the given class. Provision for this might also result in simpler/cleaner code for cases where IModelBinder may currently be required/used.

@rafikiassumani-msft rafikiassumani-msft added the feature-minimal-actions Controller-like actions for endpoint routing label Apr 19, 2022
@brunolins16 brunolins16 changed the title Add support for arugment list surrogates in minimal APIs Add support for argument list surrogates in minimal APIs Apr 19, 2022
@brunolins16 brunolins16 linked a pull request Apr 22, 2022 that will close this issue
@brunolins16
Copy link
Member

brunolins16 commented Apr 25, 2022

Proposed API

Add new attribute that users can decorate their classes or delegates parameters to allow refactoring a minimal API that takes a large set of parameters into one that takes a single object with top level properties that represent what were once arguments.

namespace Microsoft.AspNetCore.Http;

+[AttributeUsage(AttributeTargets.Parameter, Inherited = false, AllowMultiple = false)]
+public sealed class ParametersAttribute : Attribute
+{ }

The idea is supporting the attribute in multiples parameters and also mixed with parameters without the atrribute.

Usage Examples

Attribute specified in the delegate parameter

app.MapGet("/products/{id}", ([Parameters]ProductRequest req, [Parameters]Paging pageInfo) =>
{    
     return req.Db.Products.Skip((req.Page - 1) * req.PageSize).Take(req.PageSize);
});

public record ProductRequest(int Id, ILogger<Program> Logger, MyDb Db);
public record Paging(int PageSize, int Page);

or

app.MapGet("/products/{id}", (int id, ILogger<Program> logger, MyDb db, [Parameters]Paging pageInfo) =>
{    
     return req.Db.Products.Skip((req.Page - 1) * req.PageSize).Take(req.PageSize);
});
public record Paging(int PageSize, int Page);

Alternative Designs

As an alternative design, we could also create a public interface (marker).

namespace Microsoft.AspNetCore.Http.Metadata;

+public interface IFromParametersMetadata
+{}
namespace Microsoft.AspNetCore.Http;

+[AttributeUsage(AttributeTargets.Parameter, Inherited = false, AllowMultiple = false)]
+public sealed class ParametersAttribute : Attribute, IFromParametersMetadata
+{ }

That will allow users to create their own attribute implementation, as we have for IFromServiceMetadata, IFromRouteMetadata, etc.

@brunolins16 brunolins16 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 25, 2022
@brunolins16 brunolins16 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 2, 2022
@ghost
Copy link

ghost commented May 2, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member

halter73 commented May 5, 2022

I'm not sure that I like how generic [Parameters] is for a parameter attribute. I don't think many people would guess what the attribute does without already knowing. It almost seems redundant. Of course, a parameter is a parameter 😆

This is going to be in a global using by default as part of the Microsoft.AspNetCore.Http namespace, so it could show up in a lot of contexts. This is different than all the other attributes we currently use in route handlers like [FromBody] that lives in Microsoft.AspNetCore.Mvc.

I know what we're really trying to indicate is that the properties of the parameter are treated as a top-level parameter and that it's hard to communicate this in a reasonably short attribute name though. Maybe [Flatten] would work? That's less seemingly redundant but just as ambiguous. [PropertiesAsParameters]? Would we ever put this in another namespace? We cannot reasonably put it in Microsoft.AspNetCore.Mvc unless MVC also supports it which seems unlikely.

@davidfowl
Copy link
Member Author

I was waiting for this ugly name discussion 😄. I agree parameters is too generic. I can't wait to 🚲 shed on this.

@brunolins16
Copy link
Member

brunolins16 commented May 9, 2022

Some additional name ideas:

[AsParameters]
[Flatten]
[PropertiesAsParameters]
[ParameterList]
[Surrogate]

@halter73
Copy link
Member

halter73 commented May 9, 2022

API review notes:

  • What should we target?
    • Parameters? For sure.
    • Types? Not for now. It might make it appear that nesting should work. It's not supported by other parameter attributes in MVC.
    • Properties? No nesting! 😞
  • Name?
    • Between keeping [Parameters] and [AsParameters]. The [AsParameters] feels more like the [From...] attributes.
    • [AsParameters] wins.
  • Should the attribute be inheritable?
    • What does this even mean for parameters? Are parameter attributes inherited from the overridden method on the base type?
    • This seems pretty niche for minimal route handlers which are normally delegates or local methods. I don't think I've even seen someone use an overridden method as a route handler yet.
    • Starting out with Inherited = false seems simpler and safer. If we change our mind, I doubt it'd break many apps if we switch the attribute to be inheritable.
  • Should [FromServices] be updated to support properties?
    • Yes, but be careful that this doesn't have weird MVC side effects.
  • Are we happy with the Microsoft.AspNetCore.Http namespace? It's globally included by default.
    • Yes we're happy especially now that the attribute is [AsParameters] instead of [Parameters].

API Approved!

namespace Microsoft.AspNetCore.Http;

+[AttributeUsage(AttributeTargets.Parameter, Inherited = false, AllowMultiple = false)]
+public sealed class AsParametersAttribute : Attribute
+{ }

namespace Microsoft.AspNetCore.Mvc;

-[AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
+[AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Property, AllowMultiple = false, Inherited = true)]
public class FromServicesAttribute : Attribute, IBindingSourceMetadata, IFromServiceMetadata

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 9, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 2022
@JamesNK
Copy link
Member

JamesNK commented Jun 18, 2022

Does this feature work in MVC? If not, are there any plans to add it? It seems like the idea can be applied to both frameworks.

@JamesNK
Copy link
Member

JamesNK commented Jun 18, 2022

Issue for adding support to MVC: #42271

@brunolins16 brunolins16 added the Docs This issue tracks updating documentation label Jul 5, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks Docs This issue tracks updating documentation feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants