Bug 35989 - Searching Geographic authorities generates error
Summary: Searching Geographic authorities generates error
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: Main
Hardware: All All
: P2 major
Assignee: Matt Blenkinsop
QA Contact: Aleisha Amohia
URL:
Keywords:
: 36413 (view as bug list)
Depends on:
Blocks: 36985
  Show dependency treegraph
 
Reported: 2024-02-05 06:09 UTC by Robert Cooper
Modified: 2025-02-06 05:05 UTC (History)
10 users (show)

See Also:
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:


Attachments
Bug 35989: Stop geographic authority searches crashing (2.09 KB, patch)
2024-05-16 09:53 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35989: Stop geographic authority searches crashing (2.13 KB, patch)
2024-05-16 12:39 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35989: Stop geographic authority searches crashing (2.13 KB, patch)
2024-05-29 14:03 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35989: (QA follow-up) Add test and limit variable scope (3.33 KB, patch)
2024-05-29 14:03 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35989: (QA follow-up): Fix QA tests (4.36 KB, patch)
2024-05-30 08:24 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35989: Stop geographic authority searches crashing (2.19 KB, patch)
2024-06-04 23:17 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 35989: (QA follow-up) Add test and limit variable scope (3.38 KB, patch)
2024-06-04 23:17 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 35989: (QA follow-up): Fix QA tests (4.40 KB, patch)
2024-06-04 23:17 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.