[Bug 43694] New: Koha::REST::V1::to_xml emits child elements in random order, producing schema-invalid XML
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 Bug ID: 43694 Summary: Koha::REST::V1::to_xml emits child elements in random order, producing schema-invalid XML Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: minhpq@tinhvan.com QA Contact: tomascohen@gmail.com CC: tomascohen@gmail.com Target Milestone: --- Koha::REST::V1::to_xml builds XML by walking Perl hash keys: sub to_xml { # V1.pm line 240 my $root_key = ( keys %$json )[0]; # line 244 ... sub _json_to_xml { # line 261 foreach my $key ( keys %$json ) { # line 265 Perl randomises hash key order per process (PERL_PERTURB_KEYS, default since 5.18), so the order of child elements differs from one run to the next. The ISO 18626 schema uses xs:sequence, not xs:all, so element order is significant: a message whose children are emitted in the wrong order is invalid and a conformant receiver rejects it at parse time. The same applies to the root element: line 244 takes ( keys %$json )[0], which is only correct because these payloads happen to have exactly one top-level key. It is fragile for the same reason. How to reproduce: Run this five times on a Koha instance and compare the output lines. use Modern::Perl; use XML::LibXML; use Koha::REST::V1; my $json = { supplyingAgencyMessage => { header => { supplyingAgencyId => { agencyIdType => 'ISIL', agencyIdValue => 'X' }, requestingAgencyId => { agencyIdType => 'ISIL', agencyIdValue => 'Y' }, timestamp => '2026-09-30T00:00:00Z', requestingAgencyRequestId => '1', supplyingAgencyRequestId => '2', }, messageInfo => { reasonForMessage => 'StatusChange' }, statusInfo => { status => 'Loaned', lastChange => '2026-09-30T00:00:00Z' }, }, }; my $doc = XML::LibXML->new->parse_string( Koha::REST::V1::to_xml($json) ); my $root = $doc->documentElement; say join( ', ', map { $_->nodeName } $root->findnodes('./*') ); Observed on Koha 26.05.02, Perl v5.38.2 — five consecutive runs: header, statusInfo, messageInfo header, messageInfo, statusInfo messageInfo, header, statusInfo statusInfo, header, messageInfo messageInfo, statusInfo, header The schema requires header, messageInfo, statusInfo, deliveryInfo, returnInfo in that order (ISO-18626-v1_2.xsd), so only the second run above is valid. Nested elements are scrambled the same way: header itself must be supplyingAgencyId, requestingAgencyId, multipleItemRequestId, timestamp, requestingAgencyRequestId, supplyingAgencyRequestId, requestingAgencyAuthentication. Why this has not been noticed: 1. ISO 18626 testing so far appears to have been Koha-to-Koha. The inbound side, Koha::REST::V1::parse_xml/_parse_node, builds a hash and therefore does not care about element order, so two Koha instances understand each other whatever order they emit. 2. Koha does validate these payloads — Koha::ILL::ISO18626::is_invalid resolves the swagger definition and calls $schema->validate($json). But that validates the JSON structure, and JSON objects are unordered by definition, so the check passes while the XML that goes out on the wire is invalid. The validation gives false confidence here rather than catching the problem. This makes the defect worse than an ordinary bug: it is not deterministic. Testing an integration once and seeing it work says nothing, because the next message may be ordered differently. How it was found: Interoperability testing between Koha 26.05.02 and a third-party ISO 18626 implementation written in .NET, generated directly from the official v1.2 XSD (https://illtransactions.org/schemas/ISO-18626-v1_2.xsd). The first supplyingAgencyMessage Koha sent was rejected at XML deserialisation; retrying produced a different failure position, which is what led to the hash ordering. Suggested fix: to_xml has no way to know the required order on its own, so it needs the order to come from somewhere. Two options that both avoid changing every caller: a. Derive the order from the OpenAPI definition that already describes each message. JSON::Validator keeps the properties in the order they appear in the spec file, so the swagger definition can supply the sequence. This also keeps the spec as the single source of truth. b. Let callers pass an explicit ordering, e.g. to_xml($json, { order => {...} }), and have the ISO 18626 code supply the sequences from the XSD. Whichever is chosen, it is worth adding a test that serialises a known payload and asserts the resulting XML validates against the official XSD — a JSON-level check cannot catch this class of bug. Related: bug 43674 (ISO 18626 messages are missing the mandatory ISO18626Message envelope), also in Koha::REST::V1. Fixing either one alone still leaves Koha unable to exchange ISO 18626 messages with a conformant implementation. -- 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=43694 Phan Quang Minh <minhpq@tinhvan.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=43674 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 Phan Quang Minh <minhpq@tinhvan.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@openfifth.co.u | |k -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=43674 | Depends on| |43674 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43674 [Bug 43674] ISO 18626 messages are missing the mandatory ISO18626Message envelope -- 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=43694 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |pedro.amorim@openfifth.co.u |ity.org |k -- 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=43694 --- Comment #1 from Phan Quang Minh <minhpq@tinhvan.com> --- Correction to my own report, after testing the patch on bug 43674 against our implementation. I wrote that a conformant receiver rejects an out-of-order message at parse time. That is too strong, and in our case it was wrong. Retesting with the same message content in two element orders: - correct schema order -> our parser reads it; XSD validation passes - scrambled order -> our parser still reads it, with no data loss; XSD validation fails So .NET's XmlSerializer, which is what our implementation uses, matches child elements by name and tolerates sequence violations. A validating parser does reject the message — XSD validation fails exactly as described — but a receiver that only deserialises will not notice. I also traced what actually broke our first exchange with Koha, which I attributed to ordering in the original report. It was the timestamp format, not the order: <timestamp>2026-09-30 10:23:41</timestamp> A space instead of T, and no timezone, so not a valid xs:dateTime. That one does throw on deserialisation and does fail XSD validation. Ordering was a red herring in the failure I described. What still stands, and why I think this is worth fixing anyway: - The XML Koha emits is objectively schema-invalid, verifiably so against the official XSD, and invalid differently on each run. - Any partner that validates against the schema — which is a reasonable thing for an ILL system to do, and arguably what Koha should do to its own output — will reject the message, intermittently. - A defect that appears and disappears between runs is expensive for whoever meets it next. We only found it because we were looking at the XML rather than at whether the exchange happened to work. The reproduction script in the original report is unaffected: five runs still produce five different element orders. Sorry for the overstated impact claim. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 --- Comment #2 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Created attachment 207109 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207109&action=edit Bug 43694: Emit ISO 18626 elements in schema order Koha::REST::V1::to_xml builds XML from Perl hashes, so child elements come out in random hash order. ISO 18626 uses xs:sequence, so element order is significant and schema-conformant partners rejected the messages, differently on each attempt. xml_with_envelope now puts every element's children in the order of ISO-18626-2021-3.xsd before adding the envelope. Elements the schema does not define come last, sorted by name. Koha::REST::V1::to_xml is unchanged. Test plan: 1) Apply patch and restart_all 2) Run this a few times and note the element order is the same every time, and matches the schema (header, messageInfo, statusInfo): perl -MKoha::ILL::ISO18626 -e 'print Koha::ILL::ISO18626::xml_with_envelope({ supplyingAgencyMessage => { statusInfo => { status => "Loaned" }, messageInfo => { reasonForMessage => "StatusChange" }, header => { timestamp => "2026-09-30T00:00:00Z", requestingAgencyRequestId => 1 } } })' 3) prove t/db_dependent/Koha/ILL/ISO18626.t t/db_dependent/api/v1/iso18626/request.t Co-Authored-By: Claude Opus 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=43694 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43696 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43696 [Bug 43696] ISO 18626 date-times are not UTC -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 Phan Quang Minh <minhpq@tinhvan.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #207109|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=43694 --- Comment #3 from Phan Quang Minh <minhpq@tinhvan.com> --- Created attachment 207163 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207163&action=edit Bug 43694: Emit ISO 18626 elements in schema order -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 Phan Quang Minh <minhpq@tinhvan.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #4 from Phan Quang Minh <minhpq@tinhvan.com> --- Signed off. Tested on Koha 26.05.02 with the bug 43674 patches applied. Determinism. Ran the test plan's one-liner six times: identical output every run, and in schema order. header, messageInfo, statusInfo header, messageInfo, statusInfo header, messageInfo, statusInfo header, messageInfo, statusInfo header, messageInfo, statusInfo header, messageInfo, statusInfo Before the patch the same six runs gave six different orders, only one of which was valid. Schema validation. A supplyingAgencyMessage produced by xml_with_envelope now validates clean against ISO-18626-2021-2.xsd. That is the strongest check I can make from here, and it passes. Interoperability. Our third-party implementation keeps a compatibility shim that logs every correction it has to make to an incoming Koha message. The entry for element reordering has disappeared from the log. What is left is only the two outstanding defects: before any patches: missing envelope; timestamp format; element order; agency id after bug 43674: timestamp format; element order; agency id after this patch: timestamp format; agency id which correspond to bug 43701 and bug 43695. Two of the four workarounds are now gone. Same caveat as bug 43674: this is a package install, so I cannot run the prove step. Everything else in the test plan passes. One thing you will want to know, since this patch sorts by that schema ISO-18626-2021-3.xsd as published does not compile. Line 270: <xs:element name="sentToPatron" type="xs:type_yesNo" minOccurs="0"/> The type is in the ISO 18626 namespace, but the reference carries the xs: prefix, so it resolves to {http://www.w3.org/2001/XMLSchema}type_yesNo, which does not exist. Two independent processors refuse to load the file: XML::LibXML: element decl. 'sentToPatron', attribute 'type': The QName value '{http://www.w3.org/2001/XMLSchema}type_yesNo' does not resolve to a(n) type definition .NET: Type 'http://www.w3.org/2001/XMLSchema:type_yesNo' is not declared It is a one-character typo introduced in 2021-3 — 2021-2, v1_2 and v1_3 are all clean — and dropping the xs: prefix makes the file load and my validation above pass against it too. It does not affect your patch, which only reads the element ordering, and it does not affect messages on the wire. But it does mean nobody can currently validate against 2021-3 as published, which is worth someone raising with the schema maintainers. You are far better placed than I am to know who that is. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #207163|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=43694 --- Comment #5 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 210251 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=210251&action=edit Bug 43694: Emit ISO 18626 elements in schema order Koha::REST::V1::to_xml builds XML from Perl hashes, so child elements come out in random hash order. ISO 18626 uses xs:sequence, so element order is significant and schema-conformant partners rejected the messages, differently on each attempt. xml_with_envelope now puts every element's children in the order of ISO-18626-2021-3.xsd before adding the envelope. Elements the schema does not define come last, sorted by name. Koha::REST::V1::to_xml is unchanged. Test plan: 1) Apply patch and restart_all 2) Run this a few times and note the element order is the same every time, and matches the schema (header, messageInfo, statusInfo): perl -MKoha::ILL::ISO18626 -e 'print Koha::ILL::ISO18626::xml_with_envelope({ supplyingAgencyMessage => { statusInfo => { status => "Loaned" }, messageInfo => { reasonForMessage => "StatusChange" }, header => { timestamp => "2026-09-30T00:00:00Z", requestingAgencyRequestId => 1 } } })' 3) prove t/db_dependent/Koha/ILL/ISO18626.t t/db_dependent/api/v1/iso18626/request.t Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Phan Quang Minh <minhpq@tinhvan.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 --- Comment #6 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 210252 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=210252&action=edit Bug 43694: (QA follow-up) Validate generated messages against the XSD This patch adds a copy of the ISO 18626 2021-3 XSD to t/db_dependent/data/iso18626 and a 'xml_with_envelope() schema validation tests' subtest that builds one message of each of the six ISO 18626 message types and validates the generated XML against it. The JSON-level validation in Koha::ILL::ISO18626::is_invalid cannot catch element order problems, as JSON objects are unordered. Validating the XML on the wire against the official schema covers this class of bugs. The published 2021-3 XSD does not compile: the sentToPatron element uses type="xs:type_yesNo", which resolves to a type in the XML Schema namespace that does not exist. The Koha copy uses type="type_yesNo" instead, and a comment at the top of the file documents the change. Line endings were also converted to LF, tabs expanded to 4 spaces and trailing whitespace removed, as required by the QA tools. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove -v t/db_dependent/Koha/ILL/ISO18626.t => SUCCESS: Tests pass! All six message types validate 3. Revert the fix from the previous patch and repeat step 2: k$ git show HEAD~1 -- Koha/ILL/ISO18626.pm | git apply -R k$ prove -v t/db_dependent/Koha/ILL/ISO18626.t => FAIL: The messages do not validate (elements out of schema order) 4. Restore the fix: k$ git checkout Koha/ILL/ISO18626.pm 5. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #7 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- QA: I checked ISO18626_ELEMENT_ORDER against every child sequence in the official ISO-18626-2021-3.xsd and it matches. The header entry merges the three header types into a superset order that each of them fits inside. koha-qa.pl passes, and t/db_dependent/Koha/ILL/ISO18626.t and t/db_dependent/api/v1/iso18626/ pass. I added a QA follow-up that ships a copy of the 2021-3 XSD (with the xs:type_yesNo typo on sentToPatron corrected, since the published file does not compile) and validates one message of each of the six ISO 18626 message types against it. All six validate with the fix and all six fail without it. Note: t/db_dependent/Koha/ILL/Backend.t and Requests.t fail their no-warnings check on main too, so that is unrelated to this bug. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43694 --- Comment #8 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org