Skip to content
This repository has been archived by the owner. It is now read-only.

Adding/Removing from a collection by an index value #85

Closed
gilmishal opened this Issue Jun 5, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@gilmishal
Copy link

commented Jun 5, 2017

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.

@Eilon

This comment has been minimized.

Copy link
Member

commented Aug 28, 2017

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.

@ChainReactive

This comment has been minimized.

Copy link

commented Sep 7, 2017

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

@ChainReactive

This comment has been minimized.

Copy link

commented Sep 9, 2017

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?

@aspnet-hello

This comment has been minimized.

Copy link

commented Jan 1, 2018

This issue was moved to aspnet/AspNetCore#2432

@aspnet aspnet locked and limited conversation to collaborators Jan 1, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.