Before I start trying to fix bad/broken behaviour in buildQuery, I want to refactor it to preserve the current behaviour but in a form that will be easier to fix later. The goal of this bug will be to normalize how the parsed query parameters are formed into the actual Zebra query. Actually a lot more straightforward than it sounds...
Created attachment 28917 [details] [review] Bug 12443 - Initial re-factoring of buildQuery
Created attachment 28918 [details] [review] Bug 12443 - Initial re-factoring of buildQuery This patch reduces three repeated code fragments into a single internal subroutine, which is easier to read, has comments, and should make it easier to refactor more buildQuery code in the future. _TEST PLAN_ Before applying 1) Run a bunch of different searches in the staff client and OPAC in separate tabs 2) Apply the patch 3) Run the same searches again (maybe in yet more tabs) and notice that the results are exactly the same.
You should probably turn off TryQueryParser when doing testing as well. While QueryParser sometimes falls back to non-QueryParser, it's just easier if you turn it off explicitly for testing purposes.
Created attachment 28927 [details] [review] [SIGNED-OFF] Bug 12443 - Initial re-factoring of buildQuery This patch reduces three repeated code fragments into a single internal subroutine, which is easier to read, has comments, and should make it easier to refactor more buildQuery code in the future. _TEST PLAN_ Before applying 1) Run a bunch of different searches in the staff client and OPAC in separate tabs 2) Apply the patch 3) Run the same searches again (maybe in yet more tabs) and notice that the results are exactly the same. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Same results, no errors.
Created attachment 29116 [details] [review] [PASSED QA] Bug 12443 - Initial re-factoring of buildQuery This patch reduces three repeated code fragments into a single internal subroutine, which is easier to read, has comments, and should make it easier to refactor more buildQuery code in the future. _TEST PLAN_ Before applying 1) Run a bunch of different searches in the staff client and OPAC in separate tabs 2) Apply the patch 3) Run the same searches again (maybe in yet more tabs) and notice that the results are exactly the same. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Same results, no errors. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I'd like to see some unit tests for such a sensitive core functionality. I know it is an almost trivial change, but we are introducing a new function to C4::Search which needs to be documented (ut-driven documentation). It does, OTOH, look fine to me and would push it soon. If help is needed on writing those tests don't hesitate to write me.
(In reply to Tomás Cohen Arazi from comment #6) > I'd like to see some unit tests for such a sensitive core functionality. > I know it is an almost trivial change, but we are introducing a new function > to C4::Search which needs to be documented (ut-driven documentation). > > It does, OTOH, look fine to me and would push it soon. If help is needed on > writing those tests don't hesitate to write me. I was thinking about unit tests when I wrote the patch, but I'm not really sure what sort of test would be appropriate for this type of function. I can write the test, but I don't know what to test, if that makes sense.
(In reply to David Cook from comment #7) > (In reply to Tomás Cohen Arazi from comment #6) > > I'd like to see some unit tests for such a sensitive core functionality. > > I know it is an almost trivial change, but we are introducing a new function > > to C4::Search which needs to be documented (ut-driven documentation). > > > > It does, OTOH, look fine to me and would push it soon. If help is needed on > > writing those tests don't hesitate to write me. > > I was thinking about unit tests when I wrote the patch, but I'm not really > sure what sort of test would be appropriate for this type of function. > > I can write the test, but I don't know what to test, if that makes sense. You could start by looking at t/db_dependent/Search.t for stuff like this: <code> ( $error, $query, $simple_query, $query_cgi, $query_desc, $limit, $limit_cgi, $limit_desc, $stopwords_removed, $query_type ) = buildQuery([ 'and' ], [ 'salud', 'higiene' ], [], [], [], 0, 'en'); like($query, qr/kw\W.*salud\W.*and.*kw\W.*higiene/, "Built composed explicit-and CCL keyword query"); </code> And build scenarios that cover all conditional paths.
(In reply to Tomás Cohen Arazi from comment #8) > (In reply to David Cook from comment #7) > > (In reply to Tomás Cohen Arazi from comment #6) > > > I'd like to see some unit tests for such a sensitive core functionality. > > > I know it is an almost trivial change, but we are introducing a new function > > > to C4::Search which needs to be documented (ut-driven documentation). > > > > > > It does, OTOH, look fine to me and would push it soon. If help is needed on > > > writing those tests don't hesitate to write me. > > > > I was thinking about unit tests when I wrote the patch, but I'm not really > > sure what sort of test would be appropriate for this type of function. > > > > I can write the test, but I don't know what to test, if that makes sense. > > You could start by looking at t/db_dependent/Search.t for stuff like this: > > <code> > ( $error, $query, $simple_query, $query_cgi, > $query_desc, $limit, $limit_cgi, $limit_desc, > $stopwords_removed, $query_type ) = buildQuery([ 'and' ], [ 'salud', > 'higiene' ], [], [], [], 0, 'en'); > like($query, qr/kw\W.*salud\W.*and.*kw\W.*higiene/, "Built composed > explicit-and CCL keyword query"); > </code> > > And build scenarios that cover all conditional paths. Since I'm just changing the internals of buildQuery, shouldn't the existing plans cover that though?
Created attachment 29300 [details] [review] Bug 12443: (QA followup) Unit tests for _build_initial_query This followup adds uint tests for the newly introduced function. It is really basic, but covers all scenarios for the new function, and will help us prevent regressions in future patches. Regards To+ Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Patch pushed to master. Thanks David!
(In reply to Tomás Cohen Arazi from comment #11) > Patch pushed to master. > > Thanks David! Thanks, Tomás. I'm thinking of adding some "See Also" bugs for outstanding Search issues to fix in future rather than creating a new bugzilla issue as an omnibus bug. Off the top of my head, I'm planning to resolve existing issues with truncation and weighted queries which are causing all sorts of problems. I'll be sure to write unit tests for them as well. Thanks for adding these examples. They give me ideas about how to test my future code :D.
PSA on truncation with Zebra: It appears that the @truncation directive in ccl.properties doesn't work in versions before Yaz 4.2.21 (going back to an indeterminate time). "yaz-client -V" will tell you what version of Yaz you have. Debian Squeeze uses Yaz 4.0.11-1 Debian Wheezy uses Yaz 4.2.30 So...if you're on Wheezy, you're probably fine. If you're on Squeeze, your @truncation directive isn't working, which means the truncation character is the default "?" no matter what you put in ccl.properties. It does appear that you can provide multiple truncation characters based on the reference to "truncation_aliases" in the following code though: http://www.indexdata.com/yaz/dox/html/cclfind_8c_source.html --- 4.2.21 2011/11/23 Fix cql_sortby_to_sortkeys. Completely broken before. Make CCL's @truncation work again. Source: http://www.indexdata.com/yaz/doc/NEWS
More thoughts on C4::Search::buildQuery and Koha::QueryParser... C4::Search::buildQuery: PRO: Right/left/right&&left truncation, QueryFuzzy appears to work, QueryWeightFields works (except when QueryAutoTruncate is on), QueryStemming works with wordlists. CON: No support for phrases, lots of ugly spaghetti code which is mutually exclusive, incorrectly defines variables, etc. FUTURE: Planning to integrate "QueryWeightFields", "QueryFuzzy", "QueryAutoTruncate", and "QueryStemming" into _build_initial_query(). "QueryWeightFields" and "QueryFuzzy" could be improved. "QueryStemming" is a tough one...as it really is only useful for Word Lists...so we'll have to check the structure and make sure there are no quotation marks sneaking in as they'll cause problems. That should work all right. -- Koha::QueryParser: PRO: Automatically detects phrases, automatically detects right truncation. "Koha::QueryParser::Driver::PQF::query_plan::node" already uses "QueryAutoTruncate", some "QueryWeightFields"-like functionality (which needs a more configuration and possibly some code tweaks). CON: No left truncation, no left && right truncation, no easy way to designate a phrase (except by wrapping in quotes or by providing an alias) FUTURE: "Koha::QueryParser::Driver::PQF::query_plan::node" could make it easy to add "QueryStemming" and "QueryFuzzy". We could also add support for left truncation and left && right truncation using that same module. Improve relevance bumps (aka QueryWeightFields functionality) in queryparser.yaml.
The test case fails to build on Debian testing with: String found where operator expected at t/Search.t line 28, near "subtest "_build_initial_query tests"" (Do you need to predeclare subtest?) syntax error at t/Search.t line 28, near "subtest "_build_initial_query tests"" syntax error at t/Search.t line 158, near "}" Execution of t/Search.t aborted due to compilation errors.
(In reply to Robin Sheat from comment #15) > The test case fails to build on Debian testing with: > > String found where operator expected at t/Search.t line 28, near "subtest > "_build_initial_query tests"" > (Do you need to predeclare subtest?) > syntax error at t/Search.t line 28, near "subtest "_build_initial_query > tests"" > syntax error at t/Search.t line 158, near "}" > Execution of t/Search.t aborted due to compilation errors. Sounds like Debian testing uses an older version of Test::More than Tomas, me thinks. Same issue below, I imagine: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9142
(In reply to David Cook from comment #16) > (In reply to Robin Sheat from comment #15) > > The test case fails to build on Debian testing with: > > > > String found where operator expected at t/Search.t line 28, near "subtest > > "_build_initial_query tests"" > > (Do you need to predeclare subtest?) > > syntax error at t/Search.t line 28, near "subtest "_build_initial_query > > tests"" > > syntax error at t/Search.t line 158, near "}" > > Execution of t/Search.t aborted due to compilation errors. > > Sounds like Debian testing uses an older version of Test::More than Tomas, > me thinks. > > Same issue below, I imagine: > > http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9142 I don't think it is the same problem, as we have subtests in: db_dependent/Koha.t db_dependent/Biblio.t db_dependent/Items.t db_dependent/Search.t Search.t and they are not failing in Debian 6, Debian 7, Ubuntu 12.04 and Ubuntu 14.04. Can anyone try those on Debian testing please? Also, we definitely would benefit of a Debian testing node.
When I said testing, I meant squeeze. I forgot that I build it inside a chroot.
FYI, it fails in my squeeze chroot, but it passes in testing.
OK, I'm going to have to make another bug from this, it's currently blocking build of master, which isn't good, and I don't know how to fix it properly.