Enhancement to JsonPatch to support inheritance and overrides

[jmudavid]

Hello,
I am looking to extend JsonPatch to support the SCIM patch protocol. Specifically I am looking to add attribute filters within a list, rather than needing to rely solely on indexes. I have a fork with some basic functionality to prove it can be done, but I am trying to minimize the amount of code that needs to be maintained outside the main version. I would like to propose minor refactoring that would make it easier for me to extend JsonPatch to meet these needs. The actual logic could certainly be included in the base library, but I suspect that would cause unneeded confusion for anybody not leveraging Scim.

Basically I would like to make the following changes:

• Allow all Adapters to be inherited and their methods be overridden
• Create interfaces for ParsedPath and ObjectVisitor, in addition to allowing their methods to be overridden
• Create a new ObjectAdapterTyped which allows the implementation types of the ParsedPath and ObjectVisitor to be set via generics
• Update the existing ObjectAdapter to inherit from ObjectAdapterTyped with the current ParsedPath and ObjectVisitor types set, which should prevent any breaking changes to the interface
• Allow the Adapter selection to be customized - although this might be easier done just using a new concrete ObjectVisitor

I would also like to propose a change to the ObjectVisitor.TryTraverse which will attempt to create an empty object if the value is null. This would make it easier to Add a child value to a property that doesn't already exist. There may be limitations to the capabilities, but I believe it would be very helpful. However, this could be handled in a custom ObjectVisitor if required.

I think this will make it much easier for others to extend the JsonPatch capabilities to add additional support for standard protocols (like Scim) or simply to support additional non-standards complaint patch operations.

Since this requires some minor refactoring of the code base, I wanted to check in with the owners of the project before starting to get feedback on the overall approach.

I am also having some issues running the test project due to dependencies on Core 2.1 preview, and would like to get some assistance to ensure my changes do not cause any breaking changes for others.

Thanks!
Dave

[jmudavid]

@Eilon, I wanted to ping you quickly on this concept. I see you were in discussions with somebody else around a similar need that never materialized earlier this year. I am running into some CI build issues using the forked repo due to internal nuget references and rather than trying to solve that issue, wanted to see if getting this code pulled into JsonPatch proper would be a better use of my time. The only changes I have made are changing the scope of some methods and exposing some additional interfaces, so I believe the changes are relatively low risk while improving the flexibility. Thanks!

[Eilon]

@jmudavid would it be possible to send a PR with a rough outline of what you're looking for? I certainly don't want to waste anyone's time, but that might be the easiest way for us to get a sense of how big a change you're talking about. Don't worry about tests or anything (yet!) - I just want to see if we can easily get a sense of how much API churn there is. Thanks!

[jmudavid]

@Eilon I created the PR and while I was able to mention you, I couldn't add you as a reviewer. Here it is aspnet/JsonPatch#130 as I am sure you get a lot of mentions and didn't want it to get buried in your other alerts. Thanks!

[HappyNomad]

I see you were in discussions with somebody else around a similar need that never materialized earlier this year.

For context, I believe this refers back to #2743.

@jmudavid Thanks for raising this issue again. It looks like we have similar goals regarding extension.

@Eilon and @jmudavid Some of the changes proposed here look out of the primary goal's scope. It might be easier to manage and consider them if they're filed in individual, separate issues. Also, as @mkArtakMSFT and @Eilon explained in #2432, "the main goal of this repo is to support the JSON PATCH spec". For getting started at least, perhaps we should narrow down the scope of the current issue a bit.

Among the proposed changes listed above, these make sense to me:

• Allow all Adapters to be inherited and their methods be overridden
• Allow the Adapter selection to be customized

For the latter, please reconsider the small change suggested in #2743 to introduce:

a public interface IAdapterFactory. It defines a single method with the signature IAdapter Create( object target, JsonContract jsonContract ). Use IAdapterFactory in ObjectVisitor's private SelectAdapter method.

Making this smaller change first would alleviate large code duplication for extension scenarios. The "Allow all Adapters to be inherited and their methods be overridden" proposal could take the duplication reduction even further in certain extension scenarios. But perhaps we could start with a minor tweak, see how it goes, and then expand upon what we've learned from there.

[jmudavid]

@HappyNomad I have been reviewing the changes I made and the factory approach might be sufficient. Most of the changes I had to make were to get the right Adapter loaded from within the ObjectVisitor and then to handle the different path separators. The only Adapter I am currently extending is the ListAdapter so I could just replicate the current code, if we don't want to allow them to be overridden currently.

I will start a fresh branch using the factory approach and see if I can replicate the functionality I need without needing new interfaces for ParsedPath and ObjectVisitor, which would certainly be cleaner.

It looks like we would need to define the IAdapterFactory within the JsonPatchDocument and then pass it to the ObjectVisitor through the ObjectAdapter. Is there a cleaner or more direct approach you had in mind? Anything else seems like it is going to introduce breaking changes to the API without a lot of benefit.

Let me know what you are thinking. Thanks!

[HappyNomad]

For IAdapterFactory, I had in mind that a JsonPatchDocument.ApplyTo overload accepts an IAdapterFactory instance, like there's already one that accepts an IObjectAdapter instance. ApplyTo passes it to an ObjectAdapter constructor overload, which passes it to an ObjectVisitor constructor overload. This looks like what you described, and it's without breaking changes.

Earlier today I posted an alternative to the IAdapterFactory idea at #2743 (comment). Let me know how you think it compares. Considering your requirement for a custom path separator, it might be nice to have a class JsonPatchSettings that a JsonPatchDocument.ApplyTo overload accepts. This settings class could have both an Adpaters list property and a PathSeparator property. These changes wouldn't have to be breaking, either.

[jmudavid]

@HappyNomad, I can see injecting the IAdapterFactory directly within the ApplyTo method, rather than at the JsonPatchDocument level. This would make sense to me if there where scenarios where you wanted to apply a different IAdapterFactory to the same JsonPatchDocument instance. I wouldn't expect this to happen often, so putting it at the class level made sense to me. It may be a stylistic approach, so I could go either way. If we are going to add it at the method level, we should make sure it can be supported in all the ApplyTo methods, which to me just adds unnecessary complexity as you would end up with 5 methods. If there isn't a need to use different IAdapterFactory implementations, we can keep it simpler by including it at the class. We could just leverage the existing ApplyTo that accepts the IObjectAdapter, but we would still need to add support for the action logger. I will defer to you on which direction you prefer.

Regarding applying the AspNet formatter pattern to the adapters, I find the way the formatters are handled to be confusing for some and difficult to debug. I have worked with several of our developers to get them to work right. I think the factory is simple, straight forward, and easy for somebody to understand and implement.

I have a basic factory pattern working, let me see if I can do without the PathSeparator updates. If not, we can explore creating JsonPatchSettings following the same approach that we land on for the factory. Thoughts?

[jmudavid]

@HappyNomad, OK, let's leave it in the constructor. I thought you were indicating a preference at the method level. I agree that trying to remain consistent in an existing code base is ideal, but since some objects are passed through the constructor and others the methods, we should be good.

I was able to get all my changes re-implemented using the factory approach. I did update the Adapters to support inheritance as well since it seems like a low-risk, high-value change, although it is in a separate commit if it is decided to do one without the other. I was able to work through the path separators without needing any changes there. While not the most elegant, it keeps the overall solution much cleaner.

We could still use a settings class with only the Factory, but my expectation is that since the other classes are closed, that it may not do folks any good and simply adds another layer. If the assembly grows in the future it may become more useful, but without knowing how likely that is, I don't know if it is worth it. I am going to close the old PR and open a new one with the current factory implementation just so you can take a look. If you want to go the settings route, its just another short step from where we are.

Thanks!

[HappyNomad]

@jmudavid After experimenting with your idea to accept an IAdapterFactory in JsonPatchDocument's constructor and store it in a property on JsonPatchDocument, I've realized that such changes are extraneous to the goal we've agreed upon. We don't need to modify JsonPatchDocument at all. Since we're adding an ObjectAdapter constructor that accepts an IAdapterFactory, we can just call the existing ApplyTo overload that accepts an IObjectAdapter.

Although I can relate to your desire to bake in our custom ApplyTo behavior starting from JsonPatchDocument's constructor, I think that proposal belongs in a separate issue. I also think it should be generalized to work for the existing IObjectAdapter extension point.

[jmudavid]

@HappyNomad ,
I see where you are going. If we take that approach, I would want to add another method ApplyTo(object objectToApplyTo, Action logErrorAction, IObjectAdapter adapter). That should give enough coverage to do everything we need. Sound right to you?

[HappyNomad]

@jmudavid I don't know about "right". I see ApplyTo(object objectToApplyTo, IObjectAdapter adapter, Action logErrorAction ) does complement the existing ApplyTo overloads, which lack that parameter combination. On one hand, it's indirectly related to our agreed upon goal. On the other hand, it's not one of the minimal changes needed to reach our goal. You could create a separate issue and pull request. I don't use logErrorAction. It's your call.