Bug 15541 - Prevent normalization during matching/import process
Summary: Prevent normalization during matching/import process
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic record staging/import (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 15555
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-11 06:15 UTC by David Cook
Modified: 2017-04-02 17:42 UTC (History)
9 users (show)

See Also:
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 15541 - Prevent normalization during matching/import process (5.37 KB, patch)
2016-01-19 05:56 UTC, David Cook
Details | Diff | Splinter Review
Bug 15541 - Prevent normalization during matching/import process (5.43 KB, patch)
2016-01-20 17:03 UTC, Bouzid
Details | Diff | Splinter Review
Bug 15541: [QA Follow-up] Add new SimpleSearch argument to POD (1.69 KB, patch)
2016-04-15 09:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15541 - Prevent normalization during matching/import process (5.44 KB, patch)
2016-04-29 00:09 UTC, David Cook
Details | Diff | Splinter Review
Bug 15541 - Prevent normalization during matching/import process (6.68 KB, patch)
2016-12-01 01:49 UTC, David Cook
Details | Diff | Splinter Review
Bug 15541 - Prevent normalization during matching/import process (6.74 KB, patch)
2017-01-20 00:10 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 15541 - Prevent normalization during matching/import process (6.84 KB, patch)
2017-03-17 13:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2016-01-11 06:15:07 UTC
By default, when you're using a matching rule during an import, the data from the incoming record for a match point is normalized using the following regex:

$value =~ s/[.;:,\]\[\)\(\/'"]//g;
$value =~ s/^\s+//;
$value =~ s/\s+$//;
$value =~ s/\s+/ /g;

The first line removes all sorts of punctuation and other marks.

The second removes leading spaces.

The third removes trailing spaces.

The fourth converts multiple spaces into a single space.

--

While this might work well in many cases, it's a problem when you're trying to match using a URL.

During matching, using CHR Zebra indexing, the following occurs:
"http://libris.kb.se/resource/bib/219553" becomes "HTTPLIBRISKBSERESOURCEBIB219553"

If you're using a URL index, it's stored in Zebra as "http://libris.kb.se/resource/bib/219553", so no match is made.

If you're using a Phrase index, it's stored in Zebra as 
"http libris kb se resource bib 219553".

If you're using a Word index, it's tokenized so that each little alphanumeric piece between punctuation parts is indexed separately. 

--

The solution seems to be to either prevent normalization all together, or to "replace punctuation with spaces" in accordance with the default word-phrase-utf.chr file. I'd have to review what would need to be done for ICU indexing...

--

An additional problem is C4::Search::SimpleSearch, as it converts all colons (ie ":") into equal signs (ie "="). This is obviously a problem when you're searching for a URL as the URL is stored as "http://libris.kb.se/resource/bib/219553" but you're searching for "http=//libris.kb.se/resource/bib/219553". No match will be made.

In this case, I think we could pass an additional flag to C4::Search::SimpleSearch asking it not to normalize the query. Since it's a simple search, we are often able to make the CCL qualifier choices before passing in the query argument, so this seems reasonable.
Comment 1 David Cook 2016-01-12 05:52:06 UTC
Here's my latest findings:

Input: "http://libris.kb.se/resource/bib/219553" 

C4::Matcher::_normalize() = "HTTPLIBRISKBSERESOURCEBIB219553"
Zebra CHR = "http libris kb se resource bib 219553"
Zebra ICU = "http libriskbse resource bib 219553"

It seems to me that the smartest thing to do is NOT to normalize with C4::Matcher::_normalize(), because we're probably going to get it wrong as we have above.

Zebra indexes "http://libris.kb.se/resource/bib/219553" as "http libris kb se resource bib 219553" (CHR Phrase) or as "http libriskbse resource bib 219553" (ICU Phrase) or as "http://libris.kb.se/resource/bib/219553" (URL, which is a Charmap when using either CHR or ICU).

If we query Zebra with "http://libris.kb.se/resource/bib/219553", it will normalize the query the same way that it normalized "http://libris.kb.se/resource/bib/219553" when it was originally indexing it, and we'll get a match.

Of course, we can't necessarily stop using C4::Matcher::_normalize() as it's the default behaviour. Many people may count on that _normalize() without even knowing it... even if it's potentially working badly.

I think what I want to do is create a new normalizer which does nothing, and call it "none" or "raw". 

That way, I'm passing to Zebra the same thing that it's seen before, and it will normalize it exactly the same way and the likelihood of an accurate match increases considerably.
Comment 2 David Cook 2016-01-12 05:59:24 UTC
Of course, what I've said isn't 100% true. 

There's one more obstacle between C4::Matcher::get_matches and Zebra's normalized indexes.

And that's C4::Search::SimpleSearch(), which uses the s/:/=/g regex before sending the query. 

While that's trivial for Zebra's CHR and ICU indexing for words and phrases, since they just strip out punctuation anyway, it's a problem for Zebra's URL Charmap, which doesn't normalize URLs. 

So you'll get a failed match in that case.

So I think a pure match would have to be done without any normalization before the query gets to Zebra. That should be easy enough to implement in C4::Matcher when it uses C4::Search::SimpleSearch(). Because it's a simple search, it shouldn't be normalizing the query anyway. I'll just add a flag so that you can do an unnormalized search.
Comment 3 David Cook 2016-01-18 06:13:28 UTC
This is turning out to be harder than expected...

In C4::Matcher::get_matches, it creates a query like "qualifier,qualifier:value" for QP, or "qualifier,qualifier=value" for non-QP.

That's OK... but in C4::Search::SimpleSearch, QP gets turned off by "qualifier,qualifier" so it becomes a non-QP query... but it's created with a ":" instead of a "=".

This matters because we need a "=" instead of a ":" for the non-QP query... and we can't wholesale change ":" to "=" because of the URL syntax required by the URL register...
Comment 4 David Cook 2016-01-19 05:16:21 UTC
I've solved this one... but now I need to write up an easy test plan...
Comment 5 David Cook 2016-01-19 05:56:20 UTC Comment hidden (obsolete)
Comment 6 David Cook 2016-01-19 05:57:34 UTC
Marked this with the "Academy" keyword, although it might be a bit advanced...

More than happy to walk people through the Zebra stuff relating to this bug though.
Comment 7 Bouzid 2016-01-20 17:03:58 UTC Comment hidden (obsolete)
Comment 8 David Cook 2016-01-21 03:42:35 UTC
Thanks Bouzid!
Comment 9 Bouzid 2016-01-21 13:08:27 UTC
I test with:
File empty
File contain 1 marc record
File contain several marc record
Comment 10 Marcel de Rooy 2016-04-15 09:40:54 UTC
Created attachment 50264 [details] [review]
Bug 15541: [QA Follow-up] Add new SimpleSearch argument to POD

Also moved one line conform the order of arguments.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2016-04-15 10:08:40 UTC
QA Comment:
Thanks for your patch, David.
BTW This is imo not a trivial patch or something to signoff for an academy user, new to Koha.
[1] Although your changes in SimpleSearch are very small, it would probably help if you would add a test case in t/db_dependent/Search.t. (It seems to fail some tests currently, separate from your patch.)
[2] The source_normalizer or norms just lead me to two FIXMEs:
# FIXME normalize, substr
# FIXME - default normalizer
You introduce none and raw in the regex while this field actually does nothing (and should not have been there in this stage). Your patch starts using this field, while the only thing you want is not running _normalize for (mainly?) URLs. If we do not really solve the problem of this unused field, I think we should not touch it. BTW What would be the difference between raw and none? The regex suggests more than what we offer.
[3] I have some doubts about this new condition:
    if ($QParser && $matchpoint->{'index'} !~ m/\w,\w/) {
Somehow you managed to bump in another unfinished area here too :) 
I would rather leave the condition as it was. In the QParser branch you could choose to ignore/remove the second specifier word with a similar regex.

Changing status for now.
Comment 12 David Cook 2016-04-18 05:02:54 UTC
Thanks for your comments, Marcel! I appreciate you taking the time to look at the patch.


(In reply to Marcel de Rooy from comment #11)
> QA Comment:
> Thanks for your patch, David.
> BTW This is imo not a trivial patch or something to signoff for an academy
> user, new to Koha.

Fair enough. I think perhaps the patch grew with time and I forgot to remove the Academy keyword. 

> [1] Although your changes in SimpleSearch are very small, it would probably
> help if you would add a test case in t/db_dependent/Search.t. (It seems to
> fail some tests currently, separate from your patch.)

Hmm, also fair enough. I'll look at doing that.

> [2] The source_normalizer or norms just lead me to two FIXMEs:
> # FIXME normalize, substr
> # FIXME - default normalizer
> You introduce none and raw in the regex while this field actually does
> nothing (and should not have been there in this stage). Your patch starts
> using this field, while the only thing you want is not running _normalize
> for (mainly?) URLs. If we do not really solve the problem of this unused
> field, I think we should not touch it. BTW What would be the difference
> between raw and none? The regex suggests more than what we offer.

I'm not sure what you mean here... so I'll look at the code again. I think Kyle was having problems with normalization, so it's possible that he removed _normalize() in a different patch?

> [3] I have some doubts about this new condition:
>     if ($QParser && $matchpoint->{'index'} !~ m/\w,\w/) {
> Somehow you managed to bump in another unfinished area here too :) 
> I would rather leave the condition as it was. In the QParser branch you
> could choose to ignore/remove the second specifier word with a similar regex.
> 

Hmm, I don't think that's a good idea. You don't want to ignore/remove the second qualifier; it's important that the qualifier is in the query. You want to use it, but I don't think the QParser can do it. I think I lifted this condition from a different part of the Koha code to make the behaviour a bit more consistent. Perhaps it would be smarter to add a more universal "can_query_parse()" or something to test if the query is parseable by QParser...

> Changing status for now.
Comment 13 David Cook 2016-04-28 23:55:17 UTC
This fails to apply now anyway because of some of the ElasticSearch stuff...
Comment 14 David Cook 2016-04-28 23:59:45 UTC
Ah, easy fix fortunately...
Comment 15 David Cook 2016-04-29 00:09:22 UTC
Created attachment 50971 [details] [review]
Bug 15541 - Prevent normalization during matching/import process

This patch allows you to use the "qualifier,qualifier" syntax
in the Record Matching Rules "Search Index" when using the QueryParser.
While QueryParser doesn't support this syntax, it will now fallback
correctly to non-QueryParser functionality. Without the patch, your search
will just fail silently.

This patch also adds a "skip_normalize" flag to C4::Search::SimpleSearch(),
and sets the flag during C4::Matcher::get_matches. This prevents
the s/:/=/g and s/=/:/g normalization which happens in a heavy-handed
way to provide correct query syntax. However, get_matches() already uses
the correct syntax, so this normalization is unneeded. The normalization
also mangles URLs which causes failures when using the "url"
(ie "u") register in Zebra (see bug 15555).

This patch also creates "raw" and "none" normalizers for the
Record Matching Rules, which prevents the stripping of
spaces and punctuation by Koha prior to sending queries to Zebra.
This is important for a number of reasons. First and foremost,
Zebra does normalization better than Koha. ICU and CHR normalize
strings differently, so it's better not to try to outsmart
Zebra with pre-normalization of punctuation and spaces, as
it will lead to search problems. Second, when using the "u"
register in Zebra, you don't want to normalize the value, since
it's stored "as is" in the Zebra database. Normalization causes
search failures.

_TEST PLAN_

1) Apply patches from http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15555
2) Create a record in Koha with a 024 $a http://koha-community.org/test $2 uri
3) Do a full re-index of Zebra
4) Create a Record Matching Rule in Koha with the following details:
    Description: Test
    Match threshold: 100
    Record type: Bibliographic
    Match point 1:
        Search index: id-other,st-urx
        Score: 100
        Tag: 024
        Subfields: a
        Normalization rule: raw
5) Download your record from Koha as a .mrc file (ie isomarc, binary marc, etc)
6) Go to "Stage MARC records for import"
7) Upload your .marc file.
8) Change your "Record matching rule" to "Test"
9) Click Stage for import
10) It should say "1 records with at least one match in catalogu per matching rule "Test".
Comment 16 David Cook 2016-05-23 04:07:02 UTC
(In reply to Marcel de Rooy from comment #11)
> [1] Although your changes in SimpleSearch are very small, it would probably
> help if you would add a test case in t/db_dependent/Search.t. (It seems to
> fail some tests currently, separate from your patch.)

When I run "prove t/db_dependent/Search.t", it says that all tests are successful, although there are lots of warnings generated.

I'll review the test code to see if I can add a test...

> [2] The source_normalizer or norms just lead me to two FIXMEs:
> # FIXME normalize, substr
> # FIXME - default normalizer

These FIXME messages pre-date my patch. 

> You introduce none and raw in the regex while this field actually does
> nothing (and should not have been there in this stage). 

I don't understand this sentence.

> Your patch starts
> using this field, while the only thing you want is not running _normalize
> for (mainly?) URLs. 

URLs are certainly the main thing I want to not normalize, but honestly nothing should be normalized during the matching, as Zebra already handles normalization for search queries.

> If we do not really solve the problem of this unused
> field, I think we should not touch it. BTW What would be the difference
> between raw and none? The regex suggests more than what we offer.

There is no difference between raw and none. They're just synonyms. 

I'm open to removing _normalize() from the matching completely. Honestly, I figured that the community would be against it, so I created a way of opting out of the default _normalize(). That way default system behaviour doesn't change, while some people will know how to deactivate it.

> [3] I have some doubts about this new condition:
>     if ($QParser && $matchpoint->{'index'} !~ m/\w,\w/) {
> Somehow you managed to bump in another unfinished area here too :) 
> I would rather leave the condition as it was. In the QParser branch you
> could choose to ignore/remove the second specifier word with a similar regex.
> 

It's not really a new condition. If you look at C4::Search::SimpleSearch, you'll see that it's already there:

$QParser = C4::Context->queryparser if (C4::Context->preference('UseQueryParser') && ! ($query =~ m/\w,\w|\w=\w/));

Also, you can't remove the second specifier word, because it's essential to the query. You also can't ignore it, because the QParser can't handle it. That's why I added the same condition that's found in C4::Search::SimpleSearch. You need to catch this condition before going into the QParser code block.


--

Can the QA team indicate how to proceed? 

I'll look at adding a unit test. However, the QParser condition or one similar to it needs to stay. As for _normalize(), I'll remove it completely, if that's a change which will be accepted by the QA team and the RM.
Comment 17 David Cook 2016-05-23 04:21:44 UTC
(In reply to Marcel de Rooy from comment #11)
> [1] Although your changes in SimpleSearch are very small, it would probably
> help if you would add a test case in t/db_dependent/Search.t. (It seems to
> fail some tests currently, separate from your patch.)

I've reviewed Matcher.pm and Search.pm, an I don't think it would be possible to add a test case. The functions aren't atomic enough. The altered behaviour is internal to the functions and thus I don't see any way to automatically test it.

Changing status back to Signed Off. 

Happy to discuss further or implement changes where necessary.
Comment 18 Marcel de Rooy 2016-05-23 08:34:20 UTC
(In reply to David Cook from comment #16)
> When I run "prove t/db_dependent/Search.t", it says that all tests are
> successful, although there are lots of warnings generated.
Between April 15 and May 23 some things may have changed :)

> > [2] The source_normalizer or norms just lead me to two FIXMEs:
> > # FIXME normalize, substr
> > # FIXME - default normalizer
> These FIXME messages pre-date my patch. 
True. But that is obviously not my point. Although Matcher.pm has some code around norms and normalizer, actually it is just uncompleted functionality. The code just calls a simple _normalized routine. The "references to normalization subroutines (under C4::Normalize) to be applied to the source string" are not used and do not exist.

> I'm open to removing _normalize() from the matching completely. Honestly, I
> figured that the community would be against it, so I created a way of opting
> out of the default _normalize(). That way default system behaviour doesn't
> change, while some people will know how to deactivate it.
I am not suggesting to remove it.
As noted above, I would be hesitant to make certain changes in uncompleted functionality. 

> > [3] I have some doubts about this new condition:
> >     if ($QParser && $matchpoint->{'index'} !~ m/\w,\w/) {
> > Somehow you managed to bump in another unfinished area here too :) 
> > I would rather leave the condition as it was. In the QParser branch you
> > could choose to ignore/remove the second specifier word with a similar regex.
> It's not really a new condition. If you look at C4::Search::SimpleSearch,
> you'll see that it's already there:
Good point. Leave it there.

> Can the QA team indicate how to proceed? 
Could you (slightly) adjust the way to skip the normalize call for the URLs, that does not suggest that the norms functionality is present while we do not even have C4::Normalizer?
Maybe Jonathan wants to jump in too and add comments?
Comment 19 David Cook 2016-05-29 23:37:34 UTC
(In reply to Marcel de Rooy from comment #18)
> Could you (slightly) adjust the way to skip the normalize call for the URLs,
> that does not suggest that the norms functionality is present while we do
> not even have C4::Normalizer?

I'm not sure that I understand your question...

Regardless of whether or not you provide a value for "Normalization rule" on /cgi-bin/koha/admin/matching-rules.pl, the default _normalize() function will run.

While the matchpoint_component_norms.norm_routine isn't currently used in the code, I add some code that does use it for the sole purpose of detecting a "none" or "raw" normalization (ie no normalization).

Are you saying that you don't want me to add that code because it suggests to people that adding a value there (besides "none" or "raw") will have an effect?

I could a checkbox on the UI that says "Skip normalization rule" or something like that, but it would fulfil the same purpose of adding a "none" norm_routine in the database.

Sorry, but I'm still not sure what you're asking, Marcel. Honestly, there should probably be no normalization at this step. I would remove the "Normalization rule" from the code, but it affects too many existing systems. The patch I have here is the lightest change while also providing the desired functionality. I'm not sure what else a person could do to skip the normalization in a way that is backwards compatible.
Comment 20 Marcel de Rooy 2016-05-30 09:00:12 UTC
(In reply to David Cook from comment #19)
> While the matchpoint_component_norms.norm_routine isn't currently used in
> the code, I add some code that does use it for the sole purpose of detecting
> a "none" or "raw" normalization (ie no normalization).
> 
> Sorry, but I'm still not sure what you're asking, Marcel. Honestly, there
> should probably be no normalization at this step. I would remove the
> "Normalization rule" from the code, but it affects too many existing
> systems. The patch I have here is the lightest change while also providing
> the desired functionality. I'm not sure what else a person could do to skip
> the normalization in a way that is backwards compatible.

I understand your point hopefully :)
If we start using this field: Since it was meant to add a Normalize class, I think you should create at least a rudimentary one now.
Moving the _normalize code and add your none/raw normalizations.
Comment 21 David Cook 2016-05-31 02:55:43 UTC
(In reply to Marcel de Rooy from comment #20)
> I understand your point hopefully :)
> If we start using this field: Since it was meant to add a Normalize class, I
> think you should create at least a rudimentary one now.
> Moving the _normalize code and add your none/raw normalizations.

I'm not sure that I understand your point, unfortunately :(.

I think using normalization during matching is a bad idea, as Zebra already normalizes the query before processing it. Here's the problem:

Custom normalization: Strip hyphens
Zebra normalization: Strip colons

Data: "Mont-Royal: it's a mountain"

Zebra would index it as "Mont-Royal it's a mountain".
You'd retrieve that exact phrase with the query "Mont-Royal: it's a mountain", which would be transformed into "Mont-Royal it's a mountain" by Zebra.

However, with the custom normalization, your query would be "MontRoyal: it's a mountain" before going to Zebra, and then it would be "MontRoyal it's a mountain" when processed by Zebra.

If you're doing an exact search, your search will fail, even though that same data in the regular Koha search would work.

We need to normalize indexing/retrieval consistently, and the way to do that is with Zebra alone.

If there needs to be extra normalization, I think it should be done before the record even makes it to Koha...
Comment 22 Kyle M Hall 2016-07-15 10:18:20 UTC
This bug is marked as signed off, but the main patch has no sign-off. Is the status incorrect or did someone miss adding a sign-off?
Comment 23 Katrin Fischer 2016-08-01 21:42:47 UTC
It looks like bouzid.fergani@inlibro.com's sign-off got lost along the way. 

I am not sure how much the patch set was changed after the initial sign off - setting to Failed QA to get David's or Bouzid's attention.

Patches still apply.
Comment 24 David Cook 2016-08-01 23:11:59 UTC
To be honest, I don't know what I'm supposed to do about this patch anymore.

I don't think Marcel liked my patch, but his suggestions weren't feasible, so... I don't know. I'm not inclined to work on it, since I'm not sure what I need to do to get it to an acceptable point.

It's essentially a duplicate of bug 14238 anyway, so perhaps Kyle can write his own patch. I would be happy to add my sign off to that.
Comment 25 Kyle M Hall 2016-08-09 10:23:17 UTC
Considering you have written a patch, and I have not, I'd like to see yours get it in ;)

David, is this patch still in good condition? If so I'd be willing to test and sign-off.

(In reply to David Cook from comment #24)
> To be honest, I don't know what I'm supposed to do about this patch anymore.
> 
> I don't think Marcel liked my patch, but his suggestions weren't feasible,
> so... I don't know. I'm not inclined to work on it, since I'm not sure what
> I need to do to get it to an acceptable point.
> 
> It's essentially a duplicate of bug 14238 anyway, so perhaps Kyle can write
> his own patch. I would be happy to add my sign off to that.
Comment 26 David Cook 2016-08-24 04:02:06 UTC
(In reply to Kyle M Hall from comment #25)
> Considering you have written a patch, and I have not, I'd like to see yours
> get it in ;)
> 
> David, is this patch still in good condition? If so I'd be willing to test
> and sign-off.
> 

Sorry for the delay. Didn't see your comment until now. I'll have to review the patch, but I'll get back to you this week.
Comment 27 David Cook 2016-08-30 01:08:03 UTC
Kyle, I just reviewed the code, and it looks good to me.

Thanks for taking the time to test it :).
Comment 28 Tomás Cohen Arazi 2016-09-23 13:24:36 UTC
David, what do u think of bug 17318? Might obsolete this one too?
Comment 29 M. Tompsett 2016-09-23 15:01:40 UTC
Comment on attachment 50971 [details] [review]
Bug 15541 - Prevent normalization during matching/import process

Review of attachment 50971 [details] [review]:
-----------------------------------------------------------------

::: C4/Matcher.pm
@@ +660,4 @@
>  
>              my $searcher = Koha::SearchEngine::Search->new({index => $Koha::SearchEngine::BIBLIOS_INDEX});
>              ( $error, $searchresults, $total_hits ) =
> +              $searcher->simple_search_compat( $query, 0, $max_matches, undef, "skip_normalize" );

3 parameters turned to 5 with one set to undef? Wouldn't making this parameter a hashref mean cleaner code and easier adding/removing parameters in the future?
Comment 30 M. Tompsett 2016-09-23 16:06:57 UTC
As pointed out by tcohen on IRC, bug 17318 covers this and more. Marking this as a duplicate.

*** This bug has been marked as a duplicate of bug 17318 ***
Comment 31 David Cook 2016-10-11 06:09:17 UTC
Cheers, Mark. For some reason, I didn't get any of these emails... and just thought to check on the progress now. Definitely a duplicate. Tomas's patches should solved this.

As for your suggestion, regarding changing it to a hashref, yeah it would mean cleaner code, but it would also add more changes. I suppose you could argue that it falls into the "refactor" as you go, but I was trying to do the lightest touch.

Irrelevant in any case, since Tomas's patches obsolete these, but I'll keep that in mind for next time...
Comment 32 David Cook 2016-12-01 01:35:22 UTC
While part of this patch is a duplicate of bug 17318, there are some parts which are not duplicates. I'm re-opening this bug and will upload an updated patch for the unique changes.
Comment 33 David Cook 2016-12-01 01:49:14 UTC
Created attachment 57852 [details] [review]
Bug 15541 - Prevent normalization during matching/import process

This patch allows you to use the "qualifier,qualifier" syntax
in the Record Matching Rules "Search Index" when using the QueryParser.
While QueryParser doesn't support this syntax, it will now fallback
correctly to non-QueryParser functionality. Without the patch, your search
will just fail silently.

This patch also adds a "skip_normalize" option to C4::Search::SimpleSearch(),
and uses the option during C4::Matcher::get_matches. This prevents
the s/:/=/g and s/=/:/g normalization. This normalization is heavy-handed,
and while it is necessary sometimes to generate a valid CCL query or
QueryParser query, C4::Matcher::get_matches() already produces a valid
CCL query, so we don't need to do this normalization.

Additionally, this normalization causes problems when you use a
Zebra register which isn't normalized: namely the "u" register.
Strings are stored "as is", so http://localhost/koha/resource/1 is
stored as is during indexing. When you search, you need to pass
the exact same thing through the query to get a match. Using
http=//localhost/koha/resource/1 in your query will yield zero results.

_TEST PLAN_

0) Apply patch

1) Create a Record Matching Rule in Koha with the following details:
    Matching rule code: TEST
    Description: Test
    Match threshold: 100
    Record type: Bibliographic
    Match point 1:
        Search index: id-other,st-urx
        Score: 100
        Tag: 024
        Subfields: a
        Normalization rule: None
2) Create a record in Koha with an indexable URI
    2a) Default framework
    2b) 024 $a http://koha-community.org/test $2 uri
    2c) 040 $c test
    2d) 245 $a This is a test record
    2e) 942 $c Books
    2f) Save (save again if cautioned about missing fields as these should autofill)
3) Do a full re-index of Zebra
4) Download your record from Koha as a .mrc file (ie isomarc, binary marc, etc)
5) Go to "Stage MARC records for import"
    5a) Upload your .marc file.
    5b) Change your "Record matching rule" to "Test"
    5c) Click Stage for import
9) It should say "1 records with at least one match in catalog per matching rule "Test".

NOTE: For completeness, you can go through this process on a clean master branch,
and note that it will say '0 records with at least one match in catalog per matching rule "TEST"'
Comment 34 Alex Buckley 2017-01-20 00:10:20 UTC
Created attachment 59295 [details] [review]
Bug 15541 - Prevent normalization during matching/import process

This patch allows you to use the "qualifier,qualifier" syntax
in the Record Matching Rules "Search Index" when using the QueryParser.
While QueryParser doesn't support this syntax, it will now fallback
correctly to non-QueryParser functionality. Without the patch, your search
will just fail silently.

This patch also adds a "skip_normalize" option to C4::Search::SimpleSearch(),
and uses the option during C4::Matcher::get_matches. This prevents
the s/:/=/g and s/=/:/g normalization. This normalization is heavy-handed,
and while it is necessary sometimes to generate a valid CCL query or
QueryParser query, C4::Matcher::get_matches() already produces a valid
CCL query, so we don't need to do this normalization.

Additionally, this normalization causes problems when you use a
Zebra register which isn't normalized: namely the "u" register.
Strings are stored "as is", so http://localhost/koha/resource/1 is
stored as is during indexing. When you search, you need to pass
the exact same thing through the query to get a match. Using
http=//localhost/koha/resource/1 in your query will yield zero results.

_TEST PLAN_

0) Apply patch

1) Create a Record Matching Rule in Koha with the following details:
    Matching rule code: TEST
    Description: Test
    Match threshold: 100
    Record type: Bibliographic
    Match point 1:
        Search index: id-other,st-urx
        Score: 100
        Tag: 024
        Subfields: a
        Normalization rule: None
2) Create a record in Koha with an indexable URI
    2a) Default framework
    2b) 024 $a http://koha-community.org/test $2 uri
    2c) 040 $c test
    2d) 245 $a This is a test record
    2e) 942 $c Books
    2f) Save (save again if cautioned about missing fields as these should autofill)
3) Do a full re-index of Zebra
4) Download your record from Koha as a .mrc file (ie isomarc, binary marc, etc)
5) Go to "Stage MARC records for import"
    5a) Upload your .marc file.
    5b) Change your "Record matching rule" to "Test"
    5c) Click Stage for import
9) It should say "1 records with at least one match in catalog per matching rule "Test".

NOTE: For completeness, you can go through this process on a clean master branch,
and note that it will say '0 records with at least one match in catalog per matching rule "TEST"'

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 35 David Cook 2017-01-20 04:19:59 UTC
Cheers Alex :)
Comment 36 Marcel de Rooy 2017-03-17 13:40:46 UTC
Created attachment 61216 [details] [review]
Bug 15541 - Prevent normalization during matching/import process

This patch allows you to use the "qualifier,qualifier" syntax
in the Record Matching Rules "Search Index" when using the QueryParser.
While QueryParser doesn't support this syntax, it will now fallback
correctly to non-QueryParser functionality. Without the patch, your search
will just fail silently.

This patch also adds a "skip_normalize" option to C4::Search::SimpleSearch(),
and uses the option during C4::Matcher::get_matches. This prevents
the s/:/=/g and s/=/:/g normalization. This normalization is heavy-handed,
and while it is necessary sometimes to generate a valid CCL query or
QueryParser query, C4::Matcher::get_matches() already produces a valid
CCL query, so we don't need to do this normalization.

Additionally, this normalization causes problems when you use a
Zebra register which isn't normalized: namely the "u" register.
Strings are stored "as is", so http://localhost/koha/resource/1 is
stored as is during indexing. When you search, you need to pass
the exact same thing through the query to get a match. Using
http=//localhost/koha/resource/1 in your query will yield zero results.

_TEST PLAN_

0) Apply patch

1) Create a Record Matching Rule in Koha with the following details:
    Matching rule code: TEST
    Description: Test
    Match threshold: 100
    Record type: Bibliographic
    Match point 1:
        Search index: id-other,st-urx
        Score: 100
        Tag: 024
        Subfields: a
        Normalization rule: None
2) Create a record in Koha with an indexable URI
    2a) Default framework
    2b) 024 $a http://koha-community.org/test $2 uri
    2c) 040 $c test
    2d) 245 $a This is a test record
    2e) 942 $c Books
    2f) Save (save again if cautioned about missing fields as these should autofill)
3) Do a full re-index of Zebra
4) Download your record from Koha as a .mrc file (ie isomarc, binary marc, etc)
5) Go to "Stage MARC records for import"
    5a) Upload your .marc file.
    5b) Change your "Record matching rule" to "Test"
    5c) Click Stage for import
9) It should say "1 records with at least one match in catalog per matching rule "Test".

NOTE: For completeness, you can go through this process on a clean master branch,
and note that it will say '0 records with at least one match in catalog per matching rule "TEST"'

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2017-03-17 13:41:13 UTC
QA Comment:
Looks good to me. Finally :)

%options: Putting a hash in the parameter list may be tricky of course.
We always like to have unit tests. But unfortunately we have no tests for get_matches.. 

Passed QA
Comment 38 Kyle M Hall 2017-03-31 14:33:32 UTC
Pushed to master for 17.05, thanks David!
Comment 39 Katrin Fischer 2017-04-02 17:42:45 UTC
This won't get ported back to 16.11.x as it is an enhancement.