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.

[HappyNomad]

Looks good! A couple of suggestions, though.

[HappyNomad]

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

[jmudavid]

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.

[HappyNomad]

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.

[jmudavid]

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?

[HappyNomad]

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?

[jmudavid]

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.

[HappyNomad]

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.

[HappyNomad]

I suggest a JsonContract instead of a contractResolver.

[HappyNomad]

How about making this readonly (removing setter)?

[jmudavid]

Just preference I guess. I like to have my properties externally set able in case it makes unit testing easier. That being said we don't currently need it, so if you would prefer it to be read only, I can do that. Again, just let me know it is an easy fix. Thanks for the quick feedback.

[HappyNomad]

Yea, I'd prefer that it's read-only. This makes it consistent with the other constructor parameters contractResolver and logErrorAction. Constructor-set properties are generally read-only in my experience.

[HappyNomad]

As for "Add support for inheritance of built in adapters", they're straight-forward changes. Yea, this could be useful in some extension scenarios.

[jmudavid]

@HappyNomad, Updated made based on feedback. Thanks!

[HappyNomad]

Our discussion got me thinking: here I am arguing about contractResolver vs. jsonContract but none of my extension scenarios require either (I just use target is and an object type). So now I have a different suggestion. Let's pass IAdapterFactory.Create neither contractResolver nor jsonContract. Create's target parameter already contains all the information that method needs. As you pointed out, we shouldn't preclude anyone from using a contractResolver. But we're not! For AdapterFactory for example, its constructor can accept contractResolver.

[jmudavid]

You do bring up a good point, although that doesn't really follow the pattern established within the library and it would be possible to have a disconnect between the ContractResolver being used within the AdapterFactory and the rest of the library. I would vote to leave it as is, or go back to passing in the ContractResolver to Create. Nothing says that parameter needs to be used within the AdapterFactory.

[HappyNomad]

I'm not sure what you mean by "doesn't really follow the pattern established within the library and it would be possible to have a disconnect between the ContractResolver being used within the AdapterFactory and the rest of the library". ObjectVisitor stores a reference to _contractResolver, and perhaps that's the case elsewhere in the library, too.

This new suggestion actually follows the same pattern that my app will use with IAdapterFactory. That is, extra data that the factory needs will be passed into the factory's constructor.

[jmudavid]

I simply mean that all the downstream classes (ObjectAdapter, ObjectVisitor, etc.) use the same ContractResolver that was initially passed into the JsonPatchDocument. Having the AdapterFactory get its own instance out of that flow could cause a disconnect. If the AdapterFactory has its own instance, then you could pass in a different ContractResolver to the JsonPatchDocument (and the classes it creates) than the one that the AdapterFactory is using. I haven't through through the impacts of using a different resolver in the multiple areas. This could certainly be an issue where the developer needs to do it "right" it just seems like we could insulate them a little bit from accidentally doing something wrong. To your point, one could still pass in anything they want to a custom AdapterFactory, really we are arguing about the default implementation which the code should be able to make work however we want.

[HappyNomad]

I still don't follow. JsonPatchDocument.ApplyTo passes its top-level contract resolver to ObjectAdapter's constructor, which passes the same to ObjectVisitor's constructor, which would pass the same to AdapterFactory's constructor.

[jmudavid]

I don't want to go the internal route. I am actually overriding the base AdapterFactory in my implementation so I want to be able to instantiate it. Let's pass the ContractResolver into the AdapterFactory as a constructor parameter. The source code is available for a reason if somebody wants to know what is going on. This still doesn't introduce any breaking changes and does keep things flexible. I will work on that update tomorrow am and let you know when its ready for a review. It shouldn't take long to make the change.

[HappyNomad]

Sounds good, thanks. BTW, you could mark it as protected internal in that case.

[HappyNomad]

Now that I understand you want to derive from AdapterFactory and pass your subclass into JsonPatchDocument's constructor, I see how the AdapterFactory constructor requiring a contractResolver would be slightly inconvenient. You'd have to first instantiate DefaultContractResolver to pass to both your factory subclass's constructor and JsonPatchDocument's constructor. Like you said, this also raises the possibility that two different contract resolvers could get involved. In light of this, I defer to you on this one. Take any of the routes we discussed, including passing contractResolver or JsonContract into IAdapterFactory.Create if you decide that's best.

[jmudavid]

Ok, I am going to rollback to passing the contractResolver in IAdapterFactory.Create as I think it provides a good balance in flexibility while allowing the creation of the AdapterFactory to remain simple. I will let you know when the code as been pushed.

[jmudavid]

Updates made. I think we should be good to go.

[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: dotnet/aspnetcore#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]

Thanks @mkArtakMSFT, we appreciate it. Note that as I explained in dotnet/aspnetcore#2432 and dotnet/aspnetcore#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.

[rynowak]

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

[jmudavid]

@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.

[rynowak]

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

[jmudavid]

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.

[rynowak]

@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 😆

[HappyNomad]

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) .

[HappyNomad]

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.

[rynowak]

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

[jmudavid]

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

[rynowak]

Formatting, please put the : this... and everything after on a separate line (3-4 cases of this in the PR)

[HappyNomad]

Please exclude changes to this file, for the reasons I mentioned earlier.

[rynowak]

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

[rynowak]

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

[rynowak]

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

[jmudavid]

@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 ;)

[rynowak]

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.

[jmudavid]

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.

[rynowak]

Ok great, I'm convinced.

[HappyNomad]

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.

[HappyNomad]

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.

[rynowak]

I think this looks overall great. Thanks for bearing with us, I know we've put this off for a long time.

[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.

[rynowak]

If these are top-level classes, they should each have their own file

[rynowak]

@jmudavid - I think this is just about ready to go, but there are a few minor things to resolve:

• top-level classes should have their own files (in tests)
• add tests for AdapterFactory
• fix formatting of public JsonPatchDocument():this
• add argument validation to new public/protected methods

If you can't find time to get to these some time in the next few days then I'll do it and merge after.

[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]

Please exclude changes to this file, for the reasons I mentioned earlier.

[HappyNomad]

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

[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 #132 (comment) .

[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 dotnet/aspnetcore#2816 (comment). 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 dotnet/aspnetcore#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.
#135

I will close this PR.