Bugzilla – Attachment 19196 Details for
Bug 10454
Duplicate card numbers may be generated
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 10454 - Duplicate card numbers may be generated
Bug-10454---Duplicate-card-numbers-may-be-generate.patch (text/plain), 11.93 KB, created by
Mark Tompsett
on 2013-06-22 17:30:53 UTC
(
hide
)
Description:
Bug 10454 - Duplicate card numbers may be generated
Filename:
MIME Type:
Creator:
Mark Tompsett
Created:
2013-06-22 17:30:53 UTC
Size:
11.93 KB
patch
obsolete
>From dda18c086ce6a40cc176a6ab704d19e43631f452 Mon Sep 17 00:00:00 2001 >From: Mark Tompsett <mtompset@hotmail.com> >Date: Sat, 22 Jun 2013 13:00:48 -0400 >Subject: [PATCH] Bug 10454 - Duplicate card numbers may be generated > >Previously, there was a gap in time between the insertion of the >card number into borrowers and the generation. This gap meant that >the same card number could be calculated by two different processes >resulting in two different members being added with the same card >number. > >By serializing the cardnumber requests, at worst we may skip a >cardnumber, rather than duplicate one. No locking required, >because the INSERT into cardnumber_sequence is atomic! > >The reason no duplicates happen is because the sequence of >cardnumbers is calculated from the previous one, and recalculating >would generate exactly the same value. So even if two processes >are calculating the same cardnumber for a given UID record, it >will result in the same value. > >Skips can be generated. Clicking refresh while entering a patron, >for example, will increment the auto generated number multiple >times. >--- > C4/Members.pm | 180 +++++++++++++++++++++++--------- > installer/data/mysql/updatedatabase.pl | 101 ++++++++++++++++++ > kohaversion.pl | 2 +- > 3 files changed, 230 insertions(+), 53 deletions(-) > >diff --git a/C4/Members.pm b/C4/Members.pm >index 3221f53..bca4eb8 100644 >--- a/C4/Members.pm >+++ b/C4/Members.pm >@@ -881,72 +881,148 @@ sub changepassword { > > =head2 fixup_cardnumber > >-Warning: The caller is responsible for locking the members table in write >-mode, to avoid database corruption. >+Previously, there was a gap in time between the insertion of the >+card number into borrowers and the generation. This gap meant that >+the same card number could be calculated by two different processes >+resulting in two different members being added with the same card >+number. > >-=cut >+By serializing the cardnumber requests, at worst we may skip a >+cardnumber, rather than duplicate one. No locking required, >+because the INSERT into cardnumber_sequence is atomic! > >-use vars qw( @weightings ); >-my @weightings = ( 8, 4, 6, 3, 5, 2, 1 ); >+The reason no duplicates happen is because the sequence of >+cardnumbers is calculated from the previous one, and recalculating >+would generate exactly the same value. So even if two processes >+are calculating the same cardnumber for a given UID record, it >+will result in the same value. >+ >+=cut > > sub fixup_cardnumber { > my ($cardnumber) = @_; >- my $autonumber_members = C4::Context->boolean_preference('autoMemberNum') || 0; >+ my @weightings = ( 8, 4, 6, 3, 5, 2, 1 ); >+ my ( >+ $autonumber_members, >+ $checkdigit, >+ $dbh, >+ $sth, >+ $my_UID, >+ $current_UID, >+ $previous_UID, >+ $previous_cardnumber, >+ $current_cardnumber, >+ $sum, >+ $i, >+ $temp1, >+ $temp2, >+ $rem, >+ $new_cardnumber, >+ ); > > # Find out whether member numbers should be generated > # automatically. Should be either "1" or something else. > # Defaults to "0", which is interpreted as "no". >+ $autonumber_members = C4::Context->boolean_preference('autoMemberNum') || 0; > >- # if ($cardnumber !~ /\S/ && $autonumber_members) { >+ # if we don't auto-generate, then there is nothing to fixup. >+ # So return what was passed. > ($autonumber_members) or return $cardnumber; >- my $checkdigit = C4::Context->preference('checkdigit'); >- my $dbh = C4::Context->dbh; >- if ( $checkdigit and $checkdigit eq 'katipo' ) { >- >- # if checkdigit is selected, calculate katipo-style cardnumber. >- # otherwise, just use the max() >- # purpose: generate checksum'd member numbers. >- # We'll assume we just got the max value of digits 2-8 of member #'s >- # from the database and our job is to increment that by one, >- # determine the 1st and 9th digits and return the full string. >- my $sth = $dbh->prepare( >- "select max(substring(borrowers.cardnumber,2,7)) as new_num from borrowers" >- ); >- $sth->execute; >- my $data = $sth->fetchrow_hashref; >- $cardnumber = $data->{new_num}; >- if ( !$cardnumber ) { # If DB has no values, >- $cardnumber = 1000000; # start at 1000000 >- } else { >- $cardnumber += 1; >- } >- >- my $sum = 0; >- for ( my $i = 0 ; $i < 8 ; $i += 1 ) { >- # read weightings, left to right, 1 char at a time >- my $temp1 = $weightings[$i]; > >- # sequence left to right, 1 char at a time >- my $temp2 = substr( $cardnumber, $i, 1 ); >- >- # mult each char 1-7 by its corresponding weighting >- $sum += $temp1 * $temp2; >+ # determine which kind of auto-generate we are doing >+ $checkdigit = C4::Context->preference('checkdigit'); >+ >+ # grab a database handle. >+ $dbh = C4::Context->dbh; >+ >+ # first we need to show our intent to generate a cardnumber. >+ $sth = $dbh->prepare(" >+INSERT INTO cardnumber_sequence >+ (cardnumber,checkdigit) VALUES >+ ('',?); >+ "); >+ $sth->execute($checkdigit); >+ $my_UID = $dbh->last_insert_id(undef,undef,'cardnumber_sequence','UID'); >+ >+ # next we need to find the first ungenerated cardnumber. >+ # if we don't find any, another process filled them for us! >+ # if we find some, then we need to start generating. >+ $sth = $dbh->prepare(" >+SELECT MIN(UID) AS UID FROM cardnumber_sequence >+WHERE cardnumber='' AND checkdigit=?; >+ "); >+ $sth->execute($checkdigit); >+ $current_UID = $sth->fetchrow(); >+ >+ # if there is a current_UID, we need to calculate >+ while ($current_UID) { >+ >+ # so find the previous UID >+ $sth = $dbh->prepare(" >+SELECT MAX(UID) AS UID FROM cardnumber_sequence >+WHERE UID < ? AND checkdigit=?; >+ "); >+ $sth->execute($current_UID,$checkdigit); >+ $previous_UID = $sth->fetchrow(); >+ >+ # grab the previous cardnumber >+ $sth = $dbh->prepare(" >+SELECT cardnumber FROM cardnumber_sequence >+WHERE UID = ?; >+ "); >+ $sth->execute($previous_UID); >+ $previous_cardnumber = $sth->fetchrow(); >+ >+ # now calculate the current cardnumber. >+ if ($checkdigit && $checkdigit eq 'katipo') { >+ $current_cardnumber = substr($previous_cardnumber,1,7); >+ $current_cardnumber = $current_cardnumber+1; >+ $sum = 0; >+ foreach $i (1..7) { >+ # read weightings, left to right, 1 char at a time >+ $temp1 = $weightings[$i-1]; >+ >+ # sequence left to right, 1 char at a time >+ $temp2 = substr( $current_cardnumber, $i-1, 1 ); >+ >+ # mult each char 1-7 by its corresponding weighting >+ $sum += $temp1 * $temp2; >+ } >+ $rem = ( $sum % 11 ); >+ $rem = 'X' if $rem == 10; >+ $current_cardnumber = "V$current_cardnumber$rem"; >+ } >+ else { >+ $current_cardnumber = $previous_cardnumber+1; > } > >- my $rem = ( $sum % 11 ); >- $rem = 'X' if $rem == 10; >- >- return "V$cardnumber$rem"; >- } else { >- >- my $sth = $dbh->prepare( >- 'SELECT MAX( CAST( cardnumber AS SIGNED ) ) FROM borrowers WHERE cardnumber REGEXP "^-?[0-9]+$"' >- ); >- $sth->execute; >- my ($result) = $sth->fetchrow; >- return $result + 1; >- } >- return $cardnumber; # just here as a fallback/reminder >+ # update the current_UID record with the generated cardnumber. >+ $sth = $dbh->prepare(" >+UPDATE cardnumber_sequence SET cardnumber=? WHERE UID=?; >+ "); >+ $sth->execute($current_cardnumber,$current_UID); >+ >+ # we need to find the next ungenerated cardnumber. >+ # if we don't find any, we are done generating everything. >+ $sth = $dbh->prepare(" >+SELECT MIN(UID) AS UID FROM cardnumber_sequence >+WHERE cardnumber='' and checkdigit=?; >+ "); >+ $sth->execute($checkdigit); >+ $current_UID = $sth->fetchrow(); >+ >+ } >+ >+ # need already know our UID ($my_UID) >+ # let's grab the cardnumber. >+ $sth = $dbh->prepare(" >+SELECT cardnumber FROM cardnumber_sequence >+WHERE UID=?; >+ "); >+ $sth->execute($my_UID); >+ $new_cardnumber = $sth->fetchrow(); >+ >+ return $new_cardnumber; > } > > =head2 GetGuarantees >diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl >index 5963d24..7a128eb 100755 >--- a/installer/data/mysql/updatedatabase.pl >+++ b/installer/data/mysql/updatedatabase.pl >@@ -7010,6 +7010,107 @@ CREATE TABLE IF NOT EXISTS borrower_files ( > SetVersion($DBversion); > } > >+$DBversion = "3.13.00.XXX"; >+if ( CheckVersion($DBversion) ) { >+ >+ # Create new table used to serialize cardnumber requests. >+ $dbh->do(" >+CREATE TABLE IF NOT EXISTS cardnumber_sequence ( >+ UID INT(16) NOT NULL AUTO_INCREMENT, >+ cardnumber VARCHAR(16) NOT NULL, >+ checkdigit TEXT NOT NULL, >+ PRIMARY KEY (UID) >+) ENGINE=InnoDB DEFAULT CHARSET=utf8; >+ "); >+ >+ my ( >+ $dbh, >+ $sth, >+ @cardnumbers_katipo, >+ @cardnumbers_none, >+ @cardnumbers, >+ $cardnumber, >+ $data, >+ ); >+ >+ # Add 2 records for the case that borrowers is empty. >+ $dbh = C4::Context->dbh; >+ $sth = $dbh->prepare(" >+INSERT INTO cardnumber_sequence >+ (cardnumber,checkdigit) VALUES >+ ('0','none'); >+ "); >+ $sth->execute(); >+ >+ $sth = $dbh->prepare(" >+INSERT INTO cardnumber_sequence >+ (cardnumber,checkdigit) VALUES >+ ('V09999992','katipo'); >+ "); >+ $sth->execute(); >+ >+ # Convert existing borrowers records into records that >+ # were supposedly generated. >+ >+ # grab checkdigit = katipo records >+ $sth = $dbh->prepare(" >+SELECT cardnumber FROM borrowers >+WHERE SUBSTR(cardnumber,1,1) >+ NOT IN ('0','1','2','3','4','5','6','7','8','9') >+ORDER BY cardnumber; >+ "); >+ $sth->execute(); >+ $data = $sth->fetchrow_hashref(); >+ $cardnumber = undef; >+ $cardnumber = $data->{'cardnumber'} if $data->{'cardnumber'}; >+ while ($data->{'cardnumber'}) { >+ push @cardnumbers_katipo,$data->{'cardnumber'}; >+ $data = $sth->fetchrow_hashref(); >+ $cardnumber = undef; >+ $cardnumber = $data->{'cardnumber'} if $data->{'cardnumber'}; >+ } >+ >+ # grab checkdigit = none records >+ $sth = $dbh->prepare(" >+SELECT cardnumber FROM borrowers >+WHERE SUBSTR(cardnumber,1,1) >+ IN ('0','1','2','3','4','5','6','7','8','9') >+ORDER BY CAST(cardnumber AS SIGNED); >+ "); >+ $sth->execute(); >+ $data = $sth->fetchrow_hashref(); >+ $cardnumber = undef; >+ $cardnumber = $data->{'cardnumber'} if $data->{'cardnumber'}; >+ while ($data->{'cardnumber'}) { >+ push @cardnumbers_none,$data->{'cardnumber'}; >+ $data = $sth->fetchrow_hashref(); >+ $cardnumber = undef; >+ $cardnumber = $data->{'cardnumber'} if $data->{'cardnumber'}; >+ } >+ >+ # add the card numbers in. >+ foreach $cardnumber (@cardnumbers_none) { >+ $sth = $dbh->prepare(" >+INSERT INTO cardnumber_sequence >+ (cardnumber,checkdigit) VALUES >+ (?,'none'); >+ "); >+ $sth->execute($cardnumber); >+ } >+ >+ foreach $cardnumber (@cardnumbers_katipo) { >+ $sth = $dbh->prepare(" >+INSERT INTO cardnumber_sequence >+ (cardnumber,checkdigit) VALUES >+ (?,'katipo'); >+ "); >+ $sth->execute($cardnumber); >+ } >+ >+ print "Upgrade to $DBversion done (Bug 10454: Duplicate card numbers may be generated)\n"; >+ SetVersion($DBversion); >+} >+ > =head1 FUNCTIONS > > =head2 TableExists($table) >diff --git a/kohaversion.pl b/kohaversion.pl >index 71938e4..863d91f 100644 >--- a/kohaversion.pl >+++ b/kohaversion.pl >@@ -16,7 +16,7 @@ the kohaversion is divided in 4 parts : > use strict; > > sub kohaversion { >- our $VERSION = '3.13.00.008'; >+ our $VERSION = '3.13.00.XXX'; > # version needs to be set this way > # so that it can be picked up by Makefile.PL > # during install >-- >1.7.9.5
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 10454
:
19196
|
19264
|
19265
|
20800