{"action":"created","issue":{"url":"https://api.github.com/repos/neo4jrb/neo4j/issues/824","labels_url":"https://api.github.com/repos/neo4jrb/neo4j/issues/824/labels{/name}","comments_url":"https://api.github.com/repos/neo4jrb/neo4j/issues/824/comments","events_url":"https://api.github.com/repos/neo4jrb/neo4j/issues/824/events","html_url":"https://github.com/neo4jrb/neo4j/pull/824","id":85588363,"number":824,"title":"Fix missing unique option","user":{"login":"olance","id":314079,"avatar_url":"https://avatars.githubusercontent.com/u/314079?v=3","gravatar_id":"","url":"https://api.github.com/users/olance","html_url":"https://github.com/olance","followers_url":"https://api.github.com/users/olance/followers","following_url":"https://api.github.com/users/olance/following{/other_user}","gists_url":"https://api.github.com/users/olance/gists{/gist_id}","starred_url":"https://api.github.com/users/olance/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/olance/subscriptions","organizations_url":"https://api.github.com/users/olance/orgs","repos_url":"https://api.github.com/users/olance/repos","events_url":"https://api.github.com/users/olance/events{/privacy}","received_events_url":"https://api.github.com/users/olance/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":5,"created_at":"2015-06-05T17:34:13Z","updated_at":"2015-06-08T01:33:29Z","closed_at":"2015-06-08T01:29:43Z","pull_request":{"url":"https://api.github.com/repos/neo4jrb/neo4j/pulls/824","html_url":"https://github.com/neo4jrb/neo4j/pull/824","diff_url":"https://github.com/neo4jrb/neo4j/pull/824.diff","patch_url":"https://github.com/neo4jrb/neo4j/pull/824.patch"},"body":"PR originating from [this discussion](https://github.com/neo4jrb/neo4j/commit/a88eb055cb531c1015c5cdd42c2559f64753248e#diff-17f68f769f3e31e7e8d67a4f4e2e66bdR322) on a missing option in the valid options array for `has_n` relationships.\r\n\r\nIts purpose is to fix two problems:\r\n**Problem 1:** The `:unique` option is (incorrectly) not considered a valid option anymore on `has_many`/`has_one` relationships.\r\n\r\n**Problem 2:** The tests have not failed in spite of this regression.\r\n\r\n## Fixing problem 1\r\nThis was just as easy as adding `:unique` to the array of valid options\r\n\r\n## Fixing problem 2\r\nThis was a bit more tricky, so I hope my solution will be suitable.\r\n\r\nThe tests haven't picked up this regression because they create associations using `Neo4j::ActiveNode::HasN::Association.new` directly, and options validations are performed at a higher level in `Neo4j::ActiveNode::HasN::ClassMethods#has_many` and `#has_one`.\r\n\r\nSo I have moved these validations down at the `Neo4j::ActiveNode::HasN::Association` level, which has had some consequences:\r\n\r\n* `:type` is now required in the options hash of `Neo4j::ActiveNode::HasN::Association#initialize`. I changed the default value of the `options` argument from `{}` to `{ type: nil }` to keep it easy to create an association that way without any option. However I had to change some specs in `association_specs` to make sure they include a `:type` key in the options they use.\r\n\r\n* The error messages from the validation are quite helpful as they indicate the name of the failing association and its owning class. This was possible when checking within `Neo4j::ActiveNode::HasN::ClassMethods` because the module is included inside the model class that would declare the association. As this is not the case with `Neo4j::ActiveNode::HasN::Association`, I have kept the behavior like so:  \r\n\r\n  * When an option validation error occurs, `Neo4j::ActiveNode::HasN::Association` will raise the same `ArgumentError` exceptions as before but without the \"(ClassName#association_name)\" part\r\n  * Within `Neo4j::ActiveNode::HasN::ClassMethods#build_association` (which actually creates the `HasN::Association` instance), `ArgumentError` exceptions are caught and re-raised with the same message, enhanced with the \"(ClassName#association_name)\" that we are able to get at this level.\r\n\r\n* There are tests checking the content of the above error messages. These tests are run against the `has_many`/`has_one` methods. Maybe they should move to the association specs?\r\n\r\nFortunately, the end result is that no other tests have failed due to these changes but the ones that were using the `:unique` option when it was not considered valid anymore.\r\n\r\nSo I do hope these changes will make it to the codebase :)"},"comment":{"url":"https://api.github.com/repos/neo4jrb/neo4j/issues/comments/109821477","html_url":"https://github.com/neo4jrb/neo4j/pull/824#issuecomment-109821477","issue_url":"https://api.github.com/repos/neo4jrb/neo4j/issues/824","id":109821477,"user":{"login":"cheerfulstoic","id":301415,"avatar_url":"https://avatars.githubusercontent.com/u/301415?v=3","gravatar_id":"","url":"https://api.github.com/users/cheerfulstoic","html_url":"https://github.com/cheerfulstoic","followers_url":"https://api.github.com/users/cheerfulstoic/followers","following_url":"https://api.github.com/users/cheerfulstoic/following{/other_user}","gists_url":"https://api.github.com/users/cheerfulstoic/gists{/gist_id}","starred_url":"https://api.github.com/users/cheerfulstoic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cheerfulstoic/subscriptions","organizations_url":"https://api.github.com/users/cheerfulstoic/orgs","repos_url":"https://api.github.com/users/cheerfulstoic/repos","events_url":"https://api.github.com/users/cheerfulstoic/events{/privacy}","received_events_url":"https://api.github.com/users/cheerfulstoic/received_events","type":"User","site_admin":false},"created_at":"2015-06-08T01:33:29Z","updated_at":"2015-06-08T01:33:29Z","body":"Done and released as 5.0.0.rc.3.  Thanks!"}}