Bug 18003

Summary: Should deletedborrowers.borrowernumber be a primary key?
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Galen Charlton <gmcharlt>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: alex, fridolin.somers, george, josef.moravec, julian.maurice, kyle, m.de.rooy, martin.renvoize, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
URL: http://lists.koha-community.org/pipermail/koha-devel/2016-January/042207.html
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18185
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18242
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18931
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20271
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 17782    
Bug Blocks: 18081    
Attachments: Bug 18003: Example of why we would need a PK on deletedborrowers

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 (tcohen) 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 (ashimema) 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 (tcohen) 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 (ashimema) 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).