)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"91e00f61b2a1bd343fac887dfa78d32730eda965","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Peter Feiner \u003cpeter@gridcentric.ca\u003e"},{"line_number":5,"context_line":"CommitDate: 2013-11-19 10:50:08 -0500"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"fix races in sql assignment backend"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Make judicious use of transactions: one per entry into the driver."},{"line_number":10,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"AAAATn%2F%2BwaM%3D","line":7,"updated":"2013-11-20 16:12:20.000000000","message":"(NIT): It sometimes helps to follow this guide on git commit messages: \n\nhttps://wiki.openstack.org/wiki/GitCommitMessages\n\nWhere the title summarizes the change and the paragraph after describes the reason for the change.","commit_id":"8a29f0bdeb4dba387eb52a45d0d007f04c35218e"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"91e00f61b2a1bd343fac887dfa78d32730eda965","unresolved":false,"context_lines":[{"line_number":10,"context_line":""},{"line_number":11,"context_line":"Also removed pointless session.flush() calls at the ends of transactions."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Fixes bug #1246489."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: I958d48bafc7fcb95f1d9ea71e408b4cefc2469c9"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"AAAATn%2F%2BwbM%3D","line":13,"updated":"2013-11-20 16:12:20.000000000","message":"Suggest using Closes Bug: #1246489","commit_id":"8a29f0bdeb4dba387eb52a45d0d007f04c35218e"},{"author":{"_account_id":6486,"name":"Brant Knudson","email":"blk@acm.org","username":"blk-u"},"change_message_id":"f5b422f36ec11479c6cde29298297d2f743a734a","unresolved":false,"context_lines":[{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Also removed pointless session.flush() calls at the ends of transactions."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Closes-Bug: #1246489."},{"line_number":21,"context_line":"Change-Id: I958d48bafc7fcb95f1d9ea71e408b4cefc2469c9"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"AAAATn%2F%2BAVc%3D","line":20,"updated":"2013-12-03 16:26:33.000000000","message":"(nit) remove .","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"}],"keystone/assignment/backends/sql.py":[{"author":{"_account_id":2903,"name":"Morgan Fainberg","email":"morgan.fainberg@gmail.com","username":"mdrnstm"},"change_message_id":"9cfc922d9935fab5dabf6e91a1d259809d9aa5b7","unresolved":false,"context_lines":[{"line_number":365,"context_line":"        # assignment table, simplifying the logic of this (and many other)"},{"line_number":366,"context_line":"        # functions."},{"line_number":367,"context_line":""},{"line_number":368,"context_line":"        session \u003d self.get_session()"},{"line_number":369,"context_line":"        with session.begin():"},{"line_number":370,"context_line":"            assignment_list \u003d []"},{"line_number":371,"context_line":"            refs \u003d session.query(UserDomainGrant).all()"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAATn%2F%2B4Mc%3D","line":368,"updated":"2013-11-19 02:23:10.000000000","message":"Is there a reason this is not using the \"with self._transaction() as session\" context manager semantic?","commit_id":"d1c8a453a7db8845e5aedf8e437ed5cf4949a96c"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"da705c29530af01a4b96829a164f5102629af4d9","unresolved":false,"context_lines":[{"line_number":365,"context_line":"        # assignment table, simplifying the logic of this (and many other)"},{"line_number":366,"context_line":"        # functions."},{"line_number":367,"context_line":""},{"line_number":368,"context_line":"        session \u003d self.get_session()"},{"line_number":369,"context_line":"        with session.begin():"},{"line_number":370,"context_line":"            assignment_list \u003d []"},{"line_number":371,"context_line":"            refs \u003d session.query(UserDomainGrant).all()"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAATn%2F%2B1U8%3D","line":368,"in_reply_to":"AAAATn%2F%2B4Mc%3D","updated":"2013-11-19 15:45:23.000000000","message":"No reason. Will change!","commit_id":"d1c8a453a7db8845e5aedf8e437ed5cf4949a96c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"91e00f61b2a1bd343fac887dfa78d32730eda965","unresolved":false,"context_lines":[{"line_number":241,"context_line":"            return domain_ids"},{"line_number":242,"context_line":""},{"line_number":243,"context_line":"        def _project_ids_to_dicts(ids):"},{"line_number":244,"context_line":"            return [self._get_project(session, x).to_dict() for x in ids]"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"        # NOTE(henry-nash): The metadata management code doesn\u0027t always clean"},{"line_number":247,"context_line":"        # up table entries when the last role is deleted - so when checking"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAATn%2F%2BwZ4%3D","line":244,"updated":"2013-11-20 16:12:20.000000000","message":"Suggest changing \u0027x\u0027 to \u0027id\u0027 to help readability.","commit_id":"8a29f0bdeb4dba387eb52a45d0d007f04c35218e"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"91e00f61b2a1bd343fac887dfa78d32730eda965","unresolved":false,"context_lines":[{"line_number":500,"context_line":"        metadata_ref \u003d q.first()"},{"line_number":501,"context_line":"        metadata_ref.data.update(metadata)"},{"line_number":502,"context_line":""},{"line_number":503,"context_line":"        # We manually mark metadata_ref.data as modified since SQLAlchemy may"},{"line_number":504,"context_line":"        # not automatically detect the change. Why not? Well... SQLAlchemy"},{"line_number":505,"context_line":"        # knows that an attribute has changed either if (1) somebody has marked"},{"line_number":506,"context_line":"        # it as mutated, or (2) the attribute\u0027s value at load-time !\u003d the"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAATn%2F%2BwY0%3D","line":503,"updated":"2013-11-20 16:12:20.000000000","message":"Nice comment! Suggest adding # NOTE(\u003cname\u003e): here similar to the format of TODOs and FIXMEs: \n\nhttps://github.com/openstack/keystone/blob/master/HACKING.rst#todo-vs-fixme","commit_id":"8a29f0bdeb4dba387eb52a45d0d007f04c35218e"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"91e00f61b2a1bd343fac887dfa78d32730eda965","unresolved":false,"context_lines":[{"line_number":505,"context_line":"        # knows that an attribute has changed either if (1) somebody has marked"},{"line_number":506,"context_line":"        # it as mutated, or (2) the attribute\u0027s value at load-time !\u003d the"},{"line_number":507,"context_line":"        # flush-time value. Since we don\u0027t track mutations to JsonBlob columns"},{"line_number":508,"context_line":"        # (see \"Mutation Tracking\" in SQLAlchemy\u0027s documentation), we can\u0027t"},{"line_number":509,"context_line":"        # count on (1). Since metadata_ref.data is often the same object as"},{"line_number":510,"context_line":"        # metadata (i.e., we return metadata_ref.data in self._get_metadata,"},{"line_number":511,"context_line":"        # manipulate it, then pass it to self._update_metadata), the check in"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAATn%2F%2BwZY%3D","line":508,"updated":"2013-11-20 16:12:20.000000000","message":"\"Mutation Tracking\" in SQLAlchemy\u0027s documentation \n\ncan you provide a link here?","commit_id":"8a29f0bdeb4dba387eb52a45d0d007f04c35218e"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"70cf71ebd6007ccf2e10aaf35a4a3d5f332ec6d5","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        migration.db_sync(version\u003dversion)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    @contextmanager"},{"line_number":39,"context_line":"    def _transaction(self):"},{"line_number":40,"context_line":"        session \u003d self.get_session()"},{"line_number":41,"context_line":"        with session.begin():"},{"line_number":42,"context_line":"            yield session"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BusE%3D","line":39,"updated":"2013-11-21 00:18:33.000000000","message":"Two points:\n\n1. This may be useful in the sql.Base class so that it can be used in the other SQL backends.\n\n2. In the SQLAlchemy docs this call their example session_scope.  How do you feel about a name change?\n\nSo it would be:\n\n    with self._session_scope() as session\n\nInstead of:\n\n    with self._transaction() as session\n\nYou have to scroll down a little to see the examples: http://bit.ly/1c3YLEn","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"b3be5dd39a49de37beccabc118dde17362b88db9","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        migration.db_sync(version\u003dversion)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    @contextmanager"},{"line_number":39,"context_line":"    def _transaction(self):"},{"line_number":40,"context_line":"        session \u003d self.get_session()"},{"line_number":41,"context_line":"        with session.begin():"},{"line_number":42,"context_line":"            yield session"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BrZE%3D","line":39,"in_reply_to":"AAAATn%2F%2BusE%3D","updated":"2013-11-21 17:21:03.000000000","message":"I think \"transaction\" more clearly expresses intent \u0026 behavior. Everybody knows what a transaction is but a \"session\" is sqlalchemy jargon. Moreover, a session doesn\u0027t necessarily correspond to an ACID transaction, as is the case when you use autocommit\u003dTrue.","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"70cf71ebd6007ccf2e10aaf35a4a3d5f332ec6d5","unresolved":false,"context_lines":[{"line_number":39,"context_line":"    def _transaction(self):"},{"line_number":40,"context_line":"        session \u003d self.get_session()"},{"line_number":41,"context_line":"        with session.begin():"},{"line_number":42,"context_line":"            yield session"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    def _get_project(self, session, project_id):"},{"line_number":45,"context_line":"        project_ref \u003d session.query(Project).get(project_id)"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BvGU%3D","line":42,"updated":"2013-11-21 00:18:33.000000000","message":"Is there any reason that you are not explicitly committing or rolling back like:\n\n    session \u003d self.get_session()\n    session.begin()\n    try:\n        yield session\n        session.commit()\n    except:\n        session.rollback()\n        raise\n    finally:\n        session.close()","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"54df2b4660fa9e8ea4f292102d125a24e6e43251","unresolved":false,"context_lines":[{"line_number":39,"context_line":"    def _transaction(self):"},{"line_number":40,"context_line":"        session \u003d self.get_session()"},{"line_number":41,"context_line":"        with session.begin():"},{"line_number":42,"context_line":"            yield session"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    def _get_project(self, session, project_id):"},{"line_number":45,"context_line":"        project_ref \u003d session.query(Project).get(project_id)"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2Bq7Q%3D","line":42,"in_reply_to":"AAAATn%2F%2BrAo%3D","updated":"2013-11-21 18:49:15.000000000","message":"Yes, exactly. \n\nSee the mention of python\u0027s with statement at the end of the section on autocommit mode in http://docs.sqlalchemy.org/en/rel_0_9/orm/session.html#autocommit-mode.","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"120c4b59f633543d86199c98d42f44376d1e9b8a","unresolved":false,"context_lines":[{"line_number":39,"context_line":"    def _transaction(self):"},{"line_number":40,"context_line":"        session \u003d self.get_session()"},{"line_number":41,"context_line":"        with session.begin():"},{"line_number":42,"context_line":"            yield session"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    def _get_project(self, session, project_id):"},{"line_number":45,"context_line":"        project_ref \u003d session.query(Project).get(project_id)"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BrAo%3D","line":42,"in_reply_to":"AAAATn%2F%2BrZU%3D","updated":"2013-11-21 18:23:01.000000000","message":"That\u0027s interesting. I don\u0027t remember ever seeing that before. If that\u0027s the case your just creating a new method because you feel it\u0027s more expressive than \u0027with self.get_session().begin():\u0027?","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"b3be5dd39a49de37beccabc118dde17362b88db9","unresolved":false,"context_lines":[{"line_number":39,"context_line":"    def _transaction(self):"},{"line_number":40,"context_line":"        session \u003d self.get_session()"},{"line_number":41,"context_line":"        with session.begin():"},{"line_number":42,"context_line":"            yield session"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":"    def _get_project(self, session, project_id):"},{"line_number":45,"context_line":"        project_ref \u003d session.query(Project).get(project_id)"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BrZU%3D","line":42,"in_reply_to":"AAAATn%2F%2BvGU%3D","updated":"2013-11-21 17:21:03.000000000","message":"What I have is more concise and has exactly the same behavior. The key is that session.begin() returns an object with and __exit__ method that has all of the commit, rollback, and close logic.","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"70cf71ebd6007ccf2e10aaf35a4a3d5f332ec6d5","unresolved":false,"context_lines":[{"line_number":241,"context_line":"            return domain_ids"},{"line_number":242,"context_line":""},{"line_number":243,"context_line":"        def _project_ids_to_dicts(ids):"},{"line_number":244,"context_line":"            return [self._get_project(session, id).to_dict() for id in ids]"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"        # NOTE(henry-nash): The metadata management code doesn\u0027t always clean"},{"line_number":247,"context_line":"        # up table entries when the last role is deleted - so when checking"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2Buss%3D","line":244,"updated":"2013-11-21 00:18:33.000000000","message":"I know you were told in an earlier review that x should have been renamed to id, but id is a builtin and shouldn\u0027t be used as an identifier.  Maybe it could be project_id?  I am also fine with single letter names in situations like this (x, i, etc), but I\u0027m not sure how the core reviews feel about that.","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"b3be5dd39a49de37beccabc118dde17362b88db9","unresolved":false,"context_lines":[{"line_number":241,"context_line":"            return domain_ids"},{"line_number":242,"context_line":""},{"line_number":243,"context_line":"        def _project_ids_to_dicts(ids):"},{"line_number":244,"context_line":"            return [self._get_project(session, id).to_dict() for id in ids]"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"        # NOTE(henry-nash): The metadata management code doesn\u0027t always clean"},{"line_number":247,"context_line":"        # up table entries when the last role is deleted - so when checking"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BrYc%3D","line":244,"in_reply_to":"AAAATn%2F%2Buss%3D","updated":"2013-11-21 17:21:03.000000000","message":"changed to project_id","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"70cf71ebd6007ccf2e10aaf35a4a3d5f332ec6d5","unresolved":false,"context_lines":[{"line_number":334,"context_line":"            try:"},{"line_number":335,"context_line":"                metadata_ref \u003d self._get_metadata(user_id, tenant_id,"},{"line_number":336,"context_line":"                                                  session\u003dsession)"},{"line_number":337,"context_line":"            except exception.MetadataNotFound:"},{"line_number":338,"context_line":"                msg \u003d \u0027Cannot remove role that has not been granted, %s\u0027 %\\"},{"line_number":339,"context_line":"                      role_id"},{"line_number":340,"context_line":"                raise exception.RoleNotFound(message\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2Bu8E%3D","line":337,"updated":"2013-11-21 00:18:33.000000000","message":"++ Love that you limited the code in the try block.","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"70cf71ebd6007ccf2e10aaf35a4a3d5f332ec6d5","unresolved":false,"context_lines":[{"line_number":335,"context_line":"                metadata_ref \u003d self._get_metadata(user_id, tenant_id,"},{"line_number":336,"context_line":"                                                  session\u003dsession)"},{"line_number":337,"context_line":"            except exception.MetadataNotFound:"},{"line_number":338,"context_line":"                msg \u003d \u0027Cannot remove role that has not been granted, %s\u0027 %\\"},{"line_number":339,"context_line":"                      role_id"},{"line_number":340,"context_line":"                raise exception.RoleNotFound(message\u003dmsg)"},{"line_number":341,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2Bu7o%3D","line":338,"updated":"2013-11-21 00:18:33.000000000","message":"I know that you just moved the line, but since it\u0027s all green I feel it\u0027s fair game :-)\n\nThe message portion should be wrapped in _().  So something like: _(\"..... %s\") % role_id","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"b3be5dd39a49de37beccabc118dde17362b88db9","unresolved":false,"context_lines":[{"line_number":335,"context_line":"                metadata_ref \u003d self._get_metadata(user_id, tenant_id,"},{"line_number":336,"context_line":"                                                  session\u003dsession)"},{"line_number":337,"context_line":"            except exception.MetadataNotFound:"},{"line_number":338,"context_line":"                msg \u003d \u0027Cannot remove role that has not been granted, %s\u0027 %\\"},{"line_number":339,"context_line":"                      role_id"},{"line_number":340,"context_line":"                raise exception.RoleNotFound(message\u003dmsg)"},{"line_number":341,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BrW8%3D","line":338,"in_reply_to":"AAAATn%2F%2Bu7o%3D","updated":"2013-11-21 17:21:03.000000000","message":"Done","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"658a550997c808cc81efe99f7a700c55238fc286","unresolved":false,"context_lines":[{"line_number":500,"context_line":"        metadata_ref \u003d q.first()"},{"line_number":501,"context_line":"        metadata_ref.data.update(metadata)"},{"line_number":502,"context_line":""},{"line_number":503,"context_line":"        # NOTE(peter): We manually mark metadata_ref.data as modified since"},{"line_number":504,"context_line":"        # SQLAlchemy may not automatically detect the change. Why not? Well..."},{"line_number":505,"context_line":"        # SQLAlchemy knows that an attribute has changed either if (1) somebody"},{"line_number":506,"context_line":"        # has marked it as mutated, or (2) the attribute\u0027s value at load-time"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2Br0M%3D","line":503,"updated":"2013-11-21 15:23:43.000000000","message":"(NIT): Thanks for adding this. Another quick note, just make sure that the name you use is something that can be traced back to you, more often than not you see people using the name that they can be reached by on IRC #openstack-dev or something. \n\nEx. \n\n# NOTE(ldbragst):","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"b3be5dd39a49de37beccabc118dde17362b88db9","unresolved":false,"context_lines":[{"line_number":500,"context_line":"        metadata_ref \u003d q.first()"},{"line_number":501,"context_line":"        metadata_ref.data.update(metadata)"},{"line_number":502,"context_line":""},{"line_number":503,"context_line":"        # NOTE(peter): We manually mark metadata_ref.data as modified since"},{"line_number":504,"context_line":"        # SQLAlchemy may not automatically detect the change. Why not? Well..."},{"line_number":505,"context_line":"        # SQLAlchemy knows that an attribute has changed either if (1) somebody"},{"line_number":506,"context_line":"        # has marked it as mutated, or (2) the attribute\u0027s value at load-time"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BrWw%3D","line":503,"in_reply_to":"AAAATn%2F%2Br0M%3D","updated":"2013-11-21 17:21:03.000000000","message":"changed to my launchpad name pete5\n\nI used peter because that\u0027s my gerrit name. I\u0027m always surprised when I can snap that up :-)","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"40f0112dee79ef42f47f84b578d0381dd0cea0af","unresolved":false,"context_lines":[{"line_number":500,"context_line":"        metadata_ref \u003d q.first()"},{"line_number":501,"context_line":"        metadata_ref.data.update(metadata)"},{"line_number":502,"context_line":""},{"line_number":503,"context_line":"        # NOTE(peter): We manually mark metadata_ref.data as modified since"},{"line_number":504,"context_line":"        # SQLAlchemy may not automatically detect the change. Why not? Well..."},{"line_number":505,"context_line":"        # SQLAlchemy knows that an attribute has changed either if (1) somebody"},{"line_number":506,"context_line":"        # has marked it as mutated, or (2) the attribute\u0027s value at load-time"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F9944%3D","line":503,"in_reply_to":"AAAATn%2F%2BrWw%3D","updated":"2013-12-03 22:28:34.000000000","message":"Thanks again, appreciate it!","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"70cf71ebd6007ccf2e10aaf35a4a3d5f332ec6d5","unresolved":false,"context_lines":[{"line_number":514,"context_line":"        # hasn\u0027t changed."},{"line_number":515,"context_line":"        sql.flag_modified(metadata_ref, \u0027data\u0027)"},{"line_number":516,"context_line":""},{"line_number":517,"context_line":"        session.flush()"},{"line_number":518,"context_line":"        return metadata_ref"},{"line_number":519,"context_line":""},{"line_number":520,"context_line":"    # domain crud"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2Bu50%3D","line":517,"updated":"2013-11-21 00:18:33.000000000","message":"Do we still need the flush here now that this is part of a larger transaction?","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"b3be5dd39a49de37beccabc118dde17362b88db9","unresolved":false,"context_lines":[{"line_number":514,"context_line":"        # hasn\u0027t changed."},{"line_number":515,"context_line":"        sql.flag_modified(metadata_ref, \u0027data\u0027)"},{"line_number":516,"context_line":""},{"line_number":517,"context_line":"        session.flush()"},{"line_number":518,"context_line":"        return metadata_ref"},{"line_number":519,"context_line":""},{"line_number":520,"context_line":"    # domain crud"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATn%2F%2BrUA%3D","line":517,"in_reply_to":"AAAATn%2F%2Bu50%3D","updated":"2013-11-21 17:21:03.000000000","message":"All of the calls to flush() could be removed. All they do is effect outstanding ORM queries in the DBMS. The only behavioural difference could be that errors due to constraints enforced by the DBMS are detected and raised earlier in Python. Failing earlier, or failing fast, helps with debugging and precludes pointless work that will be rolled back.\n\nA side effect of flush() is that the ORM can drop state; assuming that the DBMS is more efficient than sqlalchemy, keeping the transaction\u0027s state in the DBMS is probably a good thing. However, this is a pretty hand-wavy argument, so I wouldn\u0027t put much stock in it without empirical validation.\n\nAnyhow, I just removed the flush() calls that happened immediately before the commits because they were truly pointless. I could go through and remove all of the flush calls, but I think the fail-fast argument is solid enough to justify keeping the flush() calls in.","commit_id":"46a6307429831164225b3215cdae8e2feb28f9d7"},{"author":{"_account_id":7191,"name":"Jamie Lennox","email":"jamielennox@gmail.com","username":"jamielennox"},"change_message_id":"a2aa28e1e1aeb9b1758a9fd9dcc25d91e165f8bd","unresolved":false,"context_lines":[{"line_number":40,"context_line":"        return project_ref"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":"    def get_project(self, tenant_id):"},{"line_number":43,"context_line":"        with self.transaction() as session:"},{"line_number":44,"context_line":"            return self._get_project(session, tenant_id).to_dict()"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    def get_project_by_name(self, tenant_name, domain_id):"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2BiOU%3D","line":43,"updated":"2013-11-25 00:43:56.000000000","message":"This would appear to do nothing - why do we want a transaction around a single call?","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":7191,"name":"Jamie Lennox","email":"jamielennox@gmail.com","username":"jamielennox"},"change_message_id":"a2aa28e1e1aeb9b1758a9fd9dcc25d91e165f8bd","unresolved":false,"context_lines":[{"line_number":44,"context_line":"            return self._get_project(session, tenant_id).to_dict()"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    def get_project_by_name(self, tenant_name, domain_id):"},{"line_number":47,"context_line":"        with self.transaction() as session:"},{"line_number":48,"context_line":"            query \u003d session.query(Project)"},{"line_number":49,"context_line":"            query \u003d query.filter_by(name\u003dtenant_name)"},{"line_number":50,"context_line":"            query \u003d query.filter_by(domain_id\u003ddomain_id)"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2BiOQ%3D","line":47,"updated":"2013-11-25 00:43:56.000000000","message":"ditto, single call","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":6486,"name":"Brant Knudson","email":"blk@acm.org","username":"blk-u"},"change_message_id":"f5b422f36ec11479c6cde29298297d2f743a734a","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"    def _get_metadata(self, user_id\u003dNone, tenant_id\u003dNone,"},{"line_number":67,"context_line":"                      domain_id\u003dNone, group_id\u003dNone, session\u003dNone):"},{"line_number":68,"context_line":"        # We aren\u0027t given a session when called by the manager directly."},{"line_number":69,"context_line":"        if session is None:"},{"line_number":70,"context_line":"            session \u003d self.get_session()"},{"line_number":71,"context_line":"        if user_id:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2BADw%3D","line":68,"updated":"2013-12-03 16:26:33.000000000","message":"is this a problem then, since doesn\u0027t get called in a transaction as expected?","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"a51401024f6b0d8a8d05535b1e3240c2e0f5af24","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"    def _get_metadata(self, user_id\u003dNone, tenant_id\u003dNone,"},{"line_number":67,"context_line":"                      domain_id\u003dNone, group_id\u003dNone, session\u003dNone):"},{"line_number":68,"context_line":"        # We aren\u0027t given a session when called by the manager directly."},{"line_number":69,"context_line":"        if session is None:"},{"line_number":70,"context_line":"            session \u003d self.get_session()"},{"line_number":71,"context_line":"        if user_id:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F9%2Fw4%3D","line":68,"in_reply_to":"AAAATn%2F%2BADw%3D","updated":"2013-12-03 17:08:50.000000000","message":"Yes, this can cause problems. For example, Manager.remove_user_from_project makes a bunch of queries in separate transactions to get all of the roles a user has on a tenant, then calls the driver\u0027s remove_role_from_user_and_project on each role. Since a role could be deleted between driver calls, the Manager\u0027s remove_user_from_project can return a 404 even though not all of the roles have been deleted: for example, the second role in a list of five is deleted in another transaction, then the last 3 roles aren\u0027t deleted.\n\nThere are a couple of solutions to this problem. One would be to add try: except: around driver calls that could refer to data that can be deleted:\n\n    def remove_user_from_project(...)\n    ...\n        for role_id in roles:\n            try:\n                self.driver.remove_role_from_user_and_project(user_id, tenant_id,\n                                                          role_id)\n            except RoleNotFound:\n                pass\n\nAnother solution would be to add begin \u0026 end transaction methods to the driver interface and call them at the higher level spots in the manager. A simple begin \u0026 end transaction implementation could be to serialize everything with a lock; for sql, we\u0027d obviously use database transactions.\n\nI\u0027m pretty sure the try: except: approach would be a smaller patch, so that\u0027s what I\u0027d favor implementing.","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"e8dc089f5e0f81a55c37c3aead0ae358e92c4753","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"    def _get_metadata(self, user_id\u003dNone, tenant_id\u003dNone,"},{"line_number":67,"context_line":"                      domain_id\u003dNone, group_id\u003dNone, session\u003dNone):"},{"line_number":68,"context_line":"        # We aren\u0027t given a session when called by the manager directly."},{"line_number":69,"context_line":"        if session is None:"},{"line_number":70,"context_line":"            session \u003d self.get_session()"},{"line_number":71,"context_line":"        if user_id:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F9%2Fdk%3D","line":68,"in_reply_to":"AAAATn%2F9%2Fo8%3D","updated":"2013-12-03 17:42:44.000000000","message":"Filed bug #1257390.\n\nhttps://bugs.launchpad.net/keystone/+bug/1257390","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":6486,"name":"Brant Knudson","email":"blk@acm.org","username":"blk-u"},"change_message_id":"ee80a7e68c58578a3b3306113eba3ee483699528","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"    def _get_metadata(self, user_id\u003dNone, tenant_id\u003dNone,"},{"line_number":67,"context_line":"                      domain_id\u003dNone, group_id\u003dNone, session\u003dNone):"},{"line_number":68,"context_line":"        # We aren\u0027t given a session when called by the manager directly."},{"line_number":69,"context_line":"        if session is None:"},{"line_number":70,"context_line":"            session \u003d self.get_session()"},{"line_number":71,"context_line":"        if user_id:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F9%2Fo8%3D","line":68,"in_reply_to":"AAAATn%2F9%2Fw4%3D","updated":"2013-12-03 17:15:31.000000000","message":"Suggest change add a TODO(peter@gridcentric.ca): here and submit that change separately to not hold up the rest of this.","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":6486,"name":"Brant Knudson","email":"blk@acm.org","username":"blk-u"},"change_message_id":"f5b422f36ec11479c6cde29298297d2f743a734a","unresolved":false,"context_lines":[{"line_number":169,"context_line":"                     domain_id\u003dNone, project_id\u003dNone,"},{"line_number":170,"context_line":"                     inherited_to_projects\u003dFalse):"},{"line_number":171,"context_line":"        with self.transaction() as session:"},{"line_number":172,"context_line":"            self._delete_grant(session, role_id, user_id, group_id,"},{"line_number":173,"context_line":"                               domain_id, project_id, inherited_to_projects)"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def _delete_grant(self, session, role_id, user_id\u003dNone, group_id\u003dNone,"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2BACU%3D","line":172,"updated":"2013-12-03 16:26:33.000000000","message":"use named parameters for named arguments... e.g. change \"user_id, group_id, ...\" to \"user_id\u003duser_id, group_id\u003dgroup_id, ...\"","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"a51401024f6b0d8a8d05535b1e3240c2e0f5af24","unresolved":false,"context_lines":[{"line_number":169,"context_line":"                     domain_id\u003dNone, project_id\u003dNone,"},{"line_number":170,"context_line":"                     inherited_to_projects\u003dFalse):"},{"line_number":171,"context_line":"        with self.transaction() as session:"},{"line_number":172,"context_line":"            self._delete_grant(session, role_id, user_id, group_id,"},{"line_number":173,"context_line":"                               domain_id, project_id, inherited_to_projects)"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def _delete_grant(self, session, role_id, user_id\u003dNone, group_id\u003dNone,"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F9%2Ftc%3D","line":172,"in_reply_to":"AAAATn%2F%2BACU%3D","updated":"2013-12-03 17:08:50.000000000","message":"ok","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":7725,"name":"David Stanek","email":"dstanek@dstanek.com","username":"dstanek"},"change_message_id":"b4e0822cfb66a8aa5927df76cd5cd82c332cc257","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            return domain_ids"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"        def _project_ids_to_dicts(ids):"},{"line_number":236,"context_line":"            return [self._get_project(session, project_id).to_dict()"},{"line_number":237,"context_line":"                    for project_id in ids]"},{"line_number":238,"context_line":""},{"line_number":239,"context_line":"        # NOTE(henry-nash): The metadata management code doesn\u0027t always clean"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2BE04%3D","line":236,"updated":"2013-12-02 20:05:08.000000000","message":"It would be nice for this to limit the number or querys run to pull project data. Perhaps this is one thing to keep in mind for another future patch.","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":6486,"name":"Brant Knudson","email":"blk@acm.org","username":"blk-u"},"change_message_id":"f5b422f36ec11479c6cde29298297d2f743a734a","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            return domain_ids"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"        def _project_ids_to_dicts(ids):"},{"line_number":236,"context_line":"            return [self._get_project(session, project_id).to_dict()"},{"line_number":237,"context_line":"                    for project_id in ids]"},{"line_number":238,"context_line":""},{"line_number":239,"context_line":"        # NOTE(henry-nash): The metadata management code doesn\u0027t always clean"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2BAJ4%3D","line":236,"updated":"2013-12-03 16:26:33.000000000","message":"using session before it\u0027s been defined seems weird... suggest pass in the session.","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"a51401024f6b0d8a8d05535b1e3240c2e0f5af24","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            return domain_ids"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"        def _project_ids_to_dicts(ids):"},{"line_number":236,"context_line":"            return [self._get_project(session, project_id).to_dict()"},{"line_number":237,"context_line":"                    for project_id in ids]"},{"line_number":238,"context_line":""},{"line_number":239,"context_line":"        # NOTE(henry-nash): The metadata management code doesn\u0027t always clean"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F9%2Fsw%3D","line":236,"in_reply_to":"AAAATn%2F%2BAJ4%3D","updated":"2013-12-03 17:08:50.000000000","message":"Good catch! Will do.","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"61a90788040c9613aa737b8cf98edb4b1b82336d","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            return domain_ids"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"        def _project_ids_to_dicts(ids):"},{"line_number":236,"context_line":"            return [self._get_project(session, project_id).to_dict()"},{"line_number":237,"context_line":"                    for project_id in ids]"},{"line_number":238,"context_line":""},{"line_number":239,"context_line":"        # NOTE(henry-nash): The metadata management code doesn\u0027t always clean"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2BEeU%3D","line":236,"in_reply_to":"AAAATn%2F%2BE04%3D","updated":"2013-12-02 21:13:02.000000000","message":"Agreed and agreed: this could be done with a single query and should be done in a future patch :-)","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":6486,"name":"Brant Knudson","email":"blk@acm.org","username":"blk-u"},"change_message_id":"acb2300778b1e9d5769171e12284656560d54911","unresolved":false,"context_lines":[{"line_number":169,"context_line":"                     domain_id\u003dNone, project_id\u003dNone,"},{"line_number":170,"context_line":"                     inherited_to_projects\u003dFalse):"},{"line_number":171,"context_line":"        with self.transaction() as session:"},{"line_number":172,"context_line":"            self._delete_grant(session, role_id, user_id, group_id\u003dgroup_id,"},{"line_number":173,"context_line":"                               domain_id\u003ddomain_id, project_id\u003dproject_id,"},{"line_number":174,"context_line":"                               inherited_to_projects\u003dinherited_to_projects)"},{"line_number":175,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAATn%2F9%2ByQ%3D","line":172,"updated":"2013-12-03 19:58:55.000000000","message":"use named parameters for named arguments, change \"user_id\" to \"user_id\u003duser_id\"","commit_id":"a5c90d1cbe724bbc11017b6001f4863c33347fac"}],"keystone/common/sql/core.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"91e00f61b2a1bd343fac887dfa78d32730eda965","unresolved":false,"context_lines":[{"line_number":21,"context_line":"import sqlalchemy.engine.url"},{"line_number":22,"context_line":"from sqlalchemy.exc import DisconnectionError"},{"line_number":23,"context_line":"from sqlalchemy.ext import declarative"},{"line_number":24,"context_line":"import sqlalchemy.orm.attributes"},{"line_number":25,"context_line":"from sqlalchemy.orm.attributes import InstrumentedAttribute"},{"line_number":26,"context_line":"import sqlalchemy.pool"},{"line_number":27,"context_line":"from sqlalchemy import types as sql_types"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAATn%2F%2BwYo%3D","line":24,"updated":"2013-11-20 16:12:20.000000000","message":"We are already importing sqlalchemy.orm.attributes on the line below when we are pulling in the class InstrumentedAttribute. Why not just import .attributes and refactor InstrumentedAttribute accordingly since importing a class is going to fail the pep8 checks once hacking H302 goes in?\n\nhttps://review.openstack.org/#/c/56648/","commit_id":"8a29f0bdeb4dba387eb52a45d0d007f04c35218e"},{"author":{"_account_id":6486,"name":"Brant Knudson","email":"blk@acm.org","username":"blk-u"},"change_message_id":"f5b422f36ec11479c6cde29298297d2f743a734a","unresolved":false,"context_lines":[{"line_number":15,"context_line":"# under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"\"\"\"SQL backends for the various services.\"\"\""},{"line_number":18,"context_line":"from contextlib import contextmanager"},{"line_number":19,"context_line":"import functools"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"import sqlalchemy as sql"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2BAS0%3D","line":18,"updated":"2013-12-03 16:26:33.000000000","message":"don\u0027t import objects, so change to \"import contextlib\". see https://github.com/openstack-dev/hacking/blame/master/doc/source/index.rst#L52","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"a51401024f6b0d8a8d05535b1e3240c2e0f5af24","unresolved":false,"context_lines":[{"line_number":15,"context_line":"# under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"\"\"\"SQL backends for the various services.\"\"\""},{"line_number":18,"context_line":"from contextlib import contextmanager"},{"line_number":19,"context_line":"import functools"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"import sqlalchemy as sql"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F9%2Fsk%3D","line":18,"in_reply_to":"AAAATn%2F%2BAS0%3D","updated":"2013-12-03 17:08:50.000000000","message":"ok","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":6486,"name":"Brant Knudson","email":"blk@acm.org","username":"blk-u"},"change_message_id":"f5b422f36ec11479c6cde29298297d2f743a734a","unresolved":false,"context_lines":[{"line_number":78,"context_line":"        for k, v in kwargs.items():"},{"line_number":79,"context_line":"            if hasattr(cls, k):"},{"line_number":80,"context_line":"                attr \u003d getattr(cls, k)"},{"line_number":81,"context_line":"                if isinstance(attr, sql.orm.attributes.InstrumentedAttribute):"},{"line_number":82,"context_line":"                    column \u003d attr.property.columns[0]"},{"line_number":83,"context_line":"                    if isinstance(column.type, String):"},{"line_number":84,"context_line":"                        if not isinstance(v, unicode):"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2BAUE%3D","line":81,"updated":"2013-12-03 16:26:33.000000000","message":"How is this change required for doing one transaction per operation? Suggest revert it.","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":7500,"name":"Peter Feiner","email":"peter@gridcentric.ca","username":"peter"},"change_message_id":"a51401024f6b0d8a8d05535b1e3240c2e0f5af24","unresolved":false,"context_lines":[{"line_number":78,"context_line":"        for k, v in kwargs.items():"},{"line_number":79,"context_line":"            if hasattr(cls, k):"},{"line_number":80,"context_line":"                attr \u003d getattr(cls, k)"},{"line_number":81,"context_line":"                if isinstance(attr, sql.orm.attributes.InstrumentedAttribute):"},{"line_number":82,"context_line":"                    column \u003d attr.property.columns[0]"},{"line_number":83,"context_line":"                    if isinstance(column.type, String):"},{"line_number":84,"context_line":"                        if not isinstance(v, unicode):"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F9%2FrE%3D","line":81,"in_reply_to":"AAAATn%2F%2BAUE%3D","updated":"2013-12-03 17:08:50.000000000","message":"Another reviewer had suggested making the change in the spirit of the same HACKING rule you cited above (- Do not import objects, only modules (*)) since I had to add  \"import sqlalchemy.orm.attributes\" for flag_modified.\n\nSince I want the review to converge to +[12], so somebody ought to make an executive decision :-)","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"},{"author":{"_account_id":6486,"name":"Brant Knudson","email":"blk@acm.org","username":"blk-u"},"change_message_id":"ee80a7e68c58578a3b3306113eba3ee483699528","unresolved":false,"context_lines":[{"line_number":78,"context_line":"        for k, v in kwargs.items():"},{"line_number":79,"context_line":"            if hasattr(cls, k):"},{"line_number":80,"context_line":"                attr \u003d getattr(cls, k)"},{"line_number":81,"context_line":"                if isinstance(attr, sql.orm.attributes.InstrumentedAttribute):"},{"line_number":82,"context_line":"                    column \u003d attr.property.columns[0]"},{"line_number":83,"context_line":"                    if isinstance(column.type, String):"},{"line_number":84,"context_line":"                        if not isinstance(v, unicode):"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F9%2Fog%3D","line":81,"in_reply_to":"AAAATn%2F9%2FrE%3D","updated":"2013-12-03 17:15:31.000000000","message":"I don\u0027t mind the change itself, just that a cleanup like this should be made in a separate commit.\n\nAlso, there\u0027s an exception to the rule for sqlalchemy imports: https://github.com/openstack-dev/hacking/blame/master/doc/source/index.rst#L62","commit_id":"8dd9851e66a4fc6021ef88648869842d88d922ce"}]}