Bug 14778

What about this patch guys?

I confirm that this patch fixes bug 14375.

% sudo echo 'wait_timeout = 10' >> /etc/mysql/conf.d/koha.cnf
% more test.pl
use Modern::Perl;
use C4::Context;
my $dbh = C4::Context->dbh;
$dbh->{RaiseError} = 1;
$dbh->{PrintError} = 1;

say $dbh->do('SELECT * FROM systempreferences');

sleep 11;

$dbh = C4::Context->dbh;
say $dbh->do('SELECT * FROM systempreferences');

% git checkout master
% perl test.pl
518
DBD::mysql::db do failed: MySQL server has gone away [for Statement "SELECT * FROM systempreferences"] at test.pl line 12.
DBD::mysql::db do failed: MySQL server has gone away [for Statement "SELECT * FROM systempreferences"] at test.pl line 12.

% git checkout -b bug_14726; git bz apply 14726
perl test.pl
518
518

(518 prefs, really???)

% git checkout -b bug_14726; git bz apply 14726

read bug 14778

(In reply to Jonathan Druart from comment #3)
> I confirm that this patch fixes bug 14375.

Well actually I am not sure.
I have tried again and now it passes on master and not on bug_14778.

MariaDB [koha]> delete from aqbooksellers;

% more benchmark_bookseller.pl
#!/usr/bin/perl
use Modern::Perl;
use C4::Bookseller;
use Koha::Acquisition::Bookseller;


for my $i ( 1 .. 1000 ) {
    C4::Bookseller::AddBookseller({
        name => "name$i",
    });
}

my $booksellers = Koha::Acquisition::Bookseller->search;
Koha::Acquisition::Bookseller->search;

% git checkout master
% time perl benchmark_bookseller.pl
perl benchmark_bookseller.pl  4.89s user 0.14s system 8% cpu 1:00.42 total

MariaDB [koha]> delete from aqbooksellers;
% git checkout bug_14778
% time perl benchmark_bookseller.plt
perl benchmark_bookseller.pl  5.02s user 0.17s system 8% cpu 1:00.64 total

% git checkout bug_14788 # With the 2 patches
% time perl benchmark_bookseller.pl
perl benchmark_bookseller.pl  5.68s user 0.29s system 9% cpu 1:00.04 total

I have also tried

diff --git a/Koha/Database.pm b/Koha/Database.pm
index 014554f..619ba6e 100644
--- a/Koha/Database.pm
+++ b/Koha/Database.pm
@@ -118,7 +118,10 @@ sub schema {
     my $params = shift;
 
     unless ( $params->{new} ) {
-        return $database->{schema} if defined $database->{schema};
+        if ( defined $database->{schema} ) {
+            $database->{schema}->storage->ensure_connected
+                and return $database->{schema};
+        }
     }
 
     $database->{schema} = &_new_schema();

But
% time perl benchmark_bookseller.pl
perl benchmark_bookseller.pl  7.14s user 0.18s system 11% cpu 1:03.67 total

I am not sure it is useful, DBIC certainly checks it on its side.

Note: Encoding looks good (read/write).

Jonathan. Everything looks good to me. Can we mark it "Needs signoff"? I added Galen and Kyle to the CC

Yep, this needs to be tested widely.

Great patch, but in this case C4::Context depends on C4::Database and C4::Database depends on C4::Context :(
Should we move database handler method to C4::Database ?
I know it will change a lot of code but isn't it more logical ?

(In reply to Fridolin SOMERS from comment #13)
> Great patch, but in this case C4::Context depends on C4::Database and
> C4::Database depends on C4::Context :(
> Should we move database handler method to C4::Database ?
> I know it will change a lot of code but isn't it more logical ?

Yes of course it is.
But I would like to get this one into master and then provide a huge change to correct all occurrences.

(In reply to Jonathan Druart from comment #14)
> (In reply to Fridolin SOMERS from comment #13)
> > Great patch, but in this case C4::Context depends on C4::Database and
> > C4::Database depends on C4::Context :(
> > Should we move database handler method to C4::Database ?
> > I know it will change a lot of code but isn't it more logical ?
> 
> Yes of course it is.
> But I would like to get this one into master and then provide a huge change
> to correct all occurrences.

Oki, we are on the same page then.
I'll try it.

This patchset removes DBIx::Connector usage, but is also moving the database handler creation responsability into the Koha::Database class.

It looks great, convenient and makes a lot of sense.

But right now it breaks some tests. Probably due to problems on the tests themselves:

Test Summary Report
-------------------
t/db_dependent/Borrower.t                            (Wstat: 65280 Tests: 8 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 13 tests but ran 8.
t/db_dependent/Circulation_issuingrules.t            (Wstat: 768 Tests: 7 Failed: 3)
  Failed tests:  2, 6-7
  Non-zero exit status: 3
t/db_dependent/Installer.t                           (Wstat: 65280 Tests: 1 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 9 tests but ran 1.
t/db_dependent/Items.t                               (Wstat: 65280 Tests: 6 Failed: 1)
  Failed test:  6
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 9 tests but ran 6.
t/db_dependent/Reports_Guided.t                      (Wstat: 256 Tests: 18 Failed: 1)
  Failed test:  16
  Non-zero exit status: 1
t/db_dependent/Search.t                              (Wstat: 65280 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 4 tests but ran 1.
t/db_dependent/Sitemapper.t                          (Wstat: 1536 Tests: 14 Failed: 6)
  Failed tests:  6, 8, 10, 12-14
  Non-zero exit status: 6

Ok, a brief about the errors and causes.

Trivial:
t/db_dependent/Borrower.t: transaction race, probably TestBuilder's fault.
t/db_dependent/Installer.t: Uses DBI directly, should be easy to tweak
t/db_dependent/Items.t: transaction race, probably TestBuilder's fault.
t/db_dependent/Reports_Guided.t: DBIx doesn't raise the warning the test expect.
t/db_dependent/Search.t: It seems to be failing to connect. The code actually mocks the connection, the _new_dbh function. There shouldn't be a behaviour change, but it fails.
t/db_dependent/Sitemapper.t: It seems to be failing to connect. The code actually mocks the connection, the _new_dbh function. There shouldn't be a behaviour change, but it fails.


Might be an important issue:
t/db_dependent/Circulation_issuingrules.t: results differ from expected

Could anyone interested in this bug report have a look at the last patch and try to understand what's wrong?

This is the way to go, it will take me ages to update all occurrences...

Second try
- t/Acquisition/Invoice.t: I don't understand what it tested here.
- t/Barcodes_ValueBuilder.t: It does not seem to work because of the complex select in C4::Barcodes::ValueBuilder (substring_index, etc.)
DBI Exception: DBD::SQLite::db prepare failed: no such function: SUBSTRING [for Statement "SELECT MAX(CAST(SUBSTRING(barcode,-4) AS signed)) AS number FROM items WHERE barcode REGEXP ?"] at t/Barcodes_ValueBuilder.t line 56

FTR: http://lists.scsys.co.uk/pipermail/dbix-class/2006-June/001690.html

Also, this patches fixed the "MySQL server has gone away" problem in the test server.

*** Bug 15034 has been marked as a duplicate of this bug. ***

Enjoy

Note about DBIx::Class warnings:

These changes will raise warnings:

DBIx::Class::Storage::DBI::_warn_undetermined_driver(): This version of DBIC does not yet seem to supply a driver for your particular RDBMS and/or connection method ('Mock'). While we will attempt to continue anyway, the results are likely to be underwhelming. Please upgrade DBIC, and if this message does not go away, file a bugreport including the following info:
{
  DBD => "DBD::Mock",
  DBD_VER => "1.45",
  DBIC_DRIVER => "DBIx::Class::Storage::DBI",
  DBIC_DSN => "DBI:Mock:",
  DBIC_VER => "0.082820",
  DBI_VER => "1.631"
}

They can be hidden creating a package DBIx::Class::Storage::DBI::Mock (in /usr/share/perl5/DBIx/Class/Storage/DBI/Mock.pm for instance):
===
package DBIx::Class::Storage::DBI::Mock;
use base DBIx::Class::Storage::DBI;
1;
===

We will still get:
DBIx::Class::Storage::DBI::sql_maker(): Your storage class (DBIx::Class::Storage::DBI::Mock) does not set sql_limit_dialect and you have not supplied an explicit limit_dialect in your connection_info. DBIC will attempt to use the GenericSubQ dialect, which works on most databases but can be (and often is) painfully slow. Please file an RT ticket against 'DBIx::Class::Storage::DBI::Mock'

This could be fixed later with deeper examinations.

The only (and not minor) issue I see is: tests relying on Test::DBIx::Class only make sense in t/* (i.e. the point of db_dependent tests is that they are integration tests, using the DB for real). So the problem is that if we don't have Test::DBIx::Class packaged, then the .deb packages cannot build (they need prove t/ pass clean).

So we need to either:
(a) have Test::DBIx::Class and friends packaged (Robin?)
(b) have those tests conditional to its availability:

use Module::Load::Conditional qw/check_install/;

BEGIN {
    if ( check_install( module => 'Test::DBIx::Class' ) ) {
        plan tests => 11;
    } else {
        plan skip_all => "Need Test::DBIx::Class"
    }
}


(b) is sub optimal. We want to run the tests. But (a) needs Robin's intervention I think, so I have no clue. Help!

(In reply to Tomás Cohen Arazi from comment #45)
> So we need to either:
> (a) have Test::DBIx::Class and friends packaged (Robin?)

I'll have a look to see how feasible this is. It's probably not terrible.

*** Bug 15031 has been marked as a duplicate of this bug. ***

*** Bug 13699 has been marked as a duplicate of this bug. ***

Agree with Tomas, all looks good here to me.. but he's right about either making the tests conditions or packaging up Test::DBIx::Class and friends.

Jonathan, many thanks for seeing this one through.. it's a real step in the right direction and I know was a bit of an *** to work through.

On Jessie:
  dh-make-perl --pkg-perl --build --cpan Test::DBIx::Class
build a wonderfull libtest-dbix-class-perl_0.45-1_all.deb package without any error.

Poke, just awaiting yeah or neigh from Robin on this one.. I'll happily sign everything off again once we have a definitive In/Out on packaging the test dependencies :)

(In reply to Martin Renvoize from comment #51)
> Poke, just awaiting yeah or neigh from Robin on this one.. I'll happily sign
> everything off again once we have a definitive In/Out on packaging the test
> dependencies :)

Oh bother*. I'm speaking at a conference next week and spent most of my time in the past few days preparing for that. I might get on to this in the weekend, but it's hard to promise. And next week I'll be in Hobart. So sorry for not being on to this quickly.

* said Poo as he carved Eeyore's name into a black candle.

Thanks for the reply Robin.. I'll go ahead and get the ball rolling with the signoff and we can worry about the packaging stuff after that.

I'm afraid I get a QA script failure Jonathan:  

FAIL   t/db_dependent/Barcodes_ValueBuilder.t
   OK     critic
   OK     forbidden patterns
   OK     pod
   OK     spelling
   FAIL   valid
                "my" variable $DEBUG masks earlier declaration in same scope 

Any thoughts?

Signed everything off, we decided the failure in the qa script was in fact an error in the modules itself which only presents in the qa script run (the tests do all pass if run with prove)

Well tidy up the offending module later.. it's not a killer.. more of a style thing.

Created attachment 43857 [details] [review]
[PASSED QA] Bug 14778: Get rid of DBIx::Connector

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43858 [details] [review]
[PASSED QA] Bug 14778: Make sure the dbh returned is checked by DBIC

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43859 [details] [review]
[PASSED QA] Bug 14778: Make 3 tests pass

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43860 [details] [review]
[PASSED QA] Bug 14778: Make the installer test pass - use DBI missing

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43861 [details] [review]
[PASSED QA] Bug 14778: Set PrintError as it was before

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43862 [details] [review]
[PASSED QA] Bug 14778: Example - Replace DBI::Mock with Test::DBIx::Class - Sitemapper.t

We can use Test::DBIx::Class to install fixtures before our tests.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43863 [details] [review]
[PASSED QA] Bug 14778: Mocks Koha::Database->_new_schema instead of C4::Context->_new_dbh

C4::Context::_new_dbh does not exist anymore.
Koha::Database::_new_schema should be mocked instead.

Will fix:
- t/00-load.t
- t/Breeding.t
- t/ImportBatch.t
- t/Message.t
- t/Overdues.t
- t/Prices.t
- t/RotatingCollections.t
- t/Search.t
- t/SuggestionEngine_AuthorityFile.t
- t/XSLT.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43864 [details] [review]
[PASSED QA] Bug 14778: Use mock_dbh where it should be used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43865 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Calendar.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43866 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Biblio.t

Note that it already passed before

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43867 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Images.t

Note that this tests file were completely buggy before.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43868 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/ItemType.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43869 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Koha.t

Warning: This patch modifies a module!
What's the need of the binary function here?
The data are case insensitive, so no need to use this mysql function.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43870 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Letters.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43871 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Matcher.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43872 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/Members_AttributeTypes.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43873 [details] [review]
[PASSED QA] Bug 14778: Install fixtures for t/SocialData.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43874 [details] [review]
[PASSED QA] Bug 14778: Make Barcodes_ValueBuilder.t db dependent

The get_barcode subroutines call very mysql specific functions and it's
not possible to easily use fixtures here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43875 [details] [review]
[PASSED QA] Bug 14778: Mock the dbh for t/db_dependent/Search.t

No need to mock the items' columns (?)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 43876 [details] [review]
[PASSED QA] Bug 14778: Remove t/Acquisition/Invoices.t

Using Test::DBIx::Class now, we cannot do what we did in this file with DBD::Mock.
Since the Invoice[s] subroutines are already tested in
t/db_dependent/Acquisition/Invoices.t there is no special needs to have
these ones.
Instead of loosing 2 hours on this file, I would prefer to remove it.
Feel free to provide a counter patch.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Patches pushed to master.

Thanks Jonathan!

Created attachment 44003 [details] [review]
Bug 14778: Make Search.t pass - replace SHOW COLUMNS

For an unkown reason, when executed from a test file, the 'SHOW COLUMNS'
statement does not return anything.
We need to retrieve the column list from the DBIx::Class resultset.

(In reply to Jonathan Druart from comment #50)
> On Jessie:
>   dh-make-perl --pkg-perl --build --cpan Test::DBIx::Class
> build a wonderfull libtest-dbix-class-perl_0.45-1_all.deb package without
> any error.

I suspect you had an unclean environment.

The dh-make-perl says:

Needs the following modules for which there are no debian packages available:
 - Test::mysqld
 - MooseX::Attribute::ENV
 - Test::PostgreSQL
 - DateTime::Format::Pg
 - DBIx::Class::Schema::PopulateMore
 - DBIx::Class::UUIDColumns
 - DBIx::Class::TimeStamp
 - Data::Visitor

The build (wheezy in a pbuild env) refuses to go anywhere:

The following packages have unmet dependencies:
 pbuilder-satisfydepends-dummy : Depends: libdbix-class-schema-populatemore-perl (>= 0.16) which is a virtual package.
                                 Depends: libdbix-class-timestamp-perl (>= 0.13) but it is not going to be installed.
                                 Depends: libdbix-class-uuidcolumns-perl (>= 0.02005) which is a virtual package.
                                 Depends: libmoosex-attribute-env-perl (>= 0.01) which is a virtual package.
                                 Depends: libmoosex-types-perl (>= 0.23) but it is not going to be installed.
                                 Depends: libossp-uuid-perl (>= 1.215) but it is not going to be installed.
                                 Depends: libsql-translator-perl (>= 0.11006) but it is not going to be installed.
                                 Depends: libtest-deep-perl (>= 0.106) but it is not going to be installed.
                                 Depends: libtest-mysqld-perl (>= 0.14) which is a virtual package.
                                 Depends: libtest-postgresql-perl (>= 0.09) which is a virtual package.
                                 Depends: perl (>= 5.15.2) but 5.14.2-21+deb7u2 is installed.

I don't think it's possible to make this work in a supported environment. There's no way I'm backporting a new perl version, for one :)

*** Bug 14375 has been marked as a duplicate of this bug. ***

*** Bug 14600 has been marked as a duplicate of this bug. ***

Quick note about
  Bug 14778: Install fixtures for t/Koha.t

time prove t/Koha.t now returns ~15s instead of ~2sec before

I'm using the following:

use t::lib::Mocks;

BEGIN {
    t::lib::Mocks::mock_dbh;
}

However, I'm still getting the warning:

DBIx::Class::Storage::DBI::_warn_undetermined_driver(): This version of DBIC does not yet seem to supply a driver for your particular RDBMS and/or connection method ('Mock'). While we will attempt to continue anyway, the results are likely to be underwhelming. Please upgrade DBIC, and if this message does not go away, file a bugreport including the following info:

I'm using the newest DBIx::Class, so I don't think it's a case of upgrading DBIC.

*** Bug 14742 has been marked as a duplicate of this bug. ***