Add a function for performing a JSON Patch.

Review Request #9264 — Created Oct. 12, 2017 and submitted

Information

Djblets
release-0.10.x
6ffb09c...

Reviewers

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.

Unit tests pass.

Description From Last Updated

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

Docstring?

daviddavid

Typo: THe

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

JSON Pointer doesn't necessarily know about the operation, right? So it's possible that we might have '-' here, which JSON …

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid

Should this really be localized?

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
david
  1. 
      
  2. djblets/util/json_utils.py (Diff revision 2)
     
     
    Show all issues

    Docstring?

  3. djblets/util/json_utils.py (Diff revision 2)
     
     
    Show all issues

    Typo: THe

  4. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

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

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

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

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

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

  5. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  6. djblets/util/json_utils.py (Diff revision 2)
     
     
     
     
    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.

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

  7. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  8. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  9. djblets/util/json_utils.py (Diff revision 2)
     
     
    Show all issues

    Should this really be localized?

  10. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  11. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  12. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  13. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  14. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  15. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  16. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  17. djblets/util/json_utils.py (Diff revision 2)
     
     
     
    Show all issues

    Should this really be localized?

  18. djblets/util/json_utils.py (Diff revision 2)
     
     
     
     
    Show all issues

    Should this really be localized?

  19. 
      
chipx86
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-0.10.x (ceca8ca)