Bug 25342 - Scripts not running under plack can cause duplication of ES records
Summary: Scripts not running under plack can cause duplication of ES records
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-01 14:48 UTC by Nick Clemens
Modified: 2020-05-15 22:31 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06, 19.05.11


Attachments
Bug 25342: Force ES id as string (2.01 KB, patch)
2020-05-01 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25342: Force ES id as string (2.05 KB, patch)
2020-05-02 06:59 UTC, David Nind
Details | Diff | Splinter Review
Bug 25342: Unit test (1.67 KB, patch)
2020-05-07 13:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25342: Force ES id as string (2.13 KB, patch)
2020-05-11 11:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25342: Unit test (1.74 KB, patch)
2020-05-11 11:25 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 Nick Clemens 2020-05-01 14:48:43 UTC
We received reports of duplicated records after batch modifications.

It seems that when passing the ID to elasticsearch it is parsed as a string under Plack, but is parsed as a number under CGI
Comment 1 Nick Clemens 2020-05-01 14:55:09 UTC
Created attachment 104108 [details] [review]
Bug 25342: Force ES id as string

To test:
1 - Be using ES
2 - Find a unique record by searchign for title or seomthing
3 - Edit the record
4 - Repeat search and confirm you are returned to the details page
5 - Go to Tools->MARC modification templates
6 - Add a template and define an action
7 - Go to Tools->Batch record modification
8 - Select Enter a list of record numbers
9 - Enter the biblionumber of the record you searched for
10 - Use the marc modification template you setup
11 - Complete the modification
12 - Repeat your search
13 - note you get two results, both pointing to same biblionumber
14 - Check the ES index, note that you have two entries, one with the biblionumber, and the other with #.0, e.g.:
    curl -XGET 'es:9200/koha_kohadev_biblios/data/14.0?pretty'
    curl -XGET 'es:9200/koha_kohadev_biblios/data/14?pretty'
15 - Apply patch
16 - Delete and rebuild ES index
    perl misc/search_tools/rebuild_elasticsearch.pl -d -b
17 - Repeat 2-12
18 - No duplication this time
Comment 2 David Nind 2020-05-02 06:59:10 UTC
Created attachment 104175 [details] [review]
Bug 25342: Force ES id as string

To test:
1 - Be using ES
2 - Find a unique record by searchign for title or seomthing
3 - Edit the record
4 - Repeat search and confirm you are returned to the details page
5 - Go to Tools->MARC modification templates
6 - Add a template and define an action
7 - Go to Tools->Batch record modification
8 - Select Enter a list of record numbers
9 - Enter the biblionumber of the record you searched for
10 - Use the marc modification template you setup
11 - Complete the modification
12 - Repeat your search
13 - note you get two results, both pointing to same biblionumber
14 - Check the ES index, note that you have two entries, one with the biblionumber, and the other with #.0, e.g.:
    curl -XGET 'es:9200/koha_kohadev_biblios/data/14.0?pretty'
    curl -XGET 'es:9200/koha_kohadev_biblios/data/14?pretty'
15 - Apply patch
16 - Delete and rebuild ES index
    perl misc/search_tools/rebuild_elasticsearch.pl -d -b
17 - Repeat 2-12
18 - No duplication this time

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Jonathan Druart 2020-05-04 09:10:57 UTC
Nick, this sounds familiar to me, did not we already get that kind of error with ES (int that needed to be stringified).

Still no chance to get tests?
Comment 4 David Gustafsson 2020-05-04 11:52:18 UTC Comment hidden (obsolete)
Comment 5 David Gustafsson 2020-05-04 11:53:01 UTC Comment hidden (obsolete)
Comment 6 David Gustafsson 2020-05-04 12:02:28 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens 2020-05-07 13:08:32 UTC
Created attachment 104509 [details] [review]
Bug 25342: Unit test
Comment 8 Nick Clemens 2020-05-07 13:09:33 UTC
(In reply to Jonathan Druart from comment #3)
> Nick, this sounds familiar to me, did not we already get that kind of error
> with ES (int that needed to be stringified).
> 
> Still no chance to get tests?

The test is likely to pass before this patch as well, the issue is in the sending of the request somewhere, but this proves that at least stringifying it within Koha still gives what we expect
Comment 9 Jonathan Druart 2020-05-11 09:58:39 UTC
(In reply to Nick Clemens from comment #0)
> We received reports of duplicated records after batch modifications.
> 
> It seems that when passing the ID to elasticsearch it is parsed as a string
> under Plack, but is parsed as a number under CGI

Reading the bug title I undersetood command line script, but it will actually fix script called in CGI mode, right?
Comment 10 Nick Clemens 2020-05-11 10:49:09 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to Nick Clemens from comment #0)
> > We received reports of duplicated records after batch modifications.
> > 
> > It seems that when passing the ID to elasticsearch it is parsed as a string
> > under Plack, but is parsed as a number under CGI
> 
> Reading the bug title I undersetood command line script, but it will
> actually fix script called in CGI mode, right?

Yes, scripts that are excluded from plack like batch modification of bibs and imports are affected and fixed by this
Comment 11 Jonathan Druart 2020-05-11 11:25:35 UTC
Created attachment 104685 [details] [review]
Bug 25342: Force ES id as string

To test:
1 - Be using ES
2 - Find a unique record by searchign for title or seomthing
3 - Edit the record
4 - Repeat search and confirm you are returned to the details page
5 - Go to Tools->MARC modification templates
6 - Add a template and define an action
7 - Go to Tools->Batch record modification
8 - Select Enter a list of record numbers
9 - Enter the biblionumber of the record you searched for
10 - Use the marc modification template you setup
11 - Complete the modification
12 - Repeat your search
13 - note you get two results, both pointing to same biblionumber
14 - Check the ES index, note that you have two entries, one with the biblionumber, and the other with #.0, e.g.:
    curl -XGET 'es:9200/koha_kohadev_biblios/data/14.0?pretty'
    curl -XGET 'es:9200/koha_kohadev_biblios/data/14?pretty'
15 - Apply patch
16 - Delete and rebuild ES index
    perl misc/search_tools/rebuild_elasticsearch.pl -d -b
17 - Repeat 2-12
18 - No duplication this time

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2020-05-11 11:25:39 UTC
Created attachment 104686 [details] [review]
Bug 25342: Unit test

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Martin Renvoize 2020-05-12 10:25:26 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-05-13 16:16:49 UTC
Pushed to 19.11.x for 19.11.06
Comment 15 Lucas Gass 2020-05-15 22:31:17 UTC
backported to 19.05.x for 19.05.11