Bug 18434 - Elasticsearch indexing broken with newer catmandu version
Summary: Elasticsearch indexing broken with newer catmandu version
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 18610 (view as bug list)
Depends on:
Blocks: 18610
  Show dependency treegraph
 
Reported: 2017-04-14 14:02 UTC by Nick Clemens
Modified: 2017-07-15 18:12 UTC (History)
7 users (show)

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


Attachments
Bug 18434 - Elasticsearch indexing broken with newer catmandu version (1.51 KB, patch)
2017-04-14 14:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18434 - Elasticsearch indexing broken with newer catmandu version (1.57 KB, patch)
2017-04-25 20:50 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18434: Add tests for K:SE:E::get_fixer_rules (4.97 KB, patch)
2017-05-12 18:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18434: Elasticsearch indexing broken with newer catmandu version (1.62 KB, patch)
2017-05-12 18:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18434: (followup) Remove unneeded comma in fixer rules (1.79 KB, patch)
2017-05-12 18:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18434: Add tests for K:SE:E::get_fixer_rules (9.42 KB, patch)
2017-05-15 13:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18434: Add tests for K:SE:E::get_fixer_rules (9.50 KB, patch)
2017-05-15 14:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18434: Elasticsearch indexing broken with newer catmandu version (1.62 KB, patch)
2017-05-15 14:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18434: (QA followup) Move _convert_marc_to_json tests into Indexer.t (4.74 KB, patch)
2017-05-15 14:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18434 - Followup - same changes for sort and facet fields (1.89 KB, patch)
2017-06-16 14:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18434 - Followup fix tests for sorting and factes (8.16 KB, patch)
2017-07-03 09:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18434: Add tests for K:SE:E::get_fixer_rules (9.50 KB, patch)
2017-07-03 19:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18434: Elasticsearch indexing broken with newer catmandu version (1.62 KB, patch)
2017-07-03 19:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18434: (QA followup) Move _convert_marc_to_json tests into Indexer.t (4.74 KB, patch)
2017-07-03 19:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18434 - Followup - same changes for sort and facet fields (1.94 KB, patch)
2017-07-03 19:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18434 - Followup fix tests for sorting and factes (8.22 KB, patch)
2017-07-03 19:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-04-14 14:02:54 UTC
Using the newly packaged catmandu it seems our mappings for multiple fields to a single index overwrite rather than append.

vfernandes actually pointed out the problem and solution so major thanks to him
Comment 1 Nick Clemens 2017-04-14 14:06:48 UTC
Created attachment 62171 [details] [review]
Bug 18434 - Elasticsearch indexing broken with newer catmandu version

To test:
1 - Make sure you have latest koha deps, catmandu versions should be:
    libcatmandu-marc-perl   1.09-1~kohadev1
    libcatmandu-perl        1.0304-2~kohadev1
2 - Reindex elastic
3 - Try searching and likely notice odd results
4 - Try:
curl -XGET
'http://localhost:9200/koha_kohadev_biblios/data/792?pretty=true'
with a known biblionumber and notice some null fields
5 - Apply patch
6 - Reindex
7 - Note fields are populated and search works as expected
Comment 2 Chris Cormack 2017-04-25 20:50:05 UTC
Created attachment 62702 [details] [review]
Bug 18434 - Elasticsearch indexing broken with newer catmandu version

To test:
1 - Make sure you have latest koha deps, catmandu versions should be:
    libcatmandu-marc-perl   1.09-1~kohadev1
    libcatmandu-perl        1.0304-2~kohadev1
2 - Reindex elastic
3 - Try searching and likely notice odd results
4 - Try:
curl -XGET
'http://localhost:9200/koha_kohadev_biblios/data/792?pretty=true'
with a known biblionumber and notice some null fields
5 - Apply patch
6 - Reindex
7 - Note fields are populated and search works as expected

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 3 Jonathan Druart 2017-04-26 18:37:02 UTC
How could we test this change?
Comment 4 Tomás Cohen Arazi 2017-05-12 18:47:05 UTC
Created attachment 63451 [details] [review]
Bug 18434: Add tests for K:SE:E::get_fixer_rules

This patch tries to introduce exhaustive tests for this class method.
I didn't try to provide a regression test for the current bug per-se, but
cover the current method behaviour as much as I could.

A minor bug is highlighted by this new tests, I'll provide a followup for it.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ de kohaclone
 k$ prove t/db_dependent/Koha_Elasticsearch.t
=> FAIL: The returned fixer rules are not the expected ones

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2017-05-12 18:47:13 UTC
Created attachment 63452 [details] [review]
Bug 18434: Elasticsearch indexing broken with newer catmandu version

To test:
1 - Make sure you have latest koha deps, catmandu versions should be:
    libcatmandu-marc-perl   1.09-1~kohadev1
    libcatmandu-perl        1.0304-2~kohadev1
2 - Reindex elastic
3 - Try searching and likely notice odd results
4 - Try:
curl -XGET
'http://localhost:9200/koha_kohadev_biblios/data/792?pretty=true'
with a known biblionumber and notice some null fields
5 - Apply patch
6 - Reindex
7 - Note fields are populated and search works as expected

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2017-05-12 18:47:19 UTC
Created attachment 63453 [details] [review]
Bug 18434: (followup) Remove unneeded comma in fixer rules

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2017-05-12 18:51:07 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> Created attachment 63453 [details] [review] [review]
> Bug 18434: (followup) Remove unneeded comma in fixer rules
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

I forgot to metion:

To test:
- Run:
  $ prove t/db_dependent/Koha_Elasticsearch.t
=> SUCCESS: All should be green!
Comment 8 Chris Cormack 2017-05-14 06:02:30 UTC
Should this be needs signoff, or signed off. Rather than Assigned?
Comment 9 Tomás Cohen Arazi 2017-05-14 12:46:06 UTC
(In reply to Chris Cormack from comment #8)
> Should this be needs signoff, or signed off. Rather than Assigned?

It should be pqa imho, but nick was going to double check the tests.
Comment 10 Nick Clemens 2017-05-15 13:41:09 UTC
Created attachment 63473 [details] [review]
Bug 18434: Add tests for K:SE:E::get_fixer_rules

This patch tries to introduce exhaustive tests for this class method.
I didn't try to provide a regression test for the current bug per-se, but
cover the current method behaviour as much as I could.

(kidclamp) I added a quick test of _convert_marc_to_json to use the mocking here
and illuminate what the change does, before the patches this should
fail (fields are indexed in place of one another), after it should succeed (new indexed fields are appended).

A minor bug is highlighted by this new tests, I'll provide a followup for it.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ de kohaclone
 k$ prove t/db_dependent/Koha_Elasticsearch.t
=> FAIL: The returned fixer rules are not the expected ones

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2017-05-15 14:33:19 UTC
Created attachment 63477 [details] [review]
Bug 18434: Add tests for K:SE:E::get_fixer_rules

This patch tries to introduce exhaustive tests for this class method.
I didn't try to provide a regression test for the current bug per-se, but
cover the current method behaviour as much as I could.

(kidclamp) I added a quick test of _convert_marc_to_json to use the mocking here
and illuminate what the change does, before the patches this should
fail (fields are indexed in place of one another), after it should succeed (new indexed fields are appended).

A minor bug is highlighted by this new tests, I'll provide a followup for it.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ de kohaclone
 k$ prove t/db_dependent/Koha_Elasticsearch.t
=> FAIL: The returned fixer rules are not the expected ones

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2017-05-15 14:33:23 UTC
Created attachment 63478 [details] [review]
Bug 18434: Elasticsearch indexing broken with newer catmandu version

To test:
1 - Make sure you have latest koha deps, catmandu versions should be:
    libcatmandu-marc-perl   1.09-1~kohadev1
    libcatmandu-perl        1.0304-2~kohadev1
2 - Reindex elastic
3 - Try searching and likely notice odd results
4 - Try:
curl -XGET
'http://localhost:9200/koha_kohadev_biblios/data/792?pretty=true'
with a known biblionumber and notice some null fields
5 - Apply patch
6 - Reindex
7 - Note fields are populated and search works as expected

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2017-05-15 14:47:25 UTC
Created attachment 63479 [details] [review]
Bug 18434: (QA followup) Move _convert_marc_to_json tests into Indexer.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Jonathan Druart 2017-05-18 19:09:05 UTC
2 Tests fail for me:

t/db_dependent/Koha_Elasticsearch_Indexer.t .. 1/6 
    #   Failed test 'First mapped author should be 100a'
    #   at t/db_dependent/Koha_Elasticsearch_Indexer.t line 120.
    #          got: 'ARRAY(0x67369d8)'
    #     expected: 'Author'

    #   Failed test 'Second mapped author should be 110a'
    #   at t/db_dependent/Koha_Elasticsearch_Indexer.t line 121.
    #          got: 'ARRAY(0x66f79c0)'
    #     expected: 'Corp Author'
    # Looks like you failed 2 tests of 2.

#   Failed test '_convert_marc_to_json() tests'
#   at t/db_dependent/Koha_Elasticsearch_Indexer.t line 124.
# Looks like you failed 1 test of 6.
Comment 15 Nick Clemens 2017-06-09 20:51:54 UTC
(In reply to Jonathan Druart from comment #14)
> 2 Tests fail for me:
> 
Can you post your versions and maybe add a dump of $conv so we can see what the structure looks like?
Comment 16 Jonathan Druart 2017-06-12 13:13:44 UTC
Package: libcatmandu-marc-perl
Version: 1.09-1~kohadev1

Package: libcatmandu-perl
Version: 1.0304-2~kohadev1

{
    author               [
        [0] [
            [0] [
                [0] "Author"
            ]
        ],
        [1] [
            [0] [
                [0] "Corp Author"
            ]
        ]
    ],
    author__facet        [
        [0] [
            [0] "Author"
        ],
        [1] [
            [0] "Corp Author"
        ]
    ],
    author__sort         [
        [0] [
            [0] "Author"
        ],
        [1] [
            [0] "Corp Author"
        ]
    ],
    author__suggestion   {
        input   [
            [0] "Author",
            [1] "Corp Author"
        ]
    },
    es_id                undef,
    record               [
        [0] [
            [0] "LDR",
            [1] undef,
            [2] undef,
            [3] "_",
            [4] "                        "
        ],
        [1] [
            [0] 001,
            [1] undef,
            [2] undef,
            [3] "_",
            [4] 1234567
        ],
        [2] [
            [0] 020,
            [1] " ",
            [2] " ",
            [3] "a",
            [4] 1234567890123
        ],
        [3] [
            [0] 100,
            [1] " ",
            [2] " ",
            [3] "a",
            [4] "Author"
        ],
        [4] [
            [0] 110,
            [1] " ",
            [2] " ",
            [3] "a",
            [4] "Corp Author"
        ],
        [5] [
            [0] 245,
            [1] " ",
            [2] " ",
            [3] "a",
            [4] "Title"
        ]
    ]
}
Comment 17 Nick Clemens 2017-06-16 14:05:46 UTC
Created attachment 64386 [details] [review]
Bug 18434 - Followup - same changes for sort and facet fields

To test:
1 - Index some stuff with multiple fields defined for sorting
  i.e. Authorites - make heading sortable - default is 110a and 111a for
  heading - a record with 111a empty will make the sort field empty
2 - view the record:
curl http://localhost:9200/koha_kohadev_authorities/data/30?pretty=true
3 - Note the blank field
4 - Apply patch
5 - Reindex
6 - Fields should be correctly populated

Unit tests to follow (once I have the originals working for all)
Comment 18 Jonathan Druart 2017-06-19 17:43:09 UTC
{
    author               [
        [0] [
            [0] [
                [0] "Author"
            ]
        ],
        [1] [
            [0] [
                [0] "Corp Author"
            ]
        ]
    ],
    author__facet        [
        [0] [
            [0] [
                [0] "Author"
            ]
        ],
        [1] [
            [0] [
                [0] "Corp Author"
            ]
        ]
    ],
    author__sort         [
        [0] [
            [0] [
                [0] "Author"
            ]
        ],
        [1] [
            [0] [
                [0] "Corp Author"
            ]
        ]
    ],
    author__suggestion   {
        input   [
            [0] "Author",
            [1] "Corp Author"
        ]
    },
    es_id                undef,
    record               [                                                                                                                                                                                 [21/108]
        [0] [
            [0] "LDR",
            [1] undef,
            [2] undef,
            [3] "_",
            [4] "                        "
        ],
        [1] [
            [0] 001,
            [1] undef,
            [2] undef,
            [3] "_",
            [4] 1234567
        ],
        [2] [
            [0] 020,
            [1] " ",
            [2] " ",
            [3] "a",
            [4] 1234567890123
        ],
        [3] [
            [0] 100,
            [1] " ",
            [2] " ",
            [3] "a",
            [4] "Author"
        ],
        [4] [
            [0] 110,
            [1] " ",
            [2] " ",
            [3] "a",
            [4] "Corp Author"
        ],
        [5] [
            [0] 245,
            [1] " ",
            [2] " ",
            [3] "a",
            [4] "Title"
        ]
    ]
}
Comment 19 Nick Clemens 2017-06-19 18:46:20 UTC
I am looking here, it seems we have some version issues - I sent an email to Mirko and going to talk about versions/packagin to see what we can do
Comment 20 Nick Clemens 2017-06-21 11:09:10 UTC
(In reply to Nick Clemens from comment #19)
> I am looking here, it seems we have some version issues - I sent an email to
> Mirko and going to talk about versions/packagin to see what we can do

I tested on a new kohadevbox - pulling Mirko's packaged versions everything seems to work for me now:

sudo apt-get install koha-elasticsearch 


||/ Name                                          Version                     Architecture                Description
+++-=============================================-===========================-===========================-===============================================================================================
ii  libcatmandu-marc-perl                         1.09-1~kohadev1             all                         Catmandu modules for working with MARC data
ii  libcatmandu-perl                              1.0304-2~kohadev1           all                         metadata toolkit
ii  libcatmandu-store-elasticsearch-perl          0.0507-1~kohadev1           all                         searchable store backed by Elasticsearch

It must be another module somewhere causing the extra level in your conversion - can you test with a new clean devbox and see if the error persists?
Comment 21 Nick Clemens 2017-07-03 09:43:31 UTC
Created attachment 64773 [details] [review]
Bug 18434 - Followup fix tests for sorting and factes
Comment 22 Tomás Cohen Arazi 2017-07-03 19:07:20 UTC
Created attachment 64786 [details] [review]
Bug 18434: Add tests for K:SE:E::get_fixer_rules

This patch tries to introduce exhaustive tests for this class method.
I didn't try to provide a regression test for the current bug per-se, but
cover the current method behaviour as much as I could.

(kidclamp) I added a quick test of _convert_marc_to_json to use the mocking here
and illuminate what the change does, before the patches this should
fail (fields are indexed in place of one another), after it should succeed (new indexed fields are appended).

A minor bug is highlighted by this new tests, I'll provide a followup for it.

To test:
- Run:
  $ sudo koha-shell kohadev
 k$ de kohaclone
 k$ prove t/db_dependent/Koha_Elasticsearch.t
=> FAIL: The returned fixer rules are not the expected ones

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2017-07-03 19:07:27 UTC
Created attachment 64787 [details] [review]
Bug 18434: Elasticsearch indexing broken with newer catmandu version

To test:
1 - Make sure you have latest koha deps, catmandu versions should be:
    libcatmandu-marc-perl   1.09-1~kohadev1
    libcatmandu-perl        1.0304-2~kohadev1
2 - Reindex elastic
3 - Try searching and likely notice odd results
4 - Try:
curl -XGET
'http://localhost:9200/koha_kohadev_biblios/data/792?pretty=true'
with a known biblionumber and notice some null fields
5 - Apply patch
6 - Reindex
7 - Note fields are populated and search works as expected

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2017-07-03 19:07:33 UTC
Created attachment 64788 [details] [review]
Bug 18434: (QA followup) Move _convert_marc_to_json tests into Indexer.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2017-07-03 19:07:39 UTC
Created attachment 64789 [details] [review]
Bug 18434 - Followup - same changes for sort and facet fields

To test:
1 - Index some stuff with multiple fields defined for sorting
  i.e. Authorites - make heading sortable - default is 110a and 111a for
  heading - a record with 111a empty will make the sort field empty
2 - view the record:
curl http://localhost:9200/koha_kohadev_authorities/data/30?pretty=true
3 - Note the blank field
4 - Apply patch
5 - Reindex
6 - Fields should be correctly populated

Unit tests to follow (once I have the originals working for all)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2017-07-03 19:07:45 UTC
Created attachment 64790 [details] [review]
Bug 18434 - Followup fix tests for sorting and factes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2017-07-03 19:08:48 UTC
The last two patches for this one overlap with my patches for bug 18610, this ones are
ok and i'm closing the other as duplicate.
Comment 28 Tomás Cohen Arazi 2017-07-03 19:09:14 UTC
*** Bug 18610 has been marked as a duplicate of this bug. ***
Comment 29 Jonathan Druart 2017-07-06 18:49:32 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 30 Fridolin SOMERS 2017-07-12 07:14:47 UTC
Pushed to 17.05.x, will be in 17.05.02
Comment 31 Katrin Fischer 2017-07-15 18:12:40 UTC
Doesn't apply cleanly, possibly because of other Elasticsearch related patches missing in 16.11.x. Please get in touch with me if you want to have this included.