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
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
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>
How could we test this change?
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>
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>
Created attachment 63453 [details] [review] Bug 18434: (followup) Remove unneeded comma in fixer rules Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(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!
Should this be needs signoff, or signed off. Rather than Assigned?
(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.
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>
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>
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>
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>
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.
(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?
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" ] ] }
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)
{ 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" ] ] }
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
(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?
Created attachment 64773 [details] [review] Bug 18434 - Followup fix tests for sorting and factes
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>
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>
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>
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>
Created attachment 64790 [details] [review] Bug 18434 - Followup fix tests for sorting and factes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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.
*** Bug 18610 has been marked as a duplicate of this bug. ***
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.02
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.