Bug 40915 - SIP message parsing with empty fields edge cases
Summary: SIP message parsing with empty fields edge cases
Status: Pushed to main
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: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-09-30 19:27 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-10-09 21:03 UTC (History)
5 users (show)

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


Attachments
Bug 40915: Add comprehensive regression tests for SIP MsgType message parsing and debug logging (15.64 KB, patch)
2025-09-30 19:36 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40915: Add enhanced debug logging for empty critical fields in SIP message parsing (2.43 KB, patch)
2025-09-30 19:36 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40915: Add comprehensive regression tests for SIP MsgType message parsing and debug logging (15.71 KB, patch)
2025-10-01 11:59 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40915: Add enhanced debug logging for empty critical fields in SIP message parsing (2.50 KB, patch)
2025-10-01 11:59 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40915: Add comprehensive regression tests for SIP MsgType message parsing and debug logging (15.76 KB, patch)
2025-10-09 14:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40915: Add enhanced debug logging for empty critical fields in SIP message parsing (2.55 KB, patch)
2025-10-09 14:32 UTC, Martin Renvoize (ashimema)
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 19:27:25 UTC
SIP message parsing can create empty field values that later cause DBIx::Class constraint errors when passed to Koha operations. The current parsing logic in C4::SIP::Sip::MsgType::_initialize() doesn't provide debug logging to help identify when critical fields (patron_id, item_id) are empty, making it difficult to troubleshoot "Odd number of elements in anonymous hash" errors in SIP server logs.

This includes some scenarios which cause fields to be treated as (empty) arrayrefs, which flood the logs with this kind of errors:

```
DBIx::Class::ResultSource::_minimal_valueset_satisfying_constraint(): Unable to satisfy requested constraint 'primary', missing values for column(s): 'itemnumber' at /usr/share/koha/lib/Koha/Objects.pm line 95
DBIx::Class::ResultSource::_minimal_valueset_satisfying_constraint(): Unable to satisfy requested constraint 'primary', missing values for column(s): 'borrowernumber' at /kohadevbox/koha/Koha/Objects.pm line 95
```
Comment 1 Tomás Cohen Arazi (tcohen) 2025-09-30 19:36:49 UTC
Created attachment 187120 [details] [review]
Bug 40915: Add comprehensive regression tests for SIP MsgType message parsing and debug logging

These tests verify that C4::SIP::Sip::MsgType correctly parses SIP messages without
creating problematic arrayref field values, and validates enhanced debug logging for
empty critical fields.

Test Coverage:
• Message parsing with correct protocol versions (PATRON_INFO/HOLD/RENEW use v2, CHECKOUT
uses v1)
• Empty patron_id and item_id field detection with debug logging
• Malformed delimiter handling (double delimiters, trailing empty fields)
• Multiple consecutive empty fields in single message
• Confusing delimiter patterns that could break parsing
• Verification that parsed fields are strings, not arrayrefs
• Working siplog mock infrastructure in C4::SIP::Sip::MsgType

Arrayref Prevention Tests:
Tests verify message parsing DOESN'T create empty arrayrefs that would later cause "Odd
number of elements in anonymous hash" errors when passed to Koha::Objects->find()
operations.

Enhanced Debug Logging Tests:
Tests validate that empty critical fields (patron_id, item_id) are detected during
parsing and generate appropriate debug messages to help troubleshoot downstream
constraint errors.
Comment 2 Tomás Cohen Arazi (tcohen) 2025-09-30 19:36:52 UTC
Created attachment 187121 [details] [review]
Bug 40915: Add enhanced debug logging for empty critical fields in SIP message parsing

This patch adds debug logging to C4::SIP::Sip::MsgType::_initialize() to help
identify when critical fields (patron_id, item_id) are parsed as empty values.

Empty critical fields in SIP messages can later cause DBIx::Class constraint
errors when passed to Koha::Objects->find() operations, generating confusing
'Odd number of elements in anonymous hash' errors in SIP server logs.

The enhanced logging helps administrators identify the root cause by logging
when empty patron_id or item_id fields are detected during message parsing.

To test:
1. Apply regression tests patch first
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/SIP/MsgType.t
=> FAIL: Debug logging tests fail (no enhanced logging present)
3. Apply this patch
4. Run:
   $ ktd --shell
  k$ prove t/db_dependent/SIP/MsgType.t
=> SUCCESS: All tests pass, debug logging works correctly
5. Sign off :-D
Comment 3 Kyle M Hall (khall) 2025-10-01 11:59:02 UTC
Created attachment 187182 [details] [review]
Bug 40915: Add comprehensive regression tests for SIP MsgType message parsing and debug logging

These tests verify that C4::SIP::Sip::MsgType correctly parses SIP messages without
creating problematic arrayref field values, and validates enhanced debug logging for
empty critical fields.

Test Coverage:
• Message parsing with correct protocol versions (PATRON_INFO/HOLD/RENEW use v2, CHECKOUT
uses v1)
• Empty patron_id and item_id field detection with debug logging
• Malformed delimiter handling (double delimiters, trailing empty fields)
• Multiple consecutive empty fields in single message
• Confusing delimiter patterns that could break parsing
• Verification that parsed fields are strings, not arrayrefs
• Working siplog mock infrastructure in C4::SIP::Sip::MsgType

Arrayref Prevention Tests:
Tests verify message parsing DOESN'T create empty arrayrefs that would later cause "Odd
number of elements in anonymous hash" errors when passed to Koha::Objects->find()
operations.

Enhanced Debug Logging Tests:
Tests validate that empty critical fields (patron_id, item_id) are detected during
parsing and generate appropriate debug messages to help troubleshoot downstream
constraint errors.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Kyle M Hall (khall) 2025-10-01 11:59:10 UTC
Created attachment 187183 [details] [review]
Bug 40915: Add enhanced debug logging for empty critical fields in SIP message parsing

This patch adds debug logging to C4::SIP::Sip::MsgType::_initialize() to help
identify when critical fields (patron_id, item_id) are parsed as empty values.

Empty critical fields in SIP messages can later cause DBIx::Class constraint
errors when passed to Koha::Objects->find() operations, generating confusing
'Odd number of elements in anonymous hash' errors in SIP server logs.

The enhanced logging helps administrators identify the root cause by logging
when empty patron_id or item_id fields are detected during message parsing.

To test:
1. Apply regression tests patch first
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/SIP/MsgType.t
=> FAIL: Debug logging tests fail (no enhanced logging present)
3. Apply this patch
4. Run:
   $ ktd --shell
  k$ prove t/db_dependent/SIP/MsgType.t
=> SUCCESS: All tests pass, debug logging works correctly
5. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Martin Renvoize (ashimema) 2025-10-09 14:32:15 UTC
Created attachment 187667 [details] [review]
Bug 40915: Add comprehensive regression tests for SIP MsgType message parsing and debug logging

These tests verify that C4::SIP::Sip::MsgType correctly parses SIP messages without
creating problematic arrayref field values, and validates enhanced debug logging for
empty critical fields.

Test Coverage:
• Message parsing with correct protocol versions (PATRON_INFO/HOLD/RENEW use v2, CHECKOUT
uses v1)
• Empty patron_id and item_id field detection with debug logging
• Malformed delimiter handling (double delimiters, trailing empty fields)
• Multiple consecutive empty fields in single message
• Confusing delimiter patterns that could break parsing
• Verification that parsed fields are strings, not arrayrefs
• Working siplog mock infrastructure in C4::SIP::Sip::MsgType

Arrayref Prevention Tests:
Tests verify message parsing DOESN'T create empty arrayrefs that would later cause "Odd
number of elements in anonymous hash" errors when passed to Koha::Objects->find()
operations.

Enhanced Debug Logging Tests:
Tests validate that empty critical fields (patron_id, item_id) are detected during
parsing and generate appropriate debug messages to help troubleshoot downstream
constraint errors.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 6 Martin Renvoize (ashimema) 2025-10-09 14:32:17 UTC
Created attachment 187668 [details] [review]
Bug 40915: Add enhanced debug logging for empty critical fields in SIP message parsing

This patch adds debug logging to C4::SIP::Sip::MsgType::_initialize() to help
identify when critical fields (patron_id, item_id) are parsed as empty values.

Empty critical fields in SIP messages can later cause DBIx::Class constraint
errors when passed to Koha::Objects->find() operations, generating confusing
'Odd number of elements in anonymous hash' errors in SIP server logs.

The enhanced logging helps administrators identify the root cause by logging
when empty patron_id or item_id fields are detected during message parsing.

To test:
1. Apply regression tests patch first
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/SIP/MsgType.t
=> FAIL: Debug logging tests fail (no enhanced logging present)
3. Apply this patch
4. Run:
   $ ktd --shell
  k$ prove t/db_dependent/SIP/MsgType.t
=> SUCCESS: All tests pass, debug logging works correctly
5. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 7 Lucas Gass (lukeg) 2025-10-09 21:03:09 UTC
Nice work everyone!

Pushed to main for 25.11