Bug 40910 - Log pollution for undefined/empty barcodes
Summary: Log pollution for undefined/empty barcodes
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-09-30 13:59 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-10-01 14:53 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 40910: Regression tests (5.90 KB, patch)
2025-09-30 14:17 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40910: Reduce log pollution for undefined/empty barcodes (2.85 KB, patch)
2025-09-30 14:17 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40910: Regression tests (5.97 KB, patch)
2025-10-01 14:28 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40910: Regression tests (5.97 KB, patch)
2025-10-01 14:29 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40910: Reduce log pollution for undefined/empty barcodes (2.92 KB, patch)
2025-10-01 14:29 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi (tcohen) 2025-09-30 13:59:10 UTC
Noticed in production, when `C4::SIP::ILS::Item->new` is passed an `$item_id` that is not defined or empty -or barcodedecode makes it like that- (e.g. faulty plugins) logs get flooded with DBIx::Class errors when it should never reach the ORM with a query because there's nothing to search for.

If `barcodedecode` empties the barcode, a debug log should be printed because it might be a sign of a misconfiguration or faulty plugin.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-09-30 14:17:41 UTC
Created attachment 187106 [details] [review]
Bug 40910: Regression tests

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2025-09-30 14:17:44 UTC
Created attachment 187107 [details] [review]
Bug 40910: Reduce log pollution for undefined/empty barcodes

This patch prevents warnings and DBIx::Class errors when C4::SIP::ILS::Item->new()
receives undefined, empty, or whitespace-only item_id values that could occur
from faulty plugins or malformed SIP requests.

Changes:
- Add early validation for undefined/empty item_id before barcodedecode()
- Add validation for barcodedecode() result before Koha::Items->find()
- Add debug logging when barcodedecode empties a barcode (indicates misconfiguration)
- Prevent "Odd number of elements in anonymous hash" warnings
- Prevent DBIx::Class constraint validation errors
- No functional changes - behavior preserved for valid barcodes

Test plan:
1. Apply the regression tests
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/SIP/Item.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Tests cover:
   - Valid barcode functionality (preserved)
   - Undefined item_id (no warnings)
   - Empty string item_id (no warnings)
   - Whitespace-only item_id (no warnings)
   - barcodedecode edge cases (tabs, newlines, mixed)
   - Nonexistent barcodes (expected warnings preserved)
6. Verify no warnings in SIP logs for malformed requests
7. Sign off :-D

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 3 Kyle M Hall (khall) 2025-10-01 12:09:05 UTC
I would recommend running barcode decode *after* doing all your checks on item_id, as barcodedecode can trigger plugins that will *generate* a new barcode if passed an empty barcode string ( https://github.com/bywatersolutions/koha-plugin-barcode-prefixer ) for example. 

By testing $item_id and returning if it's empty *before* running barcodedecode on it, we can ensure we don't accidentally generate a non-existent barcode for the message!
Comment 4 Kyle M Hall (khall) 2025-10-01 14:25:03 UTC
(In reply to Kyle M Hall (khall) from comment #3)
> I would recommend running barcode decode *after* doing all your checks on
> item_id, as barcodedecode can trigger plugins that will *generate* a new
> barcode if passed an empty barcode string (
> https://github.com/bywatersolutions/koha-plugin-barcode-prefixer ) for
> example. 
> 
> By testing $item_id and returning if it's empty *before* running
> barcodedecode on it, we can ensure we don't accidentally generate a
> non-existent barcode for the message!

My entire comment is invalid as Tomas already added a return on line 88.
I think the log level is good as well, we should be able to scroll back in the logs to see the message that triggered the error.
Comment 5 Kyle M Hall (khall) 2025-10-01 14:28:04 UTC
Created attachment 187195 [details] [review]
Bug 40910: Regression tests

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall (khall) 2025-10-01 14:29:46 UTC
Created attachment 187196 [details] [review]
Bug 40910: Regression tests

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall (khall) 2025-10-01 14:29:54 UTC
Created attachment 187197 [details] [review]
Bug 40910: Reduce log pollution for undefined/empty barcodes

This patch prevents warnings and DBIx::Class errors when C4::SIP::ILS::Item->new()
receives undefined, empty, or whitespace-only item_id values that could occur
from faulty plugins or malformed SIP requests.

Changes:
- Add early validation for undefined/empty item_id before barcodedecode()
- Add validation for barcodedecode() result before Koha::Items->find()
- Add debug logging when barcodedecode empties a barcode (indicates misconfiguration)
- Prevent "Odd number of elements in anonymous hash" warnings
- Prevent DBIx::Class constraint validation errors
- No functional changes - behavior preserved for valid barcodes

Test plan:
1. Apply the regression tests
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/SIP/Item.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Tests cover:
   - Valid barcode functionality (preserved)
   - Undefined item_id (no warnings)
   - Empty string item_id (no warnings)
   - Whitespace-only item_id (no warnings)
   - barcodedecode edge cases (tabs, newlines, mixed)
   - Nonexistent barcodes (expected warnings preserved)
6. Verify no warnings in SIP logs for malformed requests
7. Sign off :-D

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>