Make cookies return null when the key doesn't exist.

[yukozh]

We used to if (Cookies["xxx"] == null) to detect the key exist or not.

Now, if key xxx doesn't exist, it will return "".

The String.Empty will equals to "", We have to use Cookies.Keys.Contains("xxx") to detect the key exist or not. it's so complex.

/cc @Eilon @troydai @JunTaoLuo

[dnfclas]

Hi @kagamine, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[yukozh]

Should I add an Assert into public void Cookies_GetAndSet()?

Assert.IsNull(cookies1["name2"]);

[NickCraver]

+1 on this change, missing should be null. I think Headers should behave this way as well. It's going to be a porting painpoint. For example, with Headers I have to use Microsoft.Extensions.Primitives to do a != StringValues.Empty check...that's just weird. Can we just keep it simple here?

I assume there was discussion and reasoning behind this change in the first place though, looking forward to hearing the background.

[davidfowl]

@NickCraver aspnet/Announcements#60

[NickCraver]

@davidfowl I don't see the not-there case really well thought out there. Compare old vs. new code.
Old:

if (Request.Headers["MyHeader"] != null)

New:

if (!StringValues.IsNullOrEmpty(Request.Headers["MyHeader"]))

There may be an extension method I can't find as well? If it's there, it would require a using though - so many probably wouldn't find it.

I think it's pretty hard to argue that it's an intuitive change - I'm just not seeing the wins here for the missing case. IMO, null is a better return. With ?. and ??, it's an even bigger win.

[davidfowl]

We could add an extension method actually that's not a bad idea but it's a struct so null won't cut it.

[NickCraver]

True - I'd suggest the extension method be where intellisense will find it though, which would mean in already-used namespaces. IMO there's an argument to be made for pulling such basic types lower.

In this case might I suggest a public bool IsEmpty => Count == 0; (or possibly a different name) on StringValues? Eliminating the namespace navigation issues would be a good win here.

[benaadams]

Use TryGet? Or are you just testing it exists and don't care about its value?

If you don't care about its value ContainsKey?

[benaadams]

@NickCraver surely

Old:

if (Request.Headers["MyHeader"] != null)

New:

if (Request.Headers.ContainsKey("MyHeader"))

Or:

if (Request.Headers.TryGet("MyHeader", out header))

[benaadams]

@kagamine rather than:

if (Cookies["xxx"] == null)

Try

if (Cookies.ContainsKey("xxx"))

Or if you want the value?

if (Cookies.TryGet("xxx", out cookie))

[yukozh]

But in my computer, there was no ContainsKey with cookies.
I'm using the latest rc2.

[NickCraver]

@benaadams We often check for existence then default in such scenarios, I suppose the new format would be:

StringValues s;
var guid = Request.Headers.TryGetValue("Proxy-Guid", out s) ? s[0] : UnauthorizedGuid;

Where as before we're talking about:

var guid = Request.Headers["Proxy-Guid"] ?? UnauthorizedGuid;

I see the wins on the allocation side, but it hurts on the usability is all I'm noting - you convinced me it's an overall good change here. IMO, it's an overall win since we're after the maximum performance ultimately as well.

Semi-related: It would be really nice if the headers were ordered, since while the RFC (2616 - 4.2) says the overall key ordering doesn't matter (though individual values do) - it does matter practically. For example, we can detect a botnet faking a browser based on header order - since we know Chrome sends headers in a certain order. However, I am a practical person - I see how this wouldn't matter for 99.9999% of websites :) I'm just noting such things here that probably aren't considered in the discussions but are hit in production...or I'm totally wrong and things like this are brought up in the meetings.

For cookies though (this PR), I think a null return is still more sane - we're getting no wins with the string.Empty return, only having to call .IsNullOrEmpty() or != "" with no gains really.

[yukozh]

In convention we used to Request.Headers["Proxy-Guid"] == null, not Request.Headers.ContainsKey.

I think make the code simple is better. The new method is so complex.

I still confused about today's change, why make it return String.Empty?

[yukozh]

+1 missing should be null

[benaadams]

/cc @Tratcher

[Tratcher]

The cookies and headers issues are quite different due to the different data types being used, please open a separate bug to discuss handling missing headers.

[Tratcher]

@kagamine Yes this needs tests.

[Tratcher]

@kagamine nevermind, I'm taking care of it as part of #463