Bug 18014 - AddAuthority should respect AUTO_INCREMENT
Summary: AddAuthority should respect AUTO_INCREMENT
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 12734 17980 (view as bug list)
Depends on:
Blocks: 12722 17908
  Show dependency treegraph
 
Reported: 2017-01-30 13:50 UTC by Marcel de Rooy
Modified: 2017-06-14 22:18 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18014: General update of AuthoritiesMarc.t (1.37 KB, patch)
2017-01-31 08:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority (1.34 KB, patch)
2017-01-31 08:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18014: AddAuthority should respect AUTO_INCREMENT (4.92 KB, patch)
2017-01-31 08:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18014: General update of AuthoritiesMarc.t (1.41 KB, patch)
2017-01-31 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority (1.64 KB, patch)
2017-01-31 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18014: AddAuthority should respect AUTO_INCREMENT (4.92 KB, patch)
2017-01-31 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18014: General update of AuthoritiesMarc.t (1.42 KB, patch)
2017-01-31 19:21 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority (1.65 KB, patch)
2017-01-31 19:21 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18014: AddAuthority should respect AUTO_INCREMENT (4.93 KB, patch)
2017-01-31 19:21 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18014: General update of AuthoritiesMarc.t (1.48 KB, patch)
2017-02-03 10:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority (1.71 KB, patch)
2017-02-03 10:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18014: AddAuthority should respect AUTO_INCREMENT (4.98 KB, patch)
2017-02-03 10:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18014: General update of AuthoritiesMarc.t (1.49 KB, patch)
2017-02-03 12:57 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority (1.71 KB, patch)
2017-02-03 12:57 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18014: AddAuthority should respect AUTO_INCREMENT (5.00 KB, patch)
2017-02-03 12:57 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18014: QA follow-up: Use ok() instead of is() for '>' comparison (1.05 KB, patch)
2017-02-03 12:57 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-01-30 13:50:25 UTC

    
Comment 1 Marcel de Rooy 2017-01-31 08:52:07 UTC
Created attachment 59663 [details] [review]
Bug 18014: General update of AuthoritiesMarc.t

Test plan:
Run t/db_dependent/AuthoritiesMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2017-01-31 08:52:11 UTC
Created attachment 59664 [details] [review]
Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2017-01-31 08:52:15 UTC
Created attachment 59665 [details] [review]
Bug 18014: AddAuthority should respect AUTO_INCREMENT

Instead of using the MAX(authid)+1 logic, AddAuthority should just save
the record and get the new id. The authid column is an autoincrement.

This eliminates problems where a newly assigned authid also refers to a
previously deleted record. (And it will not cause problems when refining
the dontmerge functionality on report 9988.)

Note: ModAuthority also calls AddAuthority to update an existing record; in
that case we should not create a new record even if the record should not
be found any more (which should be exceptional).

This patch also simplifies handling of 001 in the authority record: in all
cases this field is updated now; no need to check its contents.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t
[2] Add a new authority record via the interface

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2017-01-31 08:56:13 UTC
*** Bug 12734 has been marked as a duplicate of this bug. ***
Comment 5 Marcel de Rooy 2017-01-31 09:05:43 UTC
Created attachment 59667 [details] [review]
Bug 18014: General update of AuthoritiesMarc.t

Modern::Perl, Koha::Database, etc.

Test plan:
Run t/db_dependent/AuthoritiesMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2017-01-31 09:05:46 UTC
Created attachment 59668 [details] [review]
Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority

Since AddAuthority uses max(id)+1 logic to produce the next authid,
authority id's will be reused when you delete the last record. This may
be a source of problems and will be addressed on the next patch.

This patch add a test to expose the problem.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-01-31 09:05:50 UTC
Created attachment 59669 [details] [review]
Bug 18014: AddAuthority should respect AUTO_INCREMENT

Instead of using the MAX(authid)+1 logic, AddAuthority should just save
the record and get the new id. The authid column is an autoincrement.

This eliminates problems where a newly assigned authid also refers to a
previously deleted record. (And it will not cause problems when refining
the dontmerge functionality on report 9988.)

Note: ModAuthority also calls AddAuthority to update an existing record; in
that case we should not create a new record even if the record should not
be found any more (which should be exceptional).

This patch also simplifies handling of 001 in the authority record: in all
cases this field is updated now; no need to check its contents.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t
[2] Add a new authority record via the interface

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 M. Tompsett 2017-01-31 18:42:02 UTC
Comment on attachment 59669 [details] [review]
Bug 18014: AddAuthority should respect AUTO_INCREMENT

Review of attachment 59669 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/AuthoritiesMarc.t
@@ +201,5 @@
>      my $id2 = AddAuthority( $record, undef, 'GEOGR_NAME' );
> +    isnt( $id1, $id2, 'Do not return the same id again' );
> +    t::lib::Mocks::mock_preference( 'marcflavour', 'UNIMARC' );
> +    my $id3 = AddAuthority( $record, undef, 'GEOGR_NAME' );
> +    is( $id3 > 0, 1, 'Tested AddAuthority with UNIMARC' );

ok($id3>0,'...') would have been better.
Comment 9 M. Tompsett 2017-01-31 18:55:46 UTC
Comment on attachment 59669 [details] [review]
Bug 18014: AddAuthority should respect AUTO_INCREMENT

Review of attachment 59669 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/AuthoritiesMarc.t
@@ -198,5 @@
>      my $record = C4::AuthoritiesMarc::GetAuthority(1);
>      my $id1 = AddAuthority( $record, undef, 'GEOGR_NAME' );
>      DelAuthority( $id1 );
>      my $id2 = AddAuthority( $record, undef, 'GEOGR_NAME' );
> -    is( $id1, $id2, 'FIXME: Got the same id back, let\'s fix that behavior' );

This should have been isnt in the first place, and the second patch should be expecting a failure. :)
Comment 10 M. Tompsett 2017-01-31 19:21:10 UTC
Created attachment 59700 [details] [review]
Bug 18014: General update of AuthoritiesMarc.t

Modern::Perl, Koha::Database, etc.

Test plan:
Run t/db_dependent/AuthoritiesMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 11 M. Tompsett 2017-01-31 19:21:18 UTC
Created attachment 59701 [details] [review]
Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority

Since AddAuthority uses max(id)+1 logic to produce the next authid,
authority id's will be reused when you delete the last record. This may
be a source of problems and will be addressed on the next patch.

This patch add a test to expose the problem.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 12 M. Tompsett 2017-01-31 19:21:24 UTC
Created attachment 59702 [details] [review]
Bug 18014: AddAuthority should respect AUTO_INCREMENT

Instead of using the MAX(authid)+1 logic, AddAuthority should just save
the record and get the new id. The authid column is an autoincrement.

This eliminates problems where a newly assigned authid also refers to a
previously deleted record. (And it will not cause problems when refining
the dontmerge functionality on report 9988.)

Note: ModAuthority also calls AddAuthority to update an existing record; in
that case we should not create a new record even if the record should not
be found any more (which should be exceptional).

This patch also simplifies handling of 001 in the authority record: in all
cases this field is updated now; no need to check its contents.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t
[2] Add a new authority record via the interface

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 13 M. Tompsett 2017-01-31 19:23:10 UTC
Didn't test just the first patch by itself, but tested 1&2 together and all three together. That's why I commented like I dd in comment #8 and comment #9. But the end product is good. :)
Comment 14 Marcel de Rooy 2017-02-01 12:03:01 UTC
(In reply to M. Tompsett from comment #13)
> Didn't test just the first patch by itself, but tested 1&2 together and all
> three together. That's why I commented like I dd in comment #8 and comment
> #9. But the end product is good. :)

Thanks for your feedback and signoff !
Comment 15 Nick Clemens 2017-02-03 10:51:41 UTC
Created attachment 59869 [details] [review]
Bug 18014: General update of AuthoritiesMarc.t

Modern::Perl, Koha::Database, etc.

Test plan:
Run t/db_dependent/AuthoritiesMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2017-02-03 10:51:46 UTC
Created attachment 59870 [details] [review]
Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority

Since AddAuthority uses max(id)+1 logic to produce the next authid,
authority id's will be reused when you delete the last record. This may
be a source of problems and will be addressed on the next patch.

This patch add a test to expose the problem.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2017-02-03 10:51:51 UTC
Created attachment 59871 [details] [review]
Bug 18014: AddAuthority should respect AUTO_INCREMENT

Instead of using the MAX(authid)+1 logic, AddAuthority should just save
the record and get the new id. The authid column is an autoincrement.

This eliminates problems where a newly assigned authid also refers to a
previously deleted record. (And it will not cause problems when refining
the dontmerge functionality on report 9988.)

Note: ModAuthority also calls AddAuthority to update an existing record; in
that case we should not create a new record even if the record should not
be found any more (which should be exceptional).

This patch also simplifies handling of 001 in the authority record: in all
cases this field is updated now; no need to check its contents.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t
[2] Add a new authority record via the interface

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Julian Maurice 2017-02-03 12:57:19 UTC
Created attachment 59880 [details] [review]
Bug 18014: General update of AuthoritiesMarc.t

Modern::Perl, Koha::Database, etc.

Test plan:
Run t/db_dependent/AuthoritiesMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 19 Julian Maurice 2017-02-03 12:57:22 UTC
Created attachment 59881 [details] [review]
Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority

Since AddAuthority uses max(id)+1 logic to produce the next authid,
authority id's will be reused when you delete the last record. This may
be a source of problems and will be addressed on the next patch.

This patch add a test to expose the problem.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 20 Julian Maurice 2017-02-03 12:57:26 UTC
Created attachment 59882 [details] [review]
Bug 18014: AddAuthority should respect AUTO_INCREMENT

Instead of using the MAX(authid)+1 logic, AddAuthority should just save
the record and get the new id. The authid column is an autoincrement.

This eliminates problems where a newly assigned authid also refers to a
previously deleted record. (And it will not cause problems when refining
the dontmerge functionality on report 9988.)

Note: ModAuthority also calls AddAuthority to update an existing record; in
that case we should not create a new record even if the record should not
be found any more (which should be exceptional).

This patch also simplifies handling of 001 in the authority record: in all
cases this field is updated now; no need to check its contents.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t
[2] Add a new authority record via the interface

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 21 Julian Maurice 2017-02-03 12:57:29 UTC
Created attachment 59883 [details] [review]
Bug 18014: QA follow-up: Use ok() instead of is() for '>' comparison

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 22 Julian Maurice 2017-02-03 13:00:53 UTC
Oops, I didn't see Nick already did QA earlier.
I will obsolete my patches (except for the followup) and de-obsolete Nick's patches.
Comment 23 Kyle M Hall 2017-02-14 14:02:01 UTC
Pushed to master for 17.05, thanks Julian, Marcel!
Comment 24 Katrin Fischer 2017-02-14 20:20:36 UTC
'Smells' a bit of possible behaviour change. Marcel, what do you think?
Not pushing this for 16.11.04, might reconsider later.
Comment 25 Marcel de Rooy 2017-02-15 12:11:48 UTC
(In reply to Katrin Fischer from comment #24)
> 'Smells' a bit of possible behaviour change. Marcel, what do you think?
> Not pushing this for 16.11.04, might reconsider later.

Fixing a bug is on itself a behavior change :) The only change is a good one: the deleted authid will no longer be reused. We may assume that no one relies on such wrong behavior. This would be imo an candidate for backporting.
Comment 26 Katrin Fischer 2017-02-15 12:38:24 UTC
Bit hard to admit, because you said noone would do that... But actually we enforce auth_id to be the ID from the union catalog - that way we can import the records "prelinked" as the number in $9 of the bibliographic record matches the auth_id of Koha. I admit, this is not a totally recommendable way of doing things... but there might be other strange things out there. Not sure if it would affect our workflow at the moment.
Comment 27 Marcel de Rooy 2017-02-15 13:14:21 UTC
(In reply to Katrin Fischer from comment #26)
> Bit hard to admit, because you said noone would do that... But actually we
> enforce auth_id to be the ID from the union catalog - that way we can import
> the records "prelinked" as the number in $9 of the bibliographic record
> matches the auth_id of Koha. I admit, this is not a totally recommendable
> way of doing things... but there might be other strange things out there.
> Not sure if it would affect our workflow at the moment.

Never say never :)
Comment 28 Marcel de Rooy 2017-02-18 14:57:14 UTC
*** Bug 17980 has been marked as a duplicate of this bug. ***