Move NetCore project to use .NET SDK #380

dougbu · 2023-01-22T02:08:01Z

Move NetCore project to use .NET SDK

Formatting.NetCore now targets netstandard1.3
- was Profile259 aka portable-net45+wp80+win8+wpa81, approximately netstandard1.0
- but Newtonsoft.Json.Bson supports netstandard1.3 and up
use Newtonsoft.Json.Bson in NetCore projects
can now multi-target NetCore.Test as well
nits:
- align src/ Formatting projects as much as possible
- align test/ Formatting projects as much as possible
- clean up the Microsoft.TestCommon project slightly
note: NetCore project names make no sense but I'm leaving them alone for this PR

React to using Newtonsoft.Json.Bson everywhere

remove deprecated BsonWriter mentions entirely
remove legacy BSON writer test workaround

Remove unnecessary ConcurrentDictionary class

was not available in Profile259
leave tests to confirm correct operation w/ "real" class

Remove UriQueryUtility because WebUtility is available everywhere

leave test to confirm WebUtility continues to behave about the same
- handle WebUtility being a public class
change other tests to handle WebUtility.UrlEncode(...) encoding to uppercase
- unlike UriQueryUtility.UrlEncode(...)

Add simple classes to NetCore project

use ICloneable copy to get more classes compiling
- helps NetCore in similar way to existing MediaTypeHeaderValueExtensions
use InvalidEnumArgumentException copy to simplify code and testing
both not available in netstandard1.3 (or available packages) but easily duplicated

Remove unnecessary NetCore-specific GlobalSuppressions.cs file

Use NameValueCollection everywhere

available (but not serializable) in netstandard1.3

Add MaxDepth support in NetCore assembly

remove an unnecessary suppression about MaxDepth in BaseJsonMediaTypeFormatter

Add UseDataContractJsonSerializer support in NetCore assembly

XsdDataContractExporter remains unavailable
- update XML DCS tests to handle this (previously not in NetCore assembly)
try various workarounds
- ReadOnlyStreamWithEncodingPreamble causes tests to hit JsonEncodingStreamWrapper cannot handle UTF16 BOMs dotnet/runtime#80160
- restriction to UTF8 when writing is a significant issue

Copy TranscodingStream and related files from dotnet/runtime

my runtime clone was at 88868b7a781f4e5b9037b8721f30440207a7aa42

Adjust TranscodingStream and related code to compile and run here

add conditions to avoid references to version-specific API
remove references to internal dotnet/runtime code
- rewrite some code more generically if performance impact likely minor
use Microsoft.TestCommon instead of directly relying on Xunit in tests
use C# 9.0

Use TranscodingStream in JSON and XML formatters

remove ReadOnlyStreamWithEncodingPreamble and its tests
- was insufficient in general and doesn't work correctly in netstandard1.3
- see JsonEncodingStreamWrapper cannot handle UTF16 BOMs dotnet/runtime#80160
now support a much broader set of encodings in JSON and XML formatters
- not just UTF8, UTF16BE, and UTF16LE
- JSON formatter already supported every encoding when UseDataContractJsonSerializer was false
  - no longer have this downside when property is true
  - however, quotas remain unsupported in netstandard1.3 version of this formatter when property is true
- XML formatter requirement for an XML declaration in non-UTF8 content was general and is no longer an issue

Add MediaTypeMappings support in NetCore assembly

Add IRequiredMemberSelector support in NetCore assembly

Remove unnecessary NETFX_CORE workarounds

use DefaultContractResolver, GuidAttribute, TypeDescriptor, XmlElement, XmlNode classes
use IRequiredMemberSelector interface
use RequestMessage.CreateResponse() extension method
- all available in netstandard1.3
JsonContractResolver use is something of a late Newtonsoft.Json version reaction
- was held back by missing IRequiredMemberSelector

Work around missing features in netstandard1.3

get classes now included in NetCore assembly compiling
SerializationAttribute is not available
- therefore Exception is not serializable
- therefore DefaultContractResolver.IgnoreSerializableAttribute is not available
Stream.(Begin|End)(Read|Write) and Stream.Close() are not available
nit: clean up MultipartFormDataStreamProvider whitespace

Ensure a couple more assemblies are available

don't seem to be copied into test folder in CI builds

dougbu · 2023-01-22T02:13:18Z

This PR is pretty big and complicated. I recommend going through the commits individually and concentrating on areas you're at least slightly familiar with.

@phenning, I included you because I'm removing some comments you added. Note we already have a netstandard2.0 assembly w/ none of the restrictions of the former Profile259 assembly.

@stephentoub and @GrabYourPitchforks, you're likely most interested in the first two of the three commits mentioning TranscodingStream, especially the middle one.

dougbu · 2023-01-22T02:18:34Z

dougbu · 2023-01-24T22:14:43Z

/ping reviewers

wtgodbe

Approving for the subset of commits I checked off, largely based off of the changes seeming reasonable/consistent, and the CI passing

- Formatting.NetCore now targets `netstandard1.3` - was `Profile259` aka `portable-net45+wp80+win8+wpa81`, approximately `netstandard1.0` - but Newtonsoft.Json.Bson supports `netstandard1.3` and up - use Newtonsoft.Json.Bson in NetCore projects - can now multi-target NetCore.Test as well nits: - align src/ Formatting projects as much as possible - align test/ Formatting projects as much as possible - clean up the Microsoft.TestCommon project slightly Notes: - NetCore project names make no sense but I'm leaving them alone for this PR - Formatting.NetCore does not build at this point due to `netstandard1.3` / `Profile259` differences

- remove deprecated `BsonWriter` mentions entirely - remove legacy BSON writer test workaround

- was not available in `Profile259` - leave tests to confirm correct operation w/ "real" class

- leave test to confirm `WebUtility` continues to behave about the same - handle `WebUtility` being a `public` class - change other tests to handle `WebUtility.UrlEncode(...)` encoding to uppercase - unlike `UriQueryUtility.UrlEncode(...)`

- use `ICloneable` copy to get more classes compiling - helps NetCore in similar way to existing `MediaTypeHeaderValueExtensions` - use `InvalidEnumArgumentException` copy to simplify code and testing - both not available in `netstandard1.3` (or available packages) but easily duplicated

- available (but not serializable) in `netstandard1.3`

- remove an unnecessary suppression about `MaxDepth` in `BaseJsonMediaTypeFormatter`

- `XsdDataContractExporter` remains unavailable - update XML DCS tests to handle this (previously not in NetCore assembly) - try various workarounds - `ReadOnlyStreamWithEncodingPreamble` causes tests to hit dotnet/runtime#80160 - restriction to UTF8 when writing is a significant issue

- my runtime clone was at 88868b7a781f4e5b9037b8721f30440207a7aa42

- add conditions to avoid references to version-specific API - remove references to internal dotnet/runtime code - rewrite some code more generically if performance impact likely minor - use Microsoft.TestCommon instead of directly relying on Xunit in tests - use C# 9.0

- remove `ReadOnlyStreamWithEncodingPreamble` and its tests - was insufficient in general and doesn't work correctly in `netstandard1.3` - see dotnet/runtime#80160 Now support a much broader set of encodings in JSON and XML formatters - not just UTF8, UTF16BE, and UTF16LE - JSON formatter already supported every encoding when `UseDataContractJsonSerializer` was `false` - no longer have this downside when property is `true` - however, quotas remain unsupported in `netstandard1.3` version of this formatter when property is `true` - XML formatter requirement for an XML declaration in non-UTF8 content was general and is no longer an issue

- use `DefaultContractResolver`, `GuidAttribute`, `TypeDescriptor`, `XmlElement`, `XmlNode` classes - use `IRequiredMemberSelector` interface - use `RequestMessage.CreateResponse()` extension method - all available in `netstandard1.3` - `JsonContractResolver` use is something of a late Newtonsoft.Json version reaction - was held back by missing `IRequiredMemberSelector`

- get classes now included in NetCore assembly compiling - `SerializationAttribute` is not available - therefore `Exception` is not serializable - therefore `DefaultContractResolver.IgnoreSerializableAttribute` is not available - `Stream.(Begin|End)(Read|Write)` and `Stream.Close()` are not available nit: clean up `MultipartFormDataStreamProvider` whitespace

- don't seem to be copied into test folder in CI builds

src/System.Net.Http.Formatting/Internal/TranscodingStream.cs

- new `[MethodImpl(...)]` attributes have downsides - also don't match `[StackTraceHidden]` semantics

dougbu · 2023-01-27T05:26:46Z

Rebased and addressed @stephentoub's comments.

@stephentoub any other thoughts❔ If none, please check the non-NameValueCollection commits off in the earlier checklist that aren't checked yet. They're all TranscodingStream related but I understand if parts aren't of interest to you. We could find someone to double-check the project files for example (if you prefer).

Other reviewers: Please look at the NameValueCollection commit.

stephentoub · 2023-01-27T18:35:47Z

please check the non-NameValueCollection commits off in the earlier checklist that aren't checked yet. They're all TranscodingStream related but I understand if parts aren't of interest to you. We could find someone to double-check the project files for example (if you prefer).

I don't have rights to click those checkmarks. I only reviewed the "Adjust TranscodingStream and related code to compile and run here" commit and it looked fine other than my feedback you addressed.

dougbu · 2023-01-27T18:41:49Z

I don't have rights to click those checkmarks.

That's a bit odd but thanks for letting me know where you'd focused. Much appreciated.

dougbu · 2023-01-27T18:46:39Z

To other reviewers: I also ticked the checkboxes for "Copy TranscodingStream …" because that was just a literal cp from my dotnet/runtime clone and "!fixup! Remove added AggressiveInlining requests" because that was in response to @stephentoub's comments. This leaves us w/ 3 unreviewed commits.

tools/WebStack.settings.targets

test/System.Net.Http.Formatting.Test/System.Net.Http.Formatting.Test.csproj

wtgodbe

Approving again for the last few commits I just looked at

Tratcher

These TFM changes are breaking. Will this be a new major version?

I didn't review the tests.

src/System.Net.Http.Formatting/Formatting/JsonMediaTypeFormatter.cs

src/System.Net.Http.Formatting/UriExtensions.cs

- mention `XmlDictionaryReader` instead

dougbu · 2023-01-28T07:16:53Z

@Tratcher I addressed your comment on the "Use TranscodingStream …" commit in the latest "!fixup!" commit. Would you say you're signing off on those two commits❔

Still one open question on versioning and public API surface to you @Tratcher

Tratcher · 2023-01-28T21:05:33Z

I've reviewed everything but the tests.

dougbu requested review from phenning, stephentoub, javiercn, MackinnonBuck, TanayParikh, Nick-Stanton, mkArtakMSFT, a team and GrabYourPitchforks January 22, 2023 02:08

dougbu force-pushed the dougbu/Newtonsoft.Json.part6 branch from 77a9a81 to 327f780 Compare January 22, 2023 05:51

wtgodbe approved these changes Jan 24, 2023

View reviewed changes

dougbu added 16 commits January 24, 2023 17:05

React to using Newtonsoft.Json.Bson everywhere

702846d

- remove deprecated `BsonWriter` mentions entirely - remove legacy BSON writer test workaround

Remove unnecessary ConcurrentDictionary class

d53a5b0

- was not available in `Profile259` - leave tests to confirm correct operation w/ "real" class

Remove unnecessary NetCore-specific GlobalSuppressions.cs file

163f187

Use NameValueCollection everywhere

d14845b

- available (but not serializable) in `netstandard1.3`

Add MaxDepth support in NetCore assembly

d5ef276

- remove an unnecessary suppression about `MaxDepth` in `BaseJsonMediaTypeFormatter`

Copy TranscodingStream and related files from dotnet/runtime

a51922e

- my runtime clone was at 88868b7a781f4e5b9037b8721f30440207a7aa42

Add MediaTypeMappings support in NetCore assembly

beac59f

Add IRequiredMemberSelector support in NetCore assembly

a2724a9

Ensure a couple more assemblies are available

6e87558

- don't seem to be copied into test folder in CI builds

stephentoub reviewed Jan 26, 2023

View reviewed changes

src/System.Net.Http.Formatting/Internal/TranscodingStream.cs Show resolved Hide resolved

src/System.Net.Http.Formatting/Internal/TranscodingStream.cs Outdated Show resolved Hide resolved

src/System.Net.Http.Formatting/Internal/TranscodingStream.cs Outdated Show resolved Hide resolved

!fixup! Remove added AggressiveInlining requests

8d057eb

- new `[MethodImpl(...)]` attributes have downsides - also don't match `[StackTraceHidden]` semantics

dougbu force-pushed the dougbu/Newtonsoft.Json.part6 branch from 327f780 to 8d057eb Compare January 27, 2023 05:03

wtgodbe reviewed Jan 27, 2023

View reviewed changes

tools/WebStack.settings.targets Show resolved Hide resolved

wtgodbe reviewed Jan 27, 2023

View reviewed changes

test/System.Net.Http.Formatting.Test/System.Net.Http.Formatting.Test.csproj Show resolved Hide resolved

wtgodbe approved these changes Jan 27, 2023

View reviewed changes

Tratcher approved these changes Jan 28, 2023

View reviewed changes

src/System.Net.Http.Formatting/Formatting/JsonMediaTypeFormatter.cs Outdated Show resolved Hide resolved

src/System.Net.Http.Formatting/UriExtensions.cs Show resolved Hide resolved

!fixup! Improve XmlReader comments

9844be0

- mention `XmlDictionaryReader` instead

dougbu merged commit 2ed73bc into aspnet:main Jan 28, 2023

dougbu deleted the dougbu/Newtonsoft.Json.part6 branch January 28, 2023 21:39

dougbu mentioned this pull request Feb 8, 2023

BDSA-2018-5195 vulnerability by BD hub for newtonsoft version JamesNK/Newtonsoft.Json11.0.2 #362

Closed

dougbu added this to the 3.3.0 (5.3.0) milestone Feb 26, 2023

Move NetCore project to use .NET SDK #380

Move NetCore project to use .NET SDK #380

Uh oh!

Conversation

dougbu commented Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougbu commented Jan 22, 2023

Uh oh!

dougbu commented Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougbu commented Jan 24, 2023

Uh oh!

wtgodbe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dougbu commented Jan 27, 2023

Uh oh!

stephentoub commented Jan 27, 2023

Uh oh!

dougbu commented Jan 27, 2023

Uh oh!

dougbu commented Jan 27, 2023

Uh oh!

Uh oh!

Uh oh!

wtgodbe left a comment

Choose a reason for hiding this comment

Uh oh!

Tratcher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dougbu commented Jan 28, 2023

Uh oh!

Tratcher commented Jan 28, 2023

Uh oh!

Uh oh!

dougbu commented Jan 22, 2023 •

edited

Loading

dougbu commented Jan 22, 2023 •

edited

Loading