Move and Remove don't seem to work with MariaDB, SQLAlchemy on Flask

[Torniojaws]

All the other ops work just fine, but move and remove don't seem to delete anything? Is there some limitation with for example MariaDB and/or SQLAlchemy? I am using the PyMySQL driver.

Update: See my third post in this issue with a more valid case in point. In that, the move/remove fails even when the DB column allows NULL values.

self = <test_views.TestNewsView testMethod=test_patching_things_using_remove>

    def test_patching_things_using_remove(self):
        response = self.app.patch(
            "/api/1.0/news/{}".format(int(self.news_ids[0])),
            data=json.dumps(
                [{
                    "op": "remove",
                    "path": "/author"
                }]
            ),
            content_type="application/json"
        )

        news = News.query.get_or_404(self.news_ids[0])

        self.assertEquals(200, response.status_code)
>       self.assertEquals(None, news.Author)
E       AssertionError: None != 'UnitTest Author'

The implementation:

PATCH /news/123

    def patch(self, news_id):
        """Change an existing News item partially using an instruction-based JSON, as defined by:
        https://tools.ietf.org/html/rfc6902
        """
        news_item = News.query.get_or_404(news_id)
        result = None
        try:
            self.patch_item(news_item, request.get_json())
            db.session.commit()

            news_item = News.query.get_or_404(news_id)
            result = news_item.asdict()
        except JsonPatchTestFailed:
            # This is raised when using "op": "test" and the compared values are not identical
            result = {"success": False, "result": "Existing value is not identical to tested one"}

        return make_response(jsonify(result), 200)

The method:

    def patch_item(self, news, patchdata, **kwargs):
        # Map the values to DB column names
        mapped_patchdata = []
        for p in patchdata:
            # Replace eg. /title with /Title
            p = self.patch_mapping(p)
            mapped_patchdata.append(p)
        # What I do above is to convert:
        # This: [{"op": "remove", "path": "/title"}]
        # Into: [{"op": "remove", "path": "/Title"}]

        data = news.asdict(exclude_pk=True, **kwargs)
        patch = JsonPatch(mapped_patchdata)
        data = patch.apply(data)
        news.fromdict(data)

The mapping:

    def patch_mapping(self, patch):
        """This is used to map a patch "path" or "from" to a custom value.
        Useful for when the patch path/from is not the same as the DB column name.

        Eg.
        PATCH /news/123
        [{ "op": "move", "from": "/title", "path": "/author" }]

        If the News column is "Title", having "/title" would fail to patch because the case does
        not match. So the mapping converts this:
            { "op": "move", "from": "/title", "path": "/author" }
        To this:
            { "op": "move", "from": "/Title", "path": "/Author" }
        """
        mapping = {
            "/title": "/Title",
            "/contents": "/Contents",
            "/author": "/Author"
        }

        mutable = deepcopy(patch)
        for prop in patch:
            if prop == "path" or prop == "from":
                mutable[prop] = mapping.get(patch[prop], None)
        return mutable

As mentioned, all other cases work just fine and the DB values are patched correctly. But with move and remove it seems nothing happens. I don't have any foreign keys or anything in the DB for the elements which fail.

Here's the fail for move. What is interesting is that the value is moved correctly to /author, as it passes the assert. In the beginning of the test, the value of /author is UnitTest Author. It's just the deletion of /contents that fails.

self = <test_views.TestNewsView testMethod=test_patching_things_using_move>

    def test_patching_things_using_move(self):
        response = self.app.patch(
            "/api/1.0/news/{}".format(int(self.news_ids[0])),
            data=json.dumps(
                [{
                    "op": "move",
                    "from": "/contents",
                    "path": "/author"
                }]
            ),
            content_type="application/json"
        )

        news = News.query.get_or_404(self.news_ids[0])

        self.assertEquals(200, response.status_code)
        self.assertEquals("UnitTest Contents", news.Author)
>       self.assertEquals(None, news.Contents)
E       AssertionError: None != 'UnitTest Contents'

tests/news/test_views.py:202: AssertionError

The DB:

[Torniojaws]

Bleeehhh, don't code at 3:30 AM :D

The column is nullable=False AKA NOT NULL ...and move and remove would make it NULL :)

[Torniojaws]

Hmm, actually it also happens with a column that allows NULL values (Updated):

self = <test_views.TestNewsView testMethod=test_patching_things_using_move>

    def test_patching_things_using_move(self):
         response = self.app.patch(
            "/api/1.0/news/{}".format(int(self.news_ids[0])),
            data=json.dumps(
                [{
                    "op": "move",
                    "from": "/updated",
                    "path": "/author"
                }]
            ),
            content_type="application/json"
        )

        news = News.query.get_or_404(self.news_ids[0])

        self.assertEquals(200, response.status_code)
        self.assertNotEquals("UnitTest Author", news.Author)
>       self.assertEquals(None, news.Updated)
E       AssertionError: None != datetime.datetime(2017, 9, 9, 0, 56, 18)

I have added /update to the mapping.

[stefankoegl]

python-json-patch does not - in any way - deal with databases. Can you create a self-contained example that reproduces the error (and does not depend on a database)?

[Torniojaws]

I also verified that the None does work when I use SQLAlchemy directly:

        news.Updated = None
        db.session.add(news)
        db.session.commit()

        news = News.query.get_or_404(self.news_ids[0])
        self.assertEquals(None, news.Updated)
        # This passes

Edit: Saw your comment. I will create an example now.

[Torniojaws]

Still working on the test case, but I think this is what happens:

1. After the patch is applied on the dict using move, the value is correctly removed from the patch data.
2. However, either SQLAlchemy or the database itself sees that the value does not exist in the new (patch) data and does not modify anything in the from item. Ie. it keeps whatever was there originally.

I think it would need to be explicitly defined for it to work, at least in my case.

So if the source data is eg. {"Author": "Some name", "Updated": "SomeDatetime")

Then the patch result of {"op": "move", "from": "/Updated", "path": "/Author"}

Would need to be this: {"Author": "SomeDatetime", "Updated": null}

Instead of this: {"Author": "SomeDatetime"}

So then it would make the Updated = NULL in the DB when used with SQLAlchemy.

Will test a bit more and see if that's the case.

[Torniojaws]

Yes, that is the reason. At least in SQLAlchemy + PyMySQL + MariaDB, setting the value explicitly to None will achieve the desired behaviour.

Example case, which of course will need development to check all valid elements for the same case:

    data = patch.apply(data)
    print(data)
    if "Updated" not in data:
        data["Updated"] = None

When used with {"op": "move", "from": "/Updated", "path": "/Author"} Then the outcome is:

1. Author now has the value that was in Updated
2. Updated is now NULL

[stefankoegl]

So where exactly do you see a problem with python-json-patch?

[Torniojaws]

Based on the above, I think there is no real issue in python-json-patch.

However, I think this is a fairly common use case for it. It would be great to have some extra option that allows you to toggle between removing the dict element completely (current behaviour) or setting it as None. Maybe something like this:

data = instance.asdict(exclude_pk=True, **kwargs)
patch = JsonPatch(patchdata)
data = patch.apply(data, use_nulls=True)  # This would set the "removed" item explicitly as None
instance.fromdict(data)

So, let's say the source data is: {"Author": "Some name", "Updated": "SomeDatetime")

When using move:

{"op": "move", "from": "/Updated", "path": "/Author"} data = patch.apply(data, use_nulls=True) Would result in: {"Author": "SomeDatetime", "Updated": None}

{"op": "move", "from": "/Updated", "path": "/Author"} data = patch.apply(data, use_nulls=False) Would result in: {"Author": "SomeDatetime"}

{"op": "move", "from": "/Updated", "path": "/Author"} data = patch.apply(data) Would result in: {"Author": "SomeDatetime"}

And when using remove:

{"op": "remove", "path": "/Author"} data = patch.apply(data, use_nulls=True) Would result in: {"Author": None, "Updated": "SomeDatetime"}

{"op": "remove", "path": "/Author"} data = patch.apply(data, use_nulls=False) Would result in: {"Updated": "SomeDatetime"}

{"op": "remove", "path": "/Author"} data = patch.apply(data) Would result in: {"Updated": "SomeDatetime"}

[stefankoegl]

RFC 6902 is pretty clear on that

4.4.  move

   The "move" operation removes the value at a specified location and
   adds it to the target location.

To solve your particular case, I'd recommend something like

import collections
data = instance.asdict(exclude_pk=True, **kwargs)
patch = JsonPatch(patchdata)
data = patch.apply(data)

# wrap in a defaultdict
d = collections.defaultdict(lambda: None)
d.update(data)

instance.fromdict(d)