Bug 9612 - SRU Response is different when DOM indexing is enabled
Summary: SRU Response is different when DOM indexing is enabled
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Z39.50 / SRU / OpenSearch Servers (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-14 11:32 UTC by Martin Renvoize
Modified: 2014-12-07 20:07 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9612: SRU response for DOM indexing (16.58 KB, patch)
2014-03-27 09:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9612: Zconn parameters and elementSetName in _new_Zconn (4.51 KB, patch)
2014-03-27 09:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PUSHED] Bug 9612: SRU response for DOM indexing (16.62 KB, patch)
2014-04-23 16:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PUSHED] Bug 9612: Zconn parameters and elementSetName in _new_Zconn (4.55 KB, patch)
2014-04-23 16:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 9612: Follow-up for elementSetName in Context.pm (3.52 KB, patch)
2014-05-07 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[Signed-off] Bug 9612: Follow-up for elementSetName in Context.pm (3.81 KB, patch)
2014-05-07 13:50 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9612: Follow-up for elementSetName in Context.pm (4.08 KB, patch)
2014-05-07 13:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9612: Follow-up for elementSetName in Context.pm (4.30 KB, patch)
2014-05-07 14:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9612: Follow-up for elementSetName in Context.pm (4.33 KB, patch)
2014-05-09 14:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 9612: Follow-up for elementSetName in Context.pm (4.39 KB, patch)
2014-05-14 15:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 9612: Follow-up for elementSetName in Context.pm (4.43 KB, patch)
2014-05-18 21:53 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2013-02-14 11:32:15 UTC
It looks as though the SRU server is not picking up an xslt to transform zebra results into meaningful SRU responses.

I think the file to to changed is 'retrieval-info-bib-dom.xml' but I'm unsure how to proceed with changing it to mimic the output given for a grs-1 indexed catalogue.  Maybe someone more knowledgeable of zebra configuration then I could step in?

Two public SRU servers, one with DOM indexing and one with GRS-1 Indexing follow as examples of the differences.

DOM: http://tavi.koha-ptfs.eu:9998/biblios?version=1.1&operation=searchRetrieve&recordPacking=xml&query=any=medical&startRecord=1&maximumRecords=1 

GRS-1: http://bldscat.ids.ac.uk:9998/biblios?version=1.1&operation=searchRetrieve&recordPacking=xml&query=any=medical&startRecord=1&maximumRecords=1

As you can see, the GRS-1 response is much more comprehensible, and I believe is also the correct response.
Comment 1 Martin Renvoize 2013-02-14 12:24:41 UTC
OK.. So i've found that if you comment out the line:

'<retrieval syntax="xml" name="index"/> <!-- allow viewing index entries -->'

You get nice marcxml formatted responses again.. so why is this line here in the first place... I'm no zebra pro but it baffles me?
Comment 2 Jared Camins-Esakov 2013-02-14 13:11:39 UTC
(In reply to comment #1)
> OK.. So i've found that if you comment out the line:
> 
> '<retrieval syntax="xml" name="index"/> <!-- allow viewing index entries -->'
> 
> You get nice marcxml formatted responses again.. so why is this line here in
> the first place... I'm no zebra pro but it baffles me?

The idea is that it should be possible to view Zebra's indexes, but apparently we have it set up so that the indexes are displayed by default.
Comment 3 Marcel de Rooy 2014-03-20 14:30:37 UTC
I have been testing zebra/sru with dom-dom-icu again.
Worked much better now.
I plan to send a patch that includes resolving this bug next week.
Comment 4 Martin Renvoize 2014-03-25 09:05:53 UTC
I look forward to it, and will happily test.
Comment 5 Marcel de Rooy 2014-03-27 09:46:02 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2014-03-27 09:46:07 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2014-03-27 09:50:21 UTC
Have been testing this in quite some variants.
The final patches have been tested again with Makefile for both dom as well as grs1 for completeness.

In the commit message I have indicated that similar changes can be made to the debian template files. (I have not attached a patch, since I cannot test that one.) Note that the changes proposed here should imo not disrupt the package install as seen from a code perspective. I am asking Robin to confirm this.

After that the status can be set to Needs Signoff.
Comment 8 Robin Sheat 2014-03-27 21:23:19 UTC
These shouldn't break the package install, but the changes to koha-conf.xml won't be reflected there, so the bug will probably exist. It'd be good to change debian/templates/koha-conf-site.xml.in to match, and possibly also koha-create (as it looks like some of the parts for the file come from there.)
Comment 9 Marcel de Rooy 2014-03-28 08:06:04 UTC
(In reply to Robin Sheat from comment #8)
> These shouldn't break the package install, but the changes to koha-conf.xml
> won't be reflected there, so the bug will probably exist. It'd be good to
> change debian/templates/koha-conf-site.xml.in to match, and possibly also
> koha-create (as it looks like some of the parts for the file come from
> there.)

Thanks. I will open up a new report for that road. Bug 12012.
In the meantime this is ready for signoff.
Comment 10 Martin Renvoize 2014-04-23 16:48:46 UTC
Created attachment 27500 [details] [review]
[PUSHED] Bug 9612: SRU response for DOM indexing

This patch makes changes to koha-conf.xml by removing the fallback section
from biblioserver and authserver. The information is in a include file on
the same server (no need to fall back) and moreover, some information
is not up-to-date and should be moved elsewhere.

The patch also simplifies the DOM retrieval-info files for auth and bib.
And eliminates superfluous F and usmarc from the dom-config files. (I felt
the urge to remove marcxml too, but left it for now; see also the second
patch.) For reference, look at the marcxml example files of Zebra.

NOTE: This patch does not deal with the Debian package installs. In the
same way koha-conf-site.xml.in, and *-retrieval-info-* could be adjusted.

Test plan:
[1] Run at least a dev install in order to copy the new files to your
    Zebra folders. Choose for DOM indexing.
    Enable the SRU server on port 9998 (small edit in koha-conf.xml).
[2] Restart Zebra and reindex -a -b -x.
[3] Verify if a search from Koha still functions as expected.
    Check the SRU output on port 9998. NOTE: If you do not pass recordSchema,
    you should get back a marc response now (instead of index schema).
    Bonus: Add your server as a Z3950 target to another Koha install. And
    perform a Z3950 search from the other server to your new install.
    Bonus: Check response from the auth and biblio socket via yaz-client.
[4] Reindex again with -a -b but without -x.
[5] Repeat Koha search, SRU response (Z3950, yaz-client).

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2014-04-23 16:48:53 UTC
Created attachment 27501 [details] [review]
[PUSHED] Bug 9612: Zconn parameters and elementSetName in _new_Zconn

This patch makes a few adjustments in C4::Context::Zconn.
It does no longer pass the (ignored) auth, syntax parameters to _new_Zconn.
Note that auth was not completely ignored in theory, but we never pass
auth=1 [while not having user/password in koha-conf].

Furthermore, it removes the elementSetName for dom indexing. Using marcxml
here does not make a difference. It only adds a constraint on what is in
the dom-config files. (It could probably be removed there now..)

Two cosmetic code changes:
Removes unused label retry.
And moved 'servername' into the database name option.

Test plan:
When using Zebra with dom indexing, do a biblio and authority search.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2014-04-23 16:49:55 UTC
Finally got around to testing this, and it behaves as expected and also simplifies the config somewhat which I see as a good thing. Nice work M. de Rooy
Comment 13 Galen Charlton 2014-04-29 14:50:29 UTC
Marking as candidate for 3.16.0
Comment 14 Galen Charlton 2014-05-05 20:35:56 UTC
Passing QA and pushing to master.  Thanks, Marcel!
Comment 15 Owen Leonard 2014-05-06 15:33:54 UTC
All searches (staff client and OPAC) are returning bad results and git bisect blames this bug. The search seems to be finding results (it returns a count of results which looks valid), but the search results page shows no data in the table of results--no title, no author, no item information, etc.
Comment 16 Galen Charlton 2014-05-06 16:51:22 UTC
(In reply to Owen Leonard from comment #15)
> All searches (staff client and OPAC) are returning bad results and git
> bisect blames this bug. The search seems to be finding results (it returns a
> count of results which looks valid), but the search results page shows no
> data in the table of results--no title, no author, no item information, etc.

Try a 'make update_zebra_conf' and advise if that improves the situation.
Comment 17 Galen Charlton 2014-05-06 16:52:36 UTC
(and restarting zebrasrv after perl Makefile.PL && make update_zebra_conf)
Comment 18 Galen Charlton 2014-05-06 19:15:44 UTC
Testing by Owen and Bernardo indicated that they needed to do a full index rebuild before searches would work again after applying this patch.  This seems a little extreme; we need to figure out why.
Comment 19 Martin Renvoize 2014-05-07 07:29:41 UTC
I also did a rebuild when signing-off, however, I was expecting such as this patch touches the zebra configs relatively heavily. I'm not really sure how one would overcome this, but it would certainly be sensible to flag the requirement to restart and rebuild zebra during upgrade time somehow.
Comment 20 Marcel de Rooy 2014-05-07 11:30:42 UTC
(In reply to Galen Charlton from comment #18)
> Testing by Owen and Bernardo indicated that they needed to do a full index
> rebuild before searches would work again after applying this patch.  This
> seems a little extreme; we need to figure out why.

I am looking at this now. I was not aiming for a full rebuild at upgrade time..
Comment 21 Marcel de Rooy 2014-05-07 12:32:05 UTC
Can reproduce the problem with the following test in a non-package environment (manual dev install).

master back to commit 036f2a50e11dab97ef8509f2f585c4cc407ea728 [22 commits back, before 9612]
installed old zebra config, full rebuild, restart zebrasrv
reset head to 3.16.beta+  [commit 34e136a485c58c6911de59899f94a0543e483b0a]
search again without config change: same results [with config change but no restart would be fine too..]
***restarted zebrasrv: results without details (needs a rebuild)

The crux here is restarting the zebra server (as it appears, I did not restart zebrasrv every time when testing these patches..)

Restoring elementSetName=marcxml in Context.pm resolves this issue.
Patch forthcoming.
Comment 22 Marcel de Rooy 2014-05-07 12:35:02 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2014-05-07 13:02:39 UTC
Note: Will be revising the test plan little bit..
Comment 24 Marc Véron 2014-05-07 13:50:33 UTC Comment hidden (obsolete)
Comment 25 Marc Véron 2014-05-07 13:53:44 UTC
Comment on attachment 28016 [details] [review]
Bug 9612: Follow-up for elementSetName in Context.pm

I do not switch status to "Signed off" because I think it would be good having some tests with other scenarios.
Comment 26 Marcel de Rooy 2014-05-07 13:54:42 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2014-05-07 13:56:47 UTC
Thanks, Marc.
I only edited the commit message with the last patch and unintentionally removed the signoff line from Marc.
Can I put your signoff under the new version, Marc?
Comment 28 Marc Véron 2014-05-07 14:00:39 UTC
(In reply to M. de Rooy from comment #27)
> Thanks, Marc.
> I only edited the commit message with the last patch and unintentionally
> removed the signoff line from Marc.
> Can I put your signoff under the new version, Marc?

Go ahead :-)
Comment 29 Marcel de Rooy 2014-05-07 14:06:19 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2014-05-09 14:38:54 UTC Comment hidden (obsolete)
Comment 31 Martin Renvoize 2014-05-14 15:28:54 UTC Comment hidden (obsolete)
Comment 32 Martin Renvoize 2014-05-14 15:29:43 UTC
Nice to have an easy one :)

Passing QA on the follow up, I can confirm it prevents the need for a full-index rebuild.
Comment 33 Katrin Fischer 2014-05-18 21:53:07 UTC
Created attachment 28323 [details] [review]
[PASSED QA] Bug 9612: Follow-up for elementSetName in Context.pm

Restore elementSetName to marcxml for dom indexing in Zconn (Context.pm).
This prevents the need of rebuilding the index after restarting Zebra
server.
Removes the now incorrect reference to marcxml as 'superfluous' in four
dom config files.

Test plan:
[1] Do not yet apply this patch.
[2] Rebuild zebra index with the zebra config of commit
    036f2a50e11dab97ef8509f2f585c4cc407ea728.
[3] (Go back to master.) Restart your zebra server (no config change).
    You will have results without details.
    Apply this patch: you see details.
    Reset to master: no details again.
[4] Install new zebra config from master.
    Search again: you still see no details.
    Restart zebra server. Search: you see details.
    Apply this patch. Search: still details.
    Restart zebra server. Search: still details.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested in a non-package environment (manual dev install).
The package environment should work now too (results in step 4c might differ).
Progress on bug 12012 would be appropriate to sync all changes.
Tested the response of the SRU server too.

Signed-off-by: Marc Veron <veron@veron.ch>
I tested starting on a VM with Koha 3.15.00.019 installed.
Did git pull -> Koha 3.15.00.051
Result: No details in search results.
Applied patch.
Result: Search results display fine.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 34 Galen Charlton 2014-05-19 16:48:39 UTC
Pushed to master.  Thanks, Marcel!