Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1235 (closed defect: fixed)

Opened 7 years ago

Last modified 7 years ago

[PATCH] Provide workarounds for broken MySQLdb modules

Reported by: Felix.Schwarz Owned by: 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 Download (6.2 KB) - added by Felix.Schwarz 7 years ago.
This is file contains some test cases for the added functionality.
mysql_timestamp_workaround.patch Download (10.0 KB) - added by Felix.Schwarz 7 years ago.
This patch obsoletes test_mysql_incompatibility.py too
mysql_timestamp_workaround_v2.patch Download (10.0 KB) - added by Felix.Schwarz 7 years ago.
Attaching new unified diff again as per request

Change History

comment:1 Changed 7 years ago 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

Changed 7 years ago by Felix.Schwarz

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

comment:2 Changed 7 years ago 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 :-)

comment:3 Changed 7 years ago 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

comment:4 Changed 7 years ago by alberto

  • Status changed from new to assigned
  • Owner changed from anonymous to alberto
  • Summary changed from Provide workarounds for broken MySQLdb modules to [PATCH] Provide workarounds for broken MySQLdb modules

comment:5 Changed 7 years ago 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...

comment:6 Changed 7 years ago 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).

comment:7 Changed 7 years ago 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.

comment:8 Changed 7 years ago 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

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

Changed 7 years ago by Felix.Schwarz

This patch obsoletes test_mysql_incompatibility.py too

comment:9 Changed 7 years ago 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.

comment:10 Changed 7 years ago 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

Changed 7 years ago by Felix.Schwarz

Attaching new unified diff again as per request

comment:11 follow-up: ↓ 12 Changed 7 years ago 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.

comment:12 in reply to: ↑ 11 Changed 7 years ago 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

comment:13 Changed 7 years ago by alberto

  • Status changed from assigned to closed
  • Resolution set to fixed

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

ALberto

comment:14 Changed 7 years ago by alberto

That should be [2406]

Note: See TracTickets for help on using tickets.