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.

Sign up for GitHub

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

First try at implementing newly suggested ctor for providing tighter control over charset #63231

Merged
merged 3 commits into from
Mar 9, 2022
Merged

First try at implementing newly suggested ctor for providing tighter control over charset #63231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7972e1c
First try at implementing newly suggested ctor for providing tighter …
Dec 31, 2021
b7bd942
Update reference assembly so mediaType is accurate
Jan 2, 2022
15f9c98
Update per github code review
Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/libraries/System.Net.Http/ref/System.Net.Http.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,8 +437,10 @@ protected override void SerializeToStream(System.IO.Stream stream, System.Net.Tr
public partial class StringContent : System.Net.Http.ByteArrayContent
{
public StringContent(string content) : base (default(byte[])) { }
public StringContent(string content, System.Net.Http.Headers.MediaTypeHeaderValue mediaType) : base (default(byte[])) { }
Copy link
Member

@AraHaan AraHaan Dec 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that for reference code that base() should never be used. Of course this was what I thought when looking and changing the ones in System.IO.Compression.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AraHaan I'm actually clueless on that point. What should be used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I see that it already uses base() so that file should be fine to do it in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a markdown doc in this repo explaining how to update the ref files automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danmoseley Thank you for letting me know. After I follow the instructions in updating-ref-source.md I'm getting a large number of changes. Many of those changes I did not do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ideal, I think, is that the ref files are completely and only generated output. cc @safern for whether that's correct.

Meantime I suggest to just include relevant changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did a commit that hopefully updated the reference assembly correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danmoseley Was my latest commit sufficient?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brianmed looks right as far as I can tell, thanks. I guess @scalablecory plans to review now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danmoseley Just updated. Sorry it took a few days.

public StringContent(string content, System.Text.Encoding? encoding) : base (default(byte[])) { }
public StringContent(string content, System.Text.Encoding? encoding, string? mediaType) : base (default(byte[])) { }
public StringContent(string content, System.Text.Encoding? encoding, System.Net.Http.Headers.MediaTypeHeaderValue mediaType) : base (default(byte[])) { }
public StringContent(string content, System.Text.Encoding? encoding, string mediaType) : base (default(byte[])) { }
protected override System.Threading.Tasks.Task SerializeToStreamAsync(System.IO.Stream stream, System.Net.TransportContext? context, System.Threading.CancellationToken cancellationToken) { throw null; }
}
}
Expand Down Expand Up @@ -692,6 +694,7 @@ public partial class MediaTypeHeaderValue : System.ICloneable
{
protected MediaTypeHeaderValue(System.Net.Http.Headers.MediaTypeHeaderValue source) { }
public MediaTypeHeaderValue(string mediaType) { }
public MediaTypeHeaderValue(string mediaType, string? charSet) { }
public string? CharSet { get { throw null; } set { } }
[System.Diagnostics.CodeAnalysis.DisallowNullAttribute]
public string? MediaType { get { throw null; } set { } }
Expand Down
10 changes: 10 additions & 0 deletions src/libraries/System.Net.Http/src/System/Net/Http/Headers/MediaTypeHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,19 @@ protected MediaTypeHeaderValue(MediaTypeHeaderValue source)
}

public MediaTypeHeaderValue(string mediaType)
: this(mediaType, charSet: null)
{
}

public MediaTypeHeaderValue(string mediaType, string? charSet)
{
CheckMediaTypeFormat(mediaType, nameof(mediaType));
_mediaType = mediaType;

if (!string.IsNullOrEmpty(charSet))
{
CharSet = charSet;
}
}

public override string ToString()
Expand Down
22 changes: 14 additions & 8 deletions src/libraries/System.Net.Http/src/System/Net/Http/StringContent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,29 @@ public class StringContent : ByteArrayContent
private const string DefaultMediaType = "text/plain";

public StringContent(string content)
: this(content, null, null)
: this(content, DefaultStringEncoding, DefaultMediaType)
{
}

public StringContent(string content, MediaTypeHeaderValue mediaType)
: this(content, DefaultStringEncoding, mediaType)
{
}

public StringContent(string content, Encoding? encoding)
: this(content, encoding, null)
: this(content, encoding, DefaultMediaType)
{
}

public StringContent(string content, Encoding? encoding, string? mediaType)
: base(GetContentByteArray(content, encoding))
public StringContent(string content, Encoding? encoding, string mediaType)
: this(content, encoding, new MediaTypeHeaderValue(mediaType, (encoding ?? DefaultStringEncoding).WebName))
{
// 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;
public StringContent(string content, Encoding? encoding, MediaTypeHeaderValue mediaType)
: base(GetContentByteArray(content, encoding))
{
Headers.ContentType = mediaType;
}

// A StringContent is essentially a ByteArrayContent. We serialize the string into a byte-array in the
Expand Down
62 changes: 62 additions & 0 deletions src/libraries/System.Net.Http/tests/FunctionalTests/StringContentTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,67 @@ public async Task Ctor_DefineNoEncoding_DefaultEncodingUsed()
string roundTrip = new StreamReader(destination, defaultStringEncoding).ReadToEnd();
Assert.Equal(sourceString, roundTrip);
}

[Fact]
public async Task Ctor_UseCustomMediaTypeHeaderValue_SpecificEncoding()
{
// Use UTF-8 encoding to serialize a chinese string.
string sourceString = "\u4f1a\u5458\u670d\u52a1";

var mediaTypeHeaderValue = new Headers.MediaTypeHeaderValue("application/custom", Encoding.UTF8.WebName);

var content = new StringContent(sourceString, Encoding.UTF8, mediaTypeHeaderValue);

Assert.Equal("application/custom", content.Headers.ContentType.MediaType);
Assert.Equal("utf-8", content.Headers.ContentType.CharSet);

var destination = new MemoryStream(12);
await content.CopyToAsync(destination);

string destinationString = Encoding.UTF8.GetString(destination.ToArray(), 0, (int)destination.Length);

Assert.Equal(sourceString, destinationString);
}

[Fact]
public async Task Ctor_UseCustomMediaTypeHeaderValue()
{
// Use UTF-8 encoding to serialize a chinese string.
string sourceString = "\u4f1a\u5458\u670d\u52a1";

var mediaTypeHeaderValue = new Headers.MediaTypeHeaderValue("application/custom", Encoding.UTF8.WebName);

var content = new StringContent(sourceString, mediaTypeHeaderValue);

Assert.Equal("application/custom", content.Headers.ContentType.MediaType);
Assert.Equal("utf-8", content.Headers.ContentType.CharSet);

var destination = new MemoryStream(12);
await content.CopyToAsync(destination);

string destinationString = Encoding.UTF8.GetString(destination.ToArray(), 0, (int)destination.Length);

Assert.Equal(sourceString, destinationString);
}

[Fact]
public async Task Ctor_UseSpecificEncodingAndContentType()
{
// Use UTF-8 encoding to serialize a chinese string.
string sourceString = "\u4f1a\u5458\u670d\u52a1";
string contentType = "application/custom";

var content = new StringContent(sourceString, Encoding.UTF8, contentType);

Assert.Equal("application/custom", content.Headers.ContentType.MediaType);
Assert.Equal("utf-8", content.Headers.ContentType.CharSet);

var destination = new MemoryStream(12);
await content.CopyToAsync(destination);

string destinationString = Encoding.UTF8.GetString(destination.ToArray(), 0, (int)destination.Length);

Assert.Equal(sourceString, destinationString);
}
}
}