mojolicious / mojo-pg

Mojolicious :heart: PostgreSQL
https://metacpan.org/release/Mojo-Pg
Artistic License 2.0
99 stars 46 forks source link

Previously cached connections do not use a new search_path after change #22

Closed jberger closed 8 years ago

jberger commented 8 years ago

Steps to reproduce the behavior

When following the example of using search_path to implement isolation (seen here http://mojolicious.org/perldoc/Mojo/Pg#search_path) I ran into the problem of the search path not being set correctly for the connection that was already used to create the schema. I first demonstrated the problem via this test (when I realized that replacing the Mojo::Pg object would "fix" the problem).


use Mojo::Base -strict;

use Mojo::Pg;
use Test::More;

# NB each run of this test has to be on a fresh database

my $url = 'postgresql://...';
my $pg = Mojo::Pg->new($url)->auto_migrate(0);
$pg->db->query('drop schema if exists my_test cascade');
$pg->db->query('create schema my_test'); 
# uncomment the following line for the test to pass
#$pg = Mojo::Pg->new($url)->auto_migrate(0);
$pg->search_path(['my_test']);
$pg->migrations->from_data->migrate(0)->migrate;

my $count = eval { $pg->db->query('select count(*) from my_test.my_table')->array->[0] };
is $count, 1, 'correct number of items';
$count = undef;
$count = eval { $pg->db->query('select count(*) from public.my_table')->array->[0] };
ok ! defined($count), 'items not in public schema';

done_testing;

__DATA__

@@ migrations

-- 1 up

CREATE TABLE my_table (id BIGSERIAL PRIMARY KEY);
INSERT INTO my_table DEFAULT VALUES;

-- 1 down

DROP TABLE IF EXISTS my_first_table;

However as I began to realize that this was really about connection caching I realized that the following patten would work reliably and without the confusion. Note the setting of max_connections which prevents connection caching until after the schema is created and the search path would go into effect.

use Mojo::Base -strict;

use Mojo::Pg;
use Test::More;

my $url = 'postgresql://hubble:hubble@localhost/hubble';
my $pg = Mojo::Pg->new($url)->auto_migrate(0)->max_connections(0);
$pg->db->query('drop schema if exists my_test cascade');
$pg->db->query('create schema my_test'); 
$pg->max_connections(5)->search_path(['my_test']);
$pg->migrations->from_data->migrate(0)->migrate;

my $count = eval { $pg->db->query('select count(*) from my_test.my_table')->array->[0] };
is $count, 1, 'correct number of items';
$count = undef;
$count = eval { $pg->db->query('select count(*) from public.my_table')->array->[0] };
ok ! defined($count), 'items not in public schema';

done_testing;

__DATA__

@@ migrations

-- 1 up

CREATE TABLE my_table (id BIGSERIAL PRIMARY KEY);
INSERT INTO my_table DEFAULT VALUES;

-- 1 down

DROP TABLE IF EXISTS my_first_table;

Expected behavior

The example would be expected to create new objects in the current database schema. I see three possible solutions here.

  1. The easiest would be to add the ->max_connections(0) and later reset to the documentation in that example. It works without any code changes but it isn't very pretty.
  2. The hardest (but perhaps the most expected) would be to flush the connection cache whenever search_path is set. This is the least cognitive load on the user but is the most intrusive solution.
  3. A possibly halfway house is to add a method which will flush the current connections. Calling this method could be added to the noted example.
kraih commented 8 years ago

Not sure what to do here, for now i've just fixed the error in the docs. https://github.com/kraih/mojo-pg/commit/99cccdd7c2a7e659d81fd62fd361735aaaf63385

jberger commented 8 years ago

Ah, so there is a different version of point (1) (which is how you fixed the docs). I didn't think that would work because I didn't think it would let you remove the only schema in your search path.

avkhozov commented 8 years ago

I think Mojo::Pg should use attribute search_path for any new connections in _dequeue and for any returned connections in _enqueue methods.

Also might be in Mojo::Pg::Database add new method search_path for forced set new search_path for connection?

kraih commented 8 years ago

It's been two weeks, and i'm afraid i don't see this issue going anywhere.

avkhozov commented 8 years ago

So you think, that it's normal behavior when a db method can return connections with a schemes different from the search_path attribute?