[Bug 43460] New: REST API XML payload conversion breaks plugin API routes that consume XML
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 Bug ID: 43460 Summary: REST API XML payload conversion breaks plugin API routes that consume XML Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: tomascohen@gmail.com CC: tomascohen@gmail.com Target Milestone: --- Bug 37762 makes the REST API convert request bodies sent with an application/xml content type to JSON so they can be validated against Koha's swagger definitions, and convert response bodies with an application/xml content type from JSON to XML. Both hooks apply to every route, including the /contrib plugin routes. Plugins define their own API contracts and may read and output XML directly. For example, the POC for the NCIP server plugin accepts XML messages on its route. On Koha 26.05 and later the request hook replaces the incoming NCIP message with a JSON rendering of it before the plugin controller runs, destroying the message such that element attributes and ordering are lost with no way to get to that data. The response hook then dies trying to parse the plugin's XML response as JSON, turning it into a 500. -- 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=43460 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |37762 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37762 [Bug 37762] Expand ILL to allow for supplying agency/lending library workflows -- 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=43460 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- 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=43460 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=43460 --- Comment #1 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Created attachment 204964 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204964&action=edit Bug 43460: Let API routes skip the global XML<->JSON conversion hooks Koha::REST::V1's hooks convert every route's XML body to JSON and back, breaking plugin routes with native XML protocols like NCIP. A route can now set x-koha-raw-payload in its OpenAPI spec to opt out of both conversions. Co-Authored-By: Claude Sonnet 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=43460 --- Comment #2 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Created attachment 204965 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204965&action=edit Bug 43460: Add tests for x-koha-raw-payload Adds echo_raw and echo_converted routes to the Test plugin fixture, one setting x-koha-raw-payload and one without it, both echoing the request body. Posts a real NCIP-style XML message with attributes through Koha::REST::V1 via Test::Mojo: the flagged route returns the exact bytes sent, while the default route shows the controller only sees the body after attributes were silently dropped by the XML<->JSON conversion. prove t/db_dependent/Koha/REST/V1.t Co-Authored-By: Claude Sonnet 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=43460 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@openfifth.co.u | |k --- Comment #3 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Hi Kyle, apologies in advance for the trouble this may have caused / is causing. I did not notice you had assigned this to yourself and gave this a go myself. Please let me know if you agree with the approach! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|kyle@bywatersolutions.com |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=43460 --- Comment #4 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- *** Bug 43491 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 Kyle M Hall (khall) <kyle@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=43460 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #204964|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=43460 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #204965|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=43460 --- Comment #5 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207409 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207409&action=edit Bug 43460: Let API routes skip the global XML<->JSON conversion hooks Koha::REST::V1's hooks convert every route's XML body to JSON and back, breaking plugin routes with native XML protocols like NCIP. A route can now set x-koha-raw-payload in its OpenAPI spec to opt out of both conversions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.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=43460 --- Comment #6 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207410 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207410&action=edit Bug 43460: Add tests for x-koha-raw-payload Adds echo_raw and echo_converted routes to the Test plugin fixture, one setting x-koha-raw-payload and one without it, both echoing the request body. Posts a real NCIP-style XML message with attributes through Koha::REST::V1 via Test::Mojo: the flagged route returns the exact bytes sent, while the default route shows the controller only sees the body after attributes were silently dropped by the XML<->JSON conversion. prove t/db_dependent/Koha/REST/V1.t Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.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=43460 --- Comment #7 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207411 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207411&action=edit Bug 43460: (QA follow-up) Remove dead openapi.op_spec fallback * Removes the 'openapi.op_spec' fallback from the new helper and from Koha::REST::V1::Auth, it only existed in Mojolicious::Plugin::OpenAPI before 1.17 and Koha requires 5.09 * Replaces "XML<->JSON" in the helper POD, Pod renders it as "XM-JSON" 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=43460 --- Comment #8 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207412 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207412&action=edit Bug 43460: (QA follow-up) Tidy the x-koha-raw-payload tests and fixture * Declares the raw body as a string and calls valid_input in the fixture controllers, an object body returns a 400 for raw XML * Drops the /public/ prefix from the fixture routes, the namespace is the segment after /api/v1/ so it had no effect * Installs only the Test plugin in V1.t, which removes the copied BadAPIRoute warnings and the logger mock * Asserts the response content type, the response hook only skips application/xml 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=43460 --- Comment #9 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207413 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207413&action=edit Bug 43460: (QA follow-up) Document x-koha-raw-payload in PluginRoutes POD The flag only existed in the POD of a private sub in Koha::REST::V1, which is not where a plugin author looks. 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=43460 --- Comment #10 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207414 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207414&action=edit Bug 43460: (QA follow-up) Add unit tests * Covers the XML payload conversion hook * Adds the echo_authorized fixture route, the last case needs a plugin route that requires a permission and fails until the next patch 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=43460 --- Comment #11 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207415 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207415&action=edit Bug 43460: (QA follow-up) Only convert the XML payload for the endpoint action around_action also fires for the Auth#under bridge, so the conversion ran twice and the second pass always died parsing the now-JSON body. The $last guard limits it to the endpoint action. 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=43460 --- Comment #12 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207416 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207416&action=edit Bug 43460: (QA follow-up) Remove validate_json_payload * Removes validate_json_payload, it validated against the whole swagger document so it accepted every payload, its 400 was unreachable from inside Try::Tiny, and it re-bundled the spec on every call * Narrows the try to the XML parse, a body that isn't XML is still left for valid_input to reject while a failure in parse_xml is no longer hidden 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=43460 --- Comment #13 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207417 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207417&action=edit Bug 43460: (QA follow-up) Add tests for XML response bodies A route that renders its own XML the way TwiML is rendered gets a 500 from the response hook until the next patch. The xml_response fixture route shows it. 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=43460 --- Comment #14 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207418 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207418&action=edit Bug 43460: (QA follow-up) Leave a response body that isn't JSON alone The response hook decoded every application/xml body as JSON and died on anything else. A body that doesn't decode as JSON is now left alone, so plugins that build their own XML response no longer get a 500. 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=43460 --- Comment #15 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 207419 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207419&action=edit Bug 43460: (QA follow-up) Resolve the swagger spec once when converting XML payloads parse_xml resolved the whole spec on every call. The definitions are now cached per process. 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=43460 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #207409|0 |1 is obsolete| | Attachment #207410|0 |1 is obsolete| | Attachment #207411|0 |1 is obsolete| | Attachment #207412|0 |1 is obsolete| | Attachment #207413|0 |1 is obsolete| | Attachment #207414|0 |1 is obsolete| | Attachment #207415|0 |1 is obsolete| | Attachment #207416|0 |1 is obsolete| | Attachment #207417|0 |1 is obsolete| | Attachment #207418|0 |1 is obsolete| | Attachment #207419|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=43460 --- Comment #16 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207424 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207424&action=edit Bug 43460: Let API routes skip the global XML<->JSON conversion hooks Koha::REST::V1's hooks convert every route's XML body to JSON and back, breaking plugin routes with native XML protocols like NCIP. A route can now set x-koha-raw-payload in its OpenAPI spec to opt out of both conversions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #17 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207425 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207425&action=edit Bug 43460: Add tests for x-koha-raw-payload Adds echo_raw and echo_converted routes to the Test plugin fixture, one setting x-koha-raw-payload and one without it, both echoing the request body. Posts a real NCIP-style XML message with attributes through Koha::REST::V1 via Test::Mojo: the flagged route returns the exact bytes sent, while the default route shows the controller only sees the body after attributes were silently dropped by the XML<->JSON conversion. prove t/db_dependent/Koha/REST/V1.t Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #18 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207426 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207426&action=edit Bug 43460: (QA follow-up) Remove dead openapi.op_spec fallback * Removes the 'openapi.op_spec' fallback from the new helper and from Koha::REST::V1::Auth, it only existed in Mojolicious::Plugin::OpenAPI before 1.17 and Koha requires 5.09 * Replaces "XML<->JSON" in the helper POD, Pod renders it as "XM-JSON" Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #19 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207427 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207427&action=edit Bug 43460: (QA follow-up) Tidy the x-koha-raw-payload tests and fixture * Declares the raw body as a string and calls valid_input in the fixture controllers, an object body returns a 400 for raw XML * Drops the /public/ prefix from the fixture routes, the namespace is the segment after /api/v1/ so it had no effect * Installs only the Test plugin in V1.t, which removes the copied BadAPIRoute warnings and the logger mock * Asserts the response content type, the response hook only skips application/xml Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #20 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207428 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207428&action=edit Bug 43460: (QA follow-up) Document x-koha-raw-payload in PluginRoutes POD The flag only existed in the POD of a private sub in Koha::REST::V1, which is not where a plugin author looks. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #21 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207429 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207429&action=edit Bug 43460: (QA follow-up) Add unit tests * Covers the XML payload conversion hook * Adds the echo_authorized fixture route, the last case needs a plugin route that requires a permission and fails until the next patch Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #22 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207430 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207430&action=edit Bug 43460: (QA follow-up) Only convert the XML payload for the endpoint action around_action also fires for the Auth#under bridge, so the conversion ran twice and the second pass always died parsing the now-JSON body. The $last guard limits it to the endpoint action. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #23 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207431 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207431&action=edit Bug 43460: (QA follow-up) Remove validate_json_payload * Removes validate_json_payload, it validated against the whole swagger document so it accepted every payload, its 400 was unreachable from inside Try::Tiny, and it re-bundled the spec on every call * Narrows the try to the XML parse, a body that isn't XML is still left for valid_input to reject while a failure in parse_xml is no longer hidden Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #24 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207432 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207432&action=edit Bug 43460: (QA follow-up) Add tests for XML response bodies A route that renders its own XML the way TwiML is rendered gets a 500 from the response hook until the next patch. The xml_response fixture route shows it. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #25 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207433 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207433&action=edit Bug 43460: (QA follow-up) Leave a response body that isn't JSON alone The response hook decoded every application/xml body as JSON and died on anything else. A body that doesn't decode as JSON is now left alone, so plugins that build their own XML response no longer get a 500. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #26 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 207434 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=207434&action=edit Bug 43460: (QA follow-up) Resolve the swagger spec once when converting XML payloads parse_xml resolved the whole spec on every call. The definitions are now cached per process. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Mike Grgurev <mike.grgurev@pillarlibraries.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com --- Comment #27 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- I've added a signoff on behalf of Mike, but also want to pass along his question: Bug 37762 changed how all API calls return their data and here we're correcting that by adding the option to make an API call opt out of that change. Why are we not instead reverting the baseline back to what we had pre-37762 and creating a way for API calls to opt in to the conversion to JSON? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #28 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- If anyone disagrees with any of the followups, please let me know! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |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=43460 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=43460 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|tomascohen@gmail.com |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=43460 --- Comment #29 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- (In reply to Andrew Fuerste-Henry from comment #27)
I've added a signoff on behalf of Mike, but also want to pass along his question: Bug 37762 changed how all API calls return their data and here we're correcting that by adding the option to make an API call opt out of that change. Why are we not instead reverting the baseline back to what we had pre-37762 and creating a way for API calls to opt in to the conversion to JSON?
Hey Andrew, thanks for passing along Mike's question: 37762 didn't change how API calls return data in general. JSON requests and responses are untouched. The conversion only applies to application/xml traffic, which Koha's core REST API didn't support before 37762. What broke were plugin routes that handle XML themselves (like the NCIP example here). With this patchset, a plugin that returns its own XML works without any change, and a plugin route that accepts an XML request body must add x-koha-raw-payload to its spec to receive the original XML. Without it, the controller receives a JSON conversion of the body instead. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #30 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Thanks for clarifying! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #31 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 Lari Strand <lmstrand@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lmstrand@gmail.com --- Comment #32 from Lari Strand <lmstrand@gmail.com> --- This issue seems to exist in Koha 26.05.04 too. Backport? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|26.11.00 |26.11.00,26.05.06 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #33 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to 26.05.x for the upcoming 26.05.06 release. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #34 from David Cook <dcook@prosentient.com.au> --- (In reply to Pedro Amorim (ammopt) from comment #29)
Hey Andrew, thanks for passing along Mike's question: 37762 didn't change how API calls return data in general. JSON requests and responses are untouched. The conversion only applies to application/xml traffic, which Koha's core REST API didn't support before 37762. What broke were plugin routes that handle XML themselves (like the NCIP example here). With this patchset, a plugin that returns its own XML works without any change, and a plugin route that accepts an XML request body must add x-koha-raw-payload to its spec to receive the original XML. Without it, the controller receives a JSON conversion of the body instead.
I haven't reviewed the code but... wouldn't it have made more sense to make this XML to JSON conversion opt-in at the controller level? Requiring x-koha-raw-payload at the request end... makes no sense to me. We're making everything bend around the ILL? Shouldn't ILL be the exception to the rule and not the other way around? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #35 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- (In reply to David Cook from comment #34)
I haven't reviewed the code but... wouldn't it have made more sense to make this XML to JSON conversion opt-in at the controller level?
Requiring x-koha-raw-payload at the request end... makes no sense to me. We're making everything bend around the ILL? Shouldn't ILL be the exception to the rule and not the other way around?
Hi David, despite the x- prefix, x-koha-raw-payload isn't an HTTP header. It's an OpenAPI vendor extension the route author sets in the route's spec, the same way x-koha-authorization is. e.g. in a plugin's openapi.json: "post": { "x-mojo-to": "MyPlugin::Controller#ncip", "consumes": ["application/xml"], "produces": ["application/xml"], "x-koha-raw-payload": true, ... } On opt-in vs opt-out: before 37762 Koha core didn't accept generic application/xml payloads (MARCXML has its own content type and isn't affected). The intent was for them to be first-class in the REST API, validated against the same OpenAPI spec as JSON, so controllers don't need to care which format came in. For that to hold for every route without extra setup, the conversion is the default. We're not making everything bend around ILL, we're setting a standard which ISO18626 ILL just happens to be the first consumer of. Routes that want to handle the raw XML themselves (like the NCIP plugin) are the exception, hence the opt-out, at the cost of those plugins needing the flag. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #36 from David Cook <dcook@prosentient.com.au> --- (In reply to Pedro Amorim (ammopt) from comment #35)
(In reply to David Cook from comment #34)
I haven't reviewed the code but... wouldn't it have made more sense to make this XML to JSON conversion opt-in at the controller level?
Requiring x-koha-raw-payload at the request end... makes no sense to me. We're making everything bend around the ILL? Shouldn't ILL be the exception to the rule and not the other way around?
Hi David, despite the x- prefix, x-koha-raw-payload isn't an HTTP header. It's an OpenAPI vendor extension the route author sets in the route's spec, the same way x-koha-authorization is. e.g. in a plugin's openapi.json: "post": { "x-mojo-to": "MyPlugin::Controller#ncip", "consumes": ["application/xml"], "produces": ["application/xml"], "x-koha-raw-payload": true, ... }
Ah thanks for the clarification, Pedro!
On opt-in vs opt-out: before 37762 Koha core didn't accept generic application/xml payloads (MARCXML has its own content type and isn't affected).
Koha core might not have accepted application/xml but I know that there are Koha plugins that do. This looks like a breaking change that could affect an unknown number of systems.
The intent was for them to be first-class in the REST API, validated against the same OpenAPI spec as JSON, so controllers don't need to care which format came in. For that to hold for every route without extra setup, the conversion is the default.
Sorry, what's "them" in this context?
We're not making everything bend around ILL, we're setting a standard which ISO18626 ILL just happens to be the first consumer of. Routes that want to handle the raw XML themselves (like the NCIP plugin) are the exception, hence the opt-out, at the cost of those plugins needing the flag.
I suppose that you can call it setting a standard, but it reverses an existing feature and adds new dev requirements. It breaks backwards compatibility. Which is fine in a major release, although I wish we had a Bugzilla field to flag compatibility breakages so we could put it in release notes automatically... I suppose the initial conversion of XML to JSON was added in 26.05.00 (in bug 37762), so it does make sense to backport it to 26.05.x... -- But the more I think about it... yeah this still feels wrong. Sending "Content-Type: application/xml" and getting back "application/json" automatically? That's weird. If you had "Accept: application/json" that's another story... that could be a compelling case for an automatic conversion. But oh well. The genie is out of the bottle now... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #37 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #36)
But the more I think about it... yeah this still feels wrong. Sending "Content-Type: application/xml" and getting back "application/json" automatically? That's weird. If you had "Accept: application/json" that's another story... that could be a compelling case for an automatic conversion.
Ah wait... looking at Koha/REST/V1.pm it translates the JSON automatically to XML if the Content-Type was application/xml... But wouldn't it make more sense to look at "produces" from the Swagger/OpenAPI spec? And look at what the Accept header has? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #38 from David Cook <dcook@prosentient.com.au> --- (In reply to Pedro Amorim (ammopt) from comment #35)
On opt-in vs opt-out: before 37762 Koha core didn't accept generic application/xml payloads (MARCXML has its own content type and isn't affected). The intent was for them to be first-class in the REST API, validated against the same OpenAPI spec as JSON, so controllers don't need to care which format came in. For that to hold for every route without extra setup, the conversion is the default.
Now that I think about it... this sounds like a shortcoming of Mojolicious::Plugin::OpenAPI really... or our misunderstanding of it... Take a look at https://metacpan.org/release/JHTHORSEN/Mojolicious-Plugin-OpenAPI-5.12/view/... openapi.parse_request_body $hash_ref = $c->openapi->parse_request_body; Returns a structure representing the request body. The default is to parse the input as JSON: {content_type => "application/json", exists => !!$c->req->body_size, value => $c->req->json}; This method is useful to override, in case you want to parse some other structure than JSON. So... I reckon that might be a better way of doing it... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #39 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #38)
Take a look at https://metacpan.org/release/JHTHORSEN/Mojolicious-Plugin-OpenAPI-5.12/view/ lib/Mojolicious/Plugin/OpenAPI/Parameters.pm#openapi.parse_request_body
openapi.parse_request_body $hash_ref = $c->openapi->parse_request_body; Returns a structure representing the request body. The default is to parse the input as JSON:
{content_type => "application/json", exists => !!$c->req->body_size, value => $c->req->json}; This method is useful to override, in case you want to parse some other structure than JSON.
So... I reckon that might be a better way of doing it...
That is to say, you can override the registration of the "openapi.parse_request_body" helper to add support for application/xml. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |44035 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=44035 [Bug 44035] Negotiate request and response body formats in the REST API, replacing the XML/JSON hooks -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 Charles Hillen <chillen@ebsco.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chillen@ebsco.com Status|Pushed to stable |Needs documenting --- Comment #40 from Charles Hillen <chillen@ebsco.com> --- Hi Everyone, I don't mean to intrude, but I can't seem to get through to Koha Support. We at GOBI Library Solutions are continuing to see this error with customer orders: Sacred Heart University Seminary of the Southwest Malmo University Austin Presbyterian Theological Seminary San Antonio College We use a resubmit function in our management platform to retry API orders that have failed, and just today I have retried with no luck. I cannot tell from the comment threads below whether this issue is still being worked on or is considered resolved. However, as I said, we are seeing it continue since 23 Oct 2026. Please advise or redirect me as appropriate. Thank you. Charles Hillen, Director Library Technical Services GOBI Library Solutions (chillen@ebsco.com) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43460 --- Comment #41 from David Cook <dcook@prosentient.com.au> --- (In reply to Charles Hillen from comment #40)
Hi Everyone,
I don't mean to intrude, but I can't seem to get through to Koha Support. We at GOBI Library Solutions are continuing to see this error with customer orders:
Sacred Heart University Seminary of the Southwest Malmo University Austin Presbyterian Theological Seminary San Antonio College
Hi Charles, It sounds like you're after Bywater Solutions rather than the Koha Community per se. Hopefully Kyle from Bywater can help you out there. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org