Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StringContent doesn't have an option to remove charset #1478

Closed
hungquach opened this issue Nov 16, 2017 · 8 comments
Closed

StringContent doesn't have an option to remove charset #1478

hungquach opened this issue Nov 16, 2017 · 8 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@hungquach
Copy link

API shape:

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

    // Existing constructors:
    public StringContent(string content);
    public StringContent(string content, Encoding encoding);
    public StringContent(string content, Encoding encoding, string mediaType);
}

Original proposal

I'm using HttpClient to send the request but it seems StringContent doesn't have an option to remove charset and I have to do it manually

REST endpoint only support {application/json} not {application/json; charset=utf-8}

apiClient.BaseAddress = new Uri("http://endpoint:8888");
apiClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

var json = JsonConvert.SerializeObject(new Dictionary<string, string>
{
	{"username", username},
	{"password", password}
});
var content = new StringContent(json, Encoding.UTF8, "application/json");
content.Headers.Remove("Content-Type"); // "{application/json; charset=utf-8}"
content.Headers.Add("Content-Type", "application/json");
var loginResponse = await apiClient.PostAsync("api/login", content);

Environment: .NET Core 2.0, .NET Standard 2.0

[EDIT] Add C# syntax highlighting by @karelz

@davidsh
Copy link
Contributor

davidsh commented Nov 22, 2017

These are the current public constructors of StringContent.

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

All of them have the behavior of adding a charset attribute to the 'Content-Type` header. We can't change the current behavior of these APIs.

But we could possibly add a new constructor overload that takes a MediaTypeHeaderValue parameter. That would enable someone to pass in a custom value that didn't include any charset attribute:

Proposal:

public StringContent(string content, MediaTypeHeaderValue mediaTypeHeaderValue);

@hungquach
Copy link
Author

hungquach commented Dec 4, 2017

I've just found another way to remove charset from content-type

var content = new StringContent(json, Encoding.UTF8, "application/json");
content.Headers.ContentType.CharSet = string.Empty;

@Priya91
Copy link
Contributor

Priya91 commented Dec 8, 2017

The second way looks good! If we expose an API, i don't think the mediatypeheadervalue allows Encoding to be set, it there also needs to be

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

It's not clear and intuitive to figure out how to get the charset to be unset, exposing APIs sounds good.

@davidsh What do you think?

@karelz
Copy link
Member

karelz commented Oct 10, 2019

Triage: We need a little bit of validation if it works and is sufficient. Then we can send it to API review.

@willowTank
Copy link

I've just found another way to remove charset from content-type

var content = new StringContent(json, Encoding.UTF8, "application/json");
content.Headers.ContentType.CharSet = string.Empty;

Thank you very much.

@karelz karelz transferred this issue from dotnet/corefx Jan 9, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jan 9, 2020
@karelz karelz added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Jan 9, 2020
@karelz karelz added this to the 5.0 milestone Jan 9, 2020
@karelz karelz added this to To do in Networking ramp-up via automation Jan 9, 2020
ckadluba added a commit to ckadluba/runtime that referenced this issue Jan 17, 2020
Added 2 ctors to StringContent. Both allow initialization with a custom
MediaTypeHeaderValue object.

This change was proposed in issue dotnet#1478 to be able to omit the
charset label from the Content-type HTTP header when it is rendered.

Fix dotnet#1478
@scalablecory
Copy link
Contributor

@ckadluba, It would help us push this through API review if you can fulfill @karelz's ask:

Triage: We need a little bit of validation if it works and is sufficient. Then we can send it to API review.

If you can post some examples of how using this API will compare with the workaround that @hungquach identified, that will help demonstrate any benefits. Next review is 1/28, so we have some time.

@ckadluba
Copy link

ckadluba commented Jan 20, 2020

Thanks for clarifying @scalablecory. I'm not sure if the following is what you need for the API review, but i'll give it a try. If there is anything unclear or something else is required, please let me know.

Here is how to create a StringValue object with default encoding UTF-8 but without charset label on Content-type header using new ctor from the suggested API change.

var content = new StringContent(json, 
   new MediaTypeHeaderValue("application/json")
   {
      CharSet = string.Empty
   });

Compared to creating the same with the existing workaround.

var content = new StringContent(json, "application/json");
content.Headers.ContentType.CharSet = string.Empty;

The first variant has the benefit that content could be treated as immutable after beeing created. It would allow changing the StringContent class and all its child properties to be immutable and still be able to create a StringContent object that renders no charset label into the Content-type header.

With the second added ctor, also custom encodings (other than default UTF-8) could be used.

var content = new StringContent(json, Encoding.UTF7 
   new MediaTypeHeaderValue("application/json"));

But this can also be done with the existing ctor with the signature StringContent(string, Encoding, string).

var content = new StringContent(json, Encoding.UTF7, "application/json");

The charset is in both cases deducted from the passed Encoding object.

Actually these scenarios are not a yet a real benefit over the workaround. Making the full object graph immutable would probably mean significant changes in all involved classes. But the added constructors would at least remove one obstacle when going into that direction, while still allowing to customize charset and other properties.

@GrabYourPitchforks
Copy link
Member

The related item https://github.com/dotnet/corefx/issues/7864 was discussed and approved in today's API review. Closing this item.

Networking ramp-up automation moved this from To do to Done Jan 29, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http help wanted [up-for-grabs] Good issue for external contributors
Projects
Development

Successfully merging a pull request may close this issue.

9 participants