Summary

Add a function for performing a JSON Patch.

Review Request #9264 — Created Oct. 12, 2017 and submitted Oct. 26, 2017, 2:11 a.m.

Information

Owner

chipx86

Repository

Djblets

Branch

release-0.10.x

Bugs

Depends On

Commit

6ffb09c...

Reviewers

Groups

djblets

People

Description

This introduces djblets.util.json_utils.json_patch, an implementation
of JSON Patch (RFC 6902). This is another, more advanced method of
patching JSON-serializable data, sort of like with JSON Merge Patch but
with the ability to order operations, copy or move data, sanity-check
values before applying a patch, set values to null, and manipulate
specific indices of arrays. It's a good substitute for JSON Merge
Patches when more specific modifications and sanity-checking is needed.

JSON Patches consist of a list of operations to perform. These are
add, remove, replace, copy, move, and test. All operations
must pass in order for a patch to apply.

This implementation goes beyond most implementations by providing read
and write access control for keys. This allows callers to prevent
reading certain keys or writing to others, limiting the damage that, for
instance, an API call can do.

More information on JSON Patch can be found at http://jsonpatch.com/ or
https://tools.ietf.org/html/rfc6902.

Testing Done

Unit tests pass.

Issues

Description	From	Last Updated
E128 continuation line under-indented for visual indent	reviewbot	Oct. 12, 2017, 12:35 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Oct. 12, 2017, 12:35 a.m.
Docstring?	david	Oct. 21, 2017, 4:16 p.m.
Typo: THe	david	Oct. 21, 2017, 4:16 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
JSON Pointer doesn't necessarily know about the operation, right? So it's possible that we might have '-' here, which JSON …	david	Oct. 21, 2017, 5:08 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.
Should this really be localized?	david	Oct. 22, 2017, 10:46 p.m.

flake8 failed.

JSHint passed.

flake8

djblets/util/json_utils.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
djblets/util/json_utils.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```

Change Summary:


Improved documentation.
Fixed alignment issues.

Commit:

-	7282b972265569f604147a7e594c18afd530ef27
+	2df79c3eaf5620121f813340fbc19d1fb24e9250

Diff:

Revision 2 (+2690 -2)

Show changes

	djblets/util/json_utils.py
	djblets/util/tests/test_json_utils.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/util/json_utils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Docstring?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Typo: THe
```

djblets/util/json_utils.py (Diff revision 2)

The issue has been dropped. Show all issues

Should this really be localized?

chipx86 Oct. 21, 2017, 4:22 p.m.

It'll get displayed to the caller. I don't see why not?

We have errors that are localized and errors that aren't. We should decide on which way we want to go for these things. Given that this will be sent back to the client, it's probably appropriate to localize, but I'm open to changing it if there's a good reason not to.

david Oct. 21, 2017, 4:26 p.m.

Generally none of the errors returned by the API are localized. I thought at some point we decided that things which are presented in the UI should be localized but anything that's API-only are not.

chipx86 Oct. 21, 2017, 4:29 p.m.

I don't recall but we should definitely write this down in Notion. There's good arguments both ways for localizing for the API, but I'm okay not localizing for it. However, I don't know that this code should assume it's being used in an API context. I wonder if we can have the API handlers just turn off localization while processing requests.

chipx86 Oct. 21, 2017, 4:36 p.m.

So, this isn't a new problem by any means, because every single form we validate and return errors from has localized errors. Given that, I'd rather keep localized errors in this method.
If we want to force all errors to be in their original form and avoid localizing for APIs, what we can do is have WebAPIResource call django.utils.translation.deactivate_all() to turn off translations for the thread. (Translations are re-activated on the next request, based on the locale sent by the browser.) This would ensure that all strings are their original strings and don't go through the localization tables.

chipx86 Oct. 21, 2017, 6:53 p.m.

The more I think about it, the more I think error messages should be localized. Strings used as identifiers should not, but things like form errors that are going to end up displayed in a client or in the UI should (otherwise it's weird for more and more errors to be in English when the client's expecting Japanese, for instance, as more things become dynamic).

Clients have control over what language is going to be set, so if they really need the standard English errors, that's in their control.

djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```

djblets/util/json_utils.py (Diff revision 2)

The issue has been resolved. Show all issues

JSON Pointer doesn't necessarily know about the operation, right? So it's possible that we might have '-' here, which JSON Pointer says is valid, but will cause a ValueError. We should probably catch that and re-raise a JSONPatchPathError.

chipx86 Oct. 21, 2017, 4:22 p.m.

I'll update the sanity-checking that occurs before we get here to make sure we're not operating on a - for incompatible operations.

djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```
djblets/util/json_utils.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Should this really be localized?
```

Change Summary:


Added a missing docstring.
Fixed typos in docstrings.
JSONPointerEndOfList equality checks now return False if comparing against a non-JSONPointerEndOfList.
Added checks (and unit tests) to make sure paths with - aren't being used for operations that don't support them.

Commit:

-	2df79c3eaf5620121f813340fbc19d1fb24e9250
+	899e5683fd6dc1feb7e13574f84d76f3db3a9f13

Diff:

Revision 3 (+2944 -4)

Show changes

	djblets/util/json_utils.py
	djblets/util/tests/test_json_utils.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:


Renamed "target" to "doc" and fleshed out documentation to indicate that the original JSON document being patched is not actually modified itself.
Fixed a small handful of doc errors and typos.

Commit:

-	899e5683fd6dc1feb7e13574f84d76f3db3a9f13
+	6ffb09c74d36782f2c4be5bc24358d1ef0b8c550

Diff:

Revision 4 (+2947 -4)

Show changes

	djblets/util/json_utils.py
	djblets/util/tests/test_json_utils.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (ceca8ca)