Proposal: Cookie and SetCookie Properties in HttpHeaders

[deinok]

Proposal:

Add Cookie and SetCookie as Properties in HttpRequestHeaders and HttpResponseHeaders.

    namespace System.Net.Http.Headers
    {

        public sealed class HttpRequestHeaders : HttpHeaders
        {
            public CookieCollection Cookie { get; set; }
        }

        public sealed class HttpResponseHeaders : HttpHeaders
        {
            public CookieCollection SetCookie { get; set; }
        }

    }

Reason:

Recover Cookies and setting Cookies from Headers is a common action.

[deinok]

Closes dotnet/runtime#19931 -- EDIT: closed as dupe on 2017/1/28. Ref: https://github.com/dotnet/corefx/issues/1611

[karelz]

How are the 3 issues different? They seem all dupes to me. Or is dotnet/runtime#14525 slightly different?

[deinok]

@karelz dotnet/runtime#19931 Is a duplicate (This is the proposal of that issue). But dotnet/runtime#14525 its only a reference to older disscusions

[karelz]

OK, I'll close the dupe then. How is the older discussion different? Do we expect different / more outcomes from it than this API? If not, it is also a dupe.

[deinok]

The older one seems targeting ASP. The only thing i propose is to have a property to get and set the cookies in a more easily way. If required, i can give an implementation of this.

[Tratcher]

CookieCollection is likely not the best API for this, and it doesn't follow the pattern used for any of the other headers.

Proposal: use the same pattern as the other headers as well as add types specific to the different request and response formats, CookieHeaderValue and SetCookieHeaderValue. ASP. NET Core already has implementations for these. https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs

[deinok]

@Tratcher I think it can work, but let me think the benefits and inconvenients of HeaderValue and CookieCollection

[Tratcher]

Let me put it another way. What does CookieCollection get you compared to the HttpHeaderValueCollection used by all of the other headers? Why should Cookie/SetCookie be different?

[deinok]

@Tratcher For what i can see, a SetCookieHeaderValue its more or less like a Cookie an the same for CookieHeaderValue . Can you explain me what's the benefits of using the style of HeaderValue in the case we already have a model for Cookies?

A HeaderParsercan be create to parse that Cookie Headers. The benefit of using Cookie and CookieCollection is that this models are already used in System.Net.Http. Probable benefits of using HeaderValue is that we can more easly react to changes in the CookieHeaderValue.

I'm missing something?

[Tratcher]

The Cookie type isn't appropriate for requests as it includes all of the response fields, which will likely cause confusion. E.g. "I added a whole list of cookies to the request and expected it to filter out the expired ones and the ones for different domains.." etc.. CookieContainer has these features, but HttpRequestMessage.Headers.Cookie shouldn't.

CookieHeaderValue and SetCookieHeaderValue also expose static parsers that are useful. HttpHeaderValueCollection also exposes the parsers. The only other thing you need is an extension that easily converts a SetCookieHeaderValue to a CookieHeaderValue when you want to copy cookies from a response to a request.

[deinok]

For what i know servers already filter Cookies (or should do it). Take this example.

var cookie=new Cookie(){Name="name",Value="value",Expires="Thu, 01 Jan 1970 00:00:01 GMT"};

This cookie is valid but the server should not react to that and the cookie Container the same. Also it is a Set-Cookie format.

request.Headers.Cookies.Add(cookie);

Internaly it is parsed to Cookie: name=value;

The server dont react to it cause it has expired but it is sended because this is what the developer wants to do. (Don't have sense, but its a valid option.)

The parser can be CookieHeaderParser and SetCookieHeaderParser. The two accepting a Cookie instance

[Tratcher]

I think you've just demonstrated my point about people being confused by the behaviors of the extra fields. The expiration (and all other cookie params) are not transmitted on the request, only name=value, so the server can't filter them for you. Filtering only happens on the client when deciding which cookies to send before adding them to the headers.

[deinok]

@Tratcher Thanks for the explanation. @karelz Proposal updated

[karelz]

@Priya91 @davidsh @CIPop does the proposal look reasonable to you?

[Priya91]

@deinok The SetCookieHeaderValue and CookieHeaderValue types are not defined in .NET Core. Can you update the API proposal with the public surface area for those types as well. The ASP.NET ones looks good.

Conceptually the API makes sense, and some additional breaks to look out for:

1. Mono also has CookieHeaderValue under same namespace System.Net.Http.Headers. The .NET Core version type as defined in this API is gonna differ in API surface area from Mono, given that this class should only define, cookie: name=value, and Mono's API defines SetCookie header value. So these types will be different and will cause compile time breaks on libraries referencing Mono System.Net.Http.Formatting.dll and .NET Core System.Net.Http.dll.

2. The API surface area of CookieHeaderValue and SetCookieHeaderValue in Microsoft.Net.Http.Headers of ASP .NET Core is what we want in .NET Core. These will be under different namespaces, so conflict during compile time can be mitigated with namespace references. Potentially in future we can get ASP.NET to use the .NET Core types to avoid duplication of types in different frameworks.

[Tratcher]

Note everything in ASP.NET Core's Microsoft.Net.Http.Headers namespace/package are forks of System.Net.Http.Headers with proposed improvements for future unification. We're going to do another pass on these soon to reduce their allocations / improve their efficiency.

[deinok]

@Priya91 @Tratcher So what namespace CookieHeaderValue and SetCookieHeaderValue should have? System.Net.Http.Headers will be ok. Seems like there's no problem with ASP.NET Core version and i can't find any problem with Mono.

Mono System.Net.Http.Headers: https://github.com/mono/mono/tree/master/mcs/class/System.Net.Http/System.Net.Http.Headers

Should i define the contract of CookieHeaderValue and SetCookieHeaderValue ?

[Tratcher]

System.Net.Http.Formatting.dll is from ASP.NET WebApi, not Mono. Or is there a seperate mono version as well? http://aspnetwebstack.codeplex.com/SourceControl/latest#src/System.Net.Http.Formatting/Headers/CookieHeaderValue.cs

[deinok]

@Tratcher No, you are right, sorry. So, how can we solve this? Any idea?

[Priya91]

System.Net.Http.Formatting.dll is from ASP.NET WebApi, not Mono. Or is there a seperate mono version as well?

@Tratcher What's the version under Microsoft.Net.Http.Headers? Here

Mono seems to pick up aspnet sources, so that means, there's are 2 versions of CookieHeaderValue, one in System.Net.Http.Formatting.dll and other in Microsoft.Net.Http.Headers.dll

[Tratcher]

@Priya91 that's https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.Net.Http.Headers/CookieHeaderValue.cs

Looks like we'll have to fudge the name of CookieHeaderValue when porting it to corefx. CookieValue? It will only have Name and Value properties (and parsers, etc.).

[Priya91]

Looks like we'll have to fudge the name of CookieHeaderValue when porting it to corefx. CookieValue?

Discussed this with @weshaggard offline, and Wes had suggested that we had similar cases in adding new apis to collections before, and we could use the same API name, and mitigate the issue with unification in other platforms for same API surface area.

The real issue here is with the CookieHeaderValue definition in System.Net.Http.Formatting.dll, which has different api surface area than the one we want.

cc @weshaggard What would be a good option here?

[weshaggard]

If the type name is the same and the API's are different then the only real option is to create a different type name. Before we do that though we should really understand if we cannot have the same type with all the same APIs and if we can do that then we can just type-forward theirs to the one we ship here.

[Tratcher]

To add a type forward you'd have to update System.Net.Http.Formatting.dll, which isn't being actively developed.

[Tratcher]

@weshaggard the APIs are too different, we do not want the CookieHeaderValue surface area from System.Net.Http.Formatting.dll, just the namespace and type name.

[weshaggard]

OK then we will need to come up with another type name to avoid the type collision.

[deinok]

HttpCookieHeaderValue and SetHttpCookieHeaderValue or use the Cookie class?

[Priya91]

@deinok Sounds good, it could be HttpSetCookieHeaderValue though :) Can you update your proposal with the api surface area for these 2 types as well..

[Tratcher]

Or just SetCookieHeaderValue, the conflict was only for CookieHeaderValue.

[deinok]

@Priya91 and @Tratcher So HttpSetCookieHeaderValue and HttpCookieHeaderValue in namespace System.Net.Http.Headers are okey?

[davidsh]

I'm not comfortable yet with these names:

HttpSetCookieHeaderValue and HttpCookieHeaderValue

as they are inconsistent with the rest of the strongly typed property names in the current API surface with respect to using the Http naming prefix.

We'll need to work with @terrajobst to come with appropriate names that will pass API review.

[deinok]

@davidsh Okey, I will wait for you ;)

[karelz]

@deinok it's not so much about waiting, but more about proposing alternatives and discussing them here :)

[deinok]

@karelz Then, the only viable way I see is use System.Net.Cookie

[karelz]

@deinok I am not fully following up on the thread, and I am not expert on the area (like other API reviewers). It would be great to see the shape of the API of both options - with highlighted pros/cons.

[Priya91]

@deinok You can update the proposal with the api names you had initially mentioned, we don't have to block progress on API name now, if someone comes up with a better name on this thread, we can go ahead and update the proposal.

The names HttpCookieHeaderValue and HttpSetCookieHeaderValue sound alright to me, or @davidsh how about RequestCookieHeaderValue and ResponseSetCookieHeaderValue?

[deinok]

Proposal updated to the initial state

[davidsh]

Setting to Future since these would be new APIs and beyond NETStandard2.0.

[dotnet-policy-service[bot]]

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

[dotnet-policy-service[bot]]

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.