[patch] get_last_id() for MySQL returns 0

[anatoly techtonik <techtonik@…>]

Just run into a problem that cnx.insert_id() MySQLdb bindings doesn't return last inserted id. It is a known problem - see ​http://www.philihp.com/blog/2009/how-to-retrieve-the-id-after-a-mysql-insert-in-python/ for a workaround.

This affects custom fields information (they'll always be inserted into "ticket #0") and redirect after a new report is created.

The fix is trivial - get_last_id() should use cursor.lastrowid

trac/db/mysql_backend.py

@@ -214 +214 @@
         return _like_escape_re.sub(r'/\1', text)
 
     def get_last_id(self, cursor, table, column='id'):
-        return self.cnx.insert_id()
+        return cursor.lastrowid
 
     def rollback(self):
         self.cnx.ping()

Trac 0.11dev, MySQLdb.version '1.2.3c1', mysql 5.0.32

[anatoly techtonik <techtonik@…>]

patch with testcase

[Remy Blank]

Sweet, a bug report, a patch and a test case. Can't resist…

One question, though: how come we never noticed that? Does this issue only happen for specific versions of MySQLdb?

(Assigning to 0.12 for now, but we may backport it to 0.11.7 depending on how serious the issue is.)

[anatoly techtonik <techtonik@…>]

I'm in the process of rewrite of branches/0.11-stable/contrib/sourceforge2trac.py​ to convert tickets from old SF tracker system into their hosted Trac, and every inserted ticket has an id of #0 that doesn't allow to add comments to it.

[anatoly techtonik <techtonik@…>]

I do not know about specific versions of MySQLdb bindings. I am using latest binaries of 1.2.3c1 for Python 2.6 for Windows. MySQL version is the one that installed on every updated Debian Etch. You can test it yourself in VirtualBox.

If you hesitate, lastrowid is described in ​http://www.python.org/dev/peps/pep-0249/ (insert_id is not present there). ​http://phplens.com/lens/adodb/adodb-py-docs.htm mentions that insert_id is deprecated, and after reviewing this changeset ​http://www.sqlalchemy.org/trac/changeset/6613 you must be absolutely convinced about safety of porting the patch back to 0.11 =)

If you patch 0.11, I won't have to use cursor.lastrowid in SF2trac script directly which may or may not work for Trac databases other than MySQL.

[Remy Blank]

Replying to anatoly techtonik <techtonik@…>:

If you hesitate, (…)

Oh, no, I'm not hesitating. I'm just wondering how it was possible that this went unnoticed for so long :-)

[anatoly techtonik <techtonik@…>]

Probably because description of MySQL support in Trac on MySQLdb page is too scary.

We recommend that you use it only if you don't have other choice.

After this most people end with SQLite. And those brave souls that nevertheless choose MySQL probably don't create custom reports or use custom ticket fields. From those few that could potentially run into the issue nobody could make its way to this tracker.

If not SourceForge hosting that uses MySQL, I wouldn't know about this too. =)

[Christian Boos]

Replying to anatoly techtonik <techtonik@…>:

… and after reviewing this changeset ​http://www.sqlalchemy.org/trac/changeset/6613

Note that this changeset deals with ​MySQL Connector/Python, i.e. it has nothing to do with the ​MySQLdb bindings that Trac uses …

That being said, the change looks fine to me (except perhaps a need for tearDown() in the unit test, doing a reset_db()).

[anatoly techtonik <techtonik@…>]

… and after reviewing this changeset ​http://www.sqlalchemy.org/trac/changeset/6613

Note that this changeset deals with ​MySQL Connector/Python, i.e. it has nothing to do with the ​MySQLdb bindings that Trac uses …

The MySQLdb binding adapter for SQLAlchemy uses the same code, but inherited from ​http://www.sqlalchemy.org/trac/browser/sqlalchemy/trunk/lib/sqlalchemy/engine/default.py?rev=6613#L407

That being said, the change looks fine to me (except perhaps a need for tearDown() in the unit test, doing a reset_db()).

Shouldn't the test that is being executed take care of resetting DB beforehand?

[anatoly techtonik <techtonik@…>]

with tearDown

[Remy Blank]

Now I know why this wasn't noticed before: insert_id() does work, but only if it's called before commit(). Otherwise, as you noticed, it returns 0. The .lastrowid attribute works correctly in both cases.

Now, both calls to get_last_id() are done before committing, so they did work correctly on MySQL.

Patch applied in [9051]. Thanks!