Skip to content
This repository was archived by the owner on Nov 15, 2018. It is now read-only.

Updated to support Adapter Extension #132

Closed
wants to merge 9 commits into from
39 changes: 39 additions & 0 deletions src/Microsoft.AspNetCore.JsonPatch/Adapters/AdapterFactory.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Text;
using Microsoft.AspNetCore.JsonPatch.Internal;
using Newtonsoft.Json.Serialization;

namespace Microsoft.AspNetCore.JsonPatch.Adapters
{
/// <summary>
/// The default AdapterFactory to be used for resolving <see cref="IAdapter"/>.
/// </summary>
public class AdapterFactory : IAdapterFactory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense for the AdapterFactory to wrap the IContractResolver? IE, you can provide an adapter factory or a contract resolver:

  • if you customized the contract resolver but not the adapter factory then we create the default adapter factory
  • if you customized the adapter factory then you're expect to figure out how you want to handle contracts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rynowak, I see how this would simplify the code by having a single element to pass around, although it seems to me like we would be asking one class to handle two different purposes. Did you have another goal in mind for consolidating these?

Late in the PR, @HappyNomad suggested we moved the AdpaterFactory to the ApplyTo, rather than being part of the Constructor. We had different philosophical views on why, but a decision there might impact a decision here. In addition to understanding the goal behind your suggestion, I wanted to also get your thoughts on that aspect. Ultimately if we can get this rolled into production it would be great, so I don't mind a little additional refactoring if it will help us move this forward, I just would rather not do it too many more times ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was asking to try and reason about whether or not they are the came concern. The current adapter factory implementation is only thing in the system that interacts with contracts current (I think).

If we were implementing this system fresh (without JSON.NET) we would put the details about how to traverse different types on our equivalent of JsonContract.

I don't have any major concerns or strong feedback about what you and @HappyNomad have been discussing, I think both proposals are pretty reasonable. We don't have the luxury of designing something new here because it has to fit into the existing APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concrete implementations of IAdapter are what actually use the ContractResolver. The AdapterFactory only uses it to pass through. You are right that we probably could pull it into the AdapterFactory, but it would create more complexity to keep the interface backwards compatible. It looks like (quick review) that the ContractResolver can be added in various ways (constructor, converter, ApplyTo, etc) so we would need to handle the old mechanism and the new mechanism in all these locations. I agree that building it fresh it probably makes sense to have the AdpaterFactory provided it if needed. I am willing to refactor if that will help us get this merged, otherwise I am inclined to make fewer changes than are needed to meet the primary goal of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok great, I'm convinced.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late in the PR, @HappyNomad suggested we moved the AdpaterFactory to the ApplyTo, rather than being part of the Constructor.

I suggested that from the beginning, although I did thoroughly consider your idea to instead accept it in JsonPatchDocument's constructor. I concluded to stand by my original idea to only accept it in ApplyTo. I summarized my reasoning at dotnet/aspnetcore#2816 (comment) and we discussed it starting at #132 (comment) .

IAdapterFactory is a means of customizing ObjectAdapter which is a means of customizing JsonPatchDocument. The existing design is to pass ObjectAdapter to ApplyTo. You want to allow the customization of ObjectAdapter (IAdapterFactory) to be passed to JsonPatchDocument's constructor while ObjectAdapter is still passed to ApplyTo? IMHO that's making things more messy and for no good reason.

Please delegate the idea to bake custom behavior into a JsonPatchDocument instance to a separate issue rather than conflating it here. In that separate issue, I think you should consider a JsonPatchDocument constructor that accepts the more general IObjectAdapter instead of the more specific IAdapterFactory.

Please keep this pull request focused and don't touch JsonPatchDocument or JsonPatchDocument<T>. Both of our scenarios will work fine without touching those classes. The extraneous changes you suggest are unnecessary for fulfilling both of our needs. They may also benefit from further consideration in a separate issue.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have the luxury of designing something new here because it has to fit into the existing APIs.

That's exactly my point. IObjectAdapter is already being passed to ApplyTo, so let's not pretend we can undo that design decision by passing IAdapterFactory to JsonPatchDocument's constructor. Instead, let's be consistent with the existing API and leave JsonPatchDocument alone.

{
/// <inheritdoc />
public virtual IAdapter Create(object target, IContractResolver contractResolver)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just passing in the JsonContract since that's all the contractResolver will be used for in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way. The JsonContract must match the target type passed in. Since the code doesn't already have a JsonContract we can trust the developer to properly create it and pass it in, or just create it in the function. The original ObjectVisitor.SelectAdapter method generated the JsonContract in the method, so I followed the same approach. We can create it in the ObjectVisitor.SelectAdapter but if somebody tries to use an AdapterFactory outside the current scope they have to do it correctly. This seemed less error prone to me, but if you would prefer it the other way, it is an easy change. Let me know.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer passing in JsonContract instead of contractResolver. I'm assuming AdapterFactory won't be used in an unforeseen way. This way the JsonContract will be gotten once instead of by every IAdapterFactory implementation, which seems less error-prone to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it can reuse the JsonContract because that is specific to the target for that given SelectAdapter call. For example, if you look at ObjectVisitor.TryVisit' you will see that we are passing in a different target as we traverse the path. That results in a different 'JsonContract' each time. So we aren't getting any performance gains of reuse. Ultimately it comes down to putting that line of code in 'ObjectVisitor.SelectAdapter' or in 'AdapterFactory.Create'. If it goes into SelectAdapter` we are potentially limiting a future extension of the 'AdapterFactory' by only providing the 'JsonContract'. Am I missing something?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My words "gotten once" were poorly chosen. I meant to suggest that the line of code for getting the JsonContract be in only one place in the library (ObjectVisitor.SelectAdapter), instead of in every IAdapterFactory implementation. My idea is to keep AdapterFactory.Create tightly scoped to it's intended purpose. Maybe you have a potential scenario in mind that I haven't considered?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering how people might try and use the AdapterFactory for other purposes and having that code embedded gives them more control and makes it less likely for somebody to pass in the wrong JsonContract. I think it comes down to 6 or a half dozen, so I can go ahead and make the change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the difference of where we're coming from. You're arguing against redundancy of the data passed to Create. I'm arguing against redundancy of the code that generates that data. I think I found a compromise that addresses both concerns, and leads to a better API too. I posted my new suggestion on your updated code.

{
var jsonContract = contractResolver.ResolveContract(target.GetType());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add argument checking - what should the behavior be when the target is null? IIRC that should be an exception.


if (target is IList)
{
return new ListAdapter();
}
else if (jsonContract is JsonDictionaryContract jsonDictionaryContract)
{
var type = typeof(DictionaryAdapter<,>).MakeGenericType(jsonDictionaryContract.DictionaryKeyType, jsonDictionaryContract.DictionaryValueType);
return (IAdapter)Activator.CreateInstance(type);
}
else if (jsonContract is JsonDynamicContract)
{
return new DynamicObjectAdapter();
}
else
{
return new PocoAdapter();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to see some unit tests for this method now that it's an abstraction rather than a detail.

}
}
22 changes: 22 additions & 0 deletions src/Microsoft.AspNetCore.JsonPatch/Adapters/IAdapterFactory.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using Microsoft.AspNetCore.JsonPatch.Internal;
using Newtonsoft.Json.Serialization;
using System;
using System.Collections.Generic;
using System.Text;

namespace Microsoft.AspNetCore.JsonPatch.Adapters
{
/// <summary>
/// Defines the operations used for loading an <see cref="IAdapter"/> based on the current object and ContractResolver.
/// </summary>
public interface IAdapterFactory
{
/// <summary>
/// Creates an <see cref="IAdapter"/> for the current object
/// </summary>
/// <param name="target">The target object</param>
/// <param name="contractResolver">The current contract resolver</param>
/// <returns>The needed <see cref="IAdapter"/></returns>
IAdapter Create(object target, IContractResolver contractResolver);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest a JsonContract instead of a contractResolver.

}
}
31 changes: 25 additions & 6 deletions src/Microsoft.AspNetCore.JsonPatch/Adapters/ObjectAdapter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,43 @@ namespace Microsoft.AspNetCore.JsonPatch.Adapters
/// <inheritdoc />
public class ObjectAdapter : IObjectAdapterWithTest
{
/// <summary>
/// Initializes a new instance of <see cref="ObjectAdapter"/> using the default AdapterFactory.
/// </summary>
/// <param name="contractResolver">The <see cref="IContractResolver"/>.</param>
/// <param name="logErrorAction">The <see cref="Action"/> for logging <see cref="JsonPatchError"/>.</param>
public ObjectAdapter(
IContractResolver contractResolver,
Action<JsonPatchError> logErrorAction):this(contractResolver, logErrorAction, new AdapterFactory())
{
}

/// <summary>
/// Initializes a new instance of <see cref="ObjectAdapter"/>.
/// </summary>
/// <param name="contractResolver">The <see cref="IContractResolver"/>.</param>
/// <param name="logErrorAction">The <see cref="Action"/> for logging <see cref="JsonPatchError"/>.</param>
/// <param name="adapterFactory">The <see cref="IAdapterFactory"/> to use when creating adaptors.</param>
public ObjectAdapter(
IContractResolver contractResolver,
Action<JsonPatchError> logErrorAction)
Action<JsonPatchError> logErrorAction,
IAdapterFactory adapterFactory)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all we need - no need to modify JsonPatchDocument.

{
ContractResolver = contractResolver ?? throw new ArgumentNullException(nameof(contractResolver));
LogErrorAction = logErrorAction;
AdapterFactory = adapterFactory;
}

/// <summary>
/// Gets or sets the <see cref="IContractResolver"/>.
/// </summary>
public IContractResolver ContractResolver { get; }

/// <summary>
/// Gets the <see cref="IAdapterFactory"/>
/// </summary>
public IAdapterFactory AdapterFactory { get; }

/// <summary>
/// Action for logging <see cref="JsonPatchError"/>.
/// </summary>
Expand Down Expand Up @@ -75,7 +94,7 @@ private void Add(
}

var parsedPath = new ParsedPath(path);
var visitor = new ObjectVisitor(parsedPath, ContractResolver);
var visitor = new ObjectVisitor(parsedPath, ContractResolver, AdapterFactory);

var target = objectToApplyTo;
if (!visitor.TryVisit(ref target, out var adapter, out var errorMessage))
Expand Down Expand Up @@ -144,7 +163,7 @@ public void Remove(Operation operation, object objectToApplyTo)
private void Remove(string path, object objectToApplyTo, Operation operationToReport)
{
var parsedPath = new ParsedPath(path);
var visitor = new ObjectVisitor(parsedPath, ContractResolver);
var visitor = new ObjectVisitor(parsedPath, ContractResolver, AdapterFactory);

var target = objectToApplyTo;
if (!visitor.TryVisit(ref target, out var adapter, out var errorMessage))
Expand Down Expand Up @@ -175,7 +194,7 @@ public void Replace(Operation operation, object objectToApplyTo)
}

var parsedPath = new ParsedPath(operation.path);
var visitor = new ObjectVisitor(parsedPath, ContractResolver);
var visitor = new ObjectVisitor(parsedPath, ContractResolver, AdapterFactory);

var target = objectToApplyTo;
if (!visitor.TryVisit(ref target, out var adapter, out var errorMessage))
Expand Down Expand Up @@ -239,7 +258,7 @@ public void Test(Operation operation, object objectToApplyTo)
}

var parsedPath = new ParsedPath(operation.path);
var visitor = new ObjectVisitor(parsedPath, ContractResolver);
var visitor = new ObjectVisitor(parsedPath, ContractResolver, AdapterFactory);

var target = objectToApplyTo;
if (!visitor.TryVisit(ref target, out var adapter, out var errorMessage))
Expand Down Expand Up @@ -281,7 +300,7 @@ private bool TryGetValue(
propertyValue = null;

var parsedPath = new ParsedPath(fromLocation);
var visitor = new ObjectVisitor(parsedPath, ContractResolver);
var visitor = new ObjectVisitor(parsedPath, ContractResolver, AdapterFactory);

var target = objectToGetValueFrom;
if (!visitor.TryVisit(ref target, out var adapter, out var errorMessage))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist
serializer.Populate(jObjectReader, targetOperations);

// container target: the JsonPatchDocument.
var container = new JsonPatchDocument(targetOperations, new DefaultContractResolver());
var container = CreateContainer(targetOperations);

return container;
}
Expand All @@ -72,5 +72,14 @@ public override void WriteJson(JsonWriter writer, object value, JsonSerializer s
serializer.Serialize(writer, lst);
}
}

/// <summary>
/// Create the JsonPatchDocument using the appropriate constructor
/// </summary>
/// <param name="operations">The operations to assign to the JsonPatchDocument</param>
/// <returns>A new instance of the JsonPatchDocument</returns>
protected virtual JsonPatchDocument CreateContainer(List<Operation> operations ) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not totally sure what this is here for - how do you expect this to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rynowak We added this purely for extension purposes. If I remember correctly @HappyNomad needed a custom JsonPatchDocumentConverter, and breaking out this portion to be overridden allowed the rest of the ReadJson function to be used rather than being duplicated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, what are the scenarios for a custom converter? What problems are being solved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking back through the thread, @HappyNomad needed to provide his custom adapter factory as part of the deserialization process. I didn't need to for my scenario, and to be honest I never went back to understand why one of us needed it and the other didn't.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HappyNomad - care to reply?

I don't think this is blocking if we don't get a response. I just like to know what's going on 😆

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, what are the scenarios for a custom converter? What problems are being solved?

For an answer, see #132 (comment) .

But as long as I can pass IAdapterFactory to ApplyTo, I won't need it. See #132 (comment) .

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested this while thoroughly considering @jmudavid's suggestion to pass IAdapterFactory to JsonPatchDocument's constructor. He was suggesting customization via the constructor, so in a deserialization scenario I may want to access that customization ability and this would allow me to do it.

As you can see from my other comments today, I believe @jmudavid's deviation from the existing API is a mistake. Not only that, but I discovered the arguments I'd need to pass to JsonPatchDocument's constructor aren't available until ApplyTo-time anyway. So from my perspective, in the same way that the changes proposed for JsonPatchDocument are unnecessary (and even harmful), I believe the changes here can be rolled back as well.

new JsonPatchDocument(operations, new DefaultContractResolver());

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public override object ReadJson(
serializer.Populate(jObjectReader, targetOperations);

// container target: the typed JsonPatchDocument.
var container = Activator.CreateInstance(objectType, targetOperations, new DefaultContractResolver());
var container = CreateTypedContainer(objectType, targetOperations);

return container;
}
Expand All @@ -60,5 +60,16 @@ public override object ReadJson(
throw new JsonSerializationException(Resources.InvalidJsonPatchDocument, ex);
}
}

/// <summary>
/// Create the JsonPatchDocument using the appropriate constructor
/// </summary>
/// <param name="objectType">The model type for the JsonPatchDocument</param>
/// <param name="operations">The operations to assign to the JsonPatchDocument</param>
/// <returns>A new instance of the JsonPatchDocument</returns>
protected virtual object CreateTypedContainer(Type objectType, object operations)
{
return Activator.CreateInstance(objectType, operations, new DefaultContractResolver());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not totally sure what this is going to be used for... it would be good to talk through the scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. Just trying to allow custom Converters to be created with as little code duplication as possible.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.AspNetCore.JsonPatch.Internal
{
public class DictionaryAdapter<TKey, TValue> : IAdapter
{
public bool TryAdd(
public virtual bool TryAdd(
object target,
string segment,
IContractResolver contractResolver,
Expand All @@ -37,7 +37,7 @@ public bool TryAdd(
return true;
}

public bool TryGet(
public virtual bool TryGet(
object target,
string segment,
IContractResolver contractResolver,
Expand Down Expand Up @@ -66,7 +66,7 @@ public bool TryGet(
return true;
}

public bool TryRemove(
public virtual bool TryRemove(
object target,
string segment,
IContractResolver contractResolver,
Expand Down Expand Up @@ -94,7 +94,7 @@ public bool TryRemove(
return true;
}

public bool TryReplace(
public virtual bool TryReplace(
object target,
string segment,
IContractResolver contractResolver,
Expand Down Expand Up @@ -128,7 +128,7 @@ public bool TryReplace(
return true;
}

public bool TryTest(
public virtual bool TryTest(
object target,
string segment,
IContractResolver contractResolver,
Expand Down Expand Up @@ -177,7 +177,7 @@ public bool TryTest(
}
}

public bool TryTraverse(
public virtual bool TryTraverse(
object target,
string segment,
IContractResolver contractResolver,
Expand Down Expand Up @@ -208,7 +208,7 @@ public bool TryTraverse(
}
}

private bool TryConvertKey(string key, out TKey convertedKey, out string errorMessage)
protected virtual bool TryConvertKey(string key, out TKey convertedKey, out string errorMessage)
{
var conversionResult = ConversionResultProvider.ConvertTo(key, typeof(TKey));
if (conversionResult.CanBeConverted)
Expand All @@ -225,7 +225,7 @@ private bool TryConvertKey(string key, out TKey convertedKey, out string errorMe
}
}

private bool TryConvertValue(object value, out TValue convertedValue, out string errorMessage)
protected virtual bool TryConvertValue(object value, out TValue convertedValue, out string errorMessage)
{
var conversionResult = ConversionResultProvider.ConvertTo(value, typeof(TValue));
if (conversionResult.CanBeConverted)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace Microsoft.AspNetCore.JsonPatch.Internal
{
public class DynamicObjectAdapter : IAdapter
{
public bool TryAdd(
public virtual bool TryAdd(
object target,
string segment,
IContractResolver contractResolver,
Expand All @@ -31,7 +31,7 @@ public bool TryAdd(
return true;
}

public bool TryGet(
public virtual bool TryGet(
object target,
string segment,
IContractResolver contractResolver,
Expand All @@ -48,7 +48,7 @@ public bool TryGet(
return true;
}

public bool TryRemove(
public virtual bool TryRemove(
object target,
string segment,
IContractResolver contractResolver,
Expand Down Expand Up @@ -78,7 +78,7 @@ public bool TryRemove(

}

public bool TryReplace(
public virtual bool TryReplace(
object target,
string segment,
IContractResolver contractResolver,
Expand All @@ -105,7 +105,7 @@ public bool TryReplace(
return true;
}

public bool TryTest(
public virtual bool TryTest(
object target,
string segment,
IContractResolver contractResolver,
Expand Down Expand Up @@ -135,7 +135,7 @@ public bool TryTest(
}
}

public bool TryTraverse(
public virtual bool TryTraverse(
object target,
string segment,
IContractResolver contractResolver,
Expand All @@ -155,7 +155,7 @@ public bool TryTraverse(
}
}

private bool TryGetDynamicObjectProperty(
protected virtual bool TryGetDynamicObjectProperty(
object target,
IContractResolver contractResolver,
string segment,
Expand Down Expand Up @@ -191,7 +191,7 @@ private bool TryGetDynamicObjectProperty(
}
}

private bool TrySetDynamicObjectProperty(
protected virtual bool TrySetDynamicObjectProperty(
object target,
IContractResolver contractResolver,
string segment,
Expand Down Expand Up @@ -227,7 +227,7 @@ private bool TrySetDynamicObjectProperty(
}
}

private bool TryConvertValue(object value, Type propertyType, out object convertedValue)
protected virtual bool TryConvertValue(object value, Type propertyType, out object convertedValue)
{
var conversionResult = ConversionResultProvider.ConvertTo(value, propertyType);
if (!conversionResult.CanBeConverted)
Expand Down
Loading