Opened 3 years ago

Closed 3 years ago

## #11461 closed defect (fixed)

Reported by: Owned by: johna falkb normal SimpleMultiProjectPlugin major 1.0

Trac 	1.0.1
Genshi 	0.7 (without speedups)
MySQLdb 	1.2.3c1
Pygments 	1.6
Python 	2.6.6 (r266:84292, Nov 22 2013, 12:16:22) [GCC 4.4.7 20120313 (Red Hat 4.4.7-4)]
pytz 	2013.8
setuptools 	0.6
Subversion 	1.7.4 (r1295709)
jQuery	1.7.2


After installation of the plugin and restart of the server, the database update fails. The culprit appears to be in line 154 of environmentSetup.py:

cursor.execute("""ALTER TABLE smp_project ADD restrict TEXT""")


This change appears to fix this issue. (adding backtiks to "restrict")

cursor.execute("""ALTER TABLE smp_project ADD restrict TEXT""")


Created a new environment and ran update. It indicated success, but none of the smp_ tables in the database have data.

[root@fw-svn01 Testing]# trac-admin /home/trac/Testing upgrade
SimpleMultiProject database schema version is 0, should be 5

You may want to upgrade the Trac documentation now by running:



Additonally, there is the following error in the console.

ProgrammingError: (1064, "You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'restrict\n                   FROM\n                        smp_project' at line 2")


This shows up in: Manage Projects -> Projects and the "Timeline", "Roadmap", "View Tickets", and "New Ticket" items.

I've not been able to test this against MySQL, but it may be that "restrict" is a restricted word for MariaDB.

### comment:1 Changed 3 years ago by johna

Apologies.

SMP version 0.0.4dev-py2.6

### comment:2 Changed 3 years ago by johna

I confirmed that the issue existed in MySQL 5.5.33

The issue appears to be the use of restrict as a column name in the smp_project table.

I renamed all instances of restrict to restrick in the following files:

admin.py
environmentSetup.py
model.py


Redid the whole thing and it appears to be working properly. Evidently restrict is a restricted word in mysql and MariaDB

Last edited 3 years ago by rjollos (previous) (diff)

### comment:3 Changed 3 years ago by rjollos

• Description modified (diff)

### comment:4 Changed 3 years ago by falkb

Oh what a silly bug! It seems to me there's no other way than to rename it in an incompatible way for db schema version 5 of the plugin, and to force people, who upgraded already, to rename it manually. What do you think?

### comment:5 Changed 3 years ago by falkb

• Status changed from new to accepted

### comment:6 Changed 3 years ago by johna

Yeah. It is a problematic one to find this far down the roadmap.

Reserved words can be utilized as object names, but it gets kind of ugly. http://dev.mysql.com/doc/refman/5.5/en/identifiers.html

I tried enclosing the words in backticks, but python didn't like that at all and wouldn't compile the .egg

As a straight .sql file it ran just fine against the database with the reserved word(s) in backticks. You might consider packaging a .sql file for the next version and run that to change the row as needed.

Not pretty, but it might get the job done.

### comment:7 follow-up: ↓ 9 Changed 3 years ago by rjollos

• Description modified (diff)

t11461.patch has been tested with SQLite and MySQL, and I believe it will fix the issue in a cross-db compatible way. Someone more familiar with the project should probably do some thorough testing before committing the change.

### comment:8 Changed 3 years ago by rjollos

One other suggestion - a common approach is to use IEnvironmentSetupParticipant to insert entries in the ticket-custom section. This can avoid a manual step in the installation process. It would be similar to here. TimingAndEstimationPlugin does this as well: timingandestimationplugin/branches/trac0.12/timingandestimationplugin/api.py@13377#L140.

### comment:9 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 3 years ago by falkb

t11461.patch has been tested with SQLite and MySQL, and I believe it will fix the issue in a cross-db compatible way. Someone more familiar with the project should probably do some thorough testing before committing the change.

Thanks a lot, rjollos! I've tried this patch but adding new projects via insert_project() in model.py does not work anymore then. Simply, it doesn't add a new record to my SQLite db. I played around with the parameter formatting for a while but haven't found a solution yet.

### comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 3 years ago by rjollos

Thanks a lot, rjollos! I've tried this patch but adding new projects via insert_project() in model.py does not work anymore then.

I had tested that behavior and it was working okay for me when adding new projects through the admin page. Which database did you test with? What result did you get?

### comment:11 in reply to: ↑ 10 Changed 3 years ago by falkb

Thanks a lot, rjollos! I've tried this patch but adding new projects via insert_project() in model.py does not work anymore then.

I had tested that behavior and it was working okay for me when adding new projects through the admin page. Which database did you test with? What result did you get?

SQLite. Note from IRC:

hmm.... Now I saw it started to work again as I removed one of my test projects (through Admin page)
falkb	after that I was able to add new projects again
falkb	I start to think it has something to do with the db cursor
falkb	maybe sometimes it's valid and sometimes not
falkb	maybe there's an issue with the __get_db() or __get_cursor() function
falkb	and the scope of the return value of both functions
falkb	maybe it's luck whether the returned object (db or cursor) is still valid or not
falkb	but that's just a guess


### comment:12 Changed 3 years ago by falkb

This behaviour is reproducable through Admin page. After login to Trac I always fail to add new projects. It works again as soon as I have removed a project.

### comment:13 Changed 3 years ago by rjollos-test

Which version of Trac did you test with? It looks like there is an issue with Trac < 0.12. Since I've added __get_db, then db is no longer bound to self in insert_project. So we would either need to assign db to self.db in insert_project, or pass the db object to __start_transaction:

-    def __start_transacction(self):
+    def __start_transacction(self, db):
+       db = db or self.db
if VERSION < '0.12':
db.commit()
db.close()


### comment:14 Changed 3 years ago by rjollos-test

I think maybe we should drop the __get_cursor method and modify __start_transaction to accept a db object. It is really easy to get into trouble by not keeping the db object in scope while trying to use a cursor object (e.g. see #11115 and cannot operate on closed cursor). You may be okay by keeping it bound to the instance object, but I'm not sure of that.

### comment:15 Changed 3 years ago by rjollos

Those comments were by me, btw. I didn't mean to use my "test" account.

### comment:16 Changed 3 years ago by rjollos

I was also just reminded in #11234 that db.quote is only available since Trac 0.12: [trac 9062]. I suppose you could backport the methods to your plugin though if you want support for Trac < 0.12.

### comment:17 Changed 3 years ago by falkb

I only support 0.12 and 1.0 at present. I have no environment for backporting to 0.11, and 0.11 was long ago, so I don't think it's a good idea anyway.

### comment:18 Changed 3 years ago by rjollos

In that case, it would probably be easiest to just remove the __get_db, __get_cursor and __start_transaction methods.

### comment:19 Changed 3 years ago by anonymous

I spent the whole evening for figuring out how that db access must be programmed, but I always failed. Event reading http://trac.edgewall.org/wiki/TracDev/DatabaseApi thousand times did not help. Please, give me a code example as a start.

I tried for instance in model.py:

from trac.perm import PermissionSystem, IPermissionGroupProvider
...
class SmpModel(Component):
...
def insert_project(self, name, summary, description, closed, restrict):
@with_transaction(env)
def do_something(db):
query    = """INSERT INTO
smp_project (name, summary, description, closed, %s)
VALUES (%%s, %%s, %%s, %%s, %%s);""" % db.quote('restrict')

cursor = db.cursor()
cursor.execute(query, [name, summary, description, closed, restrict])
self.__start_transaction(db)


or

    # AdminPanel Methods
def insert_project(self, name, summary, description, closed, restrict):
db = None
if VERSION < '0.12':
db = self.env.get_db_cnx()
else:
query    = """INSERT INTO
smp_project (name, summary, description, closed, %s)
VALUES (%%s, %%s, %%s, %%s, %%s);""" % db.quote('restrict')

cursor = db.cursor()
cursor.execute(query, [name, summary, description, closed, restrict])
self.__start_transaction(db)


The effect is always like described above...

### comment:20 Changed 3 years ago by falkb

Editing my comment:19

1. s/Event/even
2. plus from trac.db import with_transaction in the first snippet
3. this is __start_transaction() :
 # DB Methods
def __start_transaction(self, db):
if VERSION < '0.12':
db.commit()
db.close()

Last edited 3 years ago by falkb (previous) (diff)

### comment:21 Changed 3 years ago by falkb

In 13549:

bugfix (see #11461):

• fixes model.py concerning all DB access calls (proper use of cursor and scope of db object)
• applied a patch of rjollos to fix the issue with quoting the 'restrict' SQL keyword to get it work with MariaDB

### comment:22 Changed 3 years ago by falkb

finally I get it how to use the db API (hopefully properly now), so please test

### comment:23 Changed 3 years ago by rjollos

Looks good. Since db.quote is not available until Trac 0.12, unless you backport it in your plugin then you've already broken backward compatibility with Trac < 0.12, and you could therefore just drop all of the conditionals if VERSION < '0.12' and __start_transaction.

Another approach, in order to keep it working with at least SQLite (and maybe PostgreSQL) for Trac < 0.12, would be as shown by jun66j5 in comment:6:ticket:11234:

if hasattr(db, 'quote'):
quote = db.quote  # Trac 0.12+
else:
quote = lambda name: name


This adds a dummy quote function for Trac < 0.12. If you are ambituous about making it would work Trac < 0.12, you could add a real quote function for each database type (i.e. backport [trac 9062]).

### comment:24 follow-up: ↓ 25 Changed 3 years ago by falkb

Hey rjollos, thanks for your review and your useful hints and links! My plan is a backport to 0.11 some day, that's why I keep as much if VERSION <'0.12' as I can overview. I must search a bit, but somewhere I've got a Bitnami package of 0.11 around, which I'm gonna install as test bench...

P.S.: Uhm... wouldn't it a good idea to add a state 'testing' to the trac-hacks.org ticket workflow?

### comment:25 in reply to: ↑ 24 ; follow-up: ↓ 26 Changed 3 years ago by rjollos

P.S.: Uhm... wouldn't it a good idea to add a state 'testing' to the trac-hacks.org ticket workflow?

We've talked about similar changes. Others have mentioned having a Review step in the workflow.

Adding multiple workflows to Trac is high on the list of features that I'd like to add to Trac for 1.2, and that might allow us to customize the workflow for each Component. Although, the implementation might couple the workflow to ticket types. We'd have to see how flexible it could be made without becoming overly complex.

### comment:26 in reply to: ↑ 25 Changed 3 years ago by falkb

P.S.: Uhm... wouldn't it a good idea to add a state 'testing' to the trac-hacks.org ticket workflow?

Adding multiple workflows to Trac is high on the list of features that I'd like to add to Trac for 1.2, and that might allow us to customize the workflow for each Component.

I actually thought of just installing AdvancedTicketWorkflowPlugin and define 'testing' for the whole trac-hacks.org which may be a good idea in general.

### comment:27 Changed 3 years ago by rjollos

Adding another plugin to the site requires a lot of testing, and then we take on the burden of maintaining the plugin if it doesn't already have a maintainer. You'll see from report:9?COMPONENT=TracHacks, report:9?COMPONENT=TracHacksPlugin that there is already quite a bit of work planned for the site, and additionally we'd like to get VotePlugin installed, AccountManagerPlugin 0.5 released and installed, ... and the list goes on and on. Ultimately, these things just need to be prioritized, and so far the multiple-workflow doesn't see as much as an improvement to the site as several other things.

### comment:28 Changed 3 years ago by falkb

• Keywords testing removed
• Resolution set to fixed
• Status changed from accepted to closed

After related fixes in #11484 it works now properly.

### comment:29 Changed 3 years ago by falkb

In 13735:

• fixed #11553: timestamps need to be bigint with PostgreSQL
• get rid of table 'smp_project' column 'restrict' because that is a db keyword and therefore not suitable as column name, renamed it to 'restrict_to' (see #11461)
• => plugin db schema version increased to 6