Adding/Removing from a collection by an index value

[aspnet-hello]

From @gilmishal on Monday, June 5, 2017 12:34:10 PM

I need to be able to add/remove object from a subcollection of my item - imagine the following json patch string
{"op":"remove", "path":"/a/b/123"}]
or
{"op":"add","path":"/a/b", "value":"{"a":"b"}"

where 123 is my database Index, and not a list position (which my client has no actual way of knowing about)

As of now, I see no way of actually implementing this (maybe I missed something, but I went through the code of both ListAdapter, and DictionaryAdapter).

If I use a Dictionary as the model, the remove operation will work, but the add will not (I could have the client send fake ids, but that would be really odd)

If I use a List as the model, well I won't be able to remove by a database Index (I could order the values in the exact same way the client does, but again - this is odd).

Implementing IObjectAdapter myself isn't a good option either - besides the fact that it isn't generic, and won't allow me to use multiple IObjectAdapters via DI - it takes a lot of code to actually implement properly, and will basically be copying your code with minor changes.

I don't expect it to be able to actually search a list by an Id property, but I do think this entire package should be more configurable to actually allow me to define how to search a list with a given index, without copy pasting your code and applying basic changes.

Copied from original issue: aspnet/JsonPatch#85

[aspnet-hello]

From @Eilon on Monday, August 28, 2017 1:58:28 PM

This doesn't seem like something that JSON PATCH is well-suited for. Dictionaries have keys, and lists have indexes, but the reverse is not true. It seems like in this case you want to use a list so that you can get ordinal semantics.

[aspnet-hello]

From @HappyNomad on Thursday, September 7, 2017 9:47:49 AM

@gilmishal I need this for my projects, and I'm working on my own solution now.

Implementing IObjectAdapter myself isn't a good option either ... it takes a lot of code to actually implement properly, and will basically be copying your code with minor changes.

Although not ideal, I must take this approach since the issue would otherwise be blocking me.

I can't sit around waiting, but I do have an initial suggestion for introducing a little more fine-grained extensibility into the library. The ObjectVisitor class has a private SelectAdapater (sic) method. I suggest moving this method into a factory object, correcting its spelling, and making it public. This base factory class then has virtual methods for returning each IAdapter implementation: CreateListAdapter, CreateDictionaryAdapter, CreateDynamicObjectAdapter, and CreatePocoAdapter. We as library users could then derive from this factory class, override the methods of interest, and then pass an instance into JsonPatchDocument.ApplyTo.

Alternatively, instead of the base factory class, it could be an interface. The SelectAdapter method would, in that case, stay in ObjectVisitor.

Come to think of it, are there really any scenarios for customizing the ObjectAdapter itself rather than the IAdapter implementations? I think it's worth considering replacing the IObjectAdapter extension point with an IAdapter factory one.

[aspnet-hello]

From @HappyNomad on Saturday, September 9, 2017 2:14:08 PM

The central issue is that we're dealing with entities rather than JSON objects. I've described my solution at json-patch/json-patch2#20. I hope that my suggestion there can become an RFC. In the meantime, and in case that doesn't happen, we at least @Eilon need better extensibility. How are other people using this library? I and I assume @gilmishal are using it with EF Core, which is also part of ASP.NET Core. Allowing these two libraries to be used together should be somewhat of a priority, shouldn't it?

[mkArtakMSFT]

We have no plans to build support outside of JSON-Patch specification.

[HappyNomad]

@mkArtakMSFT This issue is not for "building support outside of JSON-Patch specification". Rather, it is for improving the means of extending it. Do you want to make it as difficult as possible to do anything outside of the JSON Patch specification? In that case, you should altogether eliminate IObjectAdapter without replacing it. If making our lives unnecessarily difficult is not your goal, however, then please reopen this issue.

[Eilon]

@HappyNomad understood, but the main goal of this repo is to support the JSON PATCH spec, though it does include some limited extensibility for at least its own internal needs (such as targeting a few different data models, such as dictionaries, dynamic types, and reflection). We have no plans to add extensibility beyond that scope, however.

[HappyNomad]

@Eilon The main goal of this repro is to support the JSON Patch spec. That is the case and let that continue to be the case. That is not what this issue is about.

I am not requesting additional extensibility. On the contrary, I suggest scaling back the amount of possible extensibility to what is actually useful.

On September 7, 2017 I wrote:

are there really any scenarios for customizing the ObjectAdapter itself rather than the IAdapter implementations? I think it's worth considering replacing the IObjectAdapter extension point with an IAdapter factory one.

Look at the source code. Your explanation that "it does include some limited extensibility for at least its own internal needs" doesn't match what's there. Internally, there's no need to customize ObjectAdapter. The public extensibility point is currently IObjectAdapter. Implementing it requires copying all the "internals" to which you referred, plus a whole lot more.

What we have now is a sloppy extensibility mechanism since it requires copying and pasting a large amount of code in order to make the desired changes. I had to copy ClosedGenericMatcher.cs, ConversionResultProvider.cs, ErrorReporter.cs, IAdapter.cs, ListAdapter.cs, ObjectAdapter.cs, ObjectVisitor.cs, PocoAdapter.cs, and Resources.Designer.cs just to customize a few lines of code! Do you want to fix this situation, or keep it sloppy? If you want to fix it, then please reopen this issue.

[HappyNomad]

Since this issue wasn't repurposed, I filed a new one at #2743. I've also edited in a note that the existing extensibility mechanism, and the improvement I propose for it, is also useful when NOT diverging from the spec. The spec 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.

Again, I'm not requesting more extensibility than already exists with IObjectAdapter. Quite the opposite!

[Eilon]

@HappyNomad sorry for the delay, I've been out sick the last several days. We can continue the discussion in the new bug you filed.