Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Action results pattern #657

Closed
yishaigalatzer opened this issue Jun 12, 2014 · 3 comments
Closed

Action results pattern #657

yishaigalatzer opened this issue Jun 12, 2014 · 3 comments
Assignees
@rynowak
Labels
3 - Done enhancement
Milestone
1.0.0-beta3

Comments

@yishaigalatzer
Copy link
Contributor

Examples:

JsonResult allow status code on JsonResult.

RedirectResult exposes the url but doesn't let you change it.

Also redirectresult eagerly resolves UrlHelper just to find out if a Url is local (which means a service is resolved on every call unnecessarily).

See:
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Core/ActionResults/RedirectResult.cs#L35

Suggested pattern: Every result exposes properties as get set, so results filters can modify them and observe the values.

Services are resolved on a per needed basis, and not eagerly.

Static things like IsLocalUrl are not done as Transient services, they can either be static methods or Singleton services.
@yishaigalatzer yishaigalatzer added this to the 1.0.0-alpha2 milestone Jun 12, 2014
@javiercn
Copy link
Member

Needs discussion and break down.

This was referenced Oct 13, 2014
Closed
Closed
@rynowak rynowak modified the milestones: 6.0.0-rc1, 6.0.0-beta1 Oct 17, 2014
@rynowak rynowak assigned rynowak and unassigned yishaigalatzer Oct 23, 2014
@danroth27 danroth27 modified the milestones: 6.0.0-beta2, 6.0.0-rc1 Oct 30, 2014
@danroth27 danroth27 mentioned this issue Oct 30, 2014
Closed
@yishaigalatzer yishaigalatzer modified the milestones: 6.0.0-rc1, 6.0.0-beta2 Dec 11, 2014
@rynowak
Copy link
Member

rynowak commented Jan 16, 2015

Doing a pass and cleaning this up things that should be true in general.

Properties are get/set by default

  • Some require a not-null value (FileStreamResult)
  • Services needed by the result are get/set and default to null. They are resolved inside Execute

Exception to this is HttpStatusCodeResult and derived classes. Since all this class does is set a status code, it's not get/set. Then you can't end up with an NotFoundResult(201) or other nonsense.

Adding a StatusCode property to the more complex result types where it makes sense. (ViewResult, ContentResult, but not FileResult).

rynowak added a commit that referenced this issue Jan 16, 2015
@rynowak
Some cleanup of ActionResults - #657
6a357f6 
In general all properties are get/set so filters can change them.
 - some validate for not-null
 - where we use services it's get/set also

Services are resolved in the Execute method if not provided.

A few more ActionResults that return a body have the ability to set a
status code now (optional).
rynowak added a commit that referenced this issue Jan 16, 2015
@rynowak
Some cleanup of ActionResults - #657
b52a4e1 
In general all properties are get/set so filters can change them.
 - some validate for not-null
 - where we use services it's get/set also

Services are resolved in the Execute method if not provided.

A few more ActionResults that return a body have the ability to set a
status code now (optional).
rynowak added a commit that referenced this issue Jan 17, 2015
@rynowak
Some cleanup of ActionResults - #657
692a072 
In general all properties are get/set so filters can change them.
 - some validate for not-null
 - where we use services it's get/set also

Services are resolved in the Execute method if not provided.

A few more ActionResults that return a body have the ability to set a
status code now (optional).
@rynowak
Copy link
Member

rynowak commented Jan 17, 2015

692a072

@rynowak rynowak closed this as completed Jan 17, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@rynowak rynowak

Labels
3 - Done enhancement
Projects
None yet
Milestone
1.0.0-beta3
Development

No branches or pull requests
4 participants
@yishaigalatzer @rynowak @danroth27 @javiercn