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 method to Results/TypedResults to allow creating a ContentHttpResult with a custom status code #41444

Closed
DamianEdwards opened this issue Apr 29, 2022 · 3 comments · Fixed by #41452
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 enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing
Milestone

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Apr 29, 2022

The ContentHttpResult already supports setting a custom status code but the factory methods that allow creation of ContentHttpResult do not expose a status code parameter.

We should add the parameter to the existing Content factory methods on Results and TypedResults to allow setting a custom status code:

public static class Results
{
-    public static IResult Content(string content, string? contentType = null, Encoding? contentEncoding = null);
+    public static IResult Content(string content, string? contentType, Encoding? contentEncoding);
+    public static IResult Content(string content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null);

+    public static IResult Text(string? content, string? contentType, Encoding? contentEncoding)
+    public static IResult Text(string? content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null);
}

public static class TypedResults
{
-    public static ContentHttpResult Content(string? content, string? contentType = null, Encoding? contentEncoding = null)
+    public static ContentHttpResult Content(string content, string? contentType, Encoding? contentEncoding);
+    public static ContentHttpResult Content(string? content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null)

-    public static ContentHttpResult Text(string? content, string? contentType = null, Encoding? contentEncoding = null)
+    public static ContentHttpResult Text(string? content, string? contentType, Encoding? contentEncoding)
+    public static ContentHttpResult Text(string? content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null);
}
@DamianEdwards DamianEdwards added enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing area-web-frameworks api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 29, 2022
@ghost
Copy link

ghost commented Apr 29, 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 2, 2022

API Review Notes:

  • Do we need the two overloads?
    • Yes.
    • For this reason, removing parameter default values is acceptable in the specific case of "moving" those default values to a new method overload to eliminate ambiguity. For example, consider an existing method MyMethod(int a = 1). If you introduce an overload of MyMethod with two optional parameters a and b, you can preserve compatibility by moving the default value of a to the new overload. Now the two overloads are MyMethod(int a) and MyMethod(int a = 1, int b = 2). This pattern allows MyMethod() to compile.

    • https://docs.microsoft.com/en-us/dotnet/core/compatibility/#properties-fields-parameters-and-return-values
  • Are we going to add a statusCode parameter to any other Results method?
    • No. Dynamic status codes hurt statically-analyzable OpenAPI metadata.
    • Text is used a lot in error cases

Approved as proposed.

public static class Results
{
-    public static IResult Content(string content, string? contentType = null, Encoding? contentEncoding = null);
+    public static IResult Content(string content, string? contentType, Encoding? contentEncoding);
+    public static IResult Content(string content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null);

+    public static IResult Text(string? content, string? contentType, Encoding? contentEncoding)
+    public static IResult Text(string? content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null);
}

public static class TypedResults
{
-    public static ContentHttpResult Content(string? content, string? contentType = null, Encoding? contentEncoding = null)
+    public static ContentHttpResult Content(string content, string? contentType, Encoding? contentEncoding);
+    public static ContentHttpResult Content(string? content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null)

-    public static ContentHttpResult Text(string? content, string? contentType = null, Encoding? contentEncoding = null)
+    public static ContentHttpResult Text(string? content, string? contentType, Encoding? contentEncoding)
+    public static ContentHttpResult Text(string? content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null);
}

@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 2, 2022
@halter73
Copy link
Member

halter73 commented May 2, 2022

Post API review note:

  • I missed that we were inconsistent between string content and string? content in the API proposal. Here's the actual state of the PR which is more consistent:
public static class Results
{
-    public static IResult Content(string content, string? contentType = null, Encoding? contentEncoding = null);
+    public static IResult Content(string content, string? contentType, Encoding? contentEncoding);
+    public static IResult Content(string content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null);

+    public static IResult Text(string content, string? contentType, Encoding? contentEncoding)
+    public static IResult Text(string content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null);
}

public static class TypedResults
{
-    public static ContentHttpResult Content(string? content, string? contentType = null, Encoding? contentEncoding = null)
+    public static ContentHttpResult Content(string? content, string? contentType, Encoding? contentEncoding);
+    public static ContentHttpResult Content(string? content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null)

-    public static ContentHttpResult Text(string? content, string? contentType = null, Encoding? contentEncoding = null)
+    public static ContentHttpResult Text(string? content, string? contentType, Encoding? contentEncoding)
+    public static ContentHttpResult Text(string? content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null);
}

I think we should update all of the Results methods to take string? content to align with TypedResults. This should be non-breaking, but we can do this later all at once.

DamianEdwards added a commit that referenced this issue May 2, 2022
…code

* Added overloads on Results/TypedResults for Content/Text with status code
* Aligned the existing parameters to consistently accept nullable values for the content

Fixes #41444
@ghost ghost locked as resolved and limited conversation to collaborators Jun 1, 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 enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants