Support DynamicObject types #99

jbagga · 2017-08-07T18:20:39Z

Addresses #38

rynowak · 2017-08-07T19:16:12Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

            {
                errorMessage = null;
                nextTarget = null;
                return false;
            }

-            var dictionary = (IDictionary<string, object>)expandoObject;
+            var dictionary = (IDictionary<string, object>)dynamicObject;


This isn't a good assumption. We need to use IDynamicMetaObjectProvider directly.

jbagga · 2017-08-15T00:33:40Z

@rynowak Added a new adapter instead of replacing ExpandoObjectAdapter.

rynowak · 2017-08-17T15:06:47Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+            object value,
+            out string errorMessage)
+        {
+            if(!TrySetDynamicObjectProperty(target, contractResolver, segment, value))


rynowak · 2017-08-17T15:09:26Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+                nextTarget = null;
+                errorMessage = null;
+                return false;
+            }


Under what conditions can this happen? Should we be checking this in all of these methods and returning a good error message?

rynowak · 2017-08-17T15:09:49Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+
+            var propertyName = jsonDynamicContract?.PropertyNameResolver(segment);
+
+            var binder = CSharp.RuntimeBinder.Binder.GetMember(CSharpBinderFlags.None,


newline after (

rynowak · 2017-08-17T15:10:27Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+            var binder = CSharp.RuntimeBinder.Binder.GetMember(CSharpBinderFlags.None,
+                propertyName,
+                target.GetType(),
+                new List<CSharpArgumentInfo>{


https://en.wikipedia.org/wiki/Indent_style#Allman_style

rynowak · 2017-08-17T15:10:58Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+            string segment,
+            out object property)
+        {
+            var jsonDynamicContract = contractResolver.ResolveContract(target.GetType()) as JsonDynamicContract;


Under what conditions do we expect this cast to fail?

rynowak · 2017-08-17T15:11:42Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+        {
+            var jsonDynamicContract = contractResolver.ResolveContract(target.GetType()) as JsonDynamicContract;
+
+            var propertyName = jsonDynamicContract?.PropertyNameResolver(segment);


You'll get an ArgumentNullException later at line 156. If you are going to be defensive, make sure you are defending from the right things

rynowak · 2017-08-17T15:12:28Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+            {
+                property = null;
+                return false;
+            }


Do we put a try/catch around the rest of our property set logic?

This seems really dangerous

jbagga · 2017-08-17T23:09:54Z

@rynowak Updated

rynowak · 2017-08-18T02:31:27Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

-                propertyName,
+            var binder = CSharp.RuntimeBinder.Binder.GetMember(
+                CSharpBinderFlags.None,
+                propertyName ?? segment,


Why is this right?

@rynowak If segment is null it will throw here before it gets to the adapter.

If the contract or the propertName cannot be resolved, segment may still be a valid path for the patch document. Is that risky?

Yeah but under what conditions would any of these things happen?

rynowak · 2017-08-22T02:45:58Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+            out object property,
+            out string errorMessage)
+        {
+            var jsonDynamicContract = contractResolver.ResolveContract(target.GetType()) as JsonDynamicContract;


When can this cast fail? I think you're being defensive here with something that can't happen

rynowak · 2017-08-22T02:46:52Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+
+            var propertyName = jsonDynamicContract?.PropertyNameResolver(segment);
+
+            var binder = CSharp.RuntimeBinder.Binder.GetMember(


Add a .using if possible. If there's a conflict then use using CSharpBinder = ...

rynowak · 2017-08-22T02:47:52Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+            object target,
+            IContractResolver contractResolver,
+            string segment,
+            out object property,


This isn't the right name, it's the property's value not the property itself

rynowak · 2017-08-22T02:49:25Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+                new List<CSharpArgumentInfo>
+                {
+                    CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, null),
+                    CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, null)


Is this right? I would expect the getter to have only one argument (this).

rynowak · 2017-08-22T02:53:00Z

test/Microsoft.AspNetCore.JsonPatch.Test/Dynamic/DynamicObjectAdapterTests.cs

+            var serialized = JsonConvert.SerializeObject(patchDoc);
+            var deserialized = JsonConvert.DeserializeObject<JsonPatchDocument>(serialized);
+
+            deserialized.ApplyTo(doc);


Why do you need this? I think that you should be able to use patchDoc directly

Yes that's how I wrote them but again for consistency with existing tests I changed them to use deserialized. I'll change them to use patchDoc instead

Yeah, don't copy bad stuff 😆

rynowak · 2017-08-22T02:53:14Z

test/Microsoft.AspNetCore.JsonPatch.Test/Dynamic/DynamicObjectAdapterTests.cs

+        [Fact]
+        public void AddNewPropertyToDynamicOjectInTypedObject()
+        {
+            var doc = new NestedDTO()


avoid abbreviations like this

So rename this?

Neither doc nor NestedDTO are great names

rynowak · 2017-08-22T02:53:32Z

test/Microsoft.AspNetCore.JsonPatch.Test/Dynamic/DynamicObjectAdapterTests.cs

+using Newtonsoft.Json;
+using Xunit;
+
+namespace Microsoft.AspNetCore.JsonPatch.Test.Dynamic


This isn't really the right namespace.

rynowak · 2017-08-22T02:54:04Z

test/Microsoft.AspNetCore.JsonPatch.Test/Dynamic/DynamicObjectAdapterTests.cs

+
+namespace Microsoft.AspNetCore.JsonPatch.Test.Dynamic
+{
+    public class DynamicObjectAdapterTests


If these are unit tests then there should be calling methods on the adapter directly.

If these are integration tests, we should organize them as such.

https://github.com/aspnet/Home/wiki/Engineering-guidelines#unit-test-structure

I tried to be consistent with https://github.com/aspnet/JsonPatch/blob/dev/test/Microsoft.AspNetCore.JsonPatch.Test/Dynamic/AddOperationTests.cs and other tests here. Should I move my tests to the existing test classes? They are organized by OperationType, not by Adapters.

I'm sending one of my minions to help you sort out this mess later today. Don't fret, help is on the way

rynowak · 2017-08-22T02:57:03Z

test/Microsoft.AspNetCore.JsonPatch.Test/Dynamic/DynamicObjectAdapterTests.cs

+        }
+
+        [Fact]
+        public void ReplaceAtEndOfList()


This is wayyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy overtesting for dynamic. Most of these really feel like integration tests.

jbagga · 2017-08-24T18:06:43Z

@rynowak Added unit tests

rynowak · 2017-08-24T20:44:42Z

src/Microsoft.AspNetCore.JsonPatch/Internal/DynamicObjectAdapter.cs

+            out object value,
+            out string errorMessage)
+        {
+            var jsonDynamicContract = contractResolver.ResolveContract(target.GetType()) as JsonDynamicContract;


Please stop by so we can discuss this in person. I've commented on this about 4 times and haven't yet heard and answer.

rynowak · 2017-08-24T20:50:57Z

test/Microsoft.AspNetCore.JsonPatch.Test/DynamicObjectAdapterTest.cs

+            // Assert
+            Assert.False(removeStatus);
+            Assert.Equal($"The target location specified by path segment '{segment}' was not found.", removeErrorMessage);
+        }


These are good 👍

jbagga · 2017-08-24T21:10:41Z

@rynowak Updated

jbagga requested a review from rynowak August 7, 2017 18:20

dnfclas added the cla-already-signed label Aug 7, 2017

jbagga assigned rynowak Aug 7, 2017

rynowak reviewed Aug 7, 2017

View reviewed changes

rynowak reviewed Aug 17, 2017

View reviewed changes

rynowak reviewed Aug 18, 2017

View reviewed changes

jbagga force-pushed the jbagga/DynamicTypes38 branch from 7ef41e4 to 4531abf Compare August 21, 2017 22:36

rynowak reviewed Aug 22, 2017

View reviewed changes

jbagga added 5 commits August 23, 2017 10:51

Add new adapter

68d07cd

Feedback addressed

7ecb253

WIP

87cb8bb

WIP

1319a1b

Resolve merge conflicts

fc4211e

jbagga force-pushed the jbagga/DynamicTypes38 branch from 4531abf to fc4211e Compare August 23, 2017 18:16

Add unit tests

5934e22

jbagga mentioned this pull request Aug 24, 2017

Reorganize tests and remove redundant integration tests #105

Closed

5 tasks

rynowak reviewed Aug 24, 2017

View reviewed changes

Address feedback

a26b6d1

rynowak approved these changes Aug 24, 2017

View reviewed changes

jbagga merged commit 3d6a861 into dev Aug 24, 2017

jbagga deleted the jbagga/DynamicTypes38 branch August 24, 2017 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support DynamicObject types #99

Support DynamicObject types #99

jbagga commented Aug 7, 2017

rynowak Aug 7, 2017

jbagga commented Aug 15, 2017

rynowak Aug 17, 2017

rynowak Aug 17, 2017

rynowak Aug 17, 2017

rynowak Aug 17, 2017

rynowak Aug 17, 2017

rynowak Aug 17, 2017

rynowak Aug 17, 2017

jbagga commented Aug 17, 2017

rynowak Aug 18, 2017

jbagga Aug 18, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

jbagga Aug 22, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

jbagga Aug 22, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

jbagga Aug 22, 2017

rynowak Aug 22, 2017

rynowak Aug 22, 2017

jbagga commented Aug 24, 2017

rynowak Aug 24, 2017

rynowak Aug 24, 2017

jbagga commented Aug 24, 2017


		var propertyName = jsonDynamicContract?.PropertyNameResolver(segment);

		var binder = CSharp.RuntimeBinder.Binder.GetMember(CSharpBinderFlags.None,

Support DynamicObject types #99

Support DynamicObject types #99

Conversation

jbagga commented Aug 7, 2017

Choose a reason for hiding this comment

jbagga commented Aug 15, 2017

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

jbagga commented Aug 17, 2017

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

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

jbagga commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Aug 24, 2017