Move JSON.NET specific features to a separate assembly #5146

pranavkm · 2018-12-17T02:32:28Z

No description provided.

src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs

...crosoft.AspNetCore.Mvc.NewtonsoftJson/DependencyInjection/MvcJsonMvcCoreBuilderExtensions.cs

...c/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/Microsoft.AspNetCore.Mvc.NewtonsoftJson.csproj

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/NewtonsoftJsonControllerExtensions.cs

src/Mvc/src/Microsoft.AspNetCore.Mvc.ViewFeatures/SessionStateTempDataProvider.cs

src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiBehaviorTest.cs

...c/Microsoft.AspNetCore.Mvc.NewtonsoftJson/DependencyInjection/MvcJsonMvcBuilderExtensions.cs

...crosoft.AspNetCore.Mvc.NewtonsoftJson/DependencyInjection/MvcJsonMvcCoreBuilderExtensions.cs

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/JsonHelperExtensions.cs

...c/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/Microsoft.AspNetCore.Mvc.NewtonsoftJson.csproj

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/ProblemDetailsConverter.cs

src/Mvc/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Infrastructure/DefaultTempDataSerializer.cs

src/Mvc/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Rendering/DefaultJsonHelper.cs

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/ProblemDetailsConverter.cs

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/AnnotatedProblemDetails.cs

pranavkm · 2018-12-31T19:08:15Z

🆙 📅

pranavkm · 2018-12-31T19:10:52Z

src/Mvc/benchmarkapps/BasicApi/Startup.cs

@@ -125,7 +125,7 @@ public void ConfigureServices(IServiceCollection services)
            services
                .AddMvcCore()
                .AddAuthorization()
-                .AddJsonFormatters(json => json.ContractResolver = new CamelCasePropertyNamesContractResolver())
+                .AddNewtonsoftJsonFeatures(options => options.SerializerSettings.ContractResolver = new CamelCasePropertyNamesContractResolver())


Thoughts on the name? It's a bit more than serializers. There's one casualty here - this method will add services used by views. In the past, you could use MvcCoreBuilder based extensions to avoid adding any view related features to DI. Do we need to maintain a AddNewtonsoftJsonFormatters in addition to this?

What about just AddNewtonsoftJson?

I think it's OK that we're adding the viewfeatures-defined services. They don't do anything unless they are used.

One thing we could do is move the interface types into Mvc.Abstractions.

That would reduce the number of assemblies that get loaded.

TempDataSerializer doesn't quite feel like something that belongs in abstractions. Perhaps it's ok to leave this as-is unless we get some strong feedback

src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ProblemDetails.cs

....AspNetCore.Mvc.NewtonsoftJson/DependencyInjection/NewtonsoftJsonMvcCoreBuilderExtensions.cs

src/Mvc/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Rendering/DefaultJsonHelper.cs

src/Mvc/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ServicesModelBinderIntegrationTest.cs

pranavkm · 2019-01-03T21:05:42Z

🆙 📅

JamesNK · 2019-01-03T21:19:19Z

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/ValidationProblemDetailsConverter.cs

+    /// <summary>
+    /// A RFC 7807 compliant <see cref="JsonConverter"/> for <see cref="ValidationProblemDetails"/>.
+    /// </summary>
+    public sealed class ValidationProblemDetailsConverter : JsonConverter


Do the converters need to be public?

The equivalent types for the xml serializers are public - https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/src/Microsoft.AspNetCore.Mvc.Formatters.Xml/ProblemDetailsWrapper.cs so it made sense to do so here

JamesNK · 2019-01-03T21:20:26Z

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/ValidationProblemDetailsConverter.cs

+        /// <inheritdoc />
+        public override bool CanConvert(Type objectType)
+        {
+            return objectType == typeof(ValidationProblemDetails);


Do you care about serializing sub-classes of ValidationProblemDetails?

Nope, we don't want a catch-all converter, one of the reasons I did not use JsonConverter<T>

JamesNK · 2019-01-03T21:21:06Z

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/ValidationProblemDetailsConverter.cs

+                }
+            }
+
+


nit: remove extra line

src/Mvc/src/Microsoft.AspNetCore.Mvc.NewtonsoftJson/ValidationProblemDetailsConverter.cs

src/Mvc/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Infrastructure/TempDataSerializer.cs

pranavkm · 2019-01-04T19:04:45Z

🆙 📅

natemcmaster · 2019-01-08T20:54:47Z

So, does this mean we can pull Json.NET out of the shared fx now?

pranavkm requested a review from rynowak December 17, 2018 02:32

pranavkm changed the title ~~Move JSON.NET specific features to a separate assembly~~ [WIP] Move JSON.NET specific features to a separate assembly Dec 17, 2018

pranavkm commented Dec 17, 2018

View reviewed changes