React to new System.Text.Json features #9572

pranavkm · 2019-04-19T18:16:20Z

No description provided.

pranavkm · 2019-04-19T18:18:45Z

src/Mvc/Mvc.Core/src/MvcOptions.cs

+
+            // Ignore casing when serializing values. Setting this to true keeps parity with Json.NET's
+            // behavior.
+            PropertyNameCaseInsensitive = true,


How do you feel about this? In the absence of this property, it is required to send camel-case values to successfully deserialize. Most of our tests use pascal-case when sending data.

We can update tests, but I'm not sure how sad users would be when random casing doesn't work.

I think true is the right default for MVC. Also it keeps closer to Json.NET so less friction when upgrading. If someone wants to be strict then they can always change the setting.

I just spoke to @rynowak offline and he's of the opinion that having it be case sensitive by default would be the right way to go. Users are always free to change it.

We should use the best default regardless of migration. You can always turn this ON for migrating.

I'm interested to hear arguments about case-insensitive is the right default for a REST API.

Made this case sensitive now and updated tests to be 🐫 cased

I'm interested to hear arguments about case-insensitive is the right default for a REST API.

Nothing other than it slightly reduces issues when using an API and I don't see much disadvantage. I don't feel strongly about it. As long as it is configurable

We could always revisit this once we hear what users have to say with the preview5 bits.

Nothing other than it slightly reduces issues when using an API and I don't see much disadvantage. I don't feel strongly about it. As long as it is configurable

APIs document their contract and are precise about what they support. JSON is a case sensitive format, and it's reasonable to follow along with that. I expect if we asked API users they would overwhelmingly support us following the spec.

JSON being case sensitive in the browser is prior art, not a spec. There is no spec about how JSON should map to a .NET object 😋

When it comes to JSON my philosophy has been be flexible in what you accept and exact in what you produce. It appeals to software devs to be strict but when it comes time to use the new serializer I suspect a lot of people will turn case insensitive on to ease the way.

rynowak · 2019-04-22T02:27:51Z

@ahsonkhan @steveharter FYI

ahsonkhan · 2019-04-24T01:27:25Z

src/Mvc/test/Mvc.FunctionalTests/SystemTextJsonOutputFormatterTest.cs

-        }
-
-        [Fact]
-        public override async Task Formatting_CollectionType()


Why remove these tests?

They were overriding the baseline tests https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/test/Mvc.FunctionalTests/JsonOutputFormatterTestBase.cs#L117-L129 because they were different from what we would want (i.e did not do camel-case). Now that it matches the baseline, we can have it call the one on the base.

ahsonkhan · 2019-04-24T01:27:44Z

src/Mvc/Mvc.ViewFeatures/test/Infrastructure/TempDataSerializerTestBase.cs

+        }
+
+        [Fact]
+        public virtual void RoundTripTest_StringThatIsNotCompliantGuid()


Just to verify, we are ok with this, correct?

Yup, this is fine. We just need to able to document "a" behavior so we don't find it confusing \ surprising.

ahsonkhan · 2019-04-24T01:28:18Z

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

                        }
                        else
                        {
-                            deserializedValue = stringValue;
+                            deserializedValue = item.Value.GetString();


Will the JSON strings more often than not be GUID/DateTimes (or will they primarily be regular strings)?

I'd expect the latter. It's not uncommon for users to JSON serialize on complex models so that they could put it in this dictionary. Is there a way to improve this?

ahsonkhan · 2019-04-24T01:28:48Z

src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs

@@ -194,8 +195,8 @@ protected async Task ReadAsync_ReadsValidArray_AsList(Type requestedType)

            // Assert
            Assert.False(result.HasError);
-            var integers = Assert.IsType<List<int>>(result.Model);
-            Assert.Equal(new int[] { 0, 23, 300 }, integers);


What motivated this change?

Good one. I should have brought this up - @layomia was there a reason System.Text.Json returns a type that wraps List<T> rather than just returning a List<T>? Not that it ultimately matters, but if I remember correctly we agreed on just doing a List<T> like Json.NET does.

Also cc @steveharter

pranavkm commented Apr 19, 2019

View reviewed changes

Eilon added the area-mvc label Apr 19, 2019

pranavkm force-pushed the prkrishn/systemtextjsonhelper branch from 8f39208 to c138325 Compare April 19, 2019 23:31

pranavkm force-pushed the prkrishn/set-options branch from fe648e5 to e577bcc Compare April 19, 2019 23:35

pranavkm changed the base branch from prkrishn/systemtextjsonhelper to master April 21, 2019 19:12

React to new System.Text.Json features

dbe4b80

pranavkm force-pushed the prkrishn/set-options branch from e577bcc to dbe4b80 Compare April 21, 2019 19:20

rynowak approved these changes Apr 22, 2019

View reviewed changes

pranavkm merged commit 418f3d8 into master Apr 22, 2019

pranavkm deleted the prkrishn/set-options branch April 22, 2019 03:48

ahsonkhan reviewed Apr 24, 2019

View reviewed changes

React to new System.Text.Json features #9572

React to new System.Text.Json features #9572

Conversation

pranavkm commented Apr 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak commented Apr 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Apr 22, 2019 •

edited

Loading

ahsonkhan Apr 24, 2019 •

edited

Loading

pranavkm Apr 24, 2019 •

edited

Loading