[Bug 43757] New: EDIFACT parser drops syntax 4 interchanges silently and dies on a missing UNA
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 Bug ID: 43757 Summary: EDIFACT parser drops syntax 4 interchanges silently and dies on a missing UNA Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Acquisitions Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org Target Milestone: --- Koha::Edifact::service_string_advice only accepts the exact string ":+.? '" and the failure paths are silent or fatal: 1. An interchange using syntax version 4 service characters, for example UNA:+.?*' , is not accepted. In syntax version 3 the fifth UNA character is reserved (a space); in syntax version 4 it is the repetition separator (default asterisk). Koha does not use repetition, so it can simply be ignored, but at present the file is thrown away. Result on main: the parser only carps "Non standard Service String Advice", Koha::Edifact->message_array returns 0 messages, process_invoice (and process_quote / process_ordrsp) then mark the message 'received' having done nothing. An invoice is lost without any error being shown to staff. 2. An interchange that does not start with UNA makes Koha::Edifact->new croak ("File does not start with a Service string advice"). The calls in edi_cron.pl are not wrapped, so the whole cron run stops: the remaining messages and accounts are not processed and the message is left in status 'processing'. Reproduced on main with a minimal interchange: - UNA:+.?*' -> messages = 0, warning only - no UNA -> dies References: ISO 9735 service string advice as profiled for syntax versions 3 and 4 (for example Odette OS11 "Structure of an EDIFACT Interchange", sections 2.2 and 4, and GS1 EANCOM syntax documentation). Note that for character sets other than UNOA/UNOB the default separators are non-printable, so a sender using printable separators is required to send UNA; a missing UNA is therefore a supplier-side error rather than something Koha should guess at. Proposed changes: a. Accept the syntax 4 service string advice by ignoring position 5, and keep refusing separators Koha cannot handle. b. When an interchange cannot be parsed, or parses to no messages, set the edifact_messages status to 'error', add an edifact_errors row with the reason, and carry on with the next message instead of marking it received or dying. (Optionally, as a follow-up, assume the standard separators when UNA is absent.) Test plan: 1. Add an INVOIC message whose interchange starts with UNA:+.?*' to edifact_messages and process it. 2. Without the patch: no invoice is created, the message is marked received, and no error is recorded. 3. With the patch: the invoice is created as it would be with UNA:+.? '. 4. Add a message with no UNA and a second, valid message, then run misc/cronjobs/edi_cron.pl (or process_invoice on both). 5. Without the patch: the cron dies on the first message. With the patch: the first message has status 'error' with an edifact_errors row, and the second is processed. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | Status|NEW |CONFIRMED Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |41852 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41852 [Bug 41852] Modernize EDI support in Koha -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Related question on the missing-UNA case, to check it belongs here rather than in a new bug. I'd read (IBM's EDIFACT documentation) that an interchange can start directly with UNB because it "includes the delimiter values". Looking at ISO 9735 more closely, that isn't right. UNB carries the syntax identifier and version (e.g. UNOC:3), sender, recipient, date/time and control reference. It carries no separator characters. Only UNA declares separators. When UNA is absent the receiver is meant to assume the defaults for that syntax level. So an interchange that starts with UNB is the "no UNA" scenario already described in point 2 of the description, and I don't think it needs a separate bug. On the optional follow-up ("assume the standard separators when UNA is absent"), the comment in the description holds. For character sets other than UNOA/UNOB the default separators are non-printable, so a sender using printable separators must send UNA. Falling back to `:+.? '` would therefore be a tolerance for non-conforming suppliers, not spec behaviour. That makes it worth doing only if we see real supplier files that do this, and I'd limit it to files that are otherwise parseable with those separators. Is anyone aware of a supplier sending UNB-first files? A sample (anonymised) would help decide whether the follow-up is warranted. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43870 Status|CONFIRMED |Needs Signoff Patch complexity|--- |Small patch Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43870 [Bug 43870] EDIFACT message with no BGM segment stops edi_cron.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208609 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208609&action=edit Bug 43757: Accept syntax 4 UNA and flag unparsable EDIFACT interchanges Koha::Edifact::service_string_advice only accepted the exact string ":+.? '". In syntax version 3 the fifth character of the UNA is reserved (a space); in syntax version 4 it is the repetition separator, default "*". Koha does not use repetition, so the character can be ignored, but an interchange starting "UNA:+.?*'" was thrown away: the parser only carped, message_array returned nothing, and process_invoice, process_quote and process_ordrsp then marked the message 'received' having done nothing. An interchange that did not start with UNA made Koha::Edifact->new croak with the start of the file in the error, which is not caught in edi_cron.pl, so the cron run stopped, left the message in 'processing' and skipped everything after it. This patch: - ignores the fifth UNA character - makes Koha::Edifact->new throw Koha::Exceptions::FileNotParsed, with a short reason, for an empty transmission, a missing or unsupported UNA and an interchange containing no messages - adds Koha::EDI::_edifact_parser, used by process_invoice, process_quote and process_ordrsp, which catches that exception, sets the message status to 'error', adds an edifact_errors row and logs it, so the cron carries on with the next message. A parser supplied by a plugin through the 'edifact' hook is still used as before. Test plan: 1. Apply the patch 2. prove t/Edifact.t t/db_dependent/Koha/EDI.t => they pass 3. git stash the changes to Koha/ and run them again => the new subtests 'service string advice' and 'unparsable interchanges' fail 4. Optionally, add an INVOICE to edifact_messages whose raw_msg starts with UNA:+.?*' and run misc/cronjobs/edi_cron.pl => the invoice is created and the message is 'received' 5. Add an INVOICE with no UNA, then a valid one, and run the cron again => the first is 'error' with a row in edifact_errors, the second is processed and the cron completes Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208610 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208610&action=edit Bug 43757: (follow-up) Assume the default separators when the UNA is missing ISO 9735 lets an interchange omit the UNA, in which case the default separators apply. Some senders do, so a file that starts directly with the UNB interchange header is read as if it had the default UNA ":+.? '" instead of being flagged as an error. Note that for character sets other than UNOA and UNOB the default separators are not printable, so a sender using the usual printable ones is required to send a UNA. This is therefore leniency towards a non-conforming sender, and is limited to a file that starts with UNB. A file starting with anything else is still refused. Test plan: 1. prove t/Edifact.t t/db_dependent/Koha/EDI.t => they pass 2. Add an INVOICE to edifact_messages whose raw_msg is a valid interchange with the leading UNA:+.? ' removed, and process it => the invoice is created and the message is 'received' 3. Change the raw_msg to start with anything other than UNA or UNB => the message is 'error' with a row in edifact_errors Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 Laura Escamilla <Laura.escamilla@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 Laura Escamilla <Laura.escamilla@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #208609|0 |1 is obsolete| | Attachment #208610|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 --- Comment #4 from Laura Escamilla <Laura.escamilla@bywatersolutions.com> --- Created attachment 208919 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208919&action=edit Bug 43757: Accept syntax 4 UNA and flag unparsable EDIFACT interchanges Koha::Edifact::service_string_advice only accepted the exact string ":+.? '". In syntax version 3 the fifth character of the UNA is reserved (a space); in syntax version 4 it is the repetition separator, default "*". Koha does not use repetition, so the character can be ignored, but an interchange starting "UNA:+.?*'" was thrown away: the parser only carped, message_array returned nothing, and process_invoice, process_quote and process_ordrsp then marked the message 'received' having done nothing. An interchange that did not start with UNA made Koha::Edifact->new croak with the start of the file in the error, which is not caught in edi_cron.pl, so the cron run stopped, left the message in 'processing' and skipped everything after it. This patch: - ignores the fifth UNA character - makes Koha::Edifact->new throw Koha::Exceptions::FileNotParsed, with a short reason, for an empty transmission, a missing or unsupported UNA and an interchange containing no messages - adds Koha::EDI::_edifact_parser, used by process_invoice, process_quote and process_ordrsp, which catches that exception, sets the message status to 'error', adds an edifact_errors row and logs it, so the cron carries on with the next message. A parser supplied by a plugin through the 'edifact' hook is still used as before. Test plan: 1. Apply the patch 2. prove t/Edifact.t t/db_dependent/Koha/EDI.t => they pass 3. git stash the changes to Koha/ and run them again => the new subtests 'service string advice' and 'unparsable interchanges' fail 4. Optionally, add an INVOICE to edifact_messages whose raw_msg starts with UNA:+.?*' and run misc/cronjobs/edi_cron.pl => the invoice is created and the message is 'received' 5. Add an INVOICE with no UNA, then a valid one, and run the cron again => the first is 'error' with a row in edifact_errors, the second is processed and the cron completes Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 --- Comment #5 from Laura Escamilla <Laura.escamilla@bywatersolutions.com> --- Created attachment 208920 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208920&action=edit Bug 43757: (follow-up) Assume the default separators when the UNA is missing ISO 9735 lets an interchange omit the UNA, in which case the default separators apply. Some senders do, so a file that starts directly with the UNB interchange header is read as if it had the default UNA ":+.? '" instead of being flagged as an error. Note that for character sets other than UNOA and UNOB the default separators are not printable, so a sender using the usual printable ones is required to send a UNA. This is therefore leniency towards a non-conforming sender, and is limited to a file that starts with UNB. A file starting with anything else is still refused. Test plan: 1. prove t/Edifact.t t/db_dependent/Koha/EDI.t => they pass 2. Add an INVOICE to edifact_messages whose raw_msg is a valid interchange with the leading UNA:+.? ' removed, and process it => the invoice is created and the message is 'received' 3. Change the raw_msg to start with anything other than UNA or UNB => the message is 'error' with a row in edifact_errors Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #208919|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #208920|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 --- Comment #6 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 209587 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209587&action=edit Bug 43757: Accept syntax 4 UNA and flag unparsable EDIFACT interchanges Koha::Edifact::service_string_advice only accepted the exact string ":+.? '". In syntax version 3 the fifth character of the UNA is reserved (a space); in syntax version 4 it is the repetition separator, default "*". Koha does not use repetition, so the character can be ignored, but an interchange starting "UNA:+.?*'" was thrown away: the parser only carped, message_array returned nothing, and process_invoice, process_quote and process_ordrsp then marked the message 'received' having done nothing. An interchange that did not start with UNA made Koha::Edifact->new croak with the start of the file in the error, which is not caught in edi_cron.pl, so the cron run stopped, left the message in 'processing' and skipped everything after it. This patch: - ignores the fifth UNA character - makes Koha::Edifact->new throw Koha::Exceptions::FileNotParsed, with a short reason, for an empty transmission, a missing or unsupported UNA and an interchange containing no messages - adds Koha::EDI::_edifact_parser, used by process_invoice, process_quote and process_ordrsp, which catches that exception, sets the message status to 'error', adds an edifact_errors row and logs it, so the cron carries on with the next message. A parser supplied by a plugin through the 'edifact' hook is still used as before. Test plan: 1. Apply the patch 2. prove t/Edifact.t t/db_dependent/Koha/EDI.t => they pass 3. git stash the changes to Koha/ and run them again => the new subtests 'service string advice' and 'unparsable interchanges' fail 4. Optionally, add an INVOICE to edifact_messages whose raw_msg starts with UNA:+.?*' and run misc/cronjobs/edi_cron.pl => the invoice is created and the message is 'received' 5. Add an INVOICE with no UNA, then a valid one, and run the cron again => the first is 'error' with a row in edifact_errors, the second is processed and the cron completes Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 --- Comment #7 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 209588 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209588&action=edit Bug 43757: (follow-up) Assume the default separators when the UNA is missing ISO 9735 lets an interchange omit the UNA, in which case the default separators apply. Some senders do, so a file that starts directly with the UNB interchange header is read as if it had the default UNA ":+.? '" instead of being flagged as an error. Note that for character sets other than UNOA and UNOB the default separators are not printable, so a sender using the usual printable ones is required to send a UNA. This is therefore leniency towards a non-conforming sender, and is limited to a file that starts with UNB. A file starting with anything else is still refused. Test plan: 1. prove t/Edifact.t t/db_dependent/Koha/EDI.t => they pass 2. Add an INVOICE to edifact_messages whose raw_msg is a valid interchange with the leading UNA:+.? ' removed, and process it => the invoice is created and the message is 'received' 3. Change the raw_msg to start with anything other than UNA or UNB => the message is 'error' with a row in edifact_errors Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 --- Comment #8 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 209589 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209589&action=edit Bug 43757: (QA follow-up) Fix misc qa issues * Removes the second 'use Try::Tiny' from Koha/EDI.pm * Removes the '__WARN__' handler around the unsupported separators test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43757 --- Comment #9 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- RM Note: This patch was both signed off and qa'ed by ByWater people. I went ahead and took QA because I think Martin and I are the only two who want to deal with EDI ;) Feel free to ask for additional qa if needed! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org