Incorrect move operation behavior

[TFleury]

When performing a move operation, target object is not moved, but it is removed from source and a copy is added to the destination.
There is no problem with value types, but there is one with reference types : reference was lost.

To handle move operation, ObjectAdapter gets the moved property, removes it from the source, then adds it to the destination. That's OK.

The issue is in ListAdapter, PocoAdapter and ExpandoAdapter (don't know why, but not in DictionaryAdapter), in TryConvertValue method where the value is copied regardless of its type.

If object's type is compatible with target property's type, the reference should be kept.

[kichalla]

@TFleury Could you share a repro where you are seeing this issue?

[TFleury]

I'd want to patch a Template object, which have a property of type IList<Theme>. Themes in the list are ordered.

Here is a simplified version of my classes :

public class Template {
    private TrackedList<Theme> _theme = new TrackedList<Theme>();
    public string Label { get; set; }
    public IList<Theme> Themes {
        retrun _themes;
    }
}

public class Theme {
    private TrackedList<Theme> _theme = new TrackedList<Theme>();
    public string Label { get; set; }
    public IList<Theme> Themes {
        retrun _themes;
    }
}

Here is a sample patch :

[
{op:"move",from:"/themes/3",path:"/themes/1"},
{op:"move",from:"/themes/4/themes/1",path:"/themes/2"}
]

TrackedList is a custom form of List to track items order, and wanted to use JsonPatch's "move" operation to reorder items in the list.

The issue is that moved item is copied, removed, and then the copy is inserted at the destination position : the reference is lost.
As a Theme can have child Themes, moved one and its children are removed, and re-created.

It's backed in a database with EntityFramework, and I have to keep reference of objects to know which entry is new, removed or updated.

[TFleury]

@kichalla can you confirm this is an issue and not the expected behavior ?
I pushed a PR to fix it, but just realized it will break 'copy' operation : object must be deep copied.

[TFleury]

I fixed the copy issue in my PR, and added matching unit tests.

[Eilon]

@kichalla can you confirm you agree with the issue?

[kichalla]

@Eilon and I discussed about this and agree that this is an issue. Changing the label to bug.