Unexpected breaking change in .NET 7

[slovely]

Description

The StringContent constructor:

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

has changed behaviour from .NET6 to .NET7. We had a particular code-path that ended up passing null into this constructor for the mediaType, which worked as expected in .NET6 and set the mime type to "text/plain". In .NET7 the same call throws ArgumentException.

The fix for this would be changing StringContent (lines 47-48) from:

public StringContent(string content, Encoding? encoding, string mediaType)
            : this(content, encoding, new MediaTypeHeaderValue(mediaType, (encoding ?? DefaultStringEncoding).WebName))

to:

public StringContent(string content, Encoding? encoding, string mediaType)
            : this(content, encoding, new MediaTypeHeaderValue(mediaType ?? DefaultMediaType, (encoding ?? DefaultStringEncoding).WebName))

(https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http/src/System/Net/Http/StringContent.cs#L47)

Reproduction Steps

Create a unit test project, targeting both .NET6 and 7:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <TargetFrameworks>net6.0;net7.0</TargetFrameworks>
        <Nullable>enable</Nullable>

        <IsPackable>false</IsPackable>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.11.0"/>
        <PackageReference Include="NUnit" Version="3.13.2"/>
        <PackageReference Include="NUnit3TestAdapter" Version="4.0.0"/>
    </ItemGroup>

</Project>

And add the following test:

public class Tests
{
    [Test]
    public void Test1()
    {
        string mimeType = null;
        var sc = new StringContent("Hello world!", Encoding.Default, mimeType);
        
        Assert.AreEqual(sc.Headers.ContentType.MediaType, "text/plain");
    }
}

From the CLI run dotnet test. The unit test passes for .NET6, but throws an error in .NET7:

Expected behavior

The behaviour on .NET6 and 7

Actual behavior

The .NET7 version throws an exception:

A total of 1 test files matched the specified pattern.
  Failed Test1 [28 ms]
  Error Message:
   System.ArgumentException : The value cannot be null or empty. (Parameter 'mediaType')
  Stack Trace:
     at System.Net.Http.Headers.MediaTypeHeaderValue.CheckMediaTypeFormat(String mediaType, String parameterName)
   at System.Net.Http.StringContent..ctor(String content, Encoding encoding, String mediaType)
   at StringContentChange.Tests.Test1() in C:\projects\git\StringContentChange\StringContentChange\UnitTest1.cs:line 13
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

Regression?

Yes, works in .NET6

Known Workarounds

Call the constructor that only takes string content, Encoding encoding parameters.

Configuration

.NET SDK:
 Version:   7.0.102
 Commit:    4bbdd14480

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19045
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\7.0.102\

Other information

No response

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

The StringContent constructor:

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

has changed behaviour from .NET6 to .NET7. We had a particular code-path that ended up passing null into this constructor for the mediaType, which worked as expected in .NET6 and set the mime type to "text/plain". In .NET7 the same call throws ArgumentException.

The fix for this would be changing StringContent (lines 47-48) from:

public StringContent(string content, Encoding? encoding, string mediaType)
            : this(content, encoding, new MediaTypeHeaderValue(mediaType, (encoding ?? DefaultStringEncoding).WebName))

to:

public StringContent(string content, Encoding? encoding, string mediaType)
            : this(content, encoding, new MediaTypeHeaderValue(mediaType ?? DefaultMediaType, (encoding ?? DefaultStringEncoding).WebName))

(https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http/src/System/Net/Http/StringContent.cs#L47)

Reproduction Steps

Create a unit test project, targeting both .NET6 and 7:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <TargetFrameworks>net6.0;net7.0</TargetFrameworks>
        <Nullable>enable</Nullable>

        <IsPackable>false</IsPackable>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.11.0"/>
        <PackageReference Include="NUnit" Version="3.13.2"/>
        <PackageReference Include="NUnit3TestAdapter" Version="4.0.0"/>
    </ItemGroup>

</Project>

And add the following test:

public class Tests
{
    [Test]
    public void Test1()
    {
        string mimeType = null;
        var sc = new StringContent("Hello world!", Encoding.Default, mimeType);
        
        Assert.AreEqual(sc.Headers.ContentType.MediaType, "text/plain");
    }
}

From the CLI run dotnet test. The unit test passes for .NET6, but throws an error in .NET7:

Expected behavior

The behaviour on .NET6 and 7

Actual behavior

The .NET7 version throws an exception:

A total of 1 test files matched the specified pattern.
  Failed Test1 [28 ms]
  Error Message:
   System.ArgumentException : The value cannot be null or empty. (Parameter 'mediaType')
  Stack Trace:
     at System.Net.Http.Headers.MediaTypeHeaderValue.CheckMediaTypeFormat(String mediaType, String parameterName)
   at System.Net.Http.StringContent..ctor(String content, Encoding encoding, String mediaType)
   at StringContentChange.Tests.Test1() in C:\projects\git\StringContentChange\StringContentChange\UnitTest1.cs:line 13
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

Regression?

Yes, works in .NET6

Known Workarounds

Call the constructor that only takes string content, Encoding encoding parameters.

Configuration

.NET SDK:
 Version:   7.0.102
 Commit:    4bbdd14480

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19045
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\7.0.102\

Other information

No response

Author:	slovely
Assignees:	-
Labels:	area-System.Net.Http, untriaged
Milestone:	-

[slovely]

Sorry, forgot to link to the .NET 6 version of the code:

https://github.com/dotnet/runtime/blob/v6.0.13/src/libraries/System.Net.Http/src/System/Net/Http/StringContent.cs#L30

MediaTypeHeaderValue headerValue = new MediaTypeHeaderValue((mediaType == null) ? DefaultMediaType : mediaType);

Can see here that it used to check for null mediaType and used DefaultMediaType in that scenario.

[campersau]

Looks like it got changed in #63231

[En3Tho]

It seems like nullability of "mediaType" has changed to disallow nulls. Do you use NRT? Compiler would certainly issue a warning here.

[slovely]

Indeed, unfortunately we haven't enabled NRT in this project. It's a future goal, but is not a quick thing for an older, large, code-base. I'm not saying what we are doing is correct, but it does feel odd that the behaviour changed and also that the two overrides:

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

don't behave the same way if mediaType == null - the first one will default mediaType to "text/plain" but the other no longer defaults.

Was just an unexpected change is all - it may have been by-design, but it wasn't included in the breaking changes list as far as I could see. Partly raised the issue so other people that google it may find a quick answer.

[stephentoub]

This seems like a bug. @dotnet/ncl, anyone know why that changed?

[MihaZupan]

Feels like an oversight in #63231

runtime/src/libraries/System.Net.Http/src/System/Net/Http/StringContent.cs

Line 48 in 9e5d843

: this(content, encoding, new MediaTypeHeaderValue(mediaType, (encoding ?? DefaultStringEncoding).WebName))

should be

-: this(content, encoding, new MediaTypeHeaderValue(mediaType, (encoding ?? DefaultStringEncoding).WebName))
+: this(content, encoding, new MediaTypeHeaderValue(mediaType ?? DefaultMediaType, (encoding ?? DefaultStringEncoding).WebName))

to match previous behavior.

[MihaZupan]

That PR also changed the nullability of mediaType on

StringContent(string content, Encoding? encoding, string mediaType)

to be non-nullable. I don't see any reasoning as to why that happened - I don't see it discussed in the API suggestion issue.

[slovely]

@MihaZupan yes, exactly that fix is what I propose. Thanks 🙏

(Please say if you want a pr, though figured it was quicker for someone who already has access to do)

[MihaZupan]

Given I don't see any discussion to the contrary on the issue/PR, I'm going to assume it was not intentional.
We'd happily accept a PR to fix this.

[stephentoub]

Is there a larger issue here? We currently allow passing a null MediaTypeHeaderValue:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/StringContent.cs

Line 59 in 58719ec

Headers.ContentType = mediaType;

(even though the parameter isn't attributed as nullable). That would seem to mean you get subtly different behavior if you pass a null string mediaType vs a null MediaTypeHeaderValue mediaType?

[slovely]

I've created a PR (#81722). Unfortunately it turns out I am not smart* enough to actually get the framework libraries running locally 😵‍💫 , so although I added a unit test I don't actually know if it passes... it really should though! (I'll try it on linux at some point to see if I have any more luck on there).

I created the PR against the main branch, but wasn't sure if it should really have been against the .NET7 branch? Please let me know if so and I can update it.

@stephentoub Yes, you are correct in that if you pass null as the MediaTypeHeaderValue you currently get a null ContentType - this test will fail:

        var sc = new StringContent("", Encoding.Default, (MediaTypeHeaderValue)null);
        Assert.AreEqual("plain/text", sc.Headers.ContentType);

However, that override was not available in .NET6, so it isn't a breaking change. Agree that makes the API behaviour unexpected though.

---

*EDIT actually did managed to get the unit tests running in the end and it does pass.

[karelz]

Triage: Looks like 3 more people ran into it ... that would make a solid case for backport as it is a regression.
Waiting on confirmation in restsharp/RestSharp#1976 (comment) from @alexeyzimarev and in #81722 (comment) from @saber-wang and @kevinle-1

[saber-wang]

@karelz Yes, I have encountered this problem, but after a long time, I can't remember how I circumvented this problem in the first place.

[kevle1]

@karelz Can confirm that we are encountering this exact issue in my organization in the upgrade to .NET 7 from .NET 6.

[antonfirsov]

Fixed for 7.0.x servicing in #83425

[MelbourneDeveloper]

This breaks my library RestClient.Net

The tests pass for 3.1 , 5, 6 but fail for 7. You can see the failing test here:

https://github.com/MelbourneDeveloper/RestClient.Net/blob/4395d4a7733b09a33bb59c4b1c09fabba0f6f96f/src/RestClient.Net.UnitTests/MainUnitTests.cs#L621

[MelbourneDeveloper]

@antonfirsov @karelz is there a workaround for this at the moment?

[MihaZupan]

You can workaround the issue by passing "text/plain" instead of null to the StringContent constructor.

[MelbourneDeveloper]

@MihaZupan, thanks, but that is not working in my case. I content type is text/plain but I still get the error

[MelbourneDeveloper]

@MihaZupan, I think I figured out the issue. I'm using a HTTP mocking framework. The issue is not on the request. It's the same issue with the response. I forced the mock to return a media type and now it seems to be working. Thanks.

[Meligy]

@MelbourneDeveloper is the response also covered by the same PR, or would need a new one?

[MelbourneDeveloper]

@Meligy, I'm not sure. I haven't dug into any of this yet.

[mcintyre321]

This bug manifests itself in popular libs like Octokit octokit/octokit.net#2659

[slovely]

@mcintyre321 I believe this fix is scheduled to be in release 7.0.6 (see #83425). The latest version is currently 7.0.5.

[karelz]

Fixed in main (8.0) in PR #81722 and in 7.0.7 in PR #83425.