Bug 35989

Summary: Searching Geographic authorities generates error
Product: Koha Reporter: Robert Cooper <rcooper>
Component: SearchingAssignee: Matt Blenkinsop <matt.blenkinsop>
Status: Pushed to oldstable --- QA Contact: Aleisha Amohia <aleisha>
Severity: major    
Priority: P2 CC: aleisha, david, dcook, domm, fridolin.somers, hagud, lucas, nick, tsycko, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36413
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
This fixes an error generated when searching geographic name authorities ("Error: Unmatched [ in regex; marked by ...".). The error was generated if an authority record had a heading in the subfields for 751 (Established Heading Linking Entry-Geographic Name) and 781 (Subdivision Linking Entry-Geographic Subdivision).
Version(s) released in:
24.11.00,24.05.02,23.11.07
Circulation function:
Bug Depends on:    
Bug Blocks: 36985    
Attachments: Bug 35989: Stop geographic authority searches crashing
Bug 35989: Stop geographic authority searches crashing
Bug 35989: Stop geographic authority searches crashing
Bug 35989: (QA follow-up) Add test and limit variable scope
Bug 35989: (QA follow-up): Fix QA tests
Bug 35989: Stop geographic authority searches crashing
Bug 35989: (QA follow-up) Add test and limit variable scope
Bug 35989: (QA follow-up): Fix QA tests

Description Robert Cooper 2024-02-05 06:09:32 UTC
1) Search Authorities from the main view (/cgi-bin/koha/authorities/authorities-home.pl)

2) From Authority type drop down select Geographic Name

3) Leave the search field blank

4) Run the search

5) The following error is returned :
Error: Unmatched [ in regex; marked by <-- HERE in m/^[ <-- HERE ]$/ at /usr/share/perl5/MARC/Field.pm line 643.


Notes: This also happens when searching for a or Australia, but not when searching for Japan

When searching using the Authority Search pop-up, an Error 500 is returned.
Comment 1 Thomas Sycko 2024-03-09 05:12:24 UTC
I am able to replicate the error when searching all authorities and geographic authorities. Searching other authority types works as expected.
Comment 2 Hugo Agud 2024-04-12 07:09:06 UTC
*** Bug 36413 has been marked as a duplicate of this bug. ***
Comment 3 Matt Blenkinsop 2024-05-16 09:53:18 UTC
Created attachment 166811 [details] [review]
Bug 35989: Stop geographic authority searches crashing

Currently when searching for a geographic authority record the search will fail if the record has a heading in a 78X field. The system tries to do a regex match against an undefined variable causing an error. This patch makes that regex match conditional on the variable being defined to allow the search to succeed.

Test plan:
1) Navigate to Authorities
2) In the search bar at the top of the page, click on the dropdown options and in the 'Authority type' field, select 'Geographic Name'
3) Click search
4) You should have a list of authorities
5) Click on any authority record and then click edit and select to edit the record
6) Click on the "7" button
7) Click on the green text next to the '781' field to get the list of fields
8) In field 'v' enter any string you like
9) Click save
10) Repeat steps 1-3, this time it should display an error message for an Unmatched [ in regex
11) Apply patch
12) restart_all
13) Refresh the page, the results should show and the string you entered in the 'v' field should display on the record you edited
Comment 4 Nick Clemens (kidclamp) 2024-05-16 12:39:28 UTC
Created attachment 166821 [details] [review]
Bug 35989: Stop geographic authority searches crashing

Currently when searching for a geographic authority record the search will fail if the record has a heading in a 78X field. The system tries to do a regex match against an undefined variable causing an error. This patch makes that regex match conditional on the variable being defined to allow the search to succeed.

Test plan:
1) Navigate to Authorities
2) In the search bar at the top of the page, click on the dropdown options and in the 'Authority type' field, select 'Geographic Name'
3) Click search
4) You should have a list of authorities
5) Click on any authority record and then click edit and select to edit the record
6) Click on the "7" button
7) Click on the green text next to the '781' field to get the list of fields
8) In field 'v' enter any string you like
9) Click save
10) Repeat steps 1-3, this time it should display an error message for an Unmatched [ in regex
11) Apply patch
12) restart_all
13) Refresh the page, the results should show and the string you entered in the 'v' field should display on the record you edited

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Nick Clemens (kidclamp) 2024-05-16 12:52:51 UTC
This fixes the immediate issue, however, the variables:
$subfields_to_report
$subfields_to_subdivision

are both being used oddly - they are initialized, but then repeatedly used in loops of different field types (4xx, 7xx, 8xx) without being cleared or reset.

If you add a 751 field before the 781 before applying the patch, the problem disappears

Also, you can then note the 781 field is repeated

I also tested by simply initializing $subfields_to_subdivision as undef rather then "" and the error disappears.

I think we need to look deeper and clean a bit more here
Comment 6 Aleisha Amohia 2024-05-27 03:22:30 UTC
(In reply to Nick Clemens (kidclamp) from comment #5)
> This fixes the immediate issue...
> 
> I think we need to look deeper and clean a bit more here

This is a major bug - we'd be quite keen to see the immediate fix get through so we're no longer getting an error. It will buy the community time to look deeper and do a proper clean, without inconveniencing the libraries. What do you think?
Comment 7 Nick Clemens (kidclamp) 2024-05-29 14:03:05 UTC
Created attachment 167244 [details] [review]
Bug 35989: Stop geographic authority searches crashing

Currently when searching for a geographic authority record the search will fail if the record has a heading in a 78X field. The system tries to do a regex match against an undefined variable causing an error. This patch makes that regex match conditional on the variable being defined to allow the search to succeed.

Test plan:
1) Navigate to Authorities
2) In the search bar at the top of the page, click on the dropdown options and in the 'Authority type' field, select 'Geographic Name'
3) Click search
4) You should have a list of authorities
5) Click on any authority record and then click edit and select to edit the record
6) Click on the "7" button
7) Click on the green text next to the '781' field to get the list of fields
8) In field 'v' enter any string you like
9) Click save
10) Repeat steps 1-3, this time it should display an error message for an Unmatched [ in regex
11) Apply patch
12) restart_all
13) Refresh the page, the results should show and the string you entered in the 'v' field should display on the record you edited

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens (kidclamp) 2024-05-29 14:03:07 UTC
Created attachment 167245 [details] [review]
Bug 35989: (QA follow-up) Add test and limit variable scope

Before this patch if a record had a 751 and a 781 you could have fields repeated.
This patch reduces the scope of the fields to subdivision variable as it is only used in processing 7xx
fields and should not be shared between fields.

I also add unit tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens (kidclamp) 2024-05-29 14:06:18 UTC
(In reply to Aleisha Amohia from comment #6)
> (In reply to Nick Clemens (kidclamp) from comment #5)
> > This fixes the immediate issue...
> > 
> > I think we need to look deeper and clean a bit more here
> 
> This is a major bug - we'd be quite keen to see the immediate fix get
> through so we're no longer getting an error. It will buy the community time
> to look deeper and do a proper clean, without inconveniencing the libraries.
> What do you think?

I added a small follow-up to fix issues this revealed - and tests - can you QA Aleisha?
Comment 10 Aleisha Amohia 2024-05-30 02:52:18 UTC
Sorry, some QA tests failing:

$ qa -v 2
testing 2 commit(s) (applied to b16e205 'a94 Koha 24.05 is here!')

Processing files before patches
|========================>| 2 / 2 (100.00%)
Processing files after patches
|========================>| 2 / 2 (100.00%)

 WARN	C4/AuthoritiesMarc.pm
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 617, now: 619)

 WARN	t/db_dependent/AuthoritiesMarc.t
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 127, now: 129)


Processing additional checks OK!


	* Was this bug sponsored by anyone?

Running tests (1)
	* Proving /kohadevbox/koha/t/db_dependent/AuthoritiesMarc.t OK!
Comment 11 Matt Blenkinsop 2024-05-30 08:24:36 UTC
Created attachment 167267 [details] [review]
Bug 35989: (QA follow-up): Fix QA tests

Some file tidying to pass the QA tests
Comment 12 Aleisha Amohia 2024-06-04 23:17:38 UTC
Created attachment 167422 [details] [review]
Bug 35989: Stop geographic authority searches crashing

Currently when searching for a geographic authority record the search will fail if the record has a heading in a 78X field. The system tries to do a regex match against an undefined variable causing an error. This patch makes that regex match conditional on the variable being defined to allow the search to succeed.

Test plan:
1) Navigate to Authorities
2) In the search bar at the top of the page, click on the dropdown options and in the 'Authority type' field, select 'Geographic Name'
3) Click search
4) You should have a list of authorities
5) Click on any authority record and then click edit and select to edit the record
6) Click on the "7" button
7) Click on the green text next to the '781' field to get the list of fields
8) In field 'v' enter any string you like
9) Click save
10) Repeat steps 1-3, this time it should display an error message for an Unmatched [ in regex
11) Apply patch
12) restart_all
13) Refresh the page, the results should show and the string you entered in the 'v' field should display on the record you edited

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 13 Aleisha Amohia 2024-06-04 23:17:41 UTC
Created attachment 167423 [details] [review]
Bug 35989: (QA follow-up) Add test and limit variable scope

Before this patch if a record had a 751 and a 781 you could have fields repeated.
This patch reduces the scope of the fields to subdivision variable as it is only used in processing 7xx
fields and should not be shared between fields.

I also add unit tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 14 Aleisha Amohia 2024-06-04 23:17:43 UTC
Created attachment 167424 [details] [review]
Bug 35989: (QA follow-up): Fix QA tests

Some file tidying to pass the QA tests

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 15 Martin Renvoize (ashimema) 2024-06-06 14:17:02 UTC
Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant
Comment 16 Aleisha Amohia 2024-06-26 22:33:20 UTC
Can confirm this is still valid in 23.11 - please backport through 24.05 as well
Comment 17 Lucas Gass (lukeg) 2024-06-27 13:37:41 UTC
Backported to 24.05.x for upcoming 24.05.02
Comment 18 Fridolin Somers 2024-07-17 13:04:06 UTC
Pushed to 23.11.x for 23.11.07
Comment 19 Wainui Witika-Park 2024-07-19 01:33:56 UTC
not backporting to 23.05.x unless requested
Comment 20 David Nind 2024-07-31 23:59:27 UTC
I've attempted to add a release note, but I'm not sure that I've described it correctly.