The invalid data must be fixed. We provided a feedback on the normal view and a check in the search_for_inconsistencies.pl maintenance script. Not sure we should handle such data issues everywhere in our code.

IMO, software should be stable, regardless of input data. Refusing such data is one thing, but falling over is quite another. Moreover, these data issues occur during normal use when importing MARC records via Z39.50.

How can this bug be reproduced?

Owen, did you read Bug #23425 ? This is the same as that, but for different pages.

It boils down to this: When "MARC::Record::new_from_xml" fails (this happens e.g. when the XML file contains an incorrectly encoded character, or when a record containing such a character is imported from a Z39.50 server), it should be handled gracefully instead of crashing Koha, thereby allowing the user to fix the invalid data.

So to reproduce:
- Get a MarcXML file with at least one incorrectly encoded character.
- Do something that causes that file to be sent to MARC::Record::new_from_xml

In CS, this is known simply as "Robustness". This is also why many programming languages have the pattern of "checked exceptions", which forces you to either handle an error/exception or re-throw it to be handled elsewhere. I'm not a Perl programmer, so I'm not sure what the Perl equivalent of that would be.

An alternative to error handling is to prevent the error from happening at all. Some text parsers allow you to replace incorrectly encoded characters by a predefined string, like "?", thereby silencing any encoding errors in the input. Whether this is acceptable, is a decision that depends on the use case.

I understand your concern Rudolf, but dealing with that exception everywhere in the code is painful.
What we proposed with bug 23425 and bug 23846 is to display the "normal" detail page with a warning, and offer a script to catch such problematic data.

You can still edit (Button Edit > Edit record from the detail page) it and save, that will fix the encoding issue.

Another alternative would be to make sure data can be properly decoded before storing it in the database. Then you only have to handle the errors / check the exceptions in places where MARC records enter the Koha system (e.g. bulk imports, Z39.50, maybe there are others). With this approach, you can consider any decoding exception for MARC data from the DB to be "unchecked", since it should not happen unless there is a bug.

(In reply to Rudolf Byker from comment #2)
> IMO, software should be stable, regardless of input data. Refusing such data
> is one thing, but falling over is quite another. Moreover, these data issues
> occur during normal use when importing MARC records via Z39.50.

I agree. I've often been frustrated by this as well. We have a habit of not being thorough enough with data validation and error-handling in Koha.

(In reply to Rudolf Byker from comment #6)
> Another alternative would be to make sure data can be properly decoded
> before storing it in the database. Then you only have to handle the errors /
> check the exceptions in places where MARC records enter the Koha system
> (e.g. bulk imports, Z39.50, maybe there are others). With this approach, you
> can consider any decoding exception for MARC data from the DB to be
> "unchecked", since it should not happen unless there is a bug.

One downside of this is that it doesn't account for third-party tools that work directly with the database rather than using Koha's import mechanisms.

So I think we should require error-handling everywhere. As Jonathan noted, it is painful (in our current architecture), but I don't think that's a reason not to do it.

(In reply to David Cook from comment #8)
> So I think we should require error-handling everywhere. As Jonathan noted,
> it is painful (in our current architecture), but I don't think that's a
> reason not to do it.

If we used more of a MVC model, it would be easier to do high-level error-handling, as we could just add a general try/catch in the controllers, and throw a 500 error or something instead with a message like "Sorry, we had trouble loading this page. Please do X, Y, Z." But... I don't see us re-architecting any time soon.

Trying to wrap all existing CGI scripts in a try/catch like that would be a nightmare too though, since there are so many CGI scripts.

I suppose one option could be to override the DIE signal via $SIG{__DIE__}, although I don't know what consequences that would have in Plack...

I suspect a "catch all" won't be all that feasible.

(In reply to Rudolf Byker from comment #0)
> Broken MARC records cause "invalid data, cannot decode object" on ISBDdetail
> and MARCdetail pages. 

Actually, could you speak more to this?

1) Is this in dev or production?
2) Is this using Plack/starman alone, or Plack/starman behind Apache, or just Apache?
3) Do you have the environmental variable DEBUG enabled?
4) How did you install Koha? (Via Debian packages?)

This sounds like an error shown by Apache to me.

I'm just looking for some context, because now that I think about it... the web server should just be sending a HTTP 500 status code and a default error document.

It sounds like it's not doing that. That's probably the thing we should be focusing on...

David, I have no idea what you are talking about neither how an answer to your questions can be useful.

Take a look at the 2 dependent bug reports if it's not clear enough.
From bug 23425 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", which is the same problem Rudolf reported here.

(In reply to David Cook from comment #10)
> 1) Is this in dev or production?
Production
> 2) Is this using Plack/starman alone, or Plack/starman behind Apache, or
> just Apache?
Just Apache (I think...)
> 3) Do you have the environmental variable DEBUG enabled?
No (I think...)
> 4) How did you install Koha? (Via Debian packages?)
Yes, from Debian pacakges.

(In reply to David Cook from comment #8)
> One downside of this is that it doesn't account for third-party tools that
> work directly with the database rather than using Koha's import mechanisms.

Wow... is that supported? Because if you support that, you technically have to make the program robust against every possible database issue, including missing tables, wrong tables, missing data, wrong data ... the list is endless. You would have to treat the database as a foreign entity. IMO, properly supporting an "alien" database like this is intractable in almost any software project.

Anyway, I agree that errors should be handled properly everywhere, whether it's painful or not. Some programming languages are just more painful to use than others...

(In reply to Jonathan Druart from comment #11)
> David, I have no idea what you are talking about neither how an answer to
> your questions can be useful.

I love your outspokenness, Jonathan. 

That's OK. When I have some time, I'll explore my own questions, and I'll report back.

(In reply to Rudolf Byker from comment #12)
> (In reply to David Cook from comment #10)
> > 1) Is this in dev or production?
> Production
> > 2) Is this using Plack/starman alone, or Plack/starman behind Apache, or
> > just Apache?
> Just Apache (I think...)
> > 3) Do you have the environmental variable DEBUG enabled?
> No (I think...)
> > 4) How did you install Koha? (Via Debian packages?)
> Yes, from Debian pacakges.

Thanks for the information!

When you got the error, did the top of the web page say "Software error:"? 

If so, it was using Apache's generic error response page and not Koha's custom error response page. 

After some testing, I've noticed that Apache will use Koha's custom more "user-friendly" error response pages for 4xx errors (like 404 and 403), but not for a 500 error.

Actually, I've tried a number of permutations for defining the "ErrorDocument 500" line in the Apache configuration and nothing is working. This is interesting because I know for a fact that I've seen a custom error response page for a 500 error in Koha before...

Aha! If I put "die" in mainpage.pl, Apache will use its generic 500 response. If I remove execute permissions for the mainpage.pl file (chmod -x mainpage.pl), Apache will use the custom Koha 500 response!

--

But... now I understand why Jonathan said my line of inquiry wouldn't be useful. While I have a legitimate line of inquiry regarding the user-friendliness of error pages, it may or may not be what you're looking for. 

For instance, with this bug report, are you looking to get a generic user-friendly web page that says "Sorry, there was an error!" or are you looking for the majority of the page to function normally and to handle the MARC error specifically to say something like "Sorry, we could not parse your MARC record. Please fix the MARC data"?

(In reply to Rudolf Byker from comment #4)
> In CS, this is known simply as "Robustness". This is also why many
> programming languages have the pattern of "checked exceptions", which forces
> you to either handle an error/exception or re-throw it to be handled
> elsewhere. I'm not a Perl programmer, so I'm not sure what the Perl
> equivalent of that would be.
> 

If I understand correctly, "checked exceptions" refer to exceptions that are caught during compile-time, but the error you're encountering is a run-time error. (As far as I know, core Perl doesn't have "checked exceptions", but they might be enabled by third-party modules. Koha is starting to use Exception::Class more, and I think it might have some support for compile-time checks. But really that's irrelevant here.)

In terms of the run-time, it seems that your Koha is being run by Apache executing the Perl scripts as CGI scripts. (The alternative being Apache acting as a reverse proxy for Perl app server Starman which loads the Perl code into its persistently worker processes and executing them.)

The final line in those CGI scripts is to write HTTP headers and parse/output a page based on a Template::Toolkit template. 

If there is a fatal exception which causes the Perl interpreter to die before it reaches the line that produces the HTTP response, then Apache doesn't have a HTTP response to send back to the browser, and it has to send back an error document. I'd have to double-check, but if the CGI script returns a non-zero exit code, I think Apache will always generate a response with a HTTP 500 status code (which makes sense).

Now I'm surprised that Apache is not sending back the custom response located at /cgi-bin/koha/errors/500.pl, as it is configured to do that. Instead it's using its generic response document, which is not user-friendly. I'm going to explore that in a separate bug report.

In any case, in order to handle exceptions in the CGI code, we'd have to wrap all the existing code in a try/catch block. While that's technically doable, there are hundreds and hundreds of CGI scripts, so that would be a massive task. 

Plus, then there is the question of what you do when you catch the exception at that high of a level. I suppose you could send a 500 HTTP status and send a user-friendly message back in a generic error template. 

Of course, there's no saying we'd have to do it for all CGI scripts all at once. We could do it incrementally starting with the ISBDdetail.pl and MARCdetail.pl scripts. 

> An alternative to error handling is to prevent the error from happening at
> all. Some text parsers allow you to replace incorrectly encoded characters
> by a predefined string, like "?", thereby silencing any encoding errors in
> the input. Whether this is acceptable, is a decision that depends on the use
> case.

Yeah, I don't think we handle encoding errors in MARC records very well. This is a pet peeve of mine.

(In reply to Rudolf Byker from comment #13)
> Wow... is that supported? Because if you support that, you technically have
> to make the program robust against every possible database issue, including
> missing tables, wrong tables, missing data, wrong data ... the list is
> endless. You would have to treat the database as a foreign entity. IMO,
> properly supporting an "alien" database like this is intractable in almost
> any software project.

No, it's not "supported", but I'm sure people do it anyway. But then the responsibility for problems lays with the person manually updating the database I'd say rather than the software product.

> Anyway, I agree that errors should be handled properly everywhere, whether
> it's painful or not. Some programming languages are just more painful to use
> than others...

I agree about errors being handled everywhere. However, what does it mean to properly handle errors? I'd say that's context-dependent.

Core Perl's exception handling is painful (ie using of eval{}/if($@)), but there are third-party modules that make it easier. At this point, the main problem is that Koha just isn't consistent in its approach to error-handling, but there are people working on improving that.

Since Koha is over 20 years old and has a lot of CGI scripts, architectural changes tend to be fairly gradual.

Actually, I think I was wrong about the 500 error. It's not a generic Apache response. I think the response is actually coming from the CGI script.

I'm going to explore this more on Bug 26048.

Rudolf, you can probably ignore much of what I've said here, and instead look at Bug 26048.

(In reply to David Cook from comment #15)
> For instance, with this bug report, are you looking to get a generic
> user-friendly web page that says "Sorry, there was an error!" or are you
> looking for the majority of the page to function normally and to handle the
> MARC error specifically to say something like "Sorry, we could not parse
> your MARC record. Please fix the MARC data"?

Definitely the latter! The look and feel of the error page is not what makes in user-unfriendly. It's that fact work stops and an IT support request is generated every time some error occurs that could have (should have?) been handled by Koha.

Just to clarify, it shouldn't be "please fix the MARC data", but rather "here's how to fix it" or even "click this button to try to fix it automatically".

Then again, if you consider the DB as belonging to Koha (we should) and not to anyone who would like to run arbitrary INSERTs and UPDATEs, one could just sanitize the records on import/input and call it a day.

(In reply to David Cook from comment #16)
> If I understand correctly, "checked exceptions" refer to exceptions that are
> caught during compile-time, but the error you're encountering is a run-time
> error. (As far as I know, core Perl doesn't have "checked exceptions", but
> they might be enabled by third-party modules. Koha is starting to use
> Exception::Class more, and I think it might have some support for
> compile-time checks. But really that's irrelevant here.)

No, that's not quite right. Checked exceptions (or the same concept by a similar name) may exist in compiled and interpreted languages. Checking an exception means ensuring that, when a function is called that may throw an exception, that the exception is either caught (and then handled, rethrown, or a mixture of both) or declared in the function's contract. In a compiled language, checked means that the compiler does the checking. In interpreted languages, one would use a linter for that. Moxio has an excellent article on this. They use PHP, but the principle is language-agnostic. https://www.moxio.com/blog/34/best-practices-for-php-exception-handling

Another approach is to do what the GO language does, and some C++ projects compiled with '-fno-exceptions': Don't use exceptions! Whenever something could throw an exception, it should be caught and converted into an error code. Every function in the entire project then returns an error code along with its usual return value. For that, you either need multiple return values (Go, python, PHP, etc) or pass-by-reference (C, C++, PHP, etc). In many languages, this is arguably faster than exception handling. Some people also find it clearer.

It is clearly a problem throughout the Koha code base. Which ever solution is chosen, it should be applied consistently.

> In terms of the run-time, it seems that your Koha is being run by Apache
> executing the Perl scripts as CGI scripts.

Yes, that's right.

I'm not sure what changed in the mean time, but we have been hit by this again.

I just want to update what the situation is for a MARC record with an encoding error:

On the OPAC:

- /cgi-bin/koha/opac-detail.pl : Nicely formatted Koha 500 page, but without any way to click from here to opac-MARCdetail.pl to see what's wrong.
- /cgi-bin/koha/opac-MARCdetail.pl : No error, and we can see that our place of publication was Dp3s�sseldorf instead of Düsseldorf, which is a classic encoding error.

On the intranet:

- /cgi-bin/koha/catalogue/detail.pl : Nicely formatted Koha 500 page, but without any way to click from here to addbiblio.pl to edit and fix the record.
- /cgi-bin/koha/catalogue/MARCdetail.pl : Same problem. So it's worse than on the OPAC, where we could at least see the encoding issue.
- /cgi-bin/koha/cataloguing/addbiblio.pl : No error. Here we can edit the record and fix the problem.

One has to look in `/var/log/koha/rsc/intranet-error.log` to see what went wrong. This is fine, since the OPAC is public, and we don't want to leak server errors to the public. Even the intranet is normally not used by developers, so not showing the error details there is also fine. The error is:

[Tue Jan 25 12:23:21.043483 2022] [cgi:error] [pid 7997] [client 10.248.254.20:46348] AH01215: Invalid data, cannot decode metadata object (biblio_metadata.id=23549, biblionumber=30739, format=marcxml, schema=MARC21, decoding_error=':31: parser error : PCDATA invalid Char value 27: /usr/share/koha/intranet/cgi-bin/catalogue/detail.pl, referer: https://staff.library.refstudycentre.com/cgi-bin/koha/catalogue/search.pl

It's a bit hard to explain to our librarians that, if they get stuck on /cgi-bin/koha/opac-detail.pl or /cgi-bin/koha/catalogue/detail.pl that they need to copy the biblio number from the URL, and then manually reconstruct the URL for /cgi-bin/koha/cataloguing/addbiblio.pl?biblio=12345 on the intranet domain, and then fix it there.

*** This bug has been marked as a duplicate of bug 34836 ***

I'm not sure this is related to bug 34836.
34836 is fixing an issue introduced by bug 30846.

I believe the discussion here predates bug 30846.