{"action":"created","issue":{"url":"https://api.github.com/repos/hypothesis/h/issues/3862","repository_url":"https://api.github.com/repos/hypothesis/h","labels_url":"https://api.github.com/repos/hypothesis/h/issues/3862/labels{/name}","comments_url":"https://api.github.com/repos/hypothesis/h/issues/3862/comments","events_url":"https://api.github.com/repos/hypothesis/h/issues/3862/events","html_url":"https://github.com/hypothesis/h/pull/3862","id":176712150,"number":3862,"title":"Disallow empty document titles","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":[{"url":"https://api.github.com/repos/hypothesis/h/labels/Feature:%20Activity%20Pages","name":"Feature: Activity Pages","color":"5319e7"}],"state":"open","locked":false,"assignee":{"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},"assignees":[{"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}],"milestone":null,"comments":3,"created_at":"2016-09-13T18:17:11Z","updated_at":"2016-09-15T10:49:26Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/hypothesis/h/pulls/3862","html_url":"https://github.com/hypothesis/h/pull/3862","diff_url":"https://github.com/hypothesis/h/pull/3862.diff","patch_url":"https://github.com/hypothesis/h/pull/3862.patch"},"body":"This prevents creation of `None`, `''`, whitespace-only strings and strings with leading and/or trailing whitespace as document titles, and removes and fixes these kinds of document titles that are already in the db. This will make it a lot easier for activity pages and future features to display documents without lots of edge cases where the display looks wrong - a document's title is always either `None` or a non-empty string.\r\n\r\nOn master, I can create documents with whitespace-only titles:\r\n\r\n```bash\r\n$ http POST 'http://localhost:5000/api/annotations' Authorization:'Bearer 6879-***' uri='foo' document:='{\"title\": \" \"}' | jq .document.title\r\n[\r\n  \" \"\r\n]\r\n```\r\n\r\nOr with leading and/or trailing whitespace on the document title:\r\n\r\n```bash\r\nhttp POST 'http://localhost:5000/api/annotations' Authorization:'Bearer 6879-***' uri='bar' document:='{\"title\": \" Title \"}' | jq .document.title\r\n[\r\n  \" Title \"\r\n]\r\n```\r\n\r\nIt doesn't actually seem to be possible to create a `None` or empty string document title by posting to this API, but instances of both do somehow exist in our production database:\r\n\r\n```sql\r\npostgres=# SELECT count(*) FROM document_meta WHERE type = 'title' AND '' = ANY(value);\r\n count \r\n-------\r\n   881\r\n(1 row)\r\n```\r\n\r\n```python\r\nIn [3]: len([d for d in request.db.query(models.DocumentMeta).filter_by(type=u'title') if None in d.value])\r\nOut[3]: 335\r\n```\r\n\r\nThere aren't any `document_meta` rows whose `value` cell is an empty array, but after deleting `None` and `''` and whitespace-only strings from the arrays there will be, so the migrations remove those as well.\r\n\r\nI've tested the db migrations here on prod data and looked at all the deleted and changed document metas - looks as expected.\r\n\r\nAnyway, on this branch none of these kinds of document titles will be created anymore:\r\n\r\n```bash\r\nhttp POST 'http://localhost:5000/api/annotations' Authorization:'Bearer 6879-***' uri='fi' document:='{\"title\": \" \"}' | jq .document\r\n{}\r\n```\r\n\r\n```bash\r\nhttp POST 'http://localhost:5000/api/annotations' Authorization:'Bearer 6879-***' uri='fy' document:='{\"title\": \" Leading whitespace\"}' | jq .document\r\n{\r\n  \"title\": [\r\n    \"Leading whitespace\"\r\n  ]\r\n}\r\n```\r\n\r\n```bash\r\nhttp POST 'http://localhost:5000/api/annotations' Authorization:'Bearer 6879-***' uri='fo' document:='{\"title\": \"Trailing whitespace \"}' | jq .document\r\n{\r\n  \"title\": [\r\n    \"Trailing whitespace\"\r\n  ]\r\n}\r\n```\r\n\r\n```bash\r\nhttp POST 'http://localhost:5000/api/annotations' Authorization:'Bearer 6879-***' uri='fumb' document:='{\"title\": \"\"}' | jq .document\r\n{}\r\n```\r\n\r\n```bash\r\nhttp POST 'http://localhost:5000/api/annotations' Authorization:'Bearer 6879-***' uri='fum' document:='{\"title\": null}' | jq .document\r\n{}\r\n```\r\n\r\nThe validation that achieves this is in `parse_document_claims.py` rather than in the model. I did try to add a second layer of validation to the model code as well but it was complicated by two factors: 1. I only want to filter out these kinds of values when the `type` is `'title'` and 2. `value` is an array which can be initialized, set, appended to, inserted into etc and all cases would need to be validated (but only when `type` is `'title'`)."},"comment":{"url":"https://api.github.com/repos/hypothesis/h/issues/comments/247296253","html_url":"https://github.com/hypothesis/h/pull/3862#issuecomment-247296253","issue_url":"https://api.github.com/repos/hypothesis/h/issues/3862","id":247296253,"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-09-15T10:49:26Z","updated_at":"2016-09-15T10:49:26Z","body":"@chdorner All done I think?"}}
{"actor":{"display_login":"seanh"}}