[release/3.1] Add new System.Net.Http.Json project/namespace #42879

jozkee · 2020-03-17T08:10:29Z

Port of dotnet/runtime#33459
Note that the runtime PR is still open, so please add all feedback that is not related to release/3.1 there (all new commits on that PR will be ported into here as well)

Description

Serializing and deserializing JSON payloads from the network is a very common operation for clients, especially in the upcoming Blazor environment. Right now, sending a JSON payload to the server requires multiple lines of code, which will be a major speed bump for those customers. We'd like to add extension methods on top of HttpClient that allows doing those operations with a single method call.

This creates a package that targets netstandard2.0 only.
This does not ship inbox in this branch.

Customer Impact

This provides a solution that serves as a replacement of the old-fashioned Microsoft.AspNet.WebApi.Client, that is compatible with netstandard2.0 and can be used by Blazor WebAssembly apps.

Regression

None.
This is a brand new project.

Testing

Added unit tests to verify correct behavior of API surface.
Added unit tests to verify correct behavior of transcoding semantics.

Risk

This is a whole new API that has not even shipped as a preview in runtime, and it is still on active development.

TODO: Implement transcoding for non-UTF-8 content.

…the next release

joperezr · 2020-03-17T15:55:44Z

cc: @ericstj @jeffhandley

joperezr · 2020-03-17T15:56:01Z

cc: @terrajobst

ericstj · 2020-03-17T16:11:47Z

@jozkee the tests you've added are failing:
https://dev.azure.com/dnceng/public/_build/results?buildId=561879&view=ms.vss-test-web.build-test-results-tab&runId=17681388&resultId=112227&paneView=debug

ericstj · 2020-03-17T16:13:12Z

All the failures appear to be:

System.ArgumentException : The output byte buffer is too small to contain the encoded data, encoding 'Unicode (UTF-8)' fallback 'System.Text.EncoderReplacementFallback'.\r\nParameter name: bytes

joperezr · 2020-03-17T16:28:19Z

@ericstj yeah we actually knew before hand that those tests would fail, he was in the middle of working on fixing them when we started porting the branch to release/3.1, once he fixes all those in runtime repo we will port the fix back here. I'm looking into the packaging test failures, as they are related to the new package.

stephentoub · 2020-03-17T17:28:40Z

src/System.Net.Http.Json/ref/System.Net.Http.Json.cs

+        public System.Type ObjectType { get { throw null; } }
+        public object? Value { get { throw null; } }
+        public static System.Net.Http.Json.JsonContent Create(object? inputValue, System.Type inputType, System.Net.Http.Headers.MediaTypeHeaderValue mediaType, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
+        public static System.Net.Http.Json.JsonContent Create<T>(T value, System.Net.Http.Headers.MediaTypeHeaderValue mediaType, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }


Both here and above, mediaType is supposed to have = null, right?

That's not what we agreed on API review, but I do agree on the need to have an optional mediaType or an overload that does not take it.

Or maybe it is just missing in the spec.
dotnet/runtime#33566 (comment)

That's not what we agreed on API review

https://youtu.be/_AHbjIS8_0I?t=4272

stephentoub · 2020-03-17T17:30:07Z

src/System.Net.Http.Json/ref/System.Net.Http.Json.cs

+        public System.Type ObjectType { get { throw null; } }
+        public object? Value { get { throw null; } }
+        public static System.Net.Http.Json.JsonContent Create(object? inputValue, System.Type inputType, System.Net.Http.Headers.MediaTypeHeaderValue mediaType, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
+        public static System.Net.Http.Json.JsonContent Create<T>(T value, System.Net.Http.Headers.MediaTypeHeaderValue mediaType, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }


We have T value and object? inputValue... should they have the same name?

stephentoub · 2020-03-17T17:31:25Z

src/System.Net.Http.Json/src/Properties/InternalsVisibleTo.cs

+
+using System.Runtime.CompilerServices;
+
+[assembly: InternalsVisibleTo("System.Net.Http.Json.Functional.Tests, PublicKey=00240000048000009400000006020000002400005253413100040000010001004b86c4cb78549b34bab61a3b1800e23bfeb5b3ec390074041536a7e3cbd97f5f04cf0f857155a8928eaa29ebfd11cfbbad3ba70efea7bda3226c6a8d370a4cd303f714486b6ebc225985a638471e6ef571cc92a4613c00b8fa65d61ccee0cbe5f36330c9a01f4183559f1bef24cc2917c6d913e3a541333a1d05d9bed22b38cb")]


We try very hard not to use InternalsVisibleTo; we've stamped them out in most places, and I would like not to add more. . There are a variety of reasons for this but, for example, this will prevent trimming anything in the assembly. Can we please remove this?

Sure, I can workaround this. I just added it since the original code on aspnetcore is being tested this way.

stephentoub · 2020-03-17T17:34:38Z

src/System.Net.Http.Json/src/System/Net/Http/Json/HttpContentJsonExtensions.cs

+    {
+        public static Task<object?> ReadFromJsonAsync(this HttpContent content, Type type, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default)
+        {
+            return ReadFromJsonAsyncCore(content, type, options, cancellationToken);


Nit: elsewhere we've used expression-body members for one-liners, but here we're not?

stephentoub · 2020-03-17T17:40:21Z

src/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs

+
+        private JsonContent(object? value, Type inputType, MediaTypeHeaderValue mediaType, JsonSerializerOptions? options)
+        {
+            if (mediaType == null)


I thought we were allowing mediaType to be null, and actually making it an optional parameter (defaulting to null) to avoid needing additional Create overloads that didn't take it.

In that case we need to mark it as such (MediaTypeHeaderValue?).

Yes, that was the decision in API review as far as I remember: MediaTypeHeaderValue? mediaType = null.

joperezr · 2020-03-17T17:43:08Z

@ericstj can you please take a look to aab639f to make sure that the suppression added and the package index is what you expect?

…xt.Json in netcoreapp3.1

src/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs

joperezr · 2020-03-17T17:49:39Z

Adding No Merge label to wait till the runtime PR has been approved and checked in before we merge this one.

jozkee · 2020-03-17T17:50:04Z

src/System.Net.Http.Json/ref/System.Net.Http.Json.cs

+{
+    public static partial class HttpClientJsonExtensions
+    {
+        public static System.Threading.Tasks.Task<object?> GetFromJsonAsync(this System.Net.Http.HttpClient client, string? requestUri, System.Type type, System.Text.Json.JsonSerializerOptions? options = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }


@stephentoub Should we call it Type returnType instead? here and for HttpContentJsonExtensions.ReadFromJsonAsync since that's the name of the param in JsonSerializer.Deserialize.

https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.deserialize?view=netcore-3.1#System_Text_Json_JsonSerializer_Deserialize_System_String_System_Type_System_Text_Json_JsonSerializerOptions_

ericstj · 2020-03-17T17:54:59Z

Why is code-review on content and test fixing happening in release? Is this specific to release code? If not, polish this and finish in master first. Release PR should be cherry-pick and whatever minor changes need to be done to port. I wouldn't expect tons of feedback here, it is not efficient to do that in two places.

stephentoub · 2020-03-17T17:55:02Z

src/System.Net.Http.Json/src/System/Net/Http/Json/TranscodingReadStream.cs

+                ArrayPool<byte>.Shared.Return(_byteBuffer.Array);
+
+                Debug.Assert(_overflowBuffer.Array != null);
+                ArrayPool<byte>.Shared.Return(_overflowBuffer.Array);


This should null out the various buffer fields. Otherwise, misuse of this stream could end up accessing arrays that have already been returned to the pool.

stephentoub · 2020-03-17T17:56:44Z

Why is code-review on content and test fixing happening in release?

Simply because this is the PR I heard you refer to in the meeting just now / you sent out a link to. I didn't receive any notifications that the other PR in dotnet/runtime had been updated, and saw this as being newer than the last time I looked at dotnet/runtime.

jozkee · 2020-03-17T17:58:53Z

@stephentoub for the record, the PR in dotnet/runtime already contains the set of changes that you are reviewing here.

stephentoub · 2020-03-17T18:00:14Z

Ok. I didn't receive a notification that it had been updated. I only received a link from Eric to this PR. Hence my commenting here.

I'm halfway through. How would you like me to proceed?

jozkee · 2020-03-17T18:02:33Z

Just to keep PR comments centralized, you can continue on dotnet/runtime#33459

I will address the feedback on both PRs.

… as stable on the official build

ericstj · 2020-03-17T22:00:21Z

pkg/Microsoft.Private.CoreFx.NETCoreApp/Microsoft.Private.CoreFx.NETCoreApp.pkgproj

+    <!-- Permit inbox revsion of assemblies that are part of shared framework but also ship as packages. This will
+    make sure other packages that have a reference to them will carry a package reference to the latest serviced
+    version. -->
+    <ValidatePackageSuppression Include="PermitInbox">


Why was this required? Can you please share the error message you were getting?

The private transport package was failing due to a version inconsistency, System.Text.Json was expected to be inbox with version 4.0.1.0 but was found to have 4.0.1.1. Here is the error message I got:

C:\Users\joperezr\.nuget\packages\microsoft.dotnet.build.tasks.packaging\1.0.0-beta.20113.5\build\Packaging.targets(1161,5): error : File System.Text.Json, version 4.0.1.1 was included framework package Microsoft.Private.CoreFx.NETCoreApp/4.7.0-dev.20167.1 but that version is not considered inbox in package index F:\git\corefx\pkg\Microsoft.Private.PackageBaseline\packageIndex.json. Please add it with appropriate InboxOn entry for netcoreapp3.1 or suppress this message with PermitInbox suppression. [F:\git\corefx\pkg\Microsoft.Private.CoreFx.NETCoreApp\Microsoft.Private.CoreFx.NETCoreApp.pkgproj]

src/System.Net.Http.Json/tests/FunctionalTests/TranscodingReadStreamTests.cs

ctor.

joperezr · 2020-03-25T17:40:53Z

Closing this PR in favor of #42889 which is targeting the new branch that was created for this project.

jozkee and others added 15 commits March 11, 2020 12:45

Add System.Net.Http.Json related projects

da6d3b8

Add JsonContent, HttpContent extensions, and tests.

d1e22bf

Rename PErson.cs to Person.cs

58f2ac0

Adding package for System.Net.Http.Json library

45a84bb

Improve tests, minor fixes and code clean-up.

d3be353

Remove annotations and code clean-up.

384c0bf

Add draft description and common types to NuGet pkg.

08f9910

Addres nullability issues on API surface area.

d6d7587

Add missing configuration to test project

c19f232

Addressing most of the feedback.

14aa391

TODO: Implement transcoding for non-UTF-8 content.

Fix csproj formatting and use open StrongNameKeyId

a8c59d2

Changing prereleaselabel and ensuring the package won't go stable in …

d2f858c

…the next release

Address API review comments.

686e18e

Add transcoding support for .NET 5

283818d

Port TranscodingReadWriteStream to netstandard2.0

e97f7a2

jozkee added area-System.Net.Http Servicing-consider Issue for next servicing release review area-System.Text.Json labels Mar 17, 2020

jozkee requested a review from joperezr March 17, 2020 08:10

stephentoub reviewed Mar 17, 2020

View reviewed changes

leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 17, 2020

stephentoub reviewed Mar 17, 2020

View reviewed changes

Fix package testing issue by reverting the inbox version of System.Te…

aab639f

…xt.Json in netcoreapp3.1

joperezr force-pushed the 3.1/HttpJson branch from bca12a5 to aab639f Compare March 17, 2020 17:46

stephentoub reviewed Mar 17, 2020

View reviewed changes

src/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs Outdated Show resolved Hide resolved

joperezr added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 17, 2020

jozkee commented Mar 17, 2020

View reviewed changes

stephentoub reviewed Mar 17, 2020

View reviewed changes

Adding BlockStable attribute to make sure the package won't be marked…

8debf82

… as stable on the official build

ericstj reviewed Mar 17, 2020

View reviewed changes

jozkee added 3 commits March 19, 2020 09:44

Remove netcoreapp dependencies on TranscodingStream classes.

1d24f38

Mark TransportContext as nullable to fix CI issues.

d0d1fd5

Addres issues raised by new nullablity awareness code in System.Net.Http

988ca9f

danmoseley reviewed Mar 19, 2020

View reviewed changes

src/System.Net.Http.Json/tests/FunctionalTests/TranscodingReadStreamTests.cs Outdated Show resolved Hide resolved

jozkee added 5 commits March 20, 2020 10:24

Re-enable LoopBackServer tests, address suggestions and code clean-up.

12dc58b

Use default JsonSerializerOptions when passed-in is null

845ac13

Escape a few remaining Unicode characters in test files.

d6745fe

Address nits.

653f26d

Validate inputType.IsAssignableFrom(inputValue.GetType()) on JsonContent

f9112c4

ctor.

joperezr mentioned this pull request Mar 25, 2020

Add new System.Net.Http.Json project/namespace #42889

Merged

joperezr closed this Mar 25, 2020

jozkee deleted the 3.1/HttpJson branch March 26, 2020 03:12

jozkee mentioned this pull request Apr 20, 2020

Help get System.Text.Json test coverage to 100% (or close to it) dotnet/runtime#32341

Open

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release/3.1] Add new System.Net.Http.Json project/namespace #42879

[release/3.1] Add new System.Net.Http.Json project/namespace #42879

jozkee commented Mar 17, 2020 •

edited by joperezr

Loading

joperezr commented Mar 17, 2020

joperezr commented Mar 17, 2020

ericstj commented Mar 17, 2020

ericstj commented Mar 17, 2020

joperezr commented Mar 17, 2020

stephentoub Mar 17, 2020

jozkee Mar 17, 2020

jozkee Mar 17, 2020

stephentoub Mar 17, 2020

stephentoub Mar 17, 2020

jozkee Mar 17, 2020

stephentoub Mar 17, 2020

jozkee Mar 17, 2020

stephentoub Mar 17, 2020

stephentoub Mar 17, 2020

jozkee Mar 17, 2020

stephentoub Mar 17, 2020

joperezr commented Mar 17, 2020 •

edited

Loading

joperezr commented Mar 17, 2020

jozkee Mar 17, 2020

ericstj commented Mar 17, 2020

stephentoub Mar 17, 2020

stephentoub commented Mar 17, 2020 •

edited

Loading

jozkee commented Mar 17, 2020

stephentoub commented Mar 17, 2020 •

edited

Loading

jozkee commented Mar 17, 2020 •

edited

Loading

ericstj Mar 17, 2020

joperezr Mar 17, 2020

joperezr commented Mar 25, 2020


		using System.Runtime.CompilerServices;

		[assembly: InternalsVisibleTo("System.Net.Http.Json.Functional.Tests, PublicKey=00240000048000009400000006020000002400005253413100040000010001004b86c4cb78549b34bab61a3b1800e23bfeb5b3ec390074041536a7e3cbd97f5f04cf0f857155a8928eaa29ebfd11cfbbad3ba70efea7bda3226c6a8d370a4cd303f714486b6ebc225985a638471e6ef571cc92a4613c00b8fa65d61ccee0cbe5f36330c9a01f4183559f1bef24cc2917c6d913e3a541333a1d05d9bed22b38cb")]

[release/3.1] Add new System.Net.Http.Json project/namespace #42879

[release/3.1] Add new System.Net.Http.Json project/namespace #42879

Conversation

jozkee commented Mar 17, 2020 • edited by joperezr Loading

Description

Customer Impact

Regression

Testing

Risk

joperezr commented Mar 17, 2020

joperezr commented Mar 17, 2020

ericstj commented Mar 17, 2020

ericstj commented Mar 17, 2020

joperezr commented Mar 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joperezr commented Mar 17, 2020 • edited Loading

joperezr commented Mar 17, 2020

Choose a reason for hiding this comment

ericstj commented Mar 17, 2020

Choose a reason for hiding this comment

stephentoub commented Mar 17, 2020 • edited Loading

jozkee commented Mar 17, 2020

stephentoub commented Mar 17, 2020 • edited Loading

jozkee commented Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joperezr commented Mar 25, 2020

jozkee commented Mar 17, 2020 •

edited by joperezr

Loading

joperezr commented Mar 17, 2020 •

edited

Loading

stephentoub commented Mar 17, 2020 •

edited

Loading

stephentoub commented Mar 17, 2020 •

edited

Loading

jozkee commented Mar 17, 2020 •

edited

Loading