Edgewall Software
Modify

Opened 13 years ago

Closed 10 years ago

Last modified 8 years ago

#5120 closed enhancement (fixed)

[PATCH] mysql_backend should parse parameters

Reported by: anonymous Owned by: Thijs Triemstra
Priority: high Milestone: 1.0
Component: database backend Version: devel
Severity: normal Keywords: mysql patch bitesized
Cc: dekimsey@… Branch:
Release Notes:

mysql_backend passes any additonal connection parameters to the database

API Changes:
Internal Changes:

Description

Currently, mysql_backend.py only passes db, user, passwd, host, and port to MySQLdb. Any additional params are discarded. This is particularly unfortunate (at least in my case) because a number of MySQL installs do not use the default socket location, yet:

mysql://user:password@domain:3306/database?unix_socket=path/to/socket

fails, even though the backend would easily accept it.

Attachments (5)

mysql.cnf.hack.patch (1.9 KB ) - added by dekimsey@… 13 years ago.
this is a patch to add parameter support to source:/trunk/trac/db/mysql_backend.py@6299
mysql-params-5120.patch (910 bytes ) - added by Thijs Triemstra 10 years ago.
patch against 0.12-stable r10371
mysql-params-5120-2.patch (1.2 KB ) - added by Thijs Triemstra 10 years ago.
mysql-params-5120-3.patch (2.5 KB ) - added by Thijs Triemstra 10 years ago.
against 0.12-stable r10394
mysql-params-5120-4.patch (2.7 KB ) - added by Thijs Triemstra 10 years ago.
against 0.13-trunk r10394

Download all attachments as: .zip

Change History (27)

comment:1 by Christian Boos, 13 years ago

Component: trac-admingeneral
Keywords: mysql added
Milestone: 0.120.10.5
Owner: changed from Christopher Lenz to Jonas Borgström

(moving to 0.10.x, along with other mysql related issues)

comment:2 by dekimsey@…, 13 years ago

Agreed, this is actually kind of important in my opinion as I'd really prefer not to have the mysql connect information stored inside the trac.ini file. The mysql library is able to read a configuration file instead.

I implemented a bit of a hack to get the read_default_file parameter to be passed into the connect call, I might upload a patch of it if I can figure out the correct way to pass the params hash properly to the connect sub by name. (I'm not very familiar with Python, I'm a Perl person :/)

— Danny

comment:3 by dekimsey@…, 13 years ago

Cc: dekimsey@… added

The only issue I have run into so far in implementing this was that certain parameters cannot be left as None and must either be passed as something valid or not passed at all. Some trial and error and I've worked down which is which.

I simply push those with defined values onto the params hash and then pass that to the connect method. It works for me, I have not tested it with an older mysqldb library than the if block checks for.

— Danny.

by dekimsey@…, 13 years ago

Attachment: mysql.cnf.hack.patch added

this is a patch to add parameter support to source:/trunk/trac/db/mysql_backend.py@6299

comment:4 by Christian Boos, 12 years ago

Keywords: patch added
Milestone: 0.10.60.11.6

comment:5 by Remy Blank, 11 years ago

Keywords: bitesized added

by Thijs Triemstra, 10 years ago

Attachment: mysql-params-5120.patch added

patch against 0.12-stable r10371

comment:6 by Thijs Triemstra, 10 years ago

Milestone: next-minor-0.12.x0.12.2
Owner: changed from Jonas Borgström to Thijs Triemstra
Status: newassigned
Summary: mysql_backend should parse parameters[PATCH] mysql_backend should parse parameters

The attached patch passes in the params from the connection string, like unix_socket or use_unicode.

in reply to:  2 comment:7 by Thijs Triemstra, 10 years ago

Component: generaldatabase backend
Priority: normalhigh
Severity: normalmajor

Replying to dekimsey@…:

Agreed, this is actually kind of important in my opinion as I'd really prefer not to have the mysql connect information stored inside the trac.ini file. The mysql library is able to read a configuration file instead.

Agreed.

by Thijs Triemstra, 10 years ago

Attachment: mysql-params-5120-2.patch added

comment:8 by Thijs Triemstra, 10 years ago

Updated patch also passes in -S to mysqldump when doing a backup.

comment:9 by Remy Blank, 10 years ago

I'm not sure it makes sense to pass all additional parameters to MySQLdb.connect() as keyword arguments, as the values will always be strings. I would prefer we explicitly process and pass in selected parameters.

What's the list of desired parameters? unix_socket is certainly useful, maybe also compress, named_pipe, init_command and read_default_file?

comment:10 by Remy Blank, 10 years ago

#9949 asks for a way to connect to a MySQL server via SSL, and suggests using a ssl= parameter to specify the certificates and key.

by Thijs Triemstra, 10 years ago

Attachment: mysql-params-5120-3.patch added

against 0.12-stable r10394

comment:11 by Thijs Triemstra, 10 years ago

Milestone: 0.12.20.13
Type: defectenhancement

Added a patch for 0.12 but moving the ticket to 0.13 because #8954 also has been applied there. Also going to call it an enhancement.

by Thijs Triemstra, 10 years ago

Attachment: mysql-params-5120-4.patch added

against 0.13-trunk r10394

in reply to:  10 comment:12 by Thijs Triemstra, 10 years ago

Replying to rblank:

#9949 asks for a way to connect to a MySQL server via SSL, and suggests using a ssl= parameter to specify the certificates and key.

Attached a patch that's probably not pythonic but it gets the job done ;) Once this ticket is fixed it should be easy to add the additional options for SSL in #9949.

Tested with the following connection string:

mysql://admin:admin@localhost/trac?unix_socket=/tmp/mysql.sock&compress=1&
init_command=SELECT COUNT(name) FROM wiki&read_default_file=/tmp/test.cnf&read_default_group=abc

Also updated the options to mysqldump. Wasn't able to test the named_pipe option because it's windows only. See MysqlDb docs.

comment:13 by Christian Boos, 10 years ago

Looks good, however it lacks a bit of documentation: you should also list and explain the paramters in the doc string.

comment:14 by Remy Blank, 10 years ago

Resolution: fixed
Status: assignedclosed

Slightly simpler (and more pythonic) patch applied in [10407]. The changes were only in the form, not the function. The parameters are documented in the component docstring. I have also added a warning about unknown parameters.

Thijs, would you mind quickly checking that the arguments all work as expected?

in reply to:  14 ; comment:15 by Thijs Triemstra, 10 years ago

Replying to rblank:

Slightly simpler (and more pythonic) patch applied in [10407]. The changes were only in the form, not the function. The parameters are documented in the component docstring. I have also added a warning about unknown parameters.

Thijs, would you mind quickly checking that the arguments all work as expected?

Thanks for applying the patch, it all seems to work properly here.

in reply to:  15 comment:16 by Thijs Triemstra, 10 years ago

Resolution: fixed
Status: closedreopened

Replying to thijstriemstra:

Thanks for applying the patch, it all seems to work properly here.

I forgot to test hotcopy :-/ It's now throwing:

$ trac-admin . hotcopy /tmp/tester
Hotcopying /Users/thijstriemstra/Sites/projects/opensource/test3 to /tmp/tester ...
Backing up database ...
TracError: mysqldump failed: mysqldump: unknown variable 'defaults-file='

it seems mysqldump wants the defaults-file as it's first argument, which is the reason i didn't use db_params.iteritems() in the patch to ensure that defaults-file was the first param..

comment:17 by Remy Blank, 10 years ago

Resolution: fixed
Status: reopenedclosed

Good catch! Fixed in [10410].

comment:18 by Thijs Triemstra, 10 years ago

This ticket isnt marked as closed in the trunk browser view for some reason..

in reply to:  18 ; comment:19 by Remy Blank, 10 years ago

Replying to thijstriemstra:

This ticket isnt marked as closed in the trunk browser view for some reason..

That's because the link in the changeset is to a comment in the ticket (comment:16:ticket:5120), not to the ticket itself, and we seem to miss the strikeout in that case. Which can be considered as a bug.

in reply to:  19 comment:20 by Thijs Triemstra, 10 years ago

Replying to rblank:

That's because the link in the changeset is to a comment in the ticket (comment:16:ticket:5120), not to the ticket itself, and we seem to miss the strikeout in that case. Which can be considered as a bug.

Opened #9981 for that.

comment:21 by al.willmer@…, 9 years ago

Release Notes: modified (diff)

comment:22 by Christian Boos, 8 years ago

Severity: majornormal

Lower the severity of these MySQL enhancements as other major enhancements are really the highlights of this release.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Thijs Triemstra.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Thijs Triemstra to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.