handle bad Expires better

[wfurt]

fixes #31918

This is attempt to improve handling of Expire header.
Based on the discussion, -1 and 0 will now return DateTimeOffset.MinValue as representation of "same time in back" instead of null. I also added test to verify that if header is missing we still return null;

Second part of this change will treat al derived classes as Custom. That seems to be fine as we always set types when used internally. I added test case for scenario described by @stephentoub

[davidsh]

The System.Net.Http.Unit.Tests (not FunctionalTests) are failing. They probably need to be updated due to this change:
https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F36908~2Fmerge/test~2Ffunctional~2Fcli~2F/20190415.49

[davidsh]

I think we need more tests.

For example, with this change, this test should pass now?

using System.Net.Http;
using System.Net.Http.Headers;

var content = new StringContent("Hello");
content.Headers.Add("Expires", "-1"); // This shouldn't throw an exception anymore?

Console.WriteLine(content.Headers.Expires.ToString()); // should print "-1" or "DateTime.MinValue" ?

Also,

consider this:

content.Headers.Add("Expires", "-1"); // This shouldn't throw an exception anymore or maybe it does?

// vs

content.Headers.TryAddWithoutValidation("Expires", "-1"); // This shouldn't ever throw an exception.

[davidsh]

And more interesting thoughts to consider:

using System.Net.Http;
using System.Net.Http.Headers;

var content = new StringContent("Hello");
// content.Headers.Expires should be null value.

content.Headers.TryAddWithoutValidation("Expires", "-1");
// content.Headers.Expires should be non-null value and equal to DateTime.MinValue.

[wfurt]

I updated tests for the values and all other tests are passing now.

[davidsh]

I updated tests for the values and all other tests are passing now.

Can you also answer my comments/questions above? There are several questions that need clarity about how this PR should address the parsing of the 'Expires' header.

[wfurt]

/azp run corefx-outerloop-linux

[wfurt]

/azp run corefx-outerloop-osx

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[wfurt]

/azp run corefx-outerloop-windows

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[wfurt]

I can update names. I'm re-visiting this again as content.Headers.Add("Expires", "-1") does not work.
(AddWithoutValidation does)
I think what we have is improvement but I chicken out and the derived class wi still validate - I just improved handling of particular header.
If you think this is ok, let me know.
Right now I'm still trying to figure out if I can make it work all without validation. I guess The risk is bigger in that case but it seems like that is what we converged to in linked bug.

[davidsh]

I can update names. I'm re-visiting this again as content.Headers.Add("Expires", "-1") does not work.
(AddWithoutValidation does)
I think what we have is improvement but I chicken out and the derived class wi still validate - I just improved handling of particular header.

Does the main use case (receiving 'Expires: -1' from a server) work? I assume that SocketsHttpHandler adds headers using .TryAddWithoutValidation() when adding headers received on the wire. That is the only important use case in my mind. I think it is a bonus, also, if after doing .TryAddWithoutValidation("Expires", "-1") we also get "headers.Expires == DateTimeOffset.MinValue instead of null. It is probably fine (I think) to let .Add("Expires", "-1") fail. I just wanted to make sure we are aware of that and have a test to verify that behavior.

[wfurt]

yes, receiving -1 (or other invalid values) does work and it gets turned to MinValue. I'm somewhat worried about unexpected consequences so it would be nice if we don't need to mess up with the header types. I'll update tests and I'll post new update.

[wfurt]

added Assert.Equal() to tests and updated test name