Description
Kyle M Hall (khall)
2014-03-04 18:32:27 UTC
Created attachment 25828 [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 the patch for bug 11634 2) prove t/db_dependent/Circulation.t should fail 3) Apply this patch 4) prove t/db_dependent/Circulation.t should not fail Created attachment 25829 [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 the patch for bug 11634 2) prove t/db_dependent/Circulation.t should fail 3) Apply this patch 4) prove t/db_dependent/Circulation.t should not fail I'm immediately putting this into discussion, as I'm exploring the reverse: letting DBIC manage the storage handlers. (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. 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. 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? Created attachment 28962 [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> Created attachment 28963 [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> Created attachment 28964 [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> Created attachment 28965 [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> 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. 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> 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> 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> 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> Pushed to master. Thanks Kyle and Galen! |