Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Consolidate all header parsing code #825

Closed
muratg opened this issue Apr 24, 2017 · 15 comments
Closed

Consolidate all header parsing code #825

muratg opened this issue Apr 24, 2017 · 15 comments
Assignees

Comments

@muratg
Copy link

muratg commented Apr 24, 2017

Header parsing logic is used in

  • MVC
  • StaticFiles
  • ResponseCaching

We should investigate the usages across the board and consolidate the code here.

@muratg
Copy link
Author

muratg commented May 15, 2017

Tracking issues:

@Tratcher
Copy link
Member

@Eilon #758 is complete, next we need to investigate having MVC replace their implementation with this one.

@muratg muratg modified the milestones: 2.0.0-preview3, 2.0.0-preview2 May 25, 2017
@muratg
Copy link
Author

muratg commented May 25, 2017

@Eilon Moving the rest of the work to preview3.

@muratg muratg modified the milestones: 2.0.0-preview3, 2.0.0 Jun 12, 2017
@muratg muratg modified the milestones: 2.1.0, 2.0.0 Jun 27, 2017
@muratg
Copy link
Author

muratg commented Aug 8, 2017

@Eilon could we do this in 2.1?

@Eilon
Copy link
Contributor

Eilon commented Aug 8, 2017

Yessss... @jkotalik is actually already working on some of this, starting with the toughest of them all: MediaType header value.

@Eilon Eilon removed their assignment Aug 8, 2017
@jkotalik jkotalik self-assigned this Aug 28, 2017
@jkotalik
Copy link
Contributor

@muratg @Eilon We added structured suffixes and facets to MediaTypeHeaderValue (MTHV) in HttpAbstractions. I was trying to modify MediaType in MVC to wrap MTHV, though it seemed non trivial and wouldn't have much benefit until we either remove MediaType or can deprecate it. What other header logic can be moved from StaticFiles/ResponseCaching to HttpAbstractions?

@muratg
Copy link
Author

muratg commented Oct 16, 2017

I think we can deprecate MediaType. @Eilon thoughts?

@Tratcher
Copy link
Member

Tratcher commented Oct 16, 2017

@jkotalik the idea is to at least remove the duplicate code and have MediaType refer to MTHV. MediaType's code has already been the source of multiple critical bugs.

As for StaticFiles and ResponseCaching you'll have to investigate yourself. Those components process lots of headers but they mostly use HttpAbstractions types to do it.

@rynowak
Copy link
Member

rynowak commented Oct 16, 2017

@javiercn did some research on obsoleting MediaType and I think his findings were promising. @javiercn do you remember what the conclusions were?

@rynowak
Copy link
Member

rynowak commented Oct 16, 2017

@javiercn just came by to express to me just how excited he is to share his wealth of knowledge and extensive notes in this area; alas, he had to go to an important meeting first.

@Eilon
Copy link
Contributor

Eilon commented Oct 17, 2017

I would only obsolete MediaType if we actually plan to get rid of it, say, in MVC 3.0. Would that be the plan?

@javiercn
Copy link
Member

We should have a design we are all happy with for 3.0 (one that allows efficient header non allocating iteration as a basic API and a richer strongly typed allocating API for convenience)

@Eilon
Copy link
Contributor

Eilon commented Oct 17, 2017

I set up a mtg for this for tomorrow. I invited @javiercn @muratg @jkotalik @rynowak @Tratcher .

@Eilon
Copy link
Contributor

Eilon commented Oct 18, 2017

Plan from the mtg:

  • Change MediaType into a facade that just calls into MediaTypeHeaderValue
  • Mark MediaType as obsolete
  • Find all public exposure of MediaType in MVC, obsolete them, and make sure that there are non-obsolete alternatives
  • Do basic before/after perf tests on MVC scenarios for conneg to assess whether there is a regression, and, if so, how much

@aspnet-hello
Copy link

This issue was moved to dotnet/aspnetcore#2705

@aspnet aspnet locked and limited conversation to collaborators Jan 2, 2018
@aspnet-hello aspnet-hello removed this from the 2.1.0 milestone Jan 2, 2018
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

7 participants