[PATCH] Provide workarounds for broken MySQLdb modules

[Felix.Schwarz]

Sometimes, the MySQLdb module does not support the MySQL server version. In this example, it is MySQLdb 1.0 with MySQL 4.1. Essentially, this is a configuration error as one should use the correct module version for the database. Unfortunately, one of the best known Linux "enterprise" distributions (Redhat Enterprise Linux 4) does ship with a broken configuration (see  Bug 155341 in the Redhat Bugtracker).

Especially the changed format of timestamps may cause problems such as the exception message: "ValueError?: invalid literal for int()". This error is widely seen on systems with the configuration above (see  MySQLdb-Bugtracker,  report on the Python mailinglist). Fortunately, the error is easily fixable by providing an own converter (see  Ben Last's posting).

Currently it is not possible to provide a custom converter without modifying TurboGears. In this ticket, I look for the best way to implement this in TurboGears. I like to see an algorithm which only adds the needed workaround if a connection to an MySQL database is made, the version of the MySQLdb module is below/equal 1.0 and a special configuration option (e.g. "enable_mysql41_timestamp_workaround") is true (defaults to False).

Note that this addition is not meant to be a permanent part of TurboGears. Maybe it is better to produce a kind of contributed patch which people can add to their TurboGears installation.

This ticket is used to discuss several implementation strategies.

[alberto]

Well, If it's something necessary to use MySQL with TG in some common scenarios I thiing it's best to incorporate the work-arounf in TG itself.

Can anyone with a better MySQL-fooness than me step in and comment?

Alberto

[Felix.Schwarz]

This is file contains some test cases for the added functionality.

[Felix.Schwarz]

Alberto: Please review the patches/test cases. I would be very happy if this would be included in TurboGears. One patch less for me to apply separately :-)

[Felix.Schwarz]

Some additional comments: The main point is "connection.kwconv? = conversions" (line 125) where the converters for the new connection are installed. Another interesting place is _mysql_timestamp_converter which does the actual conversion. It tries to guess if this is the new timestamp format (I'm using the same condition as MySQLdb 1.2), else MySQLdb 1.0 does the job.

The other code is mostly there to ensure that my converter is only called if MySQLdb's version is below 1.2, it is a MySQL connection and the workaround is enabled in the configuration.

fs

[alberto]

Felix,

Your patch to database.py requires MySQLdb.converters which TG does not include by default. Could you please code it in a way that it will degrade gracefully if MySQL drivers are not installed? (real apps use PostgreSQL :P) Removing [PATCH] for the moment

Thanks for your work on this!

Alberto

P.S please read diffing guidelines at my last comment of #1115. It really makes comitters' job easier...

[Felix.Schwarz]

Can you point me to exact line number? I tried to make sure that MySQLdb is only required if a MySQL connection was specified (in this case, the MySQLdb module should be installed).

[Felix.Schwarz]

One thing I would like to add: The attached patch solves another small bug in reset(): threading_local is unknown in database.py, therefore the additional sqlobject import. The test case should uncover this bug too, so I did not file a separate bug report.

[alberto]

Hi Felix,

This is the traceback when running nosetests:

======================================================================
ERROR: Test that the actual converter function is ok.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/alberto/src/python/checkouts/turbogears/turbogears/tests/test_mysql_incompatibility.py", line 159, in test_converter
    _mysql_timestamp_converter("20070114165114"))
  File "/Users/alberto/src/python/checkouts/turbogears/turbogears/database.py", line 84, in _mysql_timestamp_converter
    import MySQLdb.converters
ImportError: No module named MySQLdb.converters

----------------------------------------------------------------------

[Felix.Schwarz]

This patch obsoletes test_mysql_incompatibility.py too

[Felix.Schwarz]

I forgot to execute one test case only conditionally so the converter routine did always run. This is the only thing that really changed in the new patch. Furthermore the patch now includes everything (both files). Should be okay now.

[alberto]

Hmmm, It seems Trac has decided to go grazy... No matter how many times I download your last patch I seem to get the old one. The preview looks fine (both files appear to be patched) but the one I get when downloading is the first one (only patches database.py) and does funky thinks when applied. BTW, the one at the preview looks prefectly fine.

Can you try posting it with another name to work around it until we file this as a bug or find out wtf is going wrong?

Thanks and sorry for the inconvenience...

Alberto

[Felix.Schwarz]

Attaching new unified diff again as per request

[Felix.Schwarz]

I just remembered why I provided two separate patches here: The test case is no real "unittest" but more a kind of a acceptance test because the test needs to connect to a real MySQL database (creating a table, destroying one) as noted in the test case's docstring. Therefore I don't think the test should not be included in the standard unit test suite but would be a valuable addition to an acceptance test suite.

[alberto]

Replying to Felix.Schwarz:

I just remembered why I provided two separate patches here: The test case is no real "unittest" but more a kind of a acceptance test because the test needs to connect to a real MySQL database (creating a table, destroying one) as noted in the test case's docstring. Therefore I don't think the test should not be included in the standard unit test suite but would be a valuable addition to an acceptance test suite.

I see... well, I'll take your word for it then as I can't test this myself due to a lack of testing environment :)

Alberto

[alberto]

Comitted fix without acceptance test in [2405]. Thanks Felix! :)

ALberto

[alberto]

That should be [2406]