{"action":"created","comment":{"url":"https://api.github.com/repos/apotonick/reform/pulls/comments/32420136","id":32420136,"diff_hunk":"@@ -1,7 +1,14 @@\n class Reform::Form::UniqueValidator < ActiveModel::EachValidator\n   def validate_each(form, attribute, value)\n-    if form.model.class.where(\"#{attribute} = ?\", value).size > 0\n-      form.errors.add attribute, \"#{attribute} must be unique.\"\n+    if form.model.persisted?\n+      if form.model.class.where(\"#{attribute} = ? AND id <> ?\", value, form.model.id).size > 0","path":"lib/reform/form/validation/unique_validator.rb","position":6,"original_position":6,"commit_id":"5ebed175fc37c727764f536c672cb5fb36f50cb5","original_commit_id":"5ebed175fc37c727764f536c672cb5fb36f50cb5","user":{"login":"freemanoid","id":180154,"avatar_url":"https://avatars.githubusercontent.com/u/180154?v=3","gravatar_id":"","url":"https://api.github.com/users/freemanoid","html_url":"https://github.com/freemanoid","followers_url":"https://api.github.com/users/freemanoid/followers","following_url":"https://api.github.com/users/freemanoid/following{/other_user}","gists_url":"https://api.github.com/users/freemanoid/gists{/gist_id}","starred_url":"https://api.github.com/users/freemanoid/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/freemanoid/subscriptions","organizations_url":"https://api.github.com/users/freemanoid/orgs","repos_url":"https://api.github.com/users/freemanoid/repos","events_url":"https://api.github.com/users/freemanoid/events{/privacy}","received_events_url":"https://api.github.com/users/freemanoid/received_events","type":"User","site_admin":false},"body":"I think it's better to not interpolate an `attribute` in sql query like this because of possible escaping and quoting problems. So this can be changed to \r\n```ruby\r\nquery = form.model.class.where(attribute => value)\r\nif form.model.persisted?\r\n  query = query.merge(form.model.class.where(\"id <> ?\", form.model.id)\r\nend\r\n\r\nif query.any?\r\n    form.errors.add(attribute, \"#{attribute} must be unique.\")\r\nend\r\n```\r\n\r\nWhat do you think?\r\n\r\n","created_at":"2015-06-15T13:39:58Z","updated_at":"2015-06-15T13:39:58Z","html_url":"https://github.com/apotonick/reform/pull/233#discussion_r32420136","pull_request_url":"https://api.github.com/repos/apotonick/reform/pulls/233","_links":{"self":{"href":"https://api.github.com/repos/apotonick/reform/pulls/comments/32420136"},"html":{"href":"https://github.com/apotonick/reform/pull/233#discussion_r32420136"},"pull_request":{"href":"https://api.github.com/repos/apotonick/reform/pulls/233"}}},"pull_request":{"url":"https://api.github.com/repos/apotonick/reform/pulls/233","id":37656601,"html_url":"https://github.com/apotonick/reform/pull/233","diff_url":"https://github.com/apotonick/reform/pull/233.diff","patch_url":"https://github.com/apotonick/reform/pull/233.patch","issue_url":"https://api.github.com/repos/apotonick/reform/issues/233","number":233,"state":"open","locked":false,"title":"Improve unique validator on model update","user":{"login":"fernandes","id":517743,"avatar_url":"https://avatars.githubusercontent.com/u/517743?v=3","gravatar_id":"","url":"https://api.github.com/users/fernandes","html_url":"https://github.com/fernandes","followers_url":"https://api.github.com/users/fernandes/followers","following_url":"https://api.github.com/users/fernandes/following{/other_user}","gists_url":"https://api.github.com/users/fernandes/gists{/gist_id}","starred_url":"https://api.github.com/users/fernandes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fernandes/subscriptions","organizations_url":"https://api.github.com/users/fernandes/orgs","repos_url":"https://api.github.com/users/fernandes/repos","events_url":"https://api.github.com/users/fernandes/events{/privacy}","received_events_url":"https://api.github.com/users/fernandes/received_events","type":"User","site_admin":false},"body":"Unique validator was searching if there was some model with value the same as the form, but when we are updating (and obviously the object is already persisted), the sql will find an object, the same as we are trying to update, so we need to consider two cases:\r\n\r\n1. on create:\r\n\r\nsearch for any object with field with same value (case works perfectly)\r\n\r\n2. on update:\r\n\r\nsearch for any object with field with same value AND id different the form's model id, so we will never find the same object (now this PR is considering this case!)\r\n\r\nand now unique validation works perfectly","created_at":"2015-06-15T03:50:45Z","updated_at":"2015-06-15T13:39:58Z","closed_at":null,"merged_at":null,"merge_commit_sha":"9668a668d9dc996a7206ecec60016d13d3274b13","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/apotonick/reform/pulls/233/commits","review_comments_url":"https://api.github.com/repos/apotonick/reform/pulls/233/comments","review_comment_url":"https://api.github.com/repos/apotonick/reform/pulls/comments{/number}","comments_url":"https://api.github.com/repos/apotonick/reform/issues/233/comments","statuses_url":"https://api.github.com/repos/apotonick/reform/statuses/5ebed175fc37c727764f536c672cb5fb36f50cb5","head":{"label":"fernandes:improve_unique_validator_on_update","ref":"improve_unique_validator_on_update","sha":"5ebed175fc37c727764f536c672cb5fb36f50cb5","user":{"login":"fernandes","id":517743,"avatar_url":"https://avatars.githubusercontent.com/u/517743?v=3","gravatar_id":"","url":"https://api.github.com/users/fernandes","html_url":"https://github.com/fernandes","followers_url":"https://api.github.com/users/fernandes/followers","following_url":"https://api.github.com/users/fernandes/following{/other_user}","gists_url":"https://api.github.com/users/fernandes/gists{/gist_id}","starred_url":"https://api.github.com/users/fernandes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fernandes/subscriptions","organizations_url":"https://api.github.com/users/fernandes/orgs","repos_url":"https://api.github.com/users/fernandes/repos","events_url":"https://api.github.com/users/fernandes/events{/privacy}","received_events_url":"https://api.github.com/users/fernandes/received_events","type":"User","site_admin":false},"repo":{"id":32550359,"name":"reform","full_name":"fernandes/reform","owner":{"login":"fernandes","id":517743,"avatar_url":"https://avatars.githubusercontent.com/u/517743?v=3","gravatar_id":"","url":"https://api.github.com/users/fernandes","html_url":"https://github.com/fernandes","followers_url":"https://api.github.com/users/fernandes/followers","following_url":"https://api.github.com/users/fernandes/following{/other_user}","gists_url":"https://api.github.com/users/fernandes/gists{/gist_id}","starred_url":"https://api.github.com/users/fernandes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fernandes/subscriptions","organizations_url":"https://api.github.com/users/fernandes/orgs","repos_url":"https://api.github.com/users/fernandes/repos","events_url":"https://api.github.com/users/fernandes/events{/privacy}","received_events_url":"https://api.github.com/users/fernandes/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/fernandes/reform","description":"Form objects decoupled from models.","fork":true,"url":"https://api.github.com/repos/fernandes/reform","forks_url":"https://api.github.com/repos/fernandes/reform/forks","keys_url":"https://api.github.com/repos/fernandes/reform/keys{/key_id}","collaborators_url":"https://api.github.com/repos/fernandes/reform/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/fernandes/reform/teams","hooks_url":"https://api.github.com/repos/fernandes/reform/hooks","issue_events_url":"https://api.github.com/repos/fernandes/reform/issues/events{/number}","events_url":"https://api.github.com/repos/fernandes/reform/events","assignees_url":"https://api.github.com/repos/fernandes/reform/assignees{/user}","branches_url":"https://api.github.com/repos/fernandes/reform/branches{/branch}","tags_url":"https://api.github.com/repos/fernandes/reform/tags","blobs_url":"https://api.github.com/repos/fernandes/reform/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/fernandes/reform/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/fernandes/reform/git/refs{/sha}","trees_url":"https://api.github.com/repos/fernandes/reform/git/trees{/sha}","statuses_url":"https://api.github.com/repos/fernandes/reform/statuses/{sha}","languages_url":"https://api.github.com/repos/fernandes/reform/languages","stargazers_url":"https://api.github.com/repos/fernandes/reform/stargazers","contributors_url":"https://api.github.com/repos/fernandes/reform/contributors","subscribers_url":"https://api.github.com/repos/fernandes/reform/subscribers","subscription_url":"https://api.github.com/repos/fernandes/reform/subscription","commits_url":"https://api.github.com/repos/fernandes/reform/commits{/sha}","git_commits_url":"https://api.github.com/repos/fernandes/reform/git/commits{/sha}","comments_url":"https://api.github.com/repos/fernandes/reform/comments{/number}","issue_comment_url":"https://api.github.com/repos/fernandes/reform/issues/comments{/number}","contents_url":"https://api.github.com/repos/fernandes/reform/contents/{+path}","compare_url":"https://api.github.com/repos/fernandes/reform/compare/{base}...{head}","merges_url":"https://api.github.com/repos/fernandes/reform/merges","archive_url":"https://api.github.com/repos/fernandes/reform/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/fernandes/reform/downloads","issues_url":"https://api.github.com/repos/fernandes/reform/issues{/number}","pulls_url":"https://api.github.com/repos/fernandes/reform/pulls{/number}","milestones_url":"https://api.github.com/repos/fernandes/reform/milestones{/number}","notifications_url":"https://api.github.com/repos/fernandes/reform/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/fernandes/reform/labels{/name}","releases_url":"https://api.github.com/repos/fernandes/reform/releases{/id}","created_at":"2015-03-19T22:49:14Z","updated_at":"2015-03-19T22:49:14Z","pushed_at":"2015-06-15T03:47:12Z","git_url":"git://github.com/fernandes/reform.git","ssh_url":"git@github.com:fernandes/reform.git","clone_url":"https://github.com/fernandes/reform.git","svn_url":"https://github.com/fernandes/reform","homepage":"","size":943,"stargazers_count":0,"watchers_count":0,"language":"Ruby","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"apotonick:master","ref":"master","sha":"fe55d098ec0a82deb45a57f629fd60a3d61efb40","user":{"login":"apotonick","id":10406,"avatar_url":"https://avatars.githubusercontent.com/u/10406?v=3","gravatar_id":"","url":"https://api.github.com/users/apotonick","html_url":"https://github.com/apotonick","followers_url":"https://api.github.com/users/apotonick/followers","following_url":"https://api.github.com/users/apotonick/following{/other_user}","gists_url":"https://api.github.com/users/apotonick/gists{/gist_id}","starred_url":"https://api.github.com/users/apotonick/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/apotonick/subscriptions","organizations_url":"https://api.github.com/users/apotonick/orgs","repos_url":"https://api.github.com/users/apotonick/repos","events_url":"https://api.github.com/users/apotonick/events{/privacy}","received_events_url":"https://api.github.com/users/apotonick/received_events","type":"User","site_admin":false},"repo":{"id":9764362,"name":"reform","full_name":"apotonick/reform","owner":{"login":"apotonick","id":10406,"avatar_url":"https://avatars.githubusercontent.com/u/10406?v=3","gravatar_id":"","url":"https://api.github.com/users/apotonick","html_url":"https://github.com/apotonick","followers_url":"https://api.github.com/users/apotonick/followers","following_url":"https://api.github.com/users/apotonick/following{/other_user}","gists_url":"https://api.github.com/users/apotonick/gists{/gist_id}","starred_url":"https://api.github.com/users/apotonick/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/apotonick/subscriptions","organizations_url":"https://api.github.com/users/apotonick/orgs","repos_url":"https://api.github.com/users/apotonick/repos","events_url":"https://api.github.com/users/apotonick/events{/privacy}","received_events_url":"https://api.github.com/users/apotonick/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/apotonick/reform","description":"Form objects decoupled from models.","fork":false,"url":"https://api.github.com/repos/apotonick/reform","forks_url":"https://api.github.com/repos/apotonick/reform/forks","keys_url":"https://api.github.com/repos/apotonick/reform/keys{/key_id}","collaborators_url":"https://api.github.com/repos/apotonick/reform/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/apotonick/reform/teams","hooks_url":"https://api.github.com/repos/apotonick/reform/hooks","issue_events_url":"https://api.github.com/repos/apotonick/reform/issues/events{/number}","events_url":"https://api.github.com/repos/apotonick/reform/events","assignees_url":"https://api.github.com/repos/apotonick/reform/assignees{/user}","branches_url":"https://api.github.com/repos/apotonick/reform/branches{/branch}","tags_url":"https://api.github.com/repos/apotonick/reform/tags","blobs_url":"https://api.github.com/repos/apotonick/reform/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/apotonick/reform/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/apotonick/reform/git/refs{/sha}","trees_url":"https://api.github.com/repos/apotonick/reform/git/trees{/sha}","statuses_url":"https://api.github.com/repos/apotonick/reform/statuses/{sha}","languages_url":"https://api.github.com/repos/apotonick/reform/languages","stargazers_url":"https://api.github.com/repos/apotonick/reform/stargazers","contributors_url":"https://api.github.com/repos/apotonick/reform/contributors","subscribers_url":"https://api.github.com/repos/apotonick/reform/subscribers","subscription_url":"https://api.github.com/repos/apotonick/reform/subscription","commits_url":"https://api.github.com/repos/apotonick/reform/commits{/sha}","git_commits_url":"https://api.github.com/repos/apotonick/reform/git/commits{/sha}","comments_url":"https://api.github.com/repos/apotonick/reform/comments{/number}","issue_comment_url":"https://api.github.com/repos/apotonick/reform/issues/comments{/number}","contents_url":"https://api.github.com/repos/apotonick/reform/contents/{+path}","compare_url":"https://api.github.com/repos/apotonick/reform/compare/{base}...{head}","merges_url":"https://api.github.com/repos/apotonick/reform/merges","archive_url":"https://api.github.com/repos/apotonick/reform/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/apotonick/reform/downloads","issues_url":"https://api.github.com/repos/apotonick/reform/issues{/number}","pulls_url":"https://api.github.com/repos/apotonick/reform/pulls{/number}","milestones_url":"https://api.github.com/repos/apotonick/reform/milestones{/number}","notifications_url":"https://api.github.com/repos/apotonick/reform/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/apotonick/reform/labels{/name}","releases_url":"https://api.github.com/repos/apotonick/reform/releases{/id}","created_at":"2013-04-30T05:02:35Z","updated_at":"2015-06-15T00:42:15Z","pushed_at":"2015-06-15T03:50:46Z","git_url":"git://github.com/apotonick/reform.git","ssh_url":"git@github.com:apotonick/reform.git","clone_url":"https://github.com/apotonick/reform.git","svn_url":"https://github.com/apotonick/reform","homepage":"","size":2850,"stargazers_count":1517,"watchers_count":1517,"language":"Ruby","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":91,"mirror_url":null,"open_issues_count":45,"forks":91,"open_issues":45,"watchers":1517,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/apotonick/reform/pulls/233"},"html":{"href":"https://github.com/apotonick/reform/pull/233"},"issue":{"href":"https://api.github.com/repos/apotonick/reform/issues/233"},"comments":{"href":"https://api.github.com/repos/apotonick/reform/issues/233/comments"},"review_comments":{"href":"https://api.github.com/repos/apotonick/reform/pulls/233/comments"},"review_comment":{"href":"https://api.github.com/repos/apotonick/reform/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/apotonick/reform/pulls/233/commits"},"statuses":{"href":"https://api.github.com/repos/apotonick/reform/statuses/5ebed175fc37c727764f536c672cb5fb36f50cb5"}}}}