Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#11580 closed enhancement (fixed)

Add `tables` property to ConnectionWrapper subclasses

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: database backend Version:
Severity: normal Keywords: tables
Cc: Branch:
Release Notes:
API Changes:

Added methods get_table_names and get_column_names to the database connection classes.

Description

While working on #11512, it occurred to me that we might be able to organize the code a bit if the tables query was made to a property of the ConnectionWrapper subclasses.

Would it make sense to also have an interface class for the ConnectionWrapper subclasses (IDatabaseConnection?). It might help anyone that it trying to implement a new database type.

Attachments (0)

Change History (10)

comment:1 by Ryan J Ollos, 5 years ago

Proposed changes in log:rjollos.git:t11580.

Previously it was possible to specify the path for a MySQLdb connection when calling destroy_db. The path is used as the dbname, which maps to the schema in the SQL statement: tags/trac-1.0.1/trac/test.py@:375,378#L367 I couldn't see any reason to keep this behavior since the schema from the Postgres connection is used, and the behavior should be equivalent to both.

Let me if you have any concerns about this.

Version 0, edited 5 years ago by Ryan J Ollos (next)

comment:2 by Jun Omae, 5 years ago

It would be nice to add feature that retrieve table names from database.

But I like to add as a method rather than tables() decorated @property because I get a feeling that the attribute is an instance variable or a cached property. In addition, most attributes of ConnectionWrapper are methods.

Also, it would be useful for plugins to add get_columns(table) for retrieve column names from table.

I think we cannot add an interface IDatabaseConnection. Currently, all database connection classes aren't Component subclass. However, an interface class in Trac requires Component subclass to implement.

But I think we could an abstract class instead the interface.

comment:3 by Ryan J Ollos, 5 years ago

Thanks for the feedback. I will prepare some new changes with get_tables and get_columns, following your suggestions.

comment:4 by Ryan J Ollos, 5 years ago

Latest changes can be found in log:rjollos.git:t11580.2.

I was considering that we might want to return more info than just the column name. For instance, we could return the type and whether it is a primary key. Another possibility though would be to add a get_column_info or similar in the future, if needed.

There are two things I'm unsure of:

  • Whether there is a need to quote table names (as in #11512). The tests failed for postgres and mysql when I wrapped tablename with db.quote.
  • Whether care needs to be taken for string to unicode conversion, like that shown in: tags/trac-1.0.1/trac/db/api.py@:334-335#L331.

I should probably add some additional tests in order to determine whether those things are needed.

in reply to:  4 ; comment:5 by Jun Omae, 5 years ago

Latest changes can be found in log:rjollos.git:t11580.2.

In get_column_names() of sqlite_backend.py, I think we should use a quoted identifier.

-        rows = cursor.execute("PRAGMA table_info('%s')" % tablename)
+        rows = cursor.execute("PRAGMA table_info(%s)" % self.quote(tablename))
sqlite> CREATE TABLE `Ho)ur` (`Id` INTEGER PRIMARY KEY);
sqlite> PRAGMA table_info(`Ho)ur`);
0|Id|INTEGER|0||1
sqlite> CREATE TABLE `Ho')u``r` (`Id` INTEGER PRIMARY KEY);
sqlite> PRAGMA table_info(`Ho')u``r`);
0|Id|INTEGER|0||1

In __init__() of mysql_backend.py, I don't think we need use of basename() because a leading slash is stripped at start of __init__(), tags/trac-1.0.1/trac/db/mysql_backend.py@:246-247#L239.

-        self.schema = os.path.basename(path)
+        self.schema = path

I was considering that we might want to return more info than just the column name. For instance, we could return the type and whether it is a primary key.

Sounds good.

in reply to:  5 comment:6 by Ryan J Ollos, 5 years ago

Replying to jomae:

I was considering that we might want to return more info than just the column name. For instance, we could return the type and whether it is a primary key.

Sounds good.

Another idea would be to have get_column_names and get_table_names return only the names since often only the names are needed, but also add a method get_schema that returns the table and column names, as well as the primary keys and column types.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

in reply to:  5 comment:7 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)
Resolution: fixed
Status: newclosed

Replying to jomae:

In get_column_names() of sqlite_backend.py, I think we should use a quoted identifier.

I see now. We need to quote the parameter for get_column_names in sqlite_backend.py because string interpolation is performed. However, for the same methods in mysql_backend.py and postgres_backend.py the schema name and table name are passed as parameters so the quoting is performed by the execute function.

Committed to 1.0-stable in [12709:12710] and merged to trunk in [12711:12712].

In the future I'll look into adding a get_schema method.

comment:8 by Ryan J Ollos, 5 years ago

While working on #11581, I noticed that we should probably use the name table for the get_column_names second positional parameter rather than tablename, for consistency with drop_table, get_last_id and update_sequence.

comment:9 by Ryan J Ollos, 5 years ago

Renamed variable in [12756], merged in [12757].

comment:10 by Ryan J Ollos, 4 years ago

#12105 suggests adding get_table_names to the DatabaseManager class.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.