Bug #53128 for SQL-Statement: SQL::Dialect::ANSI is not really in INI format

Sun Dec 27 17:04:09 2009 mschwern [...] cpan.org - Ticket created

Subject:

SQL::Dialect::ANSI is not really in INI format

Answering a question on stack overflow (http://stackoverflow.com/questions/1966716/sqlparse-how-do-i-get-a-list-with-all-reserved-words) I discovered that SQL::Dialect::ANSI is not in INI format. At least not according to Wikipedia or Config::INI. Granted the INI format isn't particularly well defined, but they all seem to agree that it's "key = value" (or key: value). I can understand why you didn't want to write out "key = 1" over and over again.

IMO there's two good solutions. 1) Give each SQL::Dialects class a method to return their config as a Perl data structure, then you can use whatever data format you like. This also makes SQL::DIalects independent of SQL::Parser.

2) Store it as a Perl data structure, unless there's some hidden purpose to storing it as an ini file. This would be best, except in order to retain backwards compatibility you'll have to rewrite get_config() as an ini writer.

Mon Dec 28 08:11:48 2009 REHSACK [...] cpan.org - Correspondence added

A change there would break surely dependent modules, like DBD::Google.

I agree, the interface to the dialects isn't the best one - but it's in use by derived modules.
Further: ini-style isn't ini-format. It just should describe an easy to understand picture - sorry for the confusion. I will describe better in next release.

Mon Dec 28 08:11:48 2009 The RT System itself - Status changed from 'new' to 'open'

Mon Dec 28 16:47:51 2009 mschwern [...] cpan.org - Correspondence added

On Mon Dec 28 08:11:48 2009, REHSACK wrote:
Show quoted text

> A change there would break surely dependent modules, like DBD::Google.
>
> I agree, the interface to the dialects isn't the best one - but it's
> in use by
> derived modules.
> Further: ini-style isn't ini-format. It just should describe an easy
> to
> understand picture - sorry for the confusion. I will describe better
> in next
> release.

Sorry I didn't make it clear, leave get_config() alone and write a new method which returns it as a Perl data structure. That's 100% backwards compatible, and eliminates the need to describe what "ini-style" means and for users to write their own parser for it.

Mon Dec 28 16:52:42 2009 mschwern [...] cpan.org - Correspondence added

Oh, I see now, there's no SQL::Dialects superclass so the API is fixed. That's easy enough to fix, dialect() can inject the necessary method if it does not exist. That'll hold things together until subclasses catch up.

Let me hack up a patch real quick to demonstrate.

Mon Dec 28 21:41:45 2009 mschwern [...] cpan.org - Correspondence added

Attached is two patches. The first fixes some typos I noticed in the POD.

The second adds SQL::Dialects::Role which requires get_config() and injects get_config_as_hash(). This lets a dialect do its own parsing of its own data. You also now have a place to document how to write a SQL::Dialects.

To maintain backwards compatibility, dialects() will inject the role if necessary.

I also made dialects() handle dialects with multiple words in the name like "SQL::Dialects::ANSI::1999" and cleaned up how SQL::Parser was loading classes. Checking $@ is not safe.

Subject:

0002-Create-a-role-for-SQL-Dialects-which-allows-for-futu.patch

From e91152d6d37d7d7e7be6b23decba928ade263cee Mon Sep 17 00:00:00 2001 From: Michael G. Schwern <schwern@pobox.com> Date: Mon, 28 Dec 2009 18:35:08 -0800 Subject: [PATCH 2/2] Create a role for SQL::Dialects which allows for future injection of more code. Currently the role handles the parsing of the not-quite INI format used by SQL::Dialects. Also refactor loading of classes into a method and don't use the dangerous method of checking $@ which may be polluted. Finally, for compatibility with existing SQL::Dialects in the wild, SQL::Parser->dialect will inject the role if necessary. --- lib/SQL/Dialects/ANSI.pm | 2 + lib/SQL/Dialects/AnyData.pm | 2 + lib/SQL/Dialects/CSV.pm | 2 + lib/SQL/Dialects/Role.pm | 102 +++++++++++++++++++++++++++++++++++++++++++ lib/SQL/Parser.pm | 68 ++++++++++++++++------------ t/dialects.t | 68 ++++++++++++++++++++++++++++ 6 files changed, 215 insertions(+), 29 deletions(-) create mode 100644 lib/SQL/Dialects/Role.pm create mode 100644 t/dialects.t diff --git a/lib/SQL/Dialects/ANSI.pm b/lib/SQL/Dialects/ANSI.pm index 91c6033..c27fe3f 100755 --- a/lib/SQL/Dialects/ANSI.pm +++ b/lib/SQL/Dialects/ANSI.pm @@ -3,6 +3,8 @@ package SQL::Dialects::ANSI; use vars qw($VERSION); $VERSION = '1.23'; +use SQL::Dialects::Role; + sub get_config { return <<EOC; diff --git a/lib/SQL/Dialects/AnyData.pm b/lib/SQL/Dialects/AnyData.pm index d45acb6..ff9d850 100755 --- a/lib/SQL/Dialects/AnyData.pm +++ b/lib/SQL/Dialects/AnyData.pm @@ -3,6 +3,8 @@ package SQL::Dialects::AnyData; use vars qw($VERSION); $VERSION = '1.23'; +use SQL::Dialects::Role; + sub get_config { return <<EOC; diff --git a/lib/SQL/Dialects/CSV.pm b/lib/SQL/Dialects/CSV.pm index 245f022..4cdda43 100755 --- a/lib/SQL/Dialects/CSV.pm +++ b/lib/SQL/Dialects/CSV.pm @@ -3,6 +3,8 @@ package SQL::Dialects::CSV; use vars qw($VERSION); $VERSION = '1.23'; +use SQL::Dialects::Role; + sub get_config { return <<EOC; diff --git a/lib/SQL/Dialects/Role.pm b/lib/SQL/Dialects/Role.pm new file mode 100644 index 0000000..0b1998b --- /dev/null +++ b/lib/SQL/Dialects/Role.pm @@ -0,0 +1,102 @@ +package SQL::Dialects::Role; + +use strict; +use warnings; + +use base qw(Exporter); +our @EXPORT = qw(get_config_as_hash); + +sub get_config_as_hash { + my $class = shift; + + my @data = split( m/\n/, $class->get_config() ); + + my %config; + my $feature; + for (@data) + { + chomp; + s/^\s+//; + s/\s+$//; + next unless $_; + if (/^\[(.*)\]$/i) + { + $feature = lc $1; + $feature =~ s/\s+/_/g; + next; + } + my $newopt = uc $_; + $newopt =~ s/\s+/ /g; + $config{$feature}{$newopt} = 1; + } + + return \%config; +} + + +=head1 NAME + +SQL::Dialects::Role - The role of being a SQL::Dialect + +=head1 SYNOPSIS + + package My::SQL::Dialect; + + use SQL::Dialects::Role; + + sub get_config { + return <<CONFIG; + [SECTION] + item1 + item2 + + [ANOTHER SECTION] + item1 + item2 + CONFIG + } + +=head1 DESCRIPTION + +This adds the role of being a SQL::Dialect to your class. + +=head2 Requirements + +You must implement... + +=head3 get_config + + my $config = $class->get_config; + +Returns information about the dialect in an INI-like format. + +=head2 Implements + +The role implements... + +=head3 get_config_as_hash + + my $config = $class->get_config_as_hash; + +Returns the data represented in get_config() as a hash ref. + +Items will be upcased, sections will be lowered. + +The example in the SYNOPSIS would come back as... + + { + section => { + ITEM1 => 1, + ITEM2 => 2, + }, + another_section => { + ITEM1 => 1, + ITEM2 => 2, + } + } + +=head1 SEE ALSO + +L<SQL::Parser/dialect()> + +=cut diff --git a/lib/SQL/Parser.pm b/lib/SQL/Parser.pm index 121bcf3..904e8c1 100644 --- a/lib/SQL/Parser.pm +++ b/lib/SQL/Parser.pm @@ -277,36 +277,47 @@ sub dialect return $self->{dialect} unless $dialect; return $self->{dialect} if ( $self->{dialect_set} ); $self->{opts} = {}; - my $mod = "SQL/Dialects/$dialect.pm"; - undef $@; - eval { require "$mod"; } unless ( defined( $INC{$mod} ) ); - return $self->do_err($@) if $@; - $mod =~ s/\.pm//; - $mod =~ s"/"::"g; - my @data = split( m/\n/, $mod->get_config() ); - my $feature; - - for (@data) - { - chomp; - s/^\s+//; - s/\s+$//; - next unless $_; - if (/^\[(.*)\]$/i) - { - $feature = lc $1; - $feature =~ s/\s+/_/g; - next; - } - my $newopt = uc $_; - $newopt =~ s/\s+/ /g; - $self->{opts}->{$feature}->{$newopt} = 1; - } + my $mod_class = "SQL::Dialects::$dialect"; + + $self->_load_class($mod_class) unless $mod_class->can("get_config"); + + # This is here for backwards comaptibility with existing dialects + # before the had the role to add new methods. + $self->_inject_role("SQL::Dialects::Role", $mod_class) + unless $mod_class->can("get_config_as_hash"); + + $self->{opts} = $mod_class->get_config_as_hash; + $self->create_op_regexen(); $self->{dialect} = $dialect; $self->{dialect_set}++; } +sub _load_class { + my $self = shift; + my $class = shift; + + my $mod = $class; + $mod =~ s{::}{/}g; + $mod .= ".pm"; + + local($!, $@); + eval { require "$mod"; } or $self->do_err($@); + + return 1; +} + +sub _inject_role { + my $self = shift; + my($role, $dest) = @_; + + eval qq{ + package $dest; + use $role; + 1; + } or die "Can't inject $role into $dest: $@"; +} + sub create_op_regexen { my ($self) = @_; @@ -765,10 +776,9 @@ sub LOAD my ($package) = $str =~ /^LOAD\s+(.+)$/; $str = $package; $package =~ s/\?(\d+)\?/$self->{struct}->{literals}->[$1]/g; - my $mod = $package . '.pm'; - $mod =~ s|::|/|g; - eval { require $mod; } unless ( defined( $INC{$mod} ) ); - die "Couldn't load '$package': $@\n" if $@; + + $self->_load_class($package); + my %subs = eval '%' . $package . '::'; for my $sub ( keys %subs ) diff --git a/t/dialects.t b/t/dialects.t new file mode 100644 index 0000000..37af8b2 --- /dev/null +++ b/t/dialects.t @@ -0,0 +1,68 @@ +#!/usr/bin/perl + +use strict; +use warnings; + +use Test::More tests => 2; + +my $CLASS = "Local::Test::Dialect"; + + +# Test making a dialect +{ + package Local::Test::Dialect; + + use SQL::Dialects::Role; + + sub get_config { + # There's some deliberate whitespace abuse in here + return <<END; + +[THINGS] +elephants +FEELINGS +stuff + + +[RESERVED WORDS] +FOO +BAR +BAZ + +END + + } +} + +is_deeply $CLASS->get_config_as_hash, { + things => { + ELEPHANTS => 1, + FEELINGS => 1, + STUFF => 1, + }, + reserved_words => { + FOO => 1, + BAR => 1, + BAZ => 1 + } +}; + + +# Test role injection +{ + { + package SQL::Dialects::Test::NoRole; + + sub get_config { + return <<DONE; +[FOO] +bar +baz +DONE + } + } + + use SQL::Parser; + my $parser = SQL::Parser->new; + ok eval { $parser->dialect( "Test::NoRole" ); 1; } or diag $@; +} -- 1.6.5.3

Subject:

0001-Typos-in-the-pod.patch

From 24cdbb29203d487e7543c11bc6ece1a005c73999 Mon Sep 17 00:00:00 2001 From: Michael G. Schwern <schwern@pobox.com> Date: Mon, 28 Dec 2009 18:08:04 -0800 Subject: [PATCH 1/2] Typos in the pod --- lib/SQL/Dialects/AnyData.pm | 2 +- lib/SQL/Dialects/CSV.pm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/SQL/Dialects/AnyData.pm b/lib/SQL/Dialects/AnyData.pm index 0e32bd1..d45acb6 100755 --- a/lib/SQL/Dialects/AnyData.pm +++ b/lib/SQL/Dialects/AnyData.pm @@ -99,7 +99,7 @@ SQL::Dialects::AnyData =head1 SYNOPSIS use SQL::Dialects::AnyData; - $config = SQL::Dialects::ANSI->get_config(); + $config = SQL::Dialects::AnyData->get_config(); =head1 DESCRIPTION diff --git a/lib/SQL/Dialects/CSV.pm b/lib/SQL/Dialects/CSV.pm index 5fe7d31..245f022 100755 --- a/lib/SQL/Dialects/CSV.pm +++ b/lib/SQL/Dialects/CSV.pm @@ -90,7 +90,7 @@ SQL::Dialects::CSV =head1 SYNOPSIS use SQL::Dialects::CSV; - $config = SQL::Dialects::ANSI->get_config(); + $config = SQL::Dialects::CSV->get_config(); =head1 DESCRIPTION -- 1.6.5.3

Tue Dec 29 04:02:02 2009 REHSACK [...] cpan.org - Correspondence added

The implementation of a new super class isn't my problem. Thanks for the patch anyway :)

I have there following problems:
1) Fixed interface by widely using - so interface can't be changed.
2) I cannot simply add a base class which isn't used by the Dialects in the wildlife. Changing public inheritance is the same as changing the interface.
3) You'll never get access to the Dialect instance - it's a private attribute. Why not extending the public interface: that of SQL::Parser?

Relying on SQL::Dialect::Role returning a hash will lead you in a one way street. SQL::Statement v2.00 will use a Backus-Naur-Form parser, and there you'll never get a hash from the dialects classes. The initial request was:
Show quoted text

> Is there a function that gives me a list of all reserved words?

My proposal was:
> How about a method features() in SQL::Parser?

Best regards,
Jens

PS:
I cannot answer you're post on stackoverflow.com - to much JavaScript sources (and I'm denying everything not hosted on the same host).
We might discuss it here or in IRC (#dbi on perl.org irc server).

Tue Dec 29 04:48:26 2009 schwern [...] pobox.com - Correspondence added

CC:	Dan [...] DWright.Org
Subject:	Re: [rt.cpan.org #53128] SQL::Dialect::ANSI is not really in INI format
Date:	Tue, 29 Dec 2009 01:48:02 -0800
To:	bug-SQL-Statement [...] rt.cpan.org
From:	Michael G Schwern <schwern [...] pobox.com>

Jens Rehsack via RT wrote: Show quoted text

> The implementation of a new super class isn't my problem. Thanks for the patch > anyway :) > > I have there following problems: > 1) Fixed interface by widely using - so interface can't be changed.

dialect() accounts for the missing method by injecting the necessary role. $parser->dialect("Google") continues to work, for example. Show quoted text

> 2) I cannot simply add a base class which isn't used by the Dialects in the > wildlife. Changing public inheritance is the same as changing the interface.

It is not a base class, it is a role. There is no inheritance, its basically just injecting a method. This is kosher in the world of roles. Show quoted text

> 3) You'll never get access to the Dialect instance - it's a private attribute.

Yes, that's why I'm making the Dialects stand alone, independent of the Parser. Show quoted text

> Why not extending the public interface: that of SQL::Parser?

Because then the Dialects are coupled to the Parser, since the Parser is the only thing which knows how to do anything with the data in the Dialect. Its more flexible and easier to test them separately. Other things can then use them without having to load up SQL::Parser and all its code. I'm not personally using this. I just happened to notice the problem while answering the question on SO. Show quoted text

> Relying on SQL::Dialect::Role returning a hash will lead you in a one way > street. SQL::Statement v2.00 will use a Backus-Naur-Form parser, and there > you'll never get a hash from the dialects classes.

I don't understand how that applies here. Is the format of get_config() changing? Show quoted text

> The initial request was:

>> Is there a function that gives me a list of all reserved words?

> > My proposal was:

>> How about a method features() in SQL::Parser?

> > Best regards, > Jens > > PS: > I cannot answer you're post on stackoverflow.com - to much JavaScript sources > (and I'm denying everything not hosted on the same host).

That's a great way to not use a lot of the web. I'm *pretty sure* they're not an attack site. Consider whitelisting it. -- If at first you don't succeed--you fail. -- "Portal" demo

Mon Mar 15 17:57:02 2010 REHSACK [...] cpan.org - Correspondence added

Patch applied and delivered in S::S 1.24

Mon Mar 15 17:57:03 2010 REHSACK [...] cpan.org - Status changed from 'open' to 'resolved'

Mon Mar 15 18:00:43 2010 REHSACK [...] cpan.org - Fixed in 1.24 added