Updated to support Adapter Extension

[jmudavid]

• Added IAdapterFactory and base implementation
• Updated built in adapters to support inheritance

@HappyNomad, @Eilon, Here is the new PR with the initial factory model implementation for your review. Let me know what you think, Thanks!

[dnfclas]

All CLA requirements met.

[jmudavid]

@HappyNomad, Updated made based on feedback. Thanks!

[jmudavid]

@Eilon @HappyNomad, I am hoping one of you can help sanity check this. I added some files that I need for our local copy of the fork (until this get merged) to the wrong branch this morning which resulted in them being pulled into this PR. I corrected that quickly, but the CI build is failing now. Based on the changes I made, the errors in the CI log, and that there was another change made yesterday that seems to be around CI, I don't think my changes broke the build, but there is a problem with the configuration. Do either of you have the insight to confirm that or point me in the right direction to get it fixed if I did something?

Thanks, Dave

[HappyNomad]

@jmudavid Wish I could help, but I just don't know. Let us know if you figure it out.

[HappyNomad]

@jmudavid Although we want to minimize code additions, I find the following usage too verbose:

new JsonPatchDocument( new List<Operation>(), new DefaultContractResolver(), new IdAsIndexAdapterFactory<EntityTargetFormat>( created ) );

Not only must I include the boilerplate new List<Operation>(), new DefaultContractResolver(), but I also must add two using statements.

What do you think about adding a JsonPatchDocument constructor that accepts only an IAdapterFactory, and sets the usual values for the rest?

public JsonPatchDocument( IAdapterFactory adapterFactory )
    : this( new List<Operation>(), new DefaultContractResolver(), adapterFactory ) { }

[jmudavid]

@Eilon, Looks like the builds are still broken, and it isn't just on JsonPatch, as I saw the same issue in CORS (no commits from me). I entered a new issue, but I don't have the ability to tag is to try and get any visibility. Can you either tell me how to tag (if I can) otherwise tag is to try and raise some visibility? Here is the issue: https://github.com/aspnet/Home/issues/2946

On a side note, once we get his finished, and we finalize the changes in the PR, what are the next steps for considering the change? Thanks!

[jmudavid]

@HappyNomad, Adding new constructors is an easy fix, but we have to draw the line someplace. At this point I want to get his PR fixed and closed, then additional refinement can be made once the main functionality is available.

[Eilon]

@mkArtakMSFT can someone on your team evaluate this proposal and maybe also take a look at the test issues?

[HappyNomad]

@jmudavid Have you tried configuring deserialization to pass IAdapterFactory into JsonPatchDocument's constructor yet? First you need to defined your own contract resolver and converter:

class PatchContractResolver : DefaultContractResolver
{
    public PatchContractResolver( List<Entity> created ) => this.created = created;
    readonly List<Entity> created;

    protected override JsonConverter ResolveContractConverter( Type objectType ) =>
        objectType == typeof( JsonPatchDocument ) ? new PatchConverter( created ) : base.ResolveContractConverter( objectType );
}

class PatchConverter : JsonPatchDocumentConverter
{
    public PatchConverter( List<Entity> created ) => this.created = created;
    readonly List<Entity> created;

    protected override JsonPatchDocument CreateContainer( List<Operation> operations ) =>
        new JsonPatchDocument( operations, new DefaultContractResolver(), new IdAsIndexAdapterFactory<EntityTargetFormat>( created ) );
}

And then you can deserialize with the settings:

new JsonSerializerSettings { ContractResolver = new PatchContractResolver( created ) }

This verbosity is necessary since we decided to have JsonPatchDocument's constructor accept IAdapterFactory instead of its ApplyTo method. I actually don't mind, since I'll tuck it away in one place in my EntityJsonPatch library. To make this possible, though, the pull request should include modification of JsonPatchDocumentConverter such that the following method is added:

protected virtual JsonPatchDocument CreateContainer( List<Operation> operations ) =>
    new JsonPatchDocument( operations, new DefaultContractResolver() );

And then replace the JsonPatchDocument constructor call with a call to this method.

[HappyNomad]

There's also a simpler way to deserialize and pass in IAdapterFactory:

return new JsonPatchDocument(
    JsonConvert.DeserializeObject<List<Operation>>( await response.Content.ReadAsStringAsync() ),
    new DefaultContractResolver(),
    new IdAsIndexAdapterFactory<EntityTargetFormat>( created )
);

[jmudavid]

@HappyNomad, In my scenario I actually use a custom version of the JsonPatchDocument to support SCIM, so my custom object was able to set the IAdapterFactory rather than relying on serialization. Based on your second comment, it looks like you have a way of moving forward, but maybe not.

I see what you are saying about the JsonPatchDocumentConverter. I will need to play around with it a little to make sure I properly understand how it is leveraged so I can test it properly.

[jmudavid]

@HappyNomad, Ok both Converters have been updated so they can be overridden. I added a test to cover this scenario as well.

[HappyNomad]

@jmudavid Looks good. For deserializing and passing in IAdapterFactory on the client, I'm currently taking the less verbose approach. Next up, I have to configure the deserialization in Asp.Net Core. I might be able to get by with the simpler approach, but it's a relief to know that configuration via JsonSerializationSettings will be possible if that looks to be the better architecture. I imagine that some people will strongly prefer configuration via JsonSerializationSettings, anyway.

[HappyNomad]

@jmudavid Now I'm working on deserialization on the server. As for calling the JsonPatchDocument constructor that accepts an IAdapterFactory, I decided against doing it via JsonSerializerSettings for three reasons:

1. It precludes me from using the SharedContractResolver.
2. It's too much code for what should be a simple deserialization scenario.
3. The parameters I need to pass to my adapter factory aren't in scope until it's time to apply.

A custom input formatter is also excluded as an option for the second and third reasons. The easiest approach is for my controller action to accept a List<Operation> operations instead of a JsonPatchDocument delta. After the parameter I need to pass to my adapter factory are in scope, I wrap the operations in a JsonPatchDocument and apply them.

Seeing our decision to pass IAdapterFactory to the constructor play out here, perhaps we were wrong. Although we don't want a JsonPatchDocument instance that's configured with the default, wrong (for each of our scenarios) ApplyTo behavior, scenarios like mine require supplying ApplyTo-time parameters, probably via the adapter factory constructor. I'm glad we tried our different way, but looks like being consistent with how the current release's ApplyTo accepts ObjectAdapter is better after all.

[jmudavid]

@HappyNomad, Because the AdapterFactory is read/write on the JsonPatchDocument, why not just change it before calling ApplyTo. This seems like an easy enough update to meet your particular need where the IAdapterFactory is dependent or parameters passed in. I think what we have in place makes it easy to cover a majority of scenarios, and is flexible enough to meet your scenario without having to make any changes.

[jmudavid]

@mkArtakMSFT, I just wanted to check in to see if there was anything you or your team needed from me to review this. Thanks!

[HappyNomad]

@jmudavid Yea, setting the AdapterFactory property would work. I could also set it to null right after it's deserialized, to indicate no ApplyTo behavior with any ApplyTo call failing. BTW you forgot to null-check (with ?? throw new ArgumentNullException) the adapterFactory on its way from ApplyTo to SelectAdapter. Consider adding those checks else a NullReferenceException may be thrown.

I don't think that storing the adapter factory at JsonPatchDocument's class level is more flexible. Class level (with an AdapterFactory public setter) and ApplyTo method level are equally flexible. ApplyTo method level has two advantages:

1. In ApplyTo-time parameter scenarios like mine, calling ApplyTo requires one line of code instead of two.
2. The pull request wouldn't have to touch JsonPatchDocument or JsonPatchDocument<T>. Instead, call the ApplyTo overload that accepts an IObjectAdapter. Perhaps a smaller, more focused pull request stands a better chance?

I relate to your desire to build in the correct (for each of our scenarios) ApplyTo behavior starting from JsonPatchDocument's constructor. IObjectAdapter is the existing means of customizing ApplyTo behavior. Why limit class level customization to IAdapters? We could extend it to the more general IObjectAdapter. Rather than a constructor that accepts IAdapterFactory, consider one that accepts an IObjectAdapter factory or just an IObjectAdapter instance. It's stored in a property on JsonPatchDocument and used in the appropriate ApplyTo overloads. I think it should be a separate issue and pull request.

[jmudavid]

@HappyNomad At this point I want to minimize the changes needed to complete this PR until we can get it merged and out of a branch. I believe we have a working solution for both scenarios even though it could still be further refined.

[HappyNomad]

@jmudavid I'm not suggesting we further refine it. I'm suggesting we roll back some unnecessary parts including all modifications of JsonPatchDocument, JsonPatchDocument<T>, and JsonPatchDocumentConverter. Those changes go beyond what's necessary to fulfill the pull request's purpose. Also, the JsonPatchDocument constructor changes don't take the existing, more general extension mechanism into account.

[HappyNomad]

@mkArtakMSFT We know you're busy with other Asp.Net Core priorities, but we'd appreciate some attention paid to JSON Patch. It's been six months since I first commented about this issue and offered the solution implemented by this pull request. And it's been over two months since I provided a full clarification in https://github.com/aspnet/Home/issues/2743.

The solution only requires minor changes. I even got @jmudavid on board with a much smaller pull request than he originally proposed in https://github.com/aspnet/Home/issues/2816. A minor point of contention remains in this vein, but I'm sure @jmudavid would remove the remaining extraneous code if you express a preference for it. Otherwise, I've been willing from the beginning to submit a pull request - I was just waiting for your team to express interest.

[jmudavid]

@HappyNomad, @Eilon, I was going to start a fresh PR with the new approach, but after a fresh pull of dev, I can't run any unit tests within JsonPatch locally. I get the error:

Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Testhost process exited with error: It was not possible to find any compatible framework version
The specified framework 'Microsoft.NETCore.App', version '2.1.0-preview2-26130-04' was not found.

This message is eerily similar to the issue we are seeing in the CI process. I got the latest preview release of VS, the .NET core SDK 2.1.101, and updated my xunit and test nugets to the latest and get the same error. I can't find the framework that the project seems to be looking for. This is a fresh update from dev, with none of my changes in it. Until I get get a good baseline, I don't want to try adding my code since I can't ensure everything is functional. Are you guys seeing the same behavior?

Thanks!

[jmudavid]

@HappyNomad, @Eilon,

I don't see anyway forward with this at this point. At this point unless somebody has another idea, I am inclined to close out this PR and try again after 2.10 is officially released. @HappyNomad if you can get the current JsonPatch code running on your machine, feel free to pick the Factory pieces out of my fork and make the updates to pass the factory in through the ApplyTo method. Maybe you will have better luck getting it running than I will.

[mkArtakMSFT]

Hi @HappyNomad , @jmudavid. Sorry for my delayed response here. It seems to me that there are some design changes in this PR, which aren't really aligned with the direction we want JsonPatch to take - at least at the moment. I really appreciate all your effort to make those changes, but I would suggest to just fork this repo and have those changes in your forked version of it. The comment regarding us not making many changes in this area was correct. We will reevvaluate that after 2.1 release, so things may change in the future.

Before then, as I mentioned, the best option would really to have a forked version with your proposed changes.

Also, we would happily reference your forked version in the Readme file, if you wish - as another community-maintained version of JsonPatch.

[HappyNomad]

It seems to me that there are some design changes in this PR, which aren't really aligned with the direction we want JsonPatch to take - at least at the moment. The comment regarding us not making many changes in this area was correct.

@mkArtakMSFT I believe you are referring to my comments. I'm glad you agree, but please distinguish between the unwanted changes and introducing an IAdapterFactory interface to address https://github.com/aspnet/Home/issues/2743. The latter is a small change and not a "design change". It just makes the implementation of the existing ObjectVisitor.SelectAdapter method pluggable. In doing so, we add one constructor each to ObjectVisitor and ObjectAdapter that accepts and stores an IAdapterFactory instance.

As mentioned, for the past six months I've been willing to submit a PR. I've just been waiting for a sign of interest from you. What are your interests? You mention "the direction we want JsonPatch to take". What, specifically, does that refer to? I'm noticing a current lack of interest in JsonPatch, rather than taking it in any direction at all. Please be frank with us. Our time is valuable, too.

[mkArtakMSFT]

Hi @HappyNomad. You're right, I've looked through the comments mostly. Given that only those changes are what you propose I'll ask one of our engineers who is familiar with this codebase to review it. So far we've been trying to stick with the JsonPatch specification. Whether we'll do that going forward or not depends also on the 2.1 feedback, as we had some changes in that area.

[mkArtakMSFT]

@rynowak, can you please review this? Thanks!

[HappyNomad]

Thanks @mkArtakMSFT, we appreciate it. Note that as I explained in https://github.com/aspnet/Home/issues/2432 and https://github.com/aspnet/Home/issues/2743,

1. I don't suggest diverging from the JsonPatch spec.
2. I don't request additional extensibility.

Regarding point 1, by using the proposed IAdapterFactory I've already extended ApplyTo to work with MongoDB's BsonDocument and BsonArray. In doing so, I haven't diverged from the spec which says,

JSON Patch is a format ... for expressing a sequence of operations to apply to a target JSON document...

This format is also potentially useful in other cases in which it is necessary to make partial updates to a JSON document or to a data structure that has similar constraints (i.e., they can be serialized as an object or an array using the JSON grammar).

The spec doesn't mention .NET's System.Collections.IList, or even the object-oriented data model. We decide how to apply the patch to other data models. I'm suggesting that in some cases the library's user needs that control.

Regarding point 2, IObjectAdapter is the current extension point. Using it results in a sloppy mess of duplicating library code. I request the ability to extend the ApplyTo behavior without having to perform a brain transplant, as is the case now.

[HappyNomad]

@rynowak Glad you can look at this now. I think most of your questions can be answered by reading comments on the issue and pull request. Sorry, I don't have time to look at all of it again right now.

[jmudavid]

@rynowak, I hadn't started on the remaining items since I didn't disagree with your feedback but wanted to make sure we got the other areas worked out. I will try and get these updates made this week, although things are a little busy around here.

[HappyNomad]

Guys, even more important than the clean up you're discussing, is to talk about why those extraneous changes are being made to JsonPatchDocument and JsonPatchDocument<T>. They're contrary to the existing API and completely unnecessary. Please see my comment at https://github.com/aspnet/JsonPatch/pull/132#discussion_r187109181 .

[HappyNomad]

@jmudavid You're deriving from JsonPatchDocument, right? I think that's the main difference between our scenarios. Here's a suggestion for a much more minor change to JsonPatchDocument that would fulfill your needs without messing with the original API. Just make the ApplyTo overloads that don't accept an IObjectAdapter into virtual methods. Then in your own derived class (not the library's or mine) you can have a constructor that accepts whatever you want, and you can override ApplyTo to use that data however you want. I could live with just some virtual keywords being added to JsonPatchDocument, and you could give your subclass the behavior you want and same public API. What do you think?

[jmudavid]

@HappyNomad, I know you want to pass in the factory at the ApplyTo method, and unless I am mistaken, you can do that in the current code. You may be right that all we need is the ApplyTo, however I don't see the problem with being able to provide it in the Constructor, so the user doesn't have to create a custom ObjectAdapter. I don't believe that the current approach is contrary to the existing design as the ContractResolver can also be applied globally or directly to the ObjectAdapter. None of the changes made trigger a breaking change to the API, so I don't see the harm in allowing the Adapter to be set at either location depending on the users need.

If @rynowak feels this change should be made, then so be it, he is the gatekeeper.

[HappyNomad]

@jmudavid `

You may be right that all we need is the ApplyTo, however I don't see the problem with being able to provide it in the Constructor, so the user doesn't have to create a custom ObjectAdapter

The problem is that the "it" in your sentence is the specific IAdapterFactory rather than the general ObjectAdapter. Your analogy doesn't fit. Say someone has a custom ObjectAdapter and wants to bake it into a JsonPatchDocument instance. He can't do it with your proposed changes. The harm is in polluting the API. Typing new ObjectAdapter(..., in contrast, isn't difficult and it's what users are already doing.

[jmudavid]

An AdapterFactory implementation is not required in the JsonPatchDocument constructor. So, in your scenario, the default would be applied, but the one provided in the custom ObjectAdapter would be used. Unnecessary, possibly, but in the scenario that I have where I can set it once and reuse apply, it makes the API easier to use. I am not using a custom ObjectAdapter (now that the factory is there).

[rynowak]

I don't have major concerns with any of the suggestions being discussed here. I also don't feel like I don't know 100% of every problem that's being solved beyond "we want to extend the adapter factory" - but I also haven't been a participant in the discussion.

But that's OK, I'm here to try and make sure you both will be happy with the outcome once we complete this work.

We could go one of two ways to resolve this:

• you talk me through what exactly you want to accomplish (with code samples) OR
• (preferred) you both get to a consensus on what you like

If you need an absolute tiebreaker, I'd always lean towards the change that exposes the least new surface area 😆

[HappyNomad]

I'd always lean towards the change that exposes the least new surface area

This sounds similar to what I advocated in this context since I wrote https://github.com/aspnet/Home/issues/2816#issuecomment-370821225. I said:

But perhaps we could start with a minor tweak, see how it goes, and then expand upon what we've learned from there.

I don't have much more to say about it.

[jmudavid]

As I understand it, the current solution works well for both of us, even if it provides some capabilities that we both don't need. While I agree that minimizing the changes is ideal, I don't think there is a good reason not to give developers more flexibility one how to extend and use the API.

That being said, I am willing to perform the clean up tasks requested so we can get this merged so I don't have to maintain a fork. I am sure we all have other priorities we need to get to so continuing the philosophical debate over this isn't a wise use of anybodies time. If @HappyNomad feels strongly enough in his approach, I would suggest he create a new fork, he can cherry pick the changes he needs, and can submit a new PR and we can just close this one out.

[rynowak]

As I understand it, the current solution works well for both of us, even if it provides some capabilities that we both don't need.

Than can you just undo the changes that you don't need?

While I agree that minimizing the changes is ideal, I don't think there is a good reason not to give developers more flexibility one how to extend and use the API.

Let me explain my point of view... My goal is to limit the extensibility and flexibility of the API to set of reasonable scenarios that work well and we can maintain and support over a period of years. I don't want extras without a clear purpose attached to them.

I'd be really happy to help you, @HappyNomad or anyone else in the future accomplish reasonable things that are withing the scope of this project - but this always comes with understanding what those requirements are as a first step.

[jmudavid]

@rynowak, First off, thank you for taking some time to look at this PR with us. Trying to get this changes integrated has taken multiple months, so I am sorry if some frustration in the process is showing. Undoing changes is easy enough, but I think the discussion comes down to how a custom AdapterFactory can be applied to the JsonPatchDocument. My implementation uses the same AdapterFactory, so I have implemented it within the Constructor to simplify the usage of the API later. @HappyNomad had to provide the AdapterFactory during the call to ApplyTo due to the dynamic nature of his needs. So I can rollback, it just requires updates on how we are implement JsonPatch.

I would rather not make additional changes, simply because the code has been refactored multiple times (many as a result of feedback from @HappyNomad that I agreed with and provided a better, easier API). I don't see the current implementation increasing surface area or making it more difficult to maintain.

All that being said, I (as I am sure we all do) have many balls in the air and getting this mainlined is more valuable to me than having it my way. So rather than continuing to argue about what I feel is a very minor implementation detail, I will just make the change so that we can get it mainlined rather than having this drag on for another few months. I will close this PR when the update is ready so that we can have a cleaner commit history.

[jmudavid]

@rynowak, I now remember the other reason I don't want to make code changes. I can't get the JsonPatch tests to run on my machine anymore. I was trying to troubleshoot this previously https://github.com/aspnet/Home/issues/2946 and never got anyplace.

My errors have progressed to the new SDK:

Testhost process exited with error: It was not possible to find any compatible framework version The specified framework 'Microsoft.NETCore.App', version '2.2.0-preview1-26502-01' was not found.

I can compile both projects fine, I just can't run any tests. I have tried the troubleshooting from the original issue, but it is attempting (and failing) to load an older version of the SDK (Using KoreBuild 2.2.0-preview1-17051). I pulled a fresh copy of the repo from https://github.com/aspnet/JsonPatch.git and I get the same thing. I have updated VS 2017 to the latest preview, and no luck.

I can only assume something is messed up within my environment, but reinstalling everything isn't really an good option right now, so unless I know where to look/clean I am not sure what to do. Do you have any advice/suggestions? Thanks.

[rynowak]

do a dotnet --info at the command line. The version of dotnet on the path first should be the one we put in C:\Users\<username>\.dotnet

Some more details: https://github.com/aspnet/Home/wiki/Building-from-source

[jmudavid]

Thank you for that link. It looks like I may not be able to use the Test Explorer at this point, and need to use the build.cmd file, is that right? (It at least appears that way on my machine.) That is fine if that is the case, but I know I was using the test explorer previously, so maybe there was a change made along the way, entirely possible. If using the build.cmd is the right way to test, I will go that way. Thanks!

[rynowak]

Yeah, something wierd is going on with test explorer. I'm not sure exactly what is the problem, most folks here are focused on the 2.1 RTM which is why it hasn't gotten any attention yet.

[jmudavid]

@rynowak @HappyNomad, New PR available for your review which passes directly to the ObjectAdapter. https://github.com/aspnet/JsonPatch/pull/135

I will close this PR.