Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix documented values for 'on_connect_do' in Storage::DBI->connect() #149

Open
wants to merge 1 commit into
base: maint/0.0828xx
Choose a base branch
from

Conversation

dboehmer
Copy link

Code references returning a literal string are NOT supported. Code references must return an arrayref instead.

I stumbled upon this while overriding connect() in my code and trying to support all possible arguments to the method.

I wrote this test to verify all variants:

use v5.38;

use DBI;
use DBIx::Class::Schema;
use Test2::V0;

plan tests => 9;

my $dbh = DBI->connect( 'dbi:SQLite::memory:', undef, undef, { RaiseError => 1 } );
$dbh->do('CREATE TABLE tests (id integer)');

sub t ( $on_connect_do, $name ) {
    ok lives {
        my $schema = DBIx::Class::Schema->connect( sub { $dbh }, { on_connect_do => $on_connect_do } );
        $schema->storage->dbh_do( sub { } );    # trigger actual connection
    }, $name
      or diag $@;
}

# https://metacpan.org/pod/DBIx::Class::Storage::DBI#on_connect_do

t 'INSERT INTO tests VALUES (1)', "scalar string";

t [ map "INSERT INTO tests VALUES ($_)", 2 .. 3 ], "arrayref with 2 strings";

t [ sub { 'INSERT INTO tests VALUES (4)' } ],
  "arrayref with 1 coderef that returns scalar string";    #     documented but fails

t [ sub { [ map "INSERT INTO tests VALUES ($_)", 5 .. 6 ] } ],
  "arrayref with 1 coderef that returns arrayref with 2 strings";    # not documented but works

t [ 'INSERT INTO tests VALUES (7)', sub { ['INSERT INTO tests VALUES (8)'] } ],
  "mixed arrayref with 1 string and 1 coderef that returns arrayref with a string";

t sub { 'INSERT INTO tests VALUES (999)' }, "coderef that returns a scalar string is ignored";

t sub { [ map 'INSERT INTO tests VALUES (999)', 1 .. 2 ] },
  "coderef that returns an arrayref with strings is ignored";

t sub {
    sub { ["INSERT INTO tests VALUES ('x')"] }
}, "two nested coderefs that return an arrayref with a string (is ignored)";

is $dbh->selectcol_arrayref('SELECT id FROM tests') => [ 1 .. $_ ], "created IDs 1..$_" for 8;

Result:

# Seeded srand with seed '20240218' from local date.
1..9
ok 1 - scalar string
ok 2 - arrayref with 2 strings
not ok 3 - arrayref with 1 coderef that returns scalar string
# Failed test 'arrayref with 1 coderef that returns scalar string'
# at - line 16.
# DBIx::Class::Storage::DBI::catch {...} (): Can't use string ("INSERT INTO tests VALUES (4)") as an ARRAY ref while "strict refs" in use at /home/daniel/perl5/perlbrew/perls/perl-5.38.0-threads-bullseye/lib/site_perl/5.38.0/DBIx/Class/Storage/DBI.pm line 1490. at - line 15
ok 4 - arrayref with 1 coderef that returns arrayref with 2 strings
ok 5 - mixed arrayref with 1 string and 1 coderef that returns arrayref with a string
ok 6 - coderef that returns a scalar string is ignored
ok 7 - coderef that returns an arrayref with strings is ignored
ok 8 - two nested coderefs that return an arrayref with a string (is ignored)
not ok 9 - created IDs 1..8
# Failed test 'created IDs 1..8'
# at - line 44.
# +------+------------------+----+-------+
# | PATH | GOT              | OP | CHECK |
# +------+------------------+----+-------+
# | [3]  | 5                | eq | 4     |
# | [4]  | 6                | eq | 5     |
# | [5]  | 7                | eq | 6     |
# | [6]  | 8                | eq | 7     |
# | [7]  | <DOES NOT EXIST> |    | 8     |
# +------+------------------+----+-------+

Code references returning a literal string are NOT supported.
Code references must return an arrayref instead.
@abraxxa
Copy link
Contributor

abraxxa commented Feb 19, 2024

Note that you should define sub connection to override the arguments to connect as per https://metacpan.org/pod/DBIx::Class::Schema#Overloading1.
I have that in my schema base class if you want my code as an example.

@dboehmer
Copy link
Author

@abraxxa Yes, my description was inaccurate: My code is actually overriding connection(). Still it takes the same arguments as connect() and I think its POD is wrong and should be fixed as proposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants