Bug 28290

Summary: Record matching rules with no subfields cause ISE
Product: Koha Reporter: Nick Clemens <nick>
Component: ToolsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: normal    
Priority: P5 - low CC: arthur.suzuki, fridolin.somers, george, lucas, martin.renvoize, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.07, 21.11.14
Attachments: Bug 28290: Don't send subfields to 'as_string' if none to send
Test record
Bug 28290: Don't send subfields to 'as_string' if none to send
Bug 28290: Don't send subfields to 'as_string' if none to send
Bug 28290: (follow-up) Remove debug code
Bug 28290: Don't send subfields to 'as_string' if none to send
Bug 28290: (follow-up) Unit test
Bug 28290: Don't send subfields to 'as_string' if none to send
Bug 28290: (follow-up) Unit test

Description Nick Clemens 2021-05-04 18:28:31 UTC
To recreate:
1 - Define a new matching rule with no subfields
    Match-threshold: 1000
    Record type: bibliographic record
    Search-index: Other-control-number
    Score: 1000
    Tag: 035
    Subfields:
    Offset:
    Length:
2 - Attempt to stage a record with an 035 using this rule (i will attach example to bug)
3 - Staging fails
4 - Stage again with no matching rule
5 - Manage the staged record, try to apply the matching rule
6 - It fails, 500 error
Comment 1 Nick Clemens 2021-05-04 18:31:49 UTC
Created attachment 120474 [details] [review]
Bug 28290: Don't send subfields to 'as_string' if none to send

This fixes an error where we send a blank string to Marc::Field->as_string

The subroutine fails as the regex is marked as incomplete as there is nothing between the brackets

Also fixes a missing hidden input for 'item_action' when no items are staged during import

 1 - Define a new matching rule with no subfields
    Match-threshold: 1000
    Record type: bibliographic record
    Search-index: Other-control-number
    Score: 1000
    Tag: 035
    Subfields:
    Offset:
    Length:
 2 - Attempt to stage a record with an 035 using this rule (i will attach example to bug)
 3 - Staging fails
 4 - Stage again with no matching rule
 5 - Manage the staged record, try to apply the matching rule
 6 - It fails, 500 error
 7 - Apply patch
 8 - Restart all the things
 9 - Apply rule again, it works!
10 - Stage the record again using the rule for matching
11 - It stages and matches!
Comment 2 Nick Clemens 2021-05-04 18:32:18 UTC
Created attachment 120475 [details]
Test record
Comment 3 Owen Leonard 2021-05-07 12:05:21 UTC
Created attachment 120686 [details] [review]
Bug 28290: Don't send subfields to 'as_string' if none to send

This fixes an error where we send a blank string to
Marc::Field->as_string

The subroutine fails as the regex is marked as incomplete as there is
nothing between the brackets

Also fixes a missing hidden input for 'item_action' when no items are
staged during import

 1 - Define a new matching rule with no subfields
    Match-threshold: 1000
    Record type: bibliographic record
    Search-index: Other-control-number
    Score: 1000
    Tag: 035
    Subfields:
    Offset:
    Length:
 2 - Attempt to stage a record with an 035 using this rule (i will
     attach example to bug)
 3 - Staging fails
 4 - Stage again with no matching rule
 5 - Manage the staged record, try to apply the matching rule
 6 - It fails, 500 error
 7 - Apply patch
 8 - Restart all the things
 9 - Apply rule again, it works!
10 - Stage the record again using the rule for matching
11 - It stages and matches!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 4 Katrin Fischer 2021-05-10 21:09:51 UTC
QA script explodes a bit - missing a dependency maybe? Can you please verify/check?

 FAIL	C4/Matcher.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   FAIL	  pod coverage
		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
   OK	  spelling
   FAIL	  valid
		Can't locate Carp/Always.pm in @INC (you may need to install the Carp::Always module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/qa-test-tools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/kohadev/plugins)
Comment 5 Nick Clemens 2021-05-11 10:06:38 UTC
Created attachment 120827 [details] [review]
Bug 28290: Don't send subfields to 'as_string' if none to send

This fixes an error where we send a blank string to
Marc::Field->as_string

The subroutine fails as the regex is marked as incomplete as there is
nothing between the brackets

Also fixes a missing hidden input for 'item_action' when no items are
staged during import

 1 - Define a new matching rule with no subfields
    Match-threshold: 1000
    Record type: bibliographic record
    Search-index: Other-control-number
    Score: 1000
    Tag: 035
    Subfields:
    Offset:
    Length:
 2 - Attempt to stage a record with an 035 using this rule (i will
     attach example to bug)
 3 - Staging fails
 4 - Stage again with no matching rule
 5 - Manage the staged record, try to apply the matching rule
 6 - It fails, 500 error
 7 - Apply patch
 8 - Restart all the things
 9 - Apply rule again, it works!
10 - Stage the record again using the rule for matching
11 - It stages and matches!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Nick Clemens 2021-05-11 10:06:41 UTC
Created attachment 120828 [details] [review]
Bug 28290: (follow-up) Remove debug code
Comment 7 Martin Renvoize 2021-05-17 13:31:23 UTC
Looks good.. but, it looks like C4::Matcher is fairly well tested.. could you add a test for this change too?
Comment 8 Fridolin Somers 2022-08-31 18:26:14 UTC
Still valid ?
Comment 9 Nick Clemens 2022-09-02 06:40:23 UTC
Created attachment 140066 [details] [review]
Bug 28290: Don't send subfields to 'as_string' if none to send

This fixes an error where we send a blank string to
Marc::Field->as_string

The subroutine fails as the regex is marked as incomplete as there is
nothing between the brackets

Also fixes a missing hidden input for 'item_action' when no items are
staged during import

 1 - Define a new matching rule with no subfields
    Match-threshold: 1000
    Record type: bibliographic record
    Search-index: Other-control-number
    Score: 1000
    Tag: 035
    Subfields:
    Offset:
    Length:
 2 - Attempt to stage a record with an 035 using this rule (i will
     attach example to bug)
 3 - Staging fails
 4 - Stage again with no matching rule
 5 - Manage the staged record, try to apply the matching rule
 6 - It fails, 500 error
 7 - Apply patch
 8 - Restart all the things
 9 - Apply rule again, it works!
10 - Stage the record again using the rule for matching
11 - It stages and matches!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Nick Clemens 2022-09-02 06:40:28 UTC
Created attachment 140067 [details] [review]
Bug 28290: (follow-up) Unit test
Comment 11 Katrin Fischer 2022-09-07 09:10:59 UTC
Created attachment 140263 [details] [review]
Bug 28290: Don't send subfields to 'as_string' if none to send

This fixes an error where we send a blank string to
Marc::Field->as_string

The subroutine fails as the regex is marked as incomplete as there is
nothing between the brackets

Also fixes a missing hidden input for 'item_action' when no items are
staged during import

 1 - Define a new matching rule with no subfields
    Match-threshold: 1000
    Record type: bibliographic record
    Search-index: Other-control-number
    Score: 1000
    Tag: 035
    Subfields:
    Offset:
    Length:
 2 - Attempt to stage a record with an 035 using this rule (i will
     attach example to bug)
 3 - Staging fails
 4 - Stage again with no matching rule
 5 - Manage the staged record, try to apply the matching rule
 6 - It fails, 500 error
 7 - Apply patch
 8 - Restart all the things
 9 - Apply rule again, it works!
10 - Stage the record again using the rule for matching
11 - It stages and matches!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2022-09-07 09:11:03 UTC
Created attachment 140264 [details] [review]
Bug 28290: (follow-up) Unit test

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Tomás Cohen Arazi 2022-09-07 20:57:59 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 14 Lucas Gass 2022-10-31 20:02:58 UTC
Backported to 22.05.x for upcoming 22.05.07 release
Comment 15 Arthur Suzuki 2022-11-14 13:33:35 UTC
applied to 21.11.x for 21.11.14
Comment 16 Victor Grousset/tuxayo 2022-11-20 00:22:06 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.