{"action":"created","issue":{"url":"https://api.github.com/repos/hypothesis/h/issues/3243","repository_url":"https://api.github.com/repos/hypothesis/h","labels_url":"https://api.github.com/repos/hypothesis/h/issues/3243/labels{/name}","comments_url":"https://api.github.com/repos/hypothesis/h/issues/3243/comments","events_url":"https://api.github.com/repos/hypothesis/h/issues/3243/events","html_url":"https://github.com/hypothesis/h/pull/3243","id":150405494,"number":3243,"title":"[Update annotations in Pg 2/3]: Do all validation in schemas","user":{"login":"seanh","id":22498,"avatar_url":"https://avatars.githubusercontent.com/u/22498?v=3","gravatar_id":"","url":"https://api.github.com/users/seanh","html_url":"https://github.com/seanh","followers_url":"https://api.github.com/users/seanh/followers","following_url":"https://api.github.com/users/seanh/following{/other_user}","gists_url":"https://api.github.com/users/seanh/gists{/gist_id}","starred_url":"https://api.github.com/users/seanh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/seanh/subscriptions","organizations_url":"https://api.github.com/users/seanh/orgs","repos_url":"https://api.github.com/users/seanh/repos","events_url":"https://api.github.com/users/seanh/events{/privacy}","received_events_url":"https://api.github.com/users/seanh/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":7,"created_at":"2016-04-22T15:48:18Z","updated_at":"2016-04-27T10:11:50Z","closed_at":"2016-04-27T10:11:50Z","pull_request":{"url":"https://api.github.com/repos/hypothesis/h/pulls/3243","html_url":"https://github.com/hypothesis/h/pull/3243","diff_url":"https://github.com/hypothesis/h/pull/3243.diff","patch_url":"https://github.com/hypothesis/h/pull/3243.patch"},"body":"When creating new annotations a couple of validation rules were\r\nimplemented in h.api.storage.create_annotation() instead of in\r\nh.api.schemas with the rest, because we didn't want h.api.schemas to\r\naccess the db:\r\n\r\n1. Replies must have the same group as their parent.\r\n\r\n   Actually this is transformation not validation - it just overwrites\r\n   the group of a reply with that of its parent.\r\n\r\n   This needed to be implemented in storage because it requires access\r\n   to the db to look up the parent annotation.\r\n\r\n2. The parent ID that a reply is in reply to must be an ID of an\r\n   annotation that actually exists in the database.\r\n   Requires db.\r\n\r\n3. The user must have permission to create annotations in the group\r\n   they're trying to create one in.\r\n\r\n   This doesn't require db access, just effective_principals, but it has\r\n   to happen _after_ 1 above because 1 might change the group so it had\r\n   to be in storage for that reason.\r\n\r\nThere's a number of reasons why it's desirable to do _all_ validation in\r\nschemas and none in storage:\r\n\r\na. Have all the validation code implemented in one place, and one call\r\n   to validate() does all validation and transformation at once.\r\n\r\nb. No validation or transformation going on in storage, keeps it simple.\r\n\r\nc. The 3 bits of validation and transformation code above are actually needed\r\n   by both storage.create_annotation() and the (to be implemented)\r\n   PostgreSQL version of storage.update_annotation(). This means we'd\r\n   have to either.\r\n\r\n   i.   Duplicate the code and tests in create_annotation() and\r\n        update_annotation(). But then we have duplication.\r\n   ii.  Add a private helper function for create_annotation() and\r\n        update_annotation() to call and unit test that function. But we\r\n        don't like unit testing private functions.\r\n  iii.  Make the private helper function public. But it shouldn't be\r\n        public as it's not intended to be part of storage's public API.\r\n   iv.  Make a new module to contain this validation code for storage to\r\n        call and unit test that module.\r\n\r\n   We _could_ do iv. but schemas.py already contains a place to put\r\n   validation code that's shared between create and update\r\n   (both CreateAnnotationSchema and UpdateAnnotationSchema inherit from\r\n   AnnotationSchema) and putting all the validation code into schemas\r\n   has the advantages a and b as well.\r\n\r\nSo move this validation code into schemas, which is going to make\r\nimplementing the PostgreSQL version of storage.update_annotation()\r\nwith no duplicate code or tests simpler.\r\n\r\nSchemas is still a pure module: it takes dicts as inputs, gives dicts as\r\noutputs, and doesn't create, update or delete anything in the db.\r\n\r\nBut it does import and read the db (via storage).\r\n\r\nIf we really don't want schemas to import storage at all then I'd\r\nsuggest [using jsonschema format functions](https://github.com/hypothesis/h/pull/3225) which would allow\r\nschemas to declare that it depends on these validation functions, while\r\nthe validation functions themselves (which need access to storage) are\r\nactually implemented in a different module, and then h.api.views injects\r\nthe validation functions from the separate module into schemas.\r\nAll validation declared in one place in schemas and carried out by one\r\ncall to validate(), no validation in storage, and no storage in schemas.\r\n\r\nBut for now this way is simple and direct."},"comment":{"url":"https://api.github.com/repos/hypothesis/h/issues/comments/215039604","html_url":"https://github.com/hypothesis/h/pull/3243#issuecomment-215039604","issue_url":"https://api.github.com/repos/hypothesis/h/issues/3243","id":215039604,"user":{"login":"seanh","id":22498,"avatar_url":"https://avatars.githubusercontent.com/u/22498?v=3","gravatar_id":"","url":"https://api.github.com/users/seanh","html_url":"https://github.com/seanh","followers_url":"https://api.github.com/users/seanh/followers","following_url":"https://api.github.com/users/seanh/following{/other_user}","gists_url":"https://api.github.com/users/seanh/gists{/gist_id}","starred_url":"https://api.github.com/users/seanh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/seanh/subscriptions","organizations_url":"https://api.github.com/users/seanh/orgs","repos_url":"https://api.github.com/users/seanh/repos","events_url":"https://api.github.com/users/seanh/events{/privacy}","received_events_url":"https://api.github.com/users/seanh/received_events","type":"User","site_admin":false},"created_at":"2016-04-27T10:11:50Z","updated_at":"2016-04-27T10:11:50Z","body":"Alright, honestly I still prefer the jsonschema formats approach, but in the interest of getting this merged I've started rewriting <https://github.com/hypothesis/h/pull/3245> and its tests to work on top of master instead of on top of this branch, and to work in the way that Nick suggested.\r\n\r\nAlso fixing the semantics of the update `PUT` requests to be more like (a broken version of) `PATCH` than `PUT`, to be compatible with the legacy behaviour."}}