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

CreatedResult should accept null location #39454

Open
1 task done
aayjaychan opened this issue Jan 12, 2022 · 5 comments
Open
1 task done

CreatedResult should accept null location #39454

aayjaychan opened this issue Jan 12, 2022 · 5 comments

Comments

@aayjaychan
Copy link

@aayjaychan aayjaychan commented Jan 12, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

RFC7231 suggests that the Location header is optional for 201 Created response.

The primary resource created by the request is identified by either a Location header field in the response or, if no Location field is received, by the effective request URI.

However, currently the constructors of CreateResult throws if null is passed to location, and ControllerBase.Created() throws if null is passed to url.

Expected Behavior

A response of 201 Created without Location header is created.

Steps To Reproduce

[HttpPost]
public ActionResult Create()
{
    return new CreatedResult((string?)null, null); // throws ArgumentNullException
}
[HttpPost]
public ActionResult Create()
{
    return Created((string?)null, null) // throws ArgumentNullException
}

Exceptions (if any)

System.ArgumentNullException: Value cannot be null. (Parameter 'location')
   at Microsoft.AspNetCore.Mvc.CreatedResult..ctor(String location, Object value)
   ...

.NET Version

6.0.101

Anything else?

No response

@khellang
Copy link
Member

@khellang khellang commented Jan 12, 2022

[HttpPost]
public ActionResult Create()
{
    return new StatusCodeResult(StatusCodes.Status201Created);
}
[HttpPost]
public ActionResult Create()
{
    return StatusCode(StatusCodes.Status201Created);
}

🤔

@khellang
Copy link
Member

@khellang khellang commented Jan 12, 2022

Since your examples pass null as the value as well, you could just use StatusCode or StatusCodeResult.

Now, I have wished for an overload of Created that has value, but not uri. A lot of times when you return back a complete result, it's unnecessary to return the location to the client as that URI would just yield the same result.

@msftbot
Copy link
Contributor

@msftbot msftbot bot commented Jan 13, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@halter73
Copy link
Member

@halter73 halter73 commented Jan 13, 2022

Strawman API proposal:

namespace Microsoft.AspNetCore.Mvc
{
    public class CreatedResult : ObjectResult
    {
+        public CreatedResult()
    
-        public CreatedResult(string location, object? value)
+        public CreatedResult(string? location, object? value)

-     public CreatedResult(Uri location, object? value)
+    public CreatedResult(Uri? location, object? value)
    }

    public abstract class ControllerBase
    {
+        public virtual CreatedResult Created()
    
-        public virtual CreatedResult Created(string uri, [ActionResultObjectValue] object? value)
+        public virtual CreatedResult Created(string? uri, [ActionResultObjectValue] object? value)

-         public virtual CreatedResult Created(Uri uri, [ActionResultObjectValue] object? value)
+        public virtual CreatedResult Created(Uri? uri, [ActionResultObjectValue] object? value)
    }
}

namespace Microsoft.AspNetCore.Http
{
    public static class Results
    {
+        public static IResult Created()
    
-        public static IResult Created(string uri, object? value)
+        public static IResult Created(string uri?, object? value)

-        public static IResult Created(Uri uri, object? value)
+        public static IResult Created(string uri?, object? value)
    }
}

I'm not super convinced that making the location/uri parameters nullable is a huge win since you'd need either (string?)null or (Uri?)null to select the right overload. We could also consider adding single parameter overloads that just take the location/uri.

@msftbot
Copy link
Contributor

@msftbot msftbot bot commented Jan 13, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants