Bug 11891 - Make Koha::Schema use C4::Context->dbh
Summary: Make Koha::Schema use C4::Context->dbh
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 11634
  Show dependency treegraph
 
Reported: 2014-03-04 18:32 UTC by Kyle M Hall
Modified: 2015-06-04 23:33 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11891 - Make Koha::Schema use C4::Context->dbh (2.54 KB, patch)
2014-03-04 18:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11891 - Make Koha::Schema use C4::Context->dbh (2.61 KB, patch)
2014-03-04 18:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11891: set up tests to require that Koha::Database uses same DB handle as C4::Context (2.69 KB, patch)
2014-06-18 23:16 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11891 - Make Koha::Schema use C4::Context->dbh (2.90 KB, patch)
2014-06-18 23:16 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11891: (follow-up) remove custom storage class (2.12 KB, patch)
2014-06-18 23:18 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11891: (follow-up) set DBIC storage flags correctly (1.20 KB, patch)
2014-06-18 23:18 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11891: set up tests to require that Koha::Database uses same DB handle as C4::Context (2.73 KB, patch)
2014-06-18 23:28 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11891 - Make Koha::Schema use C4::Context->dbh (2.94 KB, patch)
2014-06-18 23:29 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11891: (follow-up) remove custom storage class (2.17 KB, patch)
2014-06-18 23:29 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11891: (follow-up) set DBIC storage flags correctly (1.25 KB, patch)
2014-06-18 23:29 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-03-04 18:32:27 UTC
Right now there is no connection between the database handles used by C4::Context::dbh and Koha::Datbase/Schema. This makes it impossible to use transactions in unit tests to temporarily modify the database to test subroutines that take advantage of Koha::Database. This patch fixes that issue.
Comment 1 Kyle M Hall 2014-03-04 18:34:18 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-03-04 18:35:49 UTC Comment hidden (obsolete)
Comment 3 Galen Charlton 2014-03-04 18:38:39 UTC
I'm immediately putting this into discussion, as I'm exploring the reverse: letting DBIC manage the storage handlers.
Comment 4 Marcel de Rooy 2014-03-05 11:49:22 UTC
(In reply to Galen Charlton from comment #3)
> I'm immediately putting this into discussion, as I'm exploring the reverse:
> letting DBIC manage the storage handlers.

I would think too that we should move in the direction from C4 to Koha instead of the other way around. Let C4::Context ask a handle from the DBIC based classes during the transition phase.
Comment 5 Kyle M Hall 2014-03-05 12:07:43 UTC
I also agree that is a more sensible long term approach than this one.

(In reply to M. de Rooy from comment #4)
> (In reply to Galen Charlton from comment #3)
> > I'm immediately putting this into discussion, as I'm exploring the reverse:
> > letting DBIC manage the storage handlers.
> 
> I would think too that we should move in the direction from C4 to Koha
> instead of the other way around. Let C4::Context ask a handle from the DBIC
> based classes during the transition phase.
Comment 6 Jonathan Druart 2014-06-12 12:42:46 UTC
Galen, do you progress on the other solution?

In my opinion this patch could be pushed as it since it fixes some unit test failures (on master and 3.14, see t/db_dependent/Circulation_issue.t).
The other way to do could be implemented later, don't you think?
Comment 7 Galen Charlton 2014-06-18 23:16:53 UTC Comment hidden (obsolete)
Comment 8 Galen Charlton 2014-06-18 23:16:57 UTC Comment hidden (obsolete)
Comment 9 Galen Charlton 2014-06-18 23:18:04 UTC Comment hidden (obsolete)
Comment 10 Galen Charlton 2014-06-18 23:18:11 UTC Comment hidden (obsolete)
Comment 11 Galen Charlton 2014-06-18 23:18:21 UTC
OK, I've finally finished testing.  Running with Kyle's approach for now, but with follow-ups that ensure that more things are tested, that the RaiseError behavior doesn't change unexpectedly, and that it doesn't default to generating inefficient SQL statements involving a LIMIT.

Signed off.
Comment 12 Chris Cormack 2014-06-18 23:28:45 UTC
Created attachment 28966 [details] [review]
Bug 11891: set up tests to require that Koha::Database uses same DB handle as C4::Context

This patch removes changes in two cases to work around the fact that
Koha::Database and C4::Context were not using the same DBI database
handle.

To test:

[1] Run prove -v t/db_dependent/ILSDI_Services.t and
    prove -v t/db_dependent/Items.t.  Both tests
    should fail.
[2] Apply the main patches and run step 1 again.  This
    time, both tests should pass.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 13 Chris Cormack 2014-06-18 23:29:02 UTC
Created attachment 28967 [details] [review]
Bug 11891 - Make Koha::Schema use C4::Context->dbh

Right now there is no connection between the database handles used by
C4::Context::dbh and Koha::Datbase/Schema. This makes it impossible to
use transactions in unit tests to temporarily modify the database to
test subroutines that take advantage of Koha::Database. This patch fixes
that issue.

Test Plan:
1) Apply this patch
2) prove -v t/db_dependent/ILSDI_Services.t and
   prove -v t/db_dependent/Items.t and
   prove -v t/db_dependent/Circulation_issue.t should
   all start passing

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 14 Chris Cormack 2014-06-18 23:29:18 UTC
Created attachment 28968 [details] [review]
Bug 11891: (follow-up) remove custom storage class

For now, just rely on the normal DBIx::Class cleanup handler;
otherwise, the following warning is displayed in logs:

  (in cleanup) Can't locate object method "_preserve_foreign_dbh" via package "Koha::Storage" at .../Koha/Storage.pm

Using the default DBIC storage class also means that we
don't have to manually do things like set the SQL limit dialect;
meaning that we don't get the following warning message:

  DBIx::Class::ResultSet::find(): Your storage class (Koha::Storage) 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
  'Koha::Storage' . at /home/gmc/koha/C4/Members.pm line 840

Test plan: same as main patch

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 15 Chris Cormack 2014-06-18 23:29:43 UTC
Created attachment 28969 [details] [review]
Bug 11891: (follow-up) set DBIC storage flags correctly

Since we're passing an existing DBI database handle to
DBIC, and that handle doesn't have RaiseError set to true
by default, don't let DBIC override that -- for now.

Test plan: verify that the DB-dependent test suite passes.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 16 Tomás Cohen Arazi 2014-06-19 15:14:17 UTC
Pushed to master.

Thanks Kyle and Galen!