Bug 23425 - Search explodes with "invalid data, cannot decode object"
Summary: Search explodes with "invalid data, cannot decode object"
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 22144
Blocks:
  Show dependency treegraph
 
Reported: 2019-08-04 15:32 UTC by Jonathan Druart
Modified: 2019-10-17 19:33 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.04


Attachments
Bug 23425: Display the MARCXML error in the Metadata::Invalid exception message (2.07 KB, patch)
2019-08-04 15:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23425: Display the MARCXML error in the Metadata::Invalid exception message (2.12 KB, patch)
2019-09-17 18:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23425: (QA follow-up) Keep it generic (2.96 KB, patch)
2019-09-17 18:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-08-04 15:32:45 UTC
If the MARCXML data is wrong for some reasons and so MARC::Record::new_from_xml fails, a search will explodes with "invalid data, cannot decode object"

We should:
- Understand why the data is wrong
- Provide a maintenance script to catch problematic data
- Skip the record instead of exploding (or maybe not, it will hide a data problem)

It has been highlighted by bug 22144, was silently failing before those patches.
Comment 1 Jonathan Druart 2019-08-04 15:37:19 UTC
Also, Koha::Exceptions::Metadata should include the reason of the original error.
Comment 2 Jonathan Druart 2019-08-04 15:48:19 UTC
Created attachment 91948 [details] [review]
Bug 23425: Display the MARCXML error in the Metadata::Invalid exception message
Comment 3 Jonathan Druart 2019-08-04 15:49:58 UTC
(In reply to Jonathan Druart from comment #2)
> Created attachment 91948 [details] [review] [review]
> Bug 23425: Display the MARCXML error in the Metadata::Invalid exception
> message

Please apply this patch if you face this problem, then you should get more information about the root of the problem.
Comment 4 Tomás Cohen Arazi 2019-08-04 18:38:20 UTC
(In reply to Jonathan Druart from comment #0)
> If the MARCXML data is wrong for some reasons and so
> MARC::Record::new_from_xml fails, a search will explodes with "invalid data,
> cannot decode object"
> 
> We should:
> - Understand why the data is wrong
> - Provide a maintenance script to catch problematic data
> - Skip the record instead of exploding (or maybe not, it will hide a data
> problem)
> 
> It has been highlighted by bug 22144, was silently failing before those
> patches.

Do you think we should move this one forward into master?
Comment 5 Rudolf Byker 2019-08-05 07:51:54 UTC
Adding the Marc XML error is great, but there is still no way to fix the marc record from the web interface when this happens. I'm not advocating that the error be hidden (bad), but the "exploding" as you call it is worse. The correct way is to tell the user what went wrong, without breaking the UI, and giving them a way to fix the data in the UI.
Comment 6 Jonathan Druart 2019-08-05 13:11:19 UTC
(In reply to Rudolf Byker from comment #5)
> Adding the Marc XML error is great, but there is still no way to fix the
> marc record from the web interface when this happens. I'm not advocating
> that the error be hidden (bad), but the "exploding" as you call it is worse.
> The correct way is to tell the user what went wrong, without breaking the
> UI, and giving them a way to fix the data in the UI.

I wanted first to understand why the data was wrong (encoding, badly formatted, etc.) That's why I have attached a "debug patch".
Then I will provide a more global fix.
Comment 7 Tomás Cohen Arazi 2019-09-17 14:53:59 UTC
(In reply to Rudolf Byker from comment #5)
> Adding the Marc XML error is great, but there is still no way to fix the
> marc record from the web interface when this happens. I'm not advocating
> that the error be hidden (bad), but the "exploding" as you call it is worse.
> The correct way is to tell the user what went wrong, without breaking the
> UI, and giving them a way to fix the data in the UI.

One thing doesn't exclude the other. We need the information about the nature of the problem in order to provide a fix/workaround/recommendation.

I will try to move this patch into the stable branches, and keep an eye on the feedback.
Comment 8 Tomás Cohen Arazi 2019-09-17 18:09:22 UTC
Created attachment 92906 [details] [review]
Bug 23425: Display the MARCXML error in the Metadata::Invalid exception message

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2019-09-17 18:09:26 UTC
Created attachment 92907 [details] [review]
Bug 23425: (QA follow-up) Keep it generic

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2019-09-20 17:10:47 UTC
Skipping the QA step.

@RM: Feel free to revert the status, I just think this is urgent and very simple.
Comment 11 Martin Renvoize 2019-09-23 09:44:22 UTC
Nice work!

Pushed to master for 19.11.00
Comment 12 Fridolin SOMERS 2019-09-24 06:55:22 UTC
Pushed to 19.05.x for 19.05.04
Comment 13 Lucas Gass 2019-10-17 19:33:11 UTC
missing dependency for 18.11.x, no backport