Ticket #1235 (closed defect: fixed)

Opened 2 years ago

Last modified 1 year ago

[PATCH] Provide workarounds for broken MySQLdb modules

Reported by: Felix.Schwarz Assigned to: alberto
Priority: normal Milestone: 1.0.1
Component: SQLObject Version: 1.0
Severity: normal Keywords:
Cc:

Description

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.

Attachments

test_mysql_incompatibility.py (6.2 kB) - added by Felix.Schwarz on 01/14/07 10:29:49.
This is file contains some test cases for the added functionality.
mysql_timestamp_workaround.patch (10.0 kB) - added by Felix.Schwarz on 01/15/07 12:04:04.
This patch obsoletes test_mysql_incompatibility.py too
mysql_timestamp_workaround_v2.patch (10.0 kB) - added by Felix.Schwarz on 01/15/07 14:29:36.
Attaching new unified diff again as per request

Change History

01/13/07 05:10:55 changed by 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

01/14/07 10:29:49 changed by Felix.Schwarz

  • attachment test_mysql_incompatibility.py added.

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

01/14/07 10:32:18 changed by 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 :-)

01/15/07 02:07:37 changed by 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

01/15/07 03:53:27 changed by alberto

  • owner changed from anonymous to alberto.
  • status changed from new to assigned.
  • summary changed from Provide workarounds for broken MySQLdb modules to [PATCH] Provide workarounds for broken MySQLdb modules.

01/15/07 06:20:14 changed by alberto

  • summary changed from [PATCH] Provide workarounds for broken MySQLdb modules to Provide workarounds for broken MySQLdb modules.

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...

01/15/07 08:13:54 changed by 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).

01/15/07 08:23:16 changed by 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.

01/15/07 10:49:21 changed by 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

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

01/15/07 12:04:04 changed by Felix.Schwarz

  • attachment mysql_timestamp_workaround.patch added.

This patch obsoletes test_mysql_incompatibility.py too

01/15/07 12:07:20 changed by 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.

01/15/07 12:27:07 changed by 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

01/15/07 14:29:36 changed by Felix.Schwarz

  • attachment mysql_timestamp_workaround_v2.patch added.

Attaching new unified diff again as per request

(follow-up: ↓ 12 ) 01/15/07 14:32:56 changed by 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.

(in reply to: ↑ 11 ) 01/18/07 11:57:23 changed by alberto

  • summary changed from Provide workarounds for broken MySQLdb modules to [PATCH] Provide workarounds for broken MySQLdb modules.

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

01/19/07 11:28:14 changed by alberto

  • status changed from assigned to closed.
  • resolution set to fixed.

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

ALberto

01/19/07 11:40:09 changed by alberto

That should be [2406]