)]}'
{"oslo/db/sqlalchemy/session.py":[{"author":{"_account_id":11279,"name":"Angus Lees","email":"gus@inodes.org","username":"gus"},"change_message_id":"f5ae9c6d3265614901be8f707faf033423c1084a","unresolved":false,"context_lines":[{"line_number":649,"context_line":"            connection \u003d sqlalchemy.engine.url.make_url(connection)"},{"line_number":650,"context_line":"        elif not isinstance(connection, sqlalchemy.engine.url.URL):"},{"line_number":651,"context_line":"            raise TypeError(_(\u0027connection must be either a string of an \u0027"},{"line_number":652,"context_line":"                              \u0027sqlalchemy.engine.url.URL instance\u0027))"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        for checker, drivername in cls.checkers:"},{"line_number":655,"context_line":"            if not drivername.match(connection.drivername):"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_8f1aa97a","line":652,"updated":"2014-07-17 02:24:39.000000000","message":"sqlalchemy.engine.url.make_url already returns URL objects directly afaics, so this if/elif is unnecessary - just call make_url(connection) unconditionally.","commit_id":"0cb0ebbebb1b82dfd069850e393719aa439d7006"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"c4ea05d8929c0ac5a067e2cbcb698563e1a17229","unresolved":false,"context_lines":[{"line_number":649,"context_line":"            connection \u003d sqlalchemy.engine.url.make_url(connection)"},{"line_number":650,"context_line":"        elif not isinstance(connection, sqlalchemy.engine.url.URL):"},{"line_number":651,"context_line":"            raise TypeError(_(\u0027connection must be either a string of an \u0027"},{"line_number":652,"context_line":"                              \u0027sqlalchemy.engine.url.URL instance\u0027))"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        for checker, drivername in cls.checkers:"},{"line_number":655,"context_line":"            if not drivername.match(connection.drivername):"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_c54aa454","line":652,"in_reply_to":"baada198_8f1aa97a","updated":"2014-07-17 15:33:10.000000000","message":"i think this is checking that the argument passed is something other than a string or URL.\n\nHowever what I think makes this confusing is the name \"connection\" (not to mention \"sql_connection\").  It should be something like db_url or something that makes it clear this isn\u0027t a database connection.","commit_id":"0cb0ebbebb1b82dfd069850e393719aa439d7006"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"b70503078c1abdc14d64a372d1a48bd6839838f1","unresolved":false,"context_lines":[{"line_number":649,"context_line":"            connection \u003d sqlalchemy.engine.url.make_url(connection)"},{"line_number":650,"context_line":"        elif not isinstance(connection, sqlalchemy.engine.url.URL):"},{"line_number":651,"context_line":"            raise TypeError(_(\u0027connection must be either a string of an \u0027"},{"line_number":652,"context_line":"                              \u0027sqlalchemy.engine.url.URL instance\u0027))"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        for checker, drivername in cls.checkers:"},{"line_number":655,"context_line":"            if not drivername.match(connection.drivername):"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_d9696b26","line":652,"in_reply_to":"baada198_c54aa454","updated":"2014-07-21 16:12:56.000000000","message":"Done","commit_id":"0cb0ebbebb1b82dfd069850e393719aa439d7006"},{"author":{"_account_id":11279,"name":"Angus Lees","email":"gus@inodes.org","username":"gus"},"change_message_id":"f5ae9c6d3265614901be8f707faf033423c1084a","unresolved":false,"context_lines":[{"line_number":652,"context_line":"                              \u0027sqlalchemy.engine.url.URL instance\u0027))"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        for checker, drivername in cls.checkers:"},{"line_number":655,"context_line":"            if not drivername.match(connection.drivername):"},{"line_number":656,"context_line":"                continue"},{"line_number":657,"context_line":""},{"line_number":658,"context_line":"            connection \u003d checker(connection)"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_1a6255d9","line":655,"updated":"2014-07-17 02:24:39.000000000","message":"Michael should tell me if I\u0027m wrong, but I think we want to match against connection.get_dialect().driver instead of connection.drivername.  This ensures we\u0027ve been through sqlalchemy\u0027s own URL-\u003edriver registry, and cases like \"mysql://\" will get correctly mapped to \"mysqldb\".\n\nIf we do that, the internals of \"connection.get_dialect().driver\" is probably expensive enough that it is worth pulling the result into a constant outside this \"for ... in cls.checkers\" loop too, fwiw.","commit_id":"0cb0ebbebb1b82dfd069850e393719aa439d7006"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"b70503078c1abdc14d64a372d1a48bd6839838f1","unresolved":false,"context_lines":[{"line_number":652,"context_line":"                              \u0027sqlalchemy.engine.url.URL instance\u0027))"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        for checker, drivername in cls.checkers:"},{"line_number":655,"context_line":"            if not drivername.match(connection.drivername):"},{"line_number":656,"context_line":"                continue"},{"line_number":657,"context_line":""},{"line_number":658,"context_line":"            connection \u003d checker(connection)"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_79569f5f","line":655,"in_reply_to":"baada198_05a1fca8","updated":"2014-07-21 16:12:56.000000000","message":"Done","commit_id":"0cb0ebbebb1b82dfd069850e393719aa439d7006"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"c4ea05d8929c0ac5a067e2cbcb698563e1a17229","unresolved":false,"context_lines":[{"line_number":652,"context_line":"                              \u0027sqlalchemy.engine.url.URL instance\u0027))"},{"line_number":653,"context_line":""},{"line_number":654,"context_line":"        for checker, drivername in cls.checkers:"},{"line_number":655,"context_line":"            if not drivername.match(connection.drivername):"},{"line_number":656,"context_line":"                continue"},{"line_number":657,"context_line":""},{"line_number":658,"context_line":"            connection \u003d checker(connection)"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_05a1fca8","line":655,"in_reply_to":"baada198_1a6255d9","updated":"2014-07-17 15:33:10.000000000","message":"I might keep it simple and just have awareness here that \"mysql://\" is really \"mysql+mysqldb\".   If it were today, SQLAlchemy wouldn\u0027t even allow the implicit DBAPI form, and it might not be a bad idea to disallow a URL like that in openstack.\n\nWe can also just do get_dialect().driver, this isn\u0027t very expensive considering that we\u0027re in the engine creation routine in any case.  get_dialect() just returns the uninstantated dialect class.","commit_id":"0cb0ebbebb1b82dfd069850e393719aa439d7006"},{"author":{"_account_id":11279,"name":"Angus Lees","email":"gus@inodes.org","username":"gus"},"change_message_id":"f5ae9c6d3265614901be8f707faf033423c1084a","unresolved":false,"context_lines":[{"line_number":663,"context_line":"@DefaultsChecker.register(drivername\u003dr\u0027^mysql.*$\u0027)"},{"line_number":664,"context_line":"def _mysql_set_charset(connection):"},{"line_number":665,"context_line":"    if \u0027charset\u0027 not in connection.query:"},{"line_number":666,"context_line":"        connection.query[\u0027charset\u0027] \u003d \u0027utf8\u0027"},{"line_number":667,"context_line":""},{"line_number":668,"context_line":"    return connection"},{"line_number":669,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_daa7fd09","line":666,"updated":"2014-07-17 02:24:39.000000000","message":"connection.query.setdefault(\u0027charset\u0027, \u0027utf8\u0027) might be easier on the eyes, once we collect a long list of such defaults.\n\nDitto for raise_on_warnings below.","commit_id":"0cb0ebbebb1b82dfd069850e393719aa439d7006"},{"author":{"_account_id":11279,"name":"Angus Lees","email":"gus@inodes.org","username":"gus"},"change_message_id":"f69ac66c8ee719380d68084e7b0d196789073e5f","unresolved":false,"context_lines":[{"line_number":663,"context_line":"@DefaultsChecker.register(drivername\u003dr\u0027^mysql.*$\u0027)"},{"line_number":664,"context_line":"def _mysql_set_charset(connection):"},{"line_number":665,"context_line":"    if \u0027charset\u0027 not in connection.query:"},{"line_number":666,"context_line":"        connection.query[\u0027charset\u0027] \u003d \u0027utf8\u0027"},{"line_number":667,"context_line":""},{"line_number":668,"context_line":"    return connection"},{"line_number":669,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_7f45ca41","line":666,"in_reply_to":"baada198_05881c21","updated":"2014-07-18 06:53:20.000000000","message":"This charset\u003dutf8 is currently in all the devstack configs, fwiw.\n\nFor all the reasons you and I discussed earlier(*), I think we require an explicit charset\u003d\u003csomething capable of representing unicode\u003e, and utf8 seems a fine choice within that set.  The question then is whether we set it here in the code like this, or mention it in all our documentation as being \"strongly recommended\" on the connection url when using mysqldb.\n\nAfaics we\u0027ve left it up to devstack/documentation previously, so I\u0027m surprised to see this change slipping in here without further mention.  Roman: was this intended to go in this change, or is this just an example of combining options across checkers?\n\n(*) Chiefly, I think sqlalchemy+mysqldb defaults to utf8 regardless of the server settings without also being explicitly told on the client side which charset the server will be expecting.","commit_id":"0cb0ebbebb1b82dfd069850e393719aa439d7006"},{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"b70503078c1abdc14d64a372d1a48bd6839838f1","unresolved":false,"context_lines":[{"line_number":663,"context_line":"@DefaultsChecker.register(drivername\u003dr\u0027^mysql.*$\u0027)"},{"line_number":664,"context_line":"def _mysql_set_charset(connection):"},{"line_number":665,"context_line":"    if \u0027charset\u0027 not in connection.query:"},{"line_number":666,"context_line":"        connection.query[\u0027charset\u0027] \u003d \u0027utf8\u0027"},{"line_number":667,"context_line":""},{"line_number":668,"context_line":"    return connection"},{"line_number":669,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_d9508b60","line":666,"in_reply_to":"baada198_7f45ca41","updated":"2014-07-21 16:12:56.000000000","message":"Yeah, this is was more like an example. I\u0027ll introduce this as a separate change.","commit_id":"0cb0ebbebb1b82dfd069850e393719aa439d7006"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"c4ea05d8929c0ac5a067e2cbcb698563e1a17229","unresolved":false,"context_lines":[{"line_number":663,"context_line":"@DefaultsChecker.register(drivername\u003dr\u0027^mysql.*$\u0027)"},{"line_number":664,"context_line":"def _mysql_set_charset(connection):"},{"line_number":665,"context_line":"    if \u0027charset\u0027 not in connection.query:"},{"line_number":666,"context_line":"        connection.query[\u0027charset\u0027] \u003d \u0027utf8\u0027"},{"line_number":667,"context_line":""},{"line_number":668,"context_line":"    return connection"},{"line_number":669,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_05881c21","line":666,"in_reply_to":"baada198_daa7fd09","updated":"2014-07-17 15:33:10.000000000","message":"do we even support other encodings besides utf8?   or does openstack default to utf8?","commit_id":"0cb0ebbebb1b82dfd069850e393719aa439d7006"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"4983ee148a63a0b3adbf12339c3ac5811141f433","unresolved":false,"context_lines":[{"line_number":436,"context_line":"    return False"},{"line_number":437,"context_line":""},{"line_number":438,"context_line":""},{"line_number":439,"context_line":"class UrlDefaultsChecker(object):"},{"line_number":440,"context_line":"    \"\"\"Allows to register callbacks which check and modify DB URL values."},{"line_number":441,"context_line":""},{"line_number":442,"context_line":"    Registration of callbacks::"}],"source_content_type":"text/x-python","patch_set":3,"id":"baada198_22b1288f","line":439,"updated":"2014-07-21 16:18:35.000000000","message":"I want to suggest that we put this in a module oslo/db/sqlalchemy/defaults.py, do away with having a UrlDefaultsChecker class here, and then have \"checkers\" as a private module-level variable there.  This will allow the approach here to be consistent with the one taken in exc_filters.py and will also serve to keep different features isolated in their own source files.  \"session.py\" is a pretty generic name and I\u0027m trying to move it away from being a \"catchall\" for various subsystems.\n\nIf OTOH we really like UrlDefaultsChecker to be a class with \"self.checkers\" as its state, then we should change exc_filters.py to use the same approach.   One or the other ! :)","commit_id":"8db70b3d659554cb5be84d2ec1ceb1b0e0a5bab4"}]}