{"action":"created","issue":{"url":"https://api.github.com/repos/discourse/discourse/issues/6385","repository_url":"https://api.github.com/repos/discourse/discourse","labels_url":"https://api.github.com/repos/discourse/discourse/issues/6385/labels{/name}","comments_url":"https://api.github.com/repos/discourse/discourse/issues/6385/comments","events_url":"https://api.github.com/repos/discourse/discourse/issues/6385/events","html_url":"https://github.com/discourse/discourse/pull/6385","id":359119875,"node_id":"MDExOlB1bGxSZXF1ZXN0MjE0Njg4MzA2","number":6385,"title":"FIX: rescue ActiveRecord::RecordInvalid in find_or_create_by_safe!","user":{"login":"xrav3nz","id":6376558,"node_id":"MDQ6VXNlcjYzNzY1NTg=","avatar_url":"https://avatars0.githubusercontent.com/u/6376558?v=4","gravatar_id":"","url":"https://api.github.com/users/xrav3nz","html_url":"https://github.com/xrav3nz","followers_url":"https://api.github.com/users/xrav3nz/followers","following_url":"https://api.github.com/users/xrav3nz/following{/other_user}","gists_url":"https://api.github.com/users/xrav3nz/gists{/gist_id}","starred_url":"https://api.github.com/users/xrav3nz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/xrav3nz/subscriptions","organizations_url":"https://api.github.com/users/xrav3nz/orgs","repos_url":"https://api.github.com/users/xrav3nz/repos","events_url":"https://api.github.com/users/xrav3nz/events{/privacy}","received_events_url":"https://api.github.com/users/xrav3nz/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-09-11T16:15:46Z","updated_at":"2018-09-12T00:59:37Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/discourse/discourse/pulls/6385","html_url":"https://github.com/discourse/discourse/pull/6385","diff_url":"https://github.com/discourse/discourse/pull/6385.diff","patch_url":"https://github.com/discourse/discourse/pull/6385.patch"},"body":"Tracked this down from:\r\n\r\n```text\r\nJob exception: Validation failed: Post has already been taken\r\n\r\nBT\r\n/var/www/discourse/vendor/bundle/ruby/2.5.0/gems/activerecord-5.2.0/lib/active_record/validations.rb:80:in `raise_validation_error'\r\n...\r\n/var/www/discourse/lib/freedom_patches/active_record_base.rb:17:in `find_or_create_by_safe!'\r\n/var/www/discourse/lib/email/sender.rb:269:in `set_reply_key'\r\n/var/www/discourse/app/jobs/regular/user_email.rb:47:in `execute'\r\n```\r\n\r\nhttps://github.com/discourse/discourse/blob/1a01385e886b52e1d2e77c5d38d81846a736727b/lib/email/sender.rb#L268-L272\r\n\r\ni.e. basically, the record is inserted between the `find_by` and the `create_by!`.\r\n\r\nWe already [rescue `PG::UniqueViolation` and `ActiveRecord::RecordNotUnique`](https://github.com/discourse/discourse/blob/3bb4f4c5efff46f3aa1607af53a0b0b3f7a02346/lib/freedom_patches/active_record_base.rb#L18), however, they are only raised if the error is picked up by the database constraint. In this case, our AR uniqueness validation raised `ActiveRecord::RecordInvalid` (before hitting the database).\r\n\r\nI am still a bit torn if we should just catch all `ActiveRecord::RecordInvalid` within `find_or_create_by!`, **or deal with this individually in `Email::Sender#set_reply_key`** (in which we can easily check if it is a uniqueness validation error). Let me know what do you think! "},"comment":{"url":"https://api.github.com/repos/discourse/discourse/issues/comments/420475722","html_url":"https://github.com/discourse/discourse/pull/6385#issuecomment-420475722","issue_url":"https://api.github.com/repos/discourse/discourse/issues/6385","id":420475722,"node_id":"MDEyOklzc3VlQ29tbWVudDQyMDQ3NTcyMg==","user":{"login":"SamSaffron","id":5213,"node_id":"MDQ6VXNlcjUyMTM=","avatar_url":"https://avatars1.githubusercontent.com/u/5213?v=4","gravatar_id":"","url":"https://api.github.com/users/SamSaffron","html_url":"https://github.com/SamSaffron","followers_url":"https://api.github.com/users/SamSaffron/followers","following_url":"https://api.github.com/users/SamSaffron/following{/other_user}","gists_url":"https://api.github.com/users/SamSaffron/gists{/gist_id}","starred_url":"https://api.github.com/users/SamSaffron/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/SamSaffron/subscriptions","organizations_url":"https://api.github.com/users/SamSaffron/orgs","repos_url":"https://api.github.com/users/SamSaffron/repos","events_url":"https://api.github.com/users/SamSaffron/events{/privacy}","received_events_url":"https://api.github.com/users/SamSaffron/received_events","type":"User","site_admin":false},"created_at":"2018-09-12T00:59:37Z","updated_at":"2018-09-12T00:59:37Z","author_association":"MEMBER","body":"I think this is worth a shot! good catch, In Rails 6 or once the patch is backported we can try the simpler find_by(hash) || create_or_find_by(hash)"}}
{"actor":{"display_login":"SamSaffron"}}