Bug #95959 for DBIx-DBSchema: Can't get a list of tables from an SQLite3 database

Sun May 25 11:36:51 2014 simon [...] thegestalt.org - Ticket created

Subject:	Can't get a list of tables from an SQLite3 database
Date:	Sun, 25 May 2014 16:36:35 +0100
To:	bug-DBIx-DBSchema [...] rt.cpan.org
From:	Simon Wistow <simon [...] thegestalt.org>

It seems like _tables_from_dbh is effectively doing this my $sth = $dbh->table_info('', '', '', 'TABLE') or die $dbh->errstr; $sth->fetchall_arrayref([2,3]) Because $db_catalog and $db_schema are the defaults and are therefore blank. Unfortunately that doesn't seem to work on the version of SQLite I'm running. However if we change it to my $sth = $dbh->table_info('', '%', '%', 'TABLE') or die $dbh->errstr; $sth->fetchall_arrayref([2,3]) i.e set $db_schema to '%' and set the 3rd 'table' column to '%' as well ... then it all works. From reading the docs it looks like that's a sensible default for 'table' and testing (albeit only with SQLite and mysql[*]) shows that it still works that way but perhaps having a new driver settable value would be better. Thanks, Simon [*] mysql still requires my $sth = $dbh->table_info('', '', '%', 'TABLE') or die $dbh->errstr; $sth->fetchall_arrayref([2,3]) So the default for $db_schema still needs to stay ''

Tue May 27 14:27:39 2014 ivan-pause [...] 420.am - Correspondence added

FYI - the SQLite driver is contributed by the community (specifically, Jesse Vincent and In Suk Joung). I don't use or maintain this driver myself. If there are problems with the SQLite driver we'd be looking to to community folks using this driver to provide patches and updates.

Tue May 27 14:27:39 2014 The RT System itself - Status changed from 'new' to 'open'

Thu Sep 28 03:39:51 2017 http://1id.com/=nathan.anderson - Correspondence added

From:

nathana [...] fsr.com

On Tue May 27 14:27:39 2014, IVAN wrote: Show quoted text

> FYI - the SQLite driver is contributed by the community (specifically, > Jesse Vincent and In Suk Joung). I don't use or maintain this driver > myself. > > If there are problems with the SQLite driver we'd be looking to to > community folks using this driver to provide patches and updates.

Sorry to necro this bug, but it's still showing as open so I'm going to consider it fair game... I think what Simon was getting at is that the problem isn't with the DBSchema SQLite driver, and it can't be fixed there. The problem is with the architecture of the table reverse-engineering feature: it assumes (apparently incorrectly) that all DBI drivers (not the DBSchema ones) implement the table_info method the same way, so it's called (inside of a private helper subroutine) with the same parameters irregardless of underlying DB engine. I'm running into a similar problem, but with DBSchema Sybase support (which I am trying to restore to a usable state...wish me luck!). One might argue that this is a bug in DBD::Sybase itself, but I could address/work-around it in DBIx::DBSchema::DBD::Sybase if a hook was actually provided to me for this purpose. Ultimately what probably needs to happen is for a 'tables' method to be added to DBSchema::DBD that _tables_from_dbh() calls instead, and which the individual DBSchema drivers can override. If you are open to such a change, I might decide to take a swing at implementing it.

Thu Sep 28 13:16:11 2017 ivan-pause [...] 420.am - Correspondence added

I don't personally use or maintain the Sybase driver either. If you guys want the SQLite or Sybase drivers to work differently / better, we'd be looking to you to provide patches, updates and maintenance, including whatever changes you need in the base library. Hope the software can be useful for you!

Sun Oct 01 04:32:55 2017 http://1id.com/=nathan.anderson - Correspondence added

From:

nathana [...] fsr.com

On Thu Sep 28 13:16:11 2017, IVAN wrote: Show quoted text

> I don't personally use or maintain the Sybase driver either. If you > guys want the SQLite or Sybase drivers to work differently / better, > we'd be looking to you to provide patches, updates and maintenance, > including whatever changes you need in the base library.

Yup I realize you didn't write nor do you use or maintain either driver. :) Attached is my first humble crack at a rough refactorization. Let me know what you think. -- Nathan

Subject:

dbschema-new_tables_method.diff

diff -r -d -u -p a/DBSchema.pm b/DBSchema.pm --- a/DBSchema.pm 2017-10-01 00:50:20.250207398 -0700 +++ b/DBSchema.pm 2017-09-30 17:02:31.290475085 -0700 @@ -498,16 +498,8 @@ sub pretty_read { sub _tables_from_dbh { my($dbh) = @_; my $driver = _load_driver($dbh); - my $db_catalog = - scalar(eval "DBIx::DBSchema::DBD::$driver->default_db_catalog"); - my $db_schema = - scalar(eval "DBIx::DBSchema::DBD::$driver->default_db_schema"); - my $sth = $dbh->table_info($db_catalog, $db_schema, '', 'TABLE') - or die $dbh->errstr; - #map { $_->{TABLE_NAME} } grep { $_->{TABLE_TYPE} eq 'TABLE' } - # @{ $sth->fetchall_arrayref({ TABLE_NAME=>1, TABLE_TYPE=>1}) }; - map { $_->[0] } grep { $_->[1] =~ /^TABLE$/i } - @{ $sth->fetchall_arrayref([2,3]) }; + + eval "DBIx::DBSchema::DBD::$driver->tables(\$dbh)"; } =back diff -r -d -u -p a/DBSchema/DBD.pm b/DBSchema/DBD.pm --- a/DBSchema/DBD.pm 2017-10-01 00:50:20.242207399 -0700 +++ b/DBSchema/DBD.pm 2017-10-01 01:24:49.394187666 -0700 @@ -259,6 +259,21 @@ sub column_value_needs_quoting { } +sub tables { + my ($proto, $dbh, $sth) = @_; + my $db_catalog = $proto->default_db_catalog; + my $db_schema = $proto->default_db_schema; + + if(!defined($sth)) { + $sth = $dbh->table_info($db_catalog, $db_schema, '', 'TABLE') + or die $dbh->errstr; + } + #map { $_->{TABLE_NAME} } grep { $_->{TABLE_TYPE} eq 'TABLE' } + # @{ $sth->fetchall_arrayref({ TABLE_NAME=>1, TABLE_TYPE=>1}) }; + map { $_->[0] } grep { $_->[1] =~ /^TABLE$/i } + @{ $sth->fetchall_arrayref([2,3]) }; +} + =back =head1 TYPE MAPPING

Sun Oct 01 04:42:31 2017 http://1id.com/=nathan.anderson - Correspondence added

From:

nathana [...] fsr.com

Given the previous patch that I just attached above, and assuming Simon's observations about SQLite's behavior (or at least DBD::SQLite's behavior) is accurate, this change to the DBIx::DBSchema::DBD::SQLite driver *should* fix it. NOTE THAT I HAVE NOT PERSONALLY TESTED THIS; this is just an example. Another, possibly simpler, way to solve the problem (at least in the instance of SQLite) would be to simply also add a new default_db_table method to DBIx::DBSchema::DBD, then override that to return '%' within the SQLite driver. I'm not sure if it is necessarily desirable to do this or not. In fact, if we still give driver classes the ability to override this new tables method (as I would argue we should), one might further argue that we should simply do away with default_db_catalog and default_db_schema entirely, since their only real use was to change the parameters that _tables_from_dbh passed to DBI->table_info. And since that functionality has been moved to the new tables method, and that method is overrideable, there's really no reason the default values can't simply be incorporated within the method itself. -- Nathan

Subject:

dbschema-dbd-sqlite_tables.diff

diff -r -d -u -p a/DBSchema/DBD/SQLite.pm b/DBSchema/DBD/SQLite.pm --- a/DBSchema/DBD/SQLite.pm 2017-10-01 00:50:20.250207398 -0700 +++ b/DBSchema/DBD/SQLite.pm 2017-10-01 01:24:20.142187945 -0700 @@ -187,6 +187,19 @@ while ( my $row = $sth->fetchrow_hashref } +sub default_db_schema { '%'; } + +sub tables { + my($proto, $dbh) = @_; + my $db_catalog = $proto->default_db_catalog; + my $db_schema = $proto->default_db_schema; + + $sth = $dbh->table_info($db_catalog, $db_schema, '%', 'TABLE') + or die $dbh->errstr; + + $proto->SUPER::tables($dbh, $sth); +} + =pod =back

Sun Oct 01 04:47:30 2017 http://1id.com/=nathan.anderson - Correspondence added

From:

nathana [...] fsr.com

Attached is a patch to DBIx::DBSchema::DBD::Sybase that, in combination with my first patch, does fix table name retrieval for that driver. Note that I have not tested this against Sybase proper, but just against MSSQL. (I'm contemplating changes to the Sybase driver that can discern whether the database server is Sybase or MSSQL in the event things need to be done differently for each.) -- Nathan

Subject:

dbschema-dbd-sybase_tables_mssql.diff

diff -r -d -u -p a/DBSchema/DBD/Sybase.pm b/DBSchema/DBD/Sybase.pm --- a/DBSchema/DBD/Sybase.pm 2017-10-01 00:50:20.250207398 -0700 +++ b/DBSchema/DBD/Sybase.pm 2017-10-01 01:27:04.446186378 -0700 @@ -108,6 +108,16 @@ sub _is_unique { return $isunique; } +sub tables { + my($proto, $dbh) = @_; + + my $sth = $dbh->prepare("sp_tables NULL, NULL, NULL, \"'TABLE'\""); + $sth->execute + or die $dbh->errstr; + + $proto->SUPER::tables($dbh, $sth); +} + =head1 AUTHOR Charles Shapiro <charles.shapiro@numethods.com>

Sun Oct 01 15:11:14 2017 ivan-pause [...] 420.am - Correspondence added

RT-Send-CC:

nathana [...] fsr.com

On Sun Oct 01 04:32:55 2017, http://1id.com/=nathan.anderson wrote: Show quoted text

> On Thu Sep 28 13:16:11 2017, IVAN wrote: > Attached is my first humble crack at a rough refactorization. Let me > know what you think.

I don't think there's any need for eval. Pushed a slight refactoring of your changes to git. Take a look at http://freeside.biz/gitlist/DBIx-DBSchema.git/commit/e7dd1d416aab83e26b2403e6b5e9737ca674703a Unfortunately, I don't expect to have much more free time for this for at least a month, probably more.

Sun Oct 01 15:11:16 2017 ivan-pause [...] 420.am - Status changed from 'open' to 'patched'

Sun Oct 01 15:11:16 2017 ivan-pause [...] 420.am - Taken

Sun Oct 01 15:35:46 2017 ivan-pause [...] 420.am - Correspondence added

RT-Send-CC:

nathana [...] fsr.com

On Sun Oct 01 04:42:31 2017, http://1id.com/=nathan.anderson wrote: Show quoted text

> Given the previous patch that I just attached above, and assuming > Simon's observations about SQLite's behavior (or at least > DBD::SQLite's behavior) is accurate, this change to the > DBIx::DBSchema::DBD::SQLite driver *should* fix it. NOTE THAT I HAVE > NOT PERSONALLY TESTED THIS; this is just an example. > > Another, possibly simpler, way to solve the problem (at least in the > instance of SQLite) would be to simply also add a new default_db_table > method to DBIx::DBSchema::DBD, then override that to return '%' within > the SQLite driver. I'm not sure if it is necessarily desirable to do > this or not. In fact, if we still give driver classes the ability to > override this new tables method (as I would argue we should), one > might further argue that we should simply do away with > default_db_catalog and default_db_schema entirely, since their only > real use was to change the parameters that _tables_from_dbh passed to > DBI->table_info. And since that functionality has been moved to the > new tables method, and that method is overrideable, there's really no > reason the default values can't simply be incorporated within the > method itself.

I have no horse in this race. I applied the patch you provided (and fixed $sth to my $sth). If you want to implement this differently, feel free to send a new patch or pull request.

Sun Oct 01 15:52:40 2017 http://1id.com/=nathan.anderson - Correspondence added

From:

nathana [...] fsr.com

On Sun Oct 01 15:11:14 2017, IVAN wrote: Show quoted text

> Unfortunately, I don't expect to have much more free time for this for > at least a month, probably more.

Looks good, thanks, & no worries, though I will likely have some additional patches to Sybase.pm for you soon-ish that I've been testing. (People may think this gross ;-) but ultimate goal is actually to have Freeside talking to MSSQL server by way of DBD::Sybase. Perhaps by cleaning up any Postgres-isms this'll also end up having the side-effect of improving MySQL/MariaDB support, or at least make it easier to bring up-to-date.) How do you prefer I submit future patches, either for this or to Freeside proper? (I assume pull request; I guess it is about time I got off my duff and taught myself how to use Git.) -- Nathan

Sun Oct 01 16:10:24 2017 ivan-pause [...] 420.am - Correspondence added

RT-Send-CC:

nathana [...] fsr.com

On Sun Oct 01 15:52:40 2017, http://1id.com/=nathan.anderson wrote: Show quoted text

> On Sun Oct 01 15:11:14 2017, IVAN wrote: >

> > Unfortunately, I don't expect to have much more free time for this > > for > > at least a month, probably more.

> > Looks good, thanks, & no worries, though I will likely have some > additional patches to Sybase.pm for you soon-ish that I've been > testing. > [...] > How do you prefer I submit future patches,

Unified diffs or git pull requests are both great. Show quoted text

> (People may think this gross ;-) but ultimate goal is > actually to have Freeside talking to MSSQL server by way of > DBD::Sybase. Perhaps by cleaning up any Postgres-isms this'll also > end up having the side-effect of improving MySQL/MariaDB support, or > at least make it easier to bring up-to-date.) > > How do you prefer I submit future patches [...] to > Freeside proper? (I assume pull request; I guess it is about time I > got off my duff and taught myself how to use Git.)

This is the bug tracker for DBIx::DBSchema, specifically for a bug relating to SQLite. If you want to discuss open-source Freeside development, please subscribe to the freeside-devel@freeside.biz mailing list and post your message there. Thanks! Ivan

Sun Oct 01 16:21:22 2017 http://1id.com/=nathan.anderson - Correspondence added

On Sun Oct 01 16:10:24 2017, IVAN wrote: Show quoted text

> This is the bug tracker for DBIx::DBSchema, specifically for a bug > relating to SQLite. If you want to discuss open-source Freeside > development, please subscribe to the freeside-devel@freeside.biz > mailing list and post your message there. Thanks!

10-4. I only mentioned this because extant issues in DBSchema are blockers to making this happen on Freeside, and you mentioned (completely understandable/reasonable) short-term constraints on your time. And I posted what I did here because one particular issue with Sybase shared an underlying root cause with an SQLite issue, and I can't stand duplicate bugs & didn't want to be responsible for one... I will take up further discussion on other issues related to my goal on the Freeside developer list as I come up with more things to share. Thanks again, -- Nathan