Bug 18003 - Should deletedborrowers.borrowernumber be a primary key?
Summary: Should deletedborrowers.borrowernumber be a primary key?
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL: http://lists.koha-community.org/piper...
Keywords:
Depends on: 17782
Blocks: 18081
  Show dependency treegraph
 
Reported: 2017-01-27 10:43 UTC by Jonathan Druart
Modified: 2019-06-07 10:35 UTC (History)
11 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 18003: Example of why we would need a PK on deletedborrowers (3.13 KB, patch)
2017-01-27 10:51 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-01-27 10:43:14 UTC
(Raised from bug 17782)

DBIC needs a primary key to work correctly.
We could be tempt to make borrowernumber a primary key, but a use case could bring thing. Indeed if a table is empty and the mysql (mariadb behaves identically) server restarted, the Auto Increment (AI) is reset.

Problematic use case:
1. Install Koha
2. Create a patron (borrowers.borrowernumber = 1)
3. Delete it (deletedborrowers.borrowernumer = 1, and AI is set to 2
4. Restart mysql
=> borrowers is empty so AI is reset
5. Create a patron (borrowers.borrowernumber = 1)
6. Delete it
=> We will try to insert a new row with an existing borrowernumber (1) into the deletedborrowers table: BOOM

Solution 1:
Do not care about this problematic use case and prevent to delete the last patron.

Solution 2:
Add a AI primary key (id) to deletedborrowers
=> The 2 tables borrowers and deletedborrowers will differ, bugs will be expected

Solution 3:
Deal with that at code level: when a patron is created, guess the borrowernumber it will get and check if it does not already exist in the deletedborrowers table. If so, force it before inserting it

Solution 4:
Do not enforce this constraint at DB level but set the primary key to DBIC schema:
  Koha::Schema::Result::Deletedborrower: __PACKAGE__->set_primary_key("borrowernumber");
Problem:
  Koha::Database->new->schema->resultset('Deletedborrower')->find(42);
will raise a warning "DBIx::Class::Storage::DBI::select_single(): Query returned more than one row.  SQL that returns multiple rows is DEPRECATED for ->find and ->single" if 42 is duplicated, and the first matching row will be picked

Another solution?
Comment 1 Jonathan Druart 2017-01-27 10:51:23 UTC
Created attachment 59616 [details] [review]
Bug 18003: Example of why we would need a PK on deletedborrowers

These tests do not pass:
DBIx::Class::ResultSource::_pri_cols_or_die(): Operation requires a primary key to be declared on 'Deletedborrower' via set_primary_key at t/db_dependent/Koha/Patrons.t line 329
Comment 2 Josef Moravec 2017-01-30 11:16:48 UTC
(In reply to Jonathan Druart from comment #0)
> 
> Solution 1:
> Do not care about this problematic use case and prevent to delete the last
> patron.

I do not like this - it escalates the internal technical problem to end user...
 
> Solution 2:
> Add a AI primary key (id) to deletedborrowers
> => The 2 tables borrowers and deletedborrowers will differ, bugs will be
> expected

It could be possible I think, but probably not easy to make it
 
> Solution 3:
> Deal with that at code level: when a patron is created, guess the
> borrowernumber it will get and check if it does not already exist in the
> deletedborrowers table. If so, force it before inserting it
> 

A bit complicated, but probably the best one

> Solution 4:
> Do not enforce this constraint at DB level but set the primary key to DBIC
> schema:
>   Koha::Schema::Result::Deletedborrower:
> __PACKAGE__->set_primary_key("borrowernumber");
> Problem:
>   Koha::Database->new->schema->resultset('Deletedborrower')->find(42);
> will raise a warning "DBIx::Class::Storage::DBI::select_single(): Query
> returned more than one row.  SQL that returns multiple rows is DEPRECATED
> for ->find and ->single" if 42 is duplicated, and the first matching row
> will be picked

I do not like this too

>
> Another solution?


So I am for 2 or 3, 2 was what came first to my mind, but the 3 is better I think...
Comment 3 Josef Moravec 2017-01-30 11:17:54 UTC
> Another solution?

Merge borrowers and deletedborrowers to one table and mark the deleted one with a flag
Comment 4 Marcel de Rooy 2017-01-30 11:33:40 UTC
Does the problem mentioned on top only occur on an empty borrowers table with a mysql restart? Supposing that this is the case and it is quite rare, can't we just use borrowernumber as the primary key on borrowers and deletedborrowers, obviously having no constraints on deletedborrowers.

We could even add a check for borrowernumber when creating a patron, and update the borrowernumber if needed to not match deletedborrowers? Ugly, but fixing some problems.
Comment 5 Katrin Fischer 2017-01-30 11:35:54 UTC
We discussed if it also happens when you delete the last added borrower and then restart - not sure what the answer was.
Comment 6 Jonathan Druart 2017-01-31 08:44:00 UTC
(In reply to Katrin Fischer from comment #5)
> We discussed if it also happens when you delete the last added borrower and
> then restart - not sure what the answer was.

Indeed this is a case that sucks as well.
Comment 7 Jonathan Druart 2017-01-31 18:43:31 UTC
See also "Merge borrowers and deletedborrowers tables" topic on koha-devel 1 year ago:
http://lists.koha-community.org/pipermail/koha-devel/2016-January/042207.html
Comment 8 Tomás Cohen Arazi 2017-01-31 18:50:32 UTC
(In reply to Jonathan Druart from comment #7)
> See also "Merge borrowers and deletedborrowers tables" topic on koha-devel 1
> year ago:
> http://lists.koha-community.org/pipermail/koha-devel/2016-January/042207.html

We don't provide a way to recover a deleted borrower, do we?
Comment 9 Martin Renvoize 2017-02-08 13:11:34 UTC
I'd +1 the merging of tables.. but can see the annoyance it would cause people who have written reports using said tables.. was that the reason it was not done a year ago?
Comment 10 Tomás Cohen Arazi 2017-02-08 14:57:05 UTC
(In reply to Martin Renvoize from comment #9)
> I'd +1 the merging of tables.. but can see the annoyance it would cause
> people who have written reports using said tables.. was that the reason it
> was not done a year ago?

Can't we creaste a view for those?
Comment 11 Martin Renvoize 2017-02-08 15:00:30 UTC
I like the idea of creating a view.. but the only method that would work seamlessly would be to create an entirely new 'borrowers' table with a new name and create a view for each of the old borrowers and deletedborrowers tables.

Not a massive issue, but worth thinking about.. I kinda like the idea of 'doing it properly, and using views to maintain backwards compatibility with old reports'
Comment 12 Jonathan Druart 2017-02-08 17:12:28 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> (In reply to Martin Renvoize from comment #9)
> > I'd +1 the merging of tables.. but can see the annoyance it would cause
> > people who have written reports using said tables.. was that the reason it
> > was not done a year ago?
> 
> Can't we creaste a view for those?

For the info: I have created a view on bug 17835 (different need, different problem, but it is worth noting).