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

Can InputFormatterContext be replaced by ModelBindingContext #2330

Closed
pranavkm opened this issue Apr 3, 2015 · 11 comments
Closed

Can InputFormatterContext be replaced by ModelBindingContext #2330

pranavkm opened this issue Apr 3, 2015 · 11 comments

Comments

@pranavkm
Copy link
Contributor

pranavkm commented Apr 3, 2015

Today IInputFormatter get access to the ActionContext and the ModelType without getting access to all the properties that ModelBindingContext exposes. Could we replace it with ModelBindingContext so it's the same programming model when writing binders or formatters? Additionally, if we remove the dependency on ActionContext, we could change it so that formatters only rely on the ModelBinding assembly as opposed to Mvc.Core. Might make the layering a bit cleaner.

@kichalla
Copy link
Member

kichalla commented Apr 3, 2015

A possible scenario is where users might want to deserialize the request stream before model binding stage, example authorization or resource filters, in which case ModelBindingContext would not be available...just curious, what are the properties on the ModelBindingContext that you are referring to?

Also wondering other scenario where there are no parameters on the action, but the user might want to deserialize within it, in this case i guess we would not have the ModelBindingContext?

@rynowak
Copy link
Member

rynowak commented Apr 5, 2015

@pranavkm we're going to do something about the layering here as part of the current milestone. There's a few bugs already tracking it.

A possible scenario is where users might want to deserialize the request stream before model binding stage, example authorization or resource filters, in which case ModelBindingContext would not be available..

@kichalla we should more or less consider our guidance about what to do in various filter stages to be prescriptive. If it becomes hard to do something that we never intended you to do, then that's alright.

That said, I don't see InputFormatter having its own context as a problem to be solved. The layering problems related to the current state of all of this should be addressed.

@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 5, 2015

That said, I don't see InputFormatter having its own context as a problem to be solved.

I guess the discussion was more along the lines of do we need the ActionContext to flown to the formatters - they currently don't make much use of it.,

@danroth27 danroth27 added this to the 6.0.0-beta5 milestone Apr 6, 2015
@danroth27
Copy link
Member

@rynowak Please make it layer like a delicious cake.

@harshgMSFT
Copy link
Contributor

@pranavkm I do not think that we should be using ModelBindingContext for input formatters simply because there are too many model binding specific properties on MBC which makes it the MBC ( and not InputFormatterContext). The ActionContext was flown so that any custom input formatter can have access to Request data as well as anything related to the action based on which it can choose to act differently.

@rynowak can you point to the relevant layering bugs for better understanding.

@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 6, 2015

as well as anything related to the action based on which it can choose to act differently.

Shouldn't that happen as part of the ResourceFilter?

@harshgMSFT
Copy link
Contributor

as part of ResourceFilter it can add or remove an input formatter or modify a publicly exposed setting of the formatter, but if there is something that input formatter needs to decide ( not applicability but how to deserialize etc). Its really arguable but its more extensible this way.

@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 6, 2015

Wouldn't that argument also apply to model binders \ value providers \ validators?

@harshgMSFT
Copy link
Contributor

Model binders already have an operation binding context, which gives them access to all they can need. Not getting into a debate here, I am not arguing if it is necessary or not, I am just mentioning why it was done 😄 .

@rynowak
Copy link
Member

rynowak commented May 1, 2015

The other issue here is that InputFormatters need ModelState - and not just ActionContext.ModelState - that breaks scenarios where you manually call model binding.

Formatters are intentionally a bit more constrained than a model binder, and they really don't need everything. They don't need the action, if you want to make per-action decisions then use resource filters.

Proposal:

public class InputFormatterContext
{
        public HttpContext HttpContext { get; }

        public ModelStateDictionary ModelState { get; }

        public Type ModelType { get; }
}

public class OutputFormatterContext
{
        public object Object { get; set; }

        public Type DeclaredType { get; set; }

        public HttpContext HttpContext { get; set; }

        public Encoding SelectedEncoding { get; set; }

        public MediaTypeHeaderValue SelectedContentType { get; set; }

        public int? StatusCode { get; set; }

        public bool? FailedContentNegotiation { get; set; }
}

rynowak added a commit that referenced this issue May 12, 2015
This change simplifies InputFormatterContext/OutputFormatterContext by
swapping ActionContext for HttpContext.

This change is important especially for InputFormatterContext as it
decouples ModelState and ActionContext - allowing us to fix a
related bug where the _wrong_ ModelState can be passed in for a
TryUpdateModel operation.
@rynowak
Copy link
Member

rynowak commented May 12, 2015

39fe063

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants