Bug 18624

Summary: Software error when searching authorities in Elasticsearch - incorrect parameter "any" should be "all"
Product: Koha Reporter: david holoshka <david.holoshka>
Component: SearchingAssignee: Nick Clemens <nick>
Status: Pushed to Stable --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, katrin.fischer, m.de.rooy, mtj, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
Whiteboard:
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 18624 - Run time errors when searching authorities with Elastic search 5.3
Bug 18624: Minor refactoring to ease testing
Bug 18624: Regression test for 'any' vs 'all'
Bug 18624: Run time errors when searching authorities with Elastic search 5.3
Bug 18624: (followup) opac-authorities-home.tt uses 'any'
Bug 18624: Minor refactoring to ease testing
Bug 18624: Regression test for 'any' vs 'all'
Bug 18624: Run time errors when searching authorities with Elastic search 5.3
Bug 18624: (followup) opac-authorities-home.tt uses 'any'
Bug 18624: Do not return 1 in tests

Description david holoshka 2017-05-18 07:40:44 UTC
Searching for a known authority entry (eg. Bear or Meyer) results in either the following runtime errors or no result (sort none).  Sort on finds nothing

Header search only $a:
Software error:

Invalid marclist field provided: all at /usr/local/koha/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm line 433.
	Koha::SearchEngine::Elasticsearch::QueryBuilder::build_authorities_query_compat(Koha::SearchEngine::Elasticsearch::QueryBuilder=HASH(0x90e2110),
 ARRAY(0x90d4630), ARRAY(0x90d4660), ARRAY(0x914bf30), ARRAY(0x912f6b0), ARRAY(0x912f0f8), "", "") called at /usr/local/koha/authorities/authorities-home.pl line 93


Search Main Heading :

Software error:

Can't use string ("9932563") as an ARRAY ref while "strict refs" in use at /usr/local/koha/Koha/SearchEngine/Elasticsearch/Search.pm line 202.


auth index entry in elastic search:

 {
        "_index" : "koha_ulibrary_authorities",
        "_type" : "data",
        "_id" : "1756",
        "_score" : 1.0,
        "_source" : {
          "Personal-name-see-also-from" : null,
          "Meeting-name-heading" : null,
          "Personal-name" : [
            "Bear, Roberta Meyer"
          ],
          "Corporate-name-see-also-from" : null,
          "Heading-use-series-added-entry" : " ",
          "es_id" : "1756",
          "See-also-from" : null,
          "Meeting-name-see-also-from" : null,
          "LC-card-number" : null,
          "record" : [
            [
              "LDR",
              null,
              null,
              "_",
              "00262nz  a2200121o  4500"
            ],
            [
              "001",
              null,
              null,
              "_",
              "11072"
            ],
            [
              "003",
              null,
              null,
              "_",
              "LDU"
            ],
            [
              "005",
              null,
              null,
              "_",
              "20170405133903.0"
            ],
            [
              "008",
              null,
              null,
              "_",
              "170405930116   a    aabn           a aaa"
            ],
            [
              "040",
              " ",
              " ",
              "a",
              "L",
              "c",
              "L"
            ],
            [
              "100",
              "1",
              " ",
              "a",
              "Bear, Roberta Meyer"
            ],
            [
              "400",
              "1",
              " ",
              "a",
              "Meyer Bear, Roberta"
            ],
            [
              "942",
              " ",
              " ",
              "a",
              "PERSO_NAME"
            ],
            [
              "999",
              " ",
              " ",
              "c",
              "1756",
              "d",
              "1756"
            ]
          ],
          "Match-heading-see-from" : null,
          "record-source" : [
            "L",
            "L"
          ],
          "Heading-use-main-or-added-entry" : " ",
          "Meeting-name-see-from" : null,
          "Personal-name-heading" : [
            "Bear, Roberta Meyer"
          ],
          "Heading-use-subject-added-entry" : "a",
          "Personal-name-see-from" : [
            "Meyer Bear, Roberta"
          ],
          "Local-number" : [
            "11072"
          ],
          "Corporate-name-see-from" : null,
          "Match" : null,
          "authtype" : [
            "PERSO_NAME"
          ],
          "Kind-of-record" : "1",
          "Match-heading" : [
            "Bear, Roberta Meyer"
          ],
          "Descriptive-cataloging-rules" : "1",
          "See-from" : null,
          "Heading" : [
            "Bear, Roberta Meyer"
          ],
          "Heading-Main" : null,
          "Subject-heading-thesaurus" : "6",
          "Meeting-name" : null
        }
      },

We are using the default field mapping.
Koha running on Debian, I have applied a patch not yet on main (Bug 18434 - Elasticsearch indexing broken with newer catmandu version ) to make the biblio search work. 
Mysql

ES 5.3  (it seems to be working fine) on oracle java 8

Marc 21

All perl catmandu modules are the latest from cpanm.
Comment 1 Nick Clemens 2017-06-16 13:56:46 UTC
Created attachment 64385 [details] [review]
Bug 18624 - Run time errors when searching authorities with Elastic search 5.3

It seems we have a syntax mismatch - any vs all

all seems to the be list we expect so updating code that way

To test:
1 - Enabled Elasticsearch
2 - Index some authorities
3 - Perform a 'Search entire record' search
4 - Internal server error (
        Invalid marclist field provided: all at
        /usr/local/koha/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
        line 433.
        )
5 - Apply patch
6 - Search should work
Comment 2 Nick Clemens 2017-06-16 14:00:22 UTC
This will take care of the first error at least.

I can't recreate the "Can't use string ("9932563") as an ARRAY ref" - can you file that as a new bug with specific instructions for that issue?

There are additional things you mention here, they should probably each go on a separate bug as well (no result, sort error, etc) it is easier to fix things one at a time :-)

I am also adding a followup to 18434 as wee need to append entries for facets and sorting as well - i will be looking at fixing the tests / diagnosing why jonathan has differing results asap.
Comment 3 Jonathan Druart 2017-07-06 15:55:18 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2017-07-06 15:55:45 UTC
Nick, please provide tests.
Comment 5 Tomás Cohen Arazi 2017-08-07 19:24:50 UTC
Created attachment 65591 [details] [review]
Bug 18624: Minor refactoring to ease testing

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2017-08-07 19:24:58 UTC
Created attachment 65592 [details] [review]
Bug 18624: Regression test for 'any' vs 'all'

This test makes it explicit that the only string producing _all as index on build_authorities_query_compat is 'all'.
To test:
- Apply this patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
=> FAIL: Test fails because the list of valid values is wrong in Koha.

Note: this list has to be in sync with the templates passing the same values. A followup will be added
to fix a discrepancy found between OPAC and Intranet.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2017-08-07 19:25:09 UTC
Created attachment 65593 [details] [review]
Bug 18624: Run time errors when searching authorities with Elastic search 5.3

It seems we have a syntax mismatch - any vs all

all seems to the be list we expect so updating code that way

To test:
1 - Enabled Elasticsearch
2 - Index some authorities
3 - Perform a 'Search entire record' search
4 - Internal server error (
        Invalid marclist field provided: all at
        /usr/local/koha/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
        line 433.
        )
5 - Run:
    $ sudo koha-shell kohadev
   k$ cd kohaclone
   k$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
=> FAIL: Tests fail because 'any is used'
6 - Apply patch
7 - Search should work
8 - Run:
   k$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
=> SUCCESS: Tests pass!
9 Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Edited the test plan so it mentions the new tests
Comment 8 Tomás Cohen Arazi 2017-08-07 19:25:16 UTC
Created attachment 65594 [details] [review]
Bug 18624: (followup) opac-authorities-home.tt uses 'any'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Marcel de Rooy 2017-08-11 09:13:25 UTC
Created attachment 65882 [details] [review]
Bug 18624: Minor refactoring to ease testing

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2017-08-11 09:13:30 UTC
Created attachment 65883 [details] [review]
Bug 18624: Regression test for 'any' vs 'all'

This test makes it explicit that the only string producing _all as index on build_authorities_query_compat is 'all'.
To test:
- Apply this patch
- Run:
  $ sudo koha-shell kohadev
 k$ cd kohaclone
 k$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
=> FAIL: Test fails because the list of valid values is wrong in Koha.

Note: this list has to be in sync with the templates passing the same values. A followup will be added
to fix a discrepancy found between OPAC and Intranet.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2017-08-11 09:13:34 UTC
Created attachment 65884 [details] [review]
Bug 18624: Run time errors when searching authorities with Elastic search 5.3

It seems we have a syntax mismatch - any vs all

all seems to the be list we expect so updating code that way

To test:
1 - Enabled Elasticsearch
2 - Index some authorities
3 - Perform a 'Search entire record' search
4 - Internal server error (
        Invalid marclist field provided: all at
        /usr/local/koha/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
        line 433.
        )
5 - Run:
    $ sudo koha-shell kohadev
   k$ cd kohaclone
   k$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
=> FAIL: Tests fail because 'any is used'
6 - Apply patch
7 - Search should work
8 - Run:
   k$ prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t
=> SUCCESS: Tests pass!
9 Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Edited the test plan so it mentions the new tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2017-08-11 09:13:39 UTC
Created attachment 65885 [details] [review]
Bug 18624: (followup) opac-authorities-home.tt uses 'any'

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2017-08-11 09:15:43 UTC
QA Comment:
Trusting on Tomas for the ElasticSearch test. Code looks good to me. A unit test is added; it passes after the third patch.
The Zebra side is fine too, also after the template change in the fourth patch, since AuthoritiesMarc does not care about all or any:
                else {    # Assume any if no index was specified
                    $attr = " \@attr 1=Any ";

Passed QA
Comment 14 Marcel de Rooy 2017-08-11 09:17:02 UTC
Side note: We are still waiting for the removal of the useless call of SearchAuthorities in QueryBuilder on bug 16976 in PNA
Comment 15 Jonathan Druart 2017-08-15 15:17:18 UTC
Created attachment 66020 [details] [review]
Bug 18624: Do not return 1 in tests
Comment 16 Jonathan Druart 2017-08-15 15:27:16 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 17 Fridolin SOMERS 2017-09-06 13:22:43 UTC
Pushed to 17.05.x, will be in 17.05.04
Comment 18 Katrin Fischer 2017-09-10 08:39:36 UTC
These patches have been pushed to 16.11.x and will be in 16.11.12.

Please note: Not all Elasticsearch related patches have been pushed to 16.11.x as there were some problems/conflicts and I received no answer on the bugs. 
This applied cleanly, so I have pushed it.
Comment 19 Katrin Fischer 2017-09-10 08:57:34 UTC
Sorry, reverted - tests were failing.

 perl t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t 

    not ok 9 - Exception thrown on invalid value in the marclist param
    #   Failed test 'Exception thrown on invalid value in the marclist param'
    #   at t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t line 53.
    # expecting: Koha::Exceptions::WrongParameter
    # found: Can't locate object method "throw" via package "Koha::Exceptions::WrongParameter" (perhaps you forgot to load "Koha::Exceptions::WrongParameter"?) at /home/vagrant/kohaclone/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm line 432.
    1..9
    # Looks like you failed 1 test of 9.
not ok 1 - build_authorities_query_compat() tests
#   Failed test 'build_authorities_query_compat() tests'
#   at t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t line 55.
# Looks like you failed 1 test of 1.
Comment 20 Mason James 2017-09-20 01:59:40 UTC
(In reply to Katrin Fischer from comment #19)
> Sorry, reverted - tests were failing.

Skipping for 16.05.x, due to failing tests