Add StringContent ctor providing tighter control over charset

[guardrex]

API Approval - iteration 2

This is being submitted again for approval because we decided additional APIs were needed.
Copied from proposal in #17036 (comment)

public class StringContent
{
   public StringContent(string content);
+  public StringContent(string content, MediaTypeHeaderValue mediaType); // Already approved in iteration 1
   public StringContent(string content, Encoding encoding);
   public StringContent(string content, Encoding encoding);
   public StringContent(string content, Encoding encoding, string mediaType);
+  public StringContent(string content, Encoding encoding, MediaTypeHeaderValue mediaType); // NEW API proposal
}

public class MediaTypeHeaderValue
{
    public MediaTypeHeaderValue(string mediaType)
+   public MediaTypeHeaderValue(string mediaType, string charSet);  // NEW API proposal
}

Previous API approval is here: #17036 (comment)

Original post

Consider the following code ...

string contentType = "application/atom+xml";
string requestPayload = "some data";
var stringContent = new StringContent(requestPayload, Encoding.UTF8, contentType);

According to Wireshark, I'm seeing a POST using HttpClient with this ...

Content-Type: application/atom+xml; charset=utf-8

Shouldn't that Content-Type value just be ...

Content-Type: application/atom+xml

?? Confused, because something is choking my Azure Table Storage requests with ...

string stringToSign = 
    $"{requestMethod}\n\n{contentType}\n{dateInRfc1123Format}\n{canonicalizedResource}";

... and since SharedKeyLite, which only requires the date and resource, works with my dateInRfc1123Format and canonicalizedResource, I've sort of narrowed it down to the contentType of the request (shown above) not matching what I'm putting into the signature sting, namely just application/atom+xml.

[benaadams]

Looks like it will always put something out https://github.com/dotnet/corefx/blob/d0dc5fc099946adc1035b34a8b1f6042eddb0c75/src/System.Net.Http/src/System/Net/Http/StringContent.cs#L29

[davidsh]

The StringContent constructor follows the same behavior as .NET Framework (Desktop).

You can always replace the Content-Type (or any header) with something else if the default behavior isn't what you want.

[JonHanna]

It's certainly invalid. RFC 2046 (RFC 6657 is also relevant) allows for the charset parameter as part of content-types only if it's text/plain or a text/* type that specifies it too allows.

[guardrex]

@benaadams @JonHanna Thanks for finding the references to both. Even if it were valid (e.g., text/plain or text/*), I think if you supply the value explicitly, the value should be honored when the header is created IMHO.

I don't know if this is what is choking my ATS REST service calls, but it's possible, since SharedKeyLite works with my date and resource. This is certainly a suspect for my failing SharedKey request failures.

@davidsh How? I get an exception if I try to mod the header by adding ...

client.DefaultRequestHeaders.Add("Content-Type", contentType);

... throws ...

System.InvalidOperationException: Misused header name. Make sure request headers are used 
with HttpRequestMessage, response headers with HttpResponseMessage, and content headers 
with HttpContent objects.
 at System.Net.Http.Headers.HttpHeaders.CheckHeaderName(String name)
 at teststandalone.HomeController.DoIt()

Here's the hacked code I'm playing with (i.e., not for production ... I'm just hacking around to get the requests working). How would I set the Content-Type explicitly to override the StringContent one given this code ..

using (var client = new HttpClient())
{
    DateTime requestDT = DateTime.UtcNow;
    UTF8Encoding utf8Encoding = new UTF8Encoding();
    string storageAccountName = "<STORAGE_ACCOUNT_NAME>";
    string tableName = "<TABLE_NAME>";
    //string requestMethod = "POST";
    string dateInRfc1123Format = requestDT.ToString("R", CultureInfo.InvariantCulture);
    string storageServiceVersion = "2015-04-05";
    string storageAccountKey = "<STORAGE_ACCOUNT_KEY>";
    string canonicalizedResource = $"/{storageAccountName}/{tableName}";
    string contentType = "application/atom+xml";

    // Content
    string requestPayload = GetRequestContentInsertXml(requestDT, "PKEY", "RKEY");
    var stringContent = new StringContent(requestPayload, Encoding.UTF8, contentType);

    // Will not authorize against the service:
    // string stringToSign = $"{requestMethod}\n\n{contentType}\n{dateInRfc1123Format}\n{canonicalizedResource}";
    // SharedKeyLite works:
    string stringToSign = $"{dateInRfc1123Format}\n{canonicalizedResource}";
    string authorizationHeader = CreateAuthorizationParameter(stringToSign, storageAccountName, storageAccountKey);
    AuthenticationHeaderValue myAuthHeaderValue = new AuthenticationHeaderValue("SharedKeyLite", authorizationHeader);

    // Headers
    client.DefaultRequestHeaders.Authorization = myAuthHeaderValue;
    client.DefaultRequestHeaders.Add("x-ms-date", dateInRfc1123Format);
    client.DefaultRequestHeaders.Add("x-ms-version", storageServiceVersion);
    client.DefaultRequestHeaders.Add("DataServiceVersion", "3.0;NetFx");
    client.DefaultRequestHeaders.Add("MaxDataServiceVersion", "3.0;NetFx");
    // Doesn't work: client.DefaultRequestHeaders.Add("Content-Type", contentType);

    var responseMessage = client.PostAsync($"https://{storageAccountName}.table.core.windows.net/{tableName}", stringContent).Result;

    responseResult.Append("StatusCode: " + responseMessage.StatusCode.ToString() + " ");

}

[davidsh]

When modifying "known" headers, you need to use the properties and not the 'DefaultRequestHeaders' method. For request entity-body headers (content), you need to modify the `HttpRequestMessage.Content.Headers1 collection. You are getting an exception telling you that basically.

So, you need code like this:

var content = new StringContent(...);
content.Headers.ContentType = ...;

You can also use methods like this as well:

var content = new StringContent(...);
content.Headers.Remove(...);
content.Headers.Add(...);

[guardrex]

@davidsh Ah ... ok ... thx ... that got the header squared away ...

stringContent.Headers.ContentType = new MediaTypeHeaderValue(contentType);

And 🎉 YES! 🎉 ... that cleared up the problem with ATS REST API: SharedKey works now! 😄 👍 🍻

My issue is solved, but do you want to leave this open given what @JonHanna reported?

[davidsh]

My issue is solved, but do you want to leave this open given what @JonHanna reported?

We will research this to determine if there are RFC issues. This code has existed for 5+ years and was previously reviewed by our RFC compliance experts. I'll leave this open for now.

[JonHanna]

I was incorrect.

While charset was originally defined only for use with text/* types, there's nothing to stop some particular type in another group using a copy of it, and RFC 3023 did exactly that with it strongly recommended to be used along with application/xml and suggested, and then RFC 4287 references that in turn in allowing charset with application/atom+xml. So it's not in the slightest invalid to have that parameter with that content type, sorry for my misleading comment.

[guardrex]

My concern about it remains: If the dev sets a value explicitly on new StringContent(), they will not be aware that charset is being added.

In any case where the header is part of a service-generated hash that will be checked against a supplied signature (without the charset, since the dev didn't know it was added to the request header), as is the case with Azure Storage REST Service, ... 💥 ... and lost time having to check a lot of little things partially because the Azure REST docs and exception messages are so poor. All the service comes back with is 'your auth header is hosed! ... check the signature.'

If the dev sets an explicit value, seems like it should be honored. At least mod the description to note that charset will be added automatically. It's possible that if I saw that pop-up in VS when I was laying down the code, it might have triggered more immediate interest in trying MediaTypeHeaderValue().

[guardrex]

Just for completeness, I checked the Azure Service REST Service using the default ...

StringContent("<data>", Encoding.UTF8, "application/atom+xml");

... but supplying the content type to the signature string as ...

string contentType = "application/atom+xml; charset=utf-8";
string stringToSign = 
    $"{requestMethod}\n\n{contentType}\n{dateInRfc1123Format}\n{canonicalizedResource}";

That works. It's still awfully cryptic tho without a little help.

[darrelmiller]

@JonHanna Usually it is the media type registration document that defines what parameters are allowed. In the application/atom+xml the registration refers explicitly back RFC3023 for the rules, as you stated.

Interestingly, the .net stack loves adding charset onto application/json but in that case charset is not defined by the media type registration. In fact RFC 7158 goes on to say,

Note: No "charset" parameter is defined for this registration.
Adding one really has no effect on compliant recipients.

It would be awesome if this could be fixed.

[davidsh]

We plan to fix this in CoreFx. We need to study if porting this to .NET Framework (Desktop) will have any app-compat issues.

[davidsh]

This will require a new overload to the StringContent constructor so that the charset isn't specified by default on the Content-Type request header. So, we'll need an API proposal for that.

[tknuts]

Hi
I have a simular problem with json post to Azure Table Storage, here is my code:

            var content = "{\"PartitionKey\":\"MemberSyncDto_4-2018\",\"RowKey\":\"1218\",\"SType\":\"3\"}"; // ,\n
            var updateContent = new StringContent(content, Encoding.UTF8, "application/json");
            updateContent.Headers.ContentType = new MediaTypeHeaderValue("application/json");

            using (var client = new HttpClient())
            {
                // Add the request headers for x-ms-date and x-ms-version.
                DateTime now = DateTime.UtcNow;
                var dateHeader = now.ToString("R", CultureInfo.InvariantCulture);

                client.DefaultRequestHeaders.Authorization = AzureStorageAuthenticationHelper.GetAuthorization(
                    storageAccountName, storageAccountKey, now, new Uri(uri));
                client.DefaultRequestHeaders.Add("x-ms-date", dateHeader);
                client.DefaultRequestHeaders.Add("x-ms-version", "2018-03-28");
                client.DefaultRequestHeaders.Add("DataServiceVersion", "3.0;NetFx");
                client.DefaultRequestHeaders.Add("MaxDataServiceVersion", "3.0;NetFx");

                Console.WriteLine("x-ms-date: {0}", dateHeader);
                Console.WriteLine("Content: {0}", content);
                Console.WriteLine("Auth: {0}", client.DefaultRequestHeaders.Authorization);
                Console.WriteLine("ContentLength: {0}", content.Length);

                var responseMessage = await client.PostAsync(uri, updateContent, cancellationToken);
                Console.WriteLine("Result: {0}", responseMessage.StatusCode);
            }
        }

Quite simpel..... If I do this in Postman it works, but i keep getting 415 error "Unsupported Media Type" :-(

Why?

Desperate now, tried everything..... Can do GET requests, but not POST. I can get the same error in Postman by removing the Content-Type header...

[tknuts]

Here is my request in Fiddler:

  POST http://unionportaldkf.table.core.windows.net/MemberSyncDto HTTP/1.1
  Authorization: SharedKeyLite unionportaldkf:cgV4/in9X79BxjbGt6yjZg+lmY7hujf/D684nz4svaU=
  x-ms-date: Thu, 23 Aug 2018 08:09:55 GMT
  x-ms-version: 2018-03-28
  DataServiceVersion: 3.0;NetFx
  MaxDataServiceVersion: 3.0;NetFx
  Content-Type: application/json
  Host: unionportaldkf.table.core.windows.net
  Content-Length: 71
  Expect: 100-continue
  Connection: Keep-Alive

  {"PartitionKey":"MyStupidPartition","RowKey":"1","DummyValue":"Thomas"}

And here the response:

HTTP/1.1 415 Unsupported Media Type
Content-Type: application/xml;charset=utf-8
Server: Windows-Azure-Table/1.0 Microsoft-HTTPAPI/2.0
x-ms-request-id: 87901c7b-6002-0011-45b8-3a01cd000000
x-ms-version: 2018-03-28
X-Content-Type-Options: nosniff
Date: Thu, 23 Aug 2018 08:09:54 GMT
Content-Length: 305

<?xml version="1.0" encoding="utf-8"?><error xmlns="http://schemas.microsoft.com/ado/2007/08/dataservices/metadata"><code>AtomFormatNotSupported</code><message xml:lang="en-US">Atom format is not supported.
RequestId:87901c7b-6002-0011-45b8-3a01cd000000
Time:2018-08-23T08:09:55.4558046Z</message></error>

Here is the one from Postman that works, what am I missing?

POST http://unionportaldkf.table.core.windows.net/MemberSyncDto HTTP/1.1
Host: unionportaldkf.table.core.windows.net
Connection: keep-alive
Content-Length: 67
Origin: chrome-extension://aicmkgpgakddgnaphhhpliifpcfhicfo
Authorization: SharedKeyLite unionportaldkf:ovks7qNgQE9v+MiaTQP9p17UfqsmxRtnTSSMKywiEUU=
Content-Type: application/json
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36
Cache-Control: no-cache
x-ms-version: 2018-03-28
X-Postman-Interceptor-Id: 38cd36bf-fcd9-05ee-18b4-6c2214a37e41
Postman-Token: a93c6dd5-8d95-c351-e002-f6b15523194b
x-ms-date: Thu, 23 Aug 2018 08:16:24 GMT
Accept: */*
Accept-Encoding: gzip, deflate
Accept-Language: da,en-US;q=0.9,en;q=0.8,de;q=0.7,de-DE;q=0.6

{"PartitionKey":"MemberSyncDto_4-2018","RowKey":"1221","SType":"3"}

[tknuts]

client.DefaultRequestHeaders.Add("Accept", "*/*");

Did the trick :-)

[stephentoub]

This will require a new overload to the StringContent constructor so that the charset isn't specified by default on the Content-Type request header. So, we'll need an API proposal for that.

@davidsh, can you elaborate on why this would require a new API?

[davidsh]

This is the current API:

public partial class StringContent : System.Net.Http.ByteArrayContent
{
    public StringContent(string content) : base (default(byte[])) { }
    public StringContent(string content, System.Text.Encoding encoding) : base (default(byte[])) { }
    public StringContent(string content, System.Text.Encoding encoding, string mediaType) : base (default(byte[])) { }
}

None of the overloads take any 'charset' parameter. But they all create one anyways. See headerValue.CharSet.

public StringContent(string content, Encoding encoding, string mediaType)
    : base(GetContentByteArray(content, encoding))
{
    // Initialize the 'Content-Type' header with information provided by parameters. 
    MediaTypeHeaderValue headerValue = new MediaTypeHeaderValue((mediaType == null) ? DefaultMediaType : mediaType);
    headerValue.CharSet = (encoding == null) ? HttpContent.DefaultStringEncoding.WebName : encoding.WebName;

    Headers.ContentType = headerValue;
}

So, we would need a new overload that could explicitly take a charset parameter (or null) etc.

[karelz]

Triage: We need to finish the API proposal.
cc @scalablecory

[darrelmiller]

@karelz What if a new overload used a MediaTypeHeaderValue as the parameter? That would seem like the simplest solution. Am I missing something?

[stephentoub]

What if a new overload used a MediaTypeHeaderValue as the parameter?

That sounds good to me:

public StringContent(string content, MediaTypeHeaderValue mediaType)

In addition to being simple, it has the added benefit that it lets you avoid a MediaTypeHeaderValue allocation per StringContent, if you want to just use the same instance over and over.

[terrajobst]

Video

We believe the correct API is this:

public StringContent(string content, MediaTypeHeaderValue mediaType)

[DaveSenn]

I would like to create a PR for this.

After the API review stephentoub mentioned the problem with the "missing" encoding.
Is this still the desired API:

public StringContent(string content, MediaTypeHeaderValue mediaType)

Or should it be (as proposed by ericsampson ):

public StringContent(string content, System.Text.Encoding encoding, string mediaType, string charset)

..To avoid parsing the CharSet of the provided MediaTypeHeaderValue and still be able to reuse the same MediaTypeHeaderValue instance, this could be an option:

public StringContent(string content, Encoding encoding, MediaTypeHeaderValue mediaType)

[scalablecory]

I'm fine with both of the APIs proposed in #17036 (comment), but this needs to go back to API review before we can accept a PR for it.

I would also look at:

class MediaTypeHeaderValue
{
   public MediaTypeHeaderValue(string mediaType, string charSet);
}

[karelz]

Triage: We would like to know details about the user scenarios to make sure we understand which APIs we should add and why.
It was not clear if the current APIs are unusable, or if there is performance concern.

@DaveSenn the above will block PR for now, sorry ... let's first clarify what we want to add properly. Thanks for understanding.

[DaveSenn]

@karelz sure np, thanks for the info.

[benaadams]

We would like to know details about the user scenarios to make sure we understand which APIs we should add and why.

Want to be abe to add a Content-Type without a charset being added; which the current api doesn't make easy.

It was not clear if the current APIs are unusable,

Currenty unusable for this purpose

or if there is performance concern.

Performance concern with the proposed api that was previously accepted as it introduces requirement for secondary parsing #17036 (comment)

[geoffkizer]

I think the current proposal is the following:

public class StringContent
{
   public StringContent(string content);
+  public StringContent(string content, MediaTypeHeaderValue mediaType);
   public StringContent(string content, Encoding encoding);
   public StringContent(string content, Encoding encoding);
   public StringContent(string content, Encoding encoding, string mediaType);
+  public StringContent(string content, Encoding encoding, MediaTypeHeaderValue mediaType);
}

Is that correct? Any further suggestions/concerns?

@scalablecory also suggested the following, which seems like something we should do as well (or at least get approved to do in the future):

public class MediaTypeHeaderValue
{
    public MediaTypeHeaderValue(string mediaType)
+   public MediaTypeHeaderValue(string mediaType, string charSet);
}

[ericsampson]

Sounds good, it would definitely be nice to get the new constructor at the same time if possible :)

[geoffkizer]

I marked this as 6.0 so it will get reviewed in the near future. That doesn't mean this is committed for 6.0.

[bartonjs]

Video

Looks good as proposed. We even discussed the casing on "charSet", and decided it's correct since it matches the existing property.

namespace System.Net.Http.Headers
{
    public class StringContent
    {
       public StringContent(string content);
+      public StringContent(string content, MediaTypeHeaderValue mediaType); // Already approved in iteration 1
       public StringContent(string content, Encoding encoding);
       public StringContent(string content, Encoding encoding);
       public StringContent(string content, Encoding encoding, string mediaType);
+      public StringContent(string content, Encoding encoding, MediaTypeHeaderValue mediaType); // NEW API proposal
    }
    
    public class MediaTypeHeaderValue
    {
        public MediaTypeHeaderValue(string mediaType)
+       public MediaTypeHeaderValue(string mediaType, string charSet);  // NEW API proposal
    }
}

[danmoseley]

Is there anyone in this issue interested in offering a PR?

[brianmed]

Is there anyone in this issue interested in offering a PR?

I am currently coding up an attempt at creating the new APIs.

In addition, I will try a PR after done.

[stephentoub]

Implemented by #63231