Bug 7722 - Insidious problem with searching
Summary: Insidious problem with searching
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: 3.6
Hardware: All All
: P5 - low normal (vote)
Assignee: Galen Charlton
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-15 20:26 UTC by Kyle M Hall
Modified: 2013-12-05 19:59 UTC (History)
3 users (show)

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


Attachments
Bug 7722 - Insidious problem with searching (1.78 KB, patch)
2012-03-15 20:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 7722 - Insidious problem with searching (1.84 KB, patch)
2012-05-14 15:15 UTC, Ian Walls
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-03-15 20:26:54 UTC
I cannot find the root cause of this issue, but multiple libraries that I am aware of have problems searching on particular search terms ( and never the same terms at the same library ). The error they get when they trigger this problem is:

Tag "" is not a valid tag. at /home/koha/kohaclone/C4/Biblio.pm line 1849

Something somewhere is adding empty keys to C4::Context->marcfromkohafield, I think it may have something to do with the analytics feature that was added.


In the while loop for TransformKohaToMarc, there is a line 

next unless my $dtm = $db_to_marc->{''}->{$name};

I don't think it's working.
If I dump $dtm, for each search, I see the dump twice.
It looks like this:
$VAR1 = [
           '952',
           'w'
         ];
 $VAR1 = [];
I think the second time, when it is empty is what's breaking this.
The next never fails because even though it is empty, it is still a valid arrayref.

The solution I have some up with is to skip over the elements where the arrayref is empty.
Comment 1 Kyle M Hall 2012-03-15 20:27:47 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-05-13 11:24:33 UTC
Do you have any suggestions how to test this patch?
Comment 3 Kyle M Hall 2012-05-14 13:16:34 UTC
(In reply to comment #2)
> Do you have any suggestions how to test this patch?
 
Unfortunately not, I was never able to find the root cause of the issue. I believe the issue is probably being caused by the analytics feature, but I was never able to pinpoint the exact code that was causing this problem. 

The error has showed up on multiple Koha installations I have worked on.

I can say the patch does indeed fix it, and does no harm. It is just another check to ensure data correctness. The original check for an arrayref is simply insufficient, as an arrayref can be valid, but have no data.
Comment 4 Ian Walls 2012-05-14 15:15:28 UTC
Created attachment 9566 [details] [review]
[PASSED QA] Bug 7722 - Insidious problem with searching

I cannot find the root cause of this issue, but multiple libraries that I am aware of have problems searching on particular search terms ( and never the same terms at the same library ). The error they get when they trigger this problem is:

Tag "" is not a valid tag. at /home/koha/kohaclone/C4/Biblio.pm line 1849

Something somewhere is adding empty keys to C4::Context->marcfromkohafield, I think it may have something to do with the analytics feature that was added.

In the while loop for TransformKohaToMarc, there is a line

next unless my $dtm = $db_to_marc->{''}->{$name};

I don't think it's working.
If I dump $dtm, for each search, I see the dump twice.
It looks like this:
$VAR1 = [
           '952',
           'w'
         ];
 $VAR1 = [];
I think the second time, when it is empty is what's breaking this.
The next never fails because even though it is empty, it is still a valid arrayref.

The solution I have some up with is to skip over the elements where the arrayref is empty.

Signed-off-by: Ian Walls <koha.sekjal@gmail.com>
Comment 5 Ian Walls 2012-05-14 15:16:12 UTC
This is a consequence of bug 6990, which had a long thread and a proposed solution similar to what you're proposing, Kyle, but no actual follow-up patch written.

Testing is... tricky.  TransformKohaToMarc isn't used very often.... it's a called as part of GetMarcItem, and in acqui/addorder.pl.  Testing would involve editing items, and adding orders.  

That said, I think Kyle's approach is sound.  We're only adding an additional line that will 'next' us if the count of elements returned in $dtm is zero.  Adding that check is indeed safe, as there should be never be a circumstance where NULL $tag or $letter should be okay.

I'm going to sign off on this and pass it through QA, as I'm quite confident this is a safe and logical addition to the code.
Comment 6 Paul Poulain 2012-05-15 14:09:01 UTC
(In reply to comment #5)
> I'm going to sign off on this and pass it through QA, as I'm quite confident
> this is a safe and logical addition to the code.

Agreed, (even if I think it would be better to fix the origin of the empty dtm)
Comment 7 Chris Cormack 2012-05-16 02:42:46 UTC
Bug fix, no string changes, will be in 3.8.1
Comment 8 Jared Camins-Esakov 2012-06-14 11:54:50 UTC
This patch will be in 3.6.6.