Bug 25026 - RaiseError must be set for the UI
Summary: RaiseError must be set for the UI
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 22001
Blocks: 25078
  Show dependency treegraph
 
Reported: 2020-03-31 14:11 UTC by Jonathan Druart
Modified: 2021-02-19 14:02 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Turning on RaiseError for the UI will propagate the SQL errors to the UI. That will prevent errors to silently fail and let developers catch issues that could exist in our codebase.
Version(s) released in:
21.05.00


Attachments
Bug 25026: Turn on RaiseError (2.55 KB, patch)
2020-11-30 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25026: Turn on RaiseError (2.61 KB, patch)
2020-12-02 16:54 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25026: Use HandleError to not raise or print error (2.32 KB, patch)
2020-12-03 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25026: Turn on RaiseError (2.67 KB, patch)
2021-02-02 11:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25026: Use HandleError to not raise or print error (2.39 KB, patch)
2021-02-02 11:19 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-03-31 14:11:14 UTC
This is a follow-up for bug 22001.

The patch "Bug 22001: Set unsafe only if RaiseError is not set" proved that unsafe => 1 and RaiseError => 0 was wrong.
We should move forward and turn RaiseError for the UI, and... see what's happening. The change goes to the right direction, but we need to know how big is the task to have a stable release with these parameters correctly set.
Comment 1 David Cook 2020-11-03 00:41:37 UTC
Yes! I am 100% in favour of turning on RaiseError for the UI. 

For what it's worth, I don't think that it will be a huge drama. 

When running on openSUSE, we have turned on RaiseError on the staff interface for many years now, and while there is an occasional fatal software error, which I try to clean up and upstream, for the most part it's quite stable.
Comment 2 David Cook 2020-11-03 00:52:15 UTC
(In reply to David Cook from comment #1)
> Yes! I am 100% in favour of turning on RaiseError for the UI. 
> 
> For what it's worth, I don't think that it will be a huge drama. 
> 
> When running on openSUSE, we have turned on RaiseError on the staff
> interface for many years now, and while there is an occasional fatal
> software error, which I try to clean up and upstream, for the most part it's
> quite stable.

Plus, we could look at moving forward on Bug 26048, so that any errors in the UI are much more user friendly.
Comment 3 David Cook 2020-11-03 04:35:18 UTC
Note that Bug 26853 is an example where RaiseError would be really helpful because it would help surface edge cases that otherwise fail with silent errors.
Comment 4 Jonathan Druart 2020-11-30 14:15:32 UTC
It would be nice to have bug 26048 before we turn this on.
Comment 5 Jonathan Druart 2020-11-30 14:34:04 UTC
Created attachment 114059 [details] [review]
Bug 25026: Turn on RaiseError

This is a follow-up for bug 22001.

The patch "Bug 22001: Set unsafe only if RaiseError is not set" proved that
unsafe => 1 and RaiseError => 0 was wrong.
We should move forward and turn RaiseError for the UI, and... see what's happening.
The change goes to the right direction, but we need to know how big is the task
to have a stable release with these parameters correctly set.

We will have a lot of fun once this patch is pushed! :)

Test plan:
0. Does not apply the patch
1. Add, somewhere in mainpage.pl
  C4::Acquisition::NewBasket();
2. restart_all
3. Hit the mainpage
=> Hum... nothing happens! However there is an error in the logs
 DBI Exception: DBD::mysql::db do failed: Column 'booksellerid' cannot be null
No basket has been created!
4. Apply the patch, restart_all
5. Hit the mainpage
=> Here we go, there was something wrong and the user is aware of that!
Comment 6 David Cook 2020-12-01 01:29:10 UTC
(In reply to Jonathan Druart from comment #4)
> It would be nice to have bug 26048 before we turn this on.

Very true
Comment 7 Jonathan Druart 2020-12-01 08:50:39 UTC
diff --git a/populate_db.pl b/populate_db.pl
index 097bbda..7d07aa6 100755
--- a/populate_db.pl
+++ b/populate_db.pl
@@ -92,8 +92,8 @@ if (     $marcflavour ne 'MARC21'
 
 my $dbh = C4::Context->dbh; # At the beginning to die if DB does not exist.
 
-my ( $prefs_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM systempreferences|);
-my ( $patrons_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM borrowers|);
+my ( $prefs_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM systempreferences|) };
+my ( $patrons_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM borrowers|) };
 if ( $prefs_count or $patrons_count ) {
     die "Database is not empty!";
 }

You will need to apply this to koha-misc4dev first.
Comment 8 Victor Grousset/tuxayo 2020-12-02 16:54:47 UTC
Created attachment 114106 [details] [review]
Bug 25026: Turn on RaiseError

This is a follow-up for bug 22001.

The patch "Bug 22001: Set unsafe only if RaiseError is not set" proved that
unsafe => 1 and RaiseError => 0 was wrong.
We should move forward and turn RaiseError for the UI, and... see what's happening.
The change goes to the right direction, but we need to know how big is the task
to have a stable release with these parameters correctly set.

We will have a lot of fun once this patch is pushed! :)

Test plan:
0. Does not apply the patch
1. Add, somewhere in mainpage.pl
  C4::Acquisition::NewBasket();
2. restart_all
3. Hit the mainpage
=> Hum... nothing happens! However there is an error in the logs
 DBI Exception: DBD::mysql::db do failed: Column 'booksellerid' cannot be null
No basket has been created!
4. Apply the patch, restart_all
5. Hit the mainpage
=> Here we go, there was something wrong and the user is aware of that!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 9 Victor Grousset/tuxayo 2020-12-02 16:58:44 UTC
It works! 💥💥💥

About comment 7 , in koha-testing-docker, I edited misc4dev/populate_db.pl and did reset_all, was that correct?
Comment 10 David Cook 2020-12-03 00:39:01 UTC
(In reply to Jonathan Druart from comment #7)
> diff --git a/populate_db.pl b/populate_db.pl
> index 097bbda..7d07aa6 100755
> --- a/populate_db.pl
> +++ b/populate_db.pl
> @@ -92,8 +92,8 @@ if (     $marcflavour ne 'MARC21'
>  
>  my $dbh = C4::Context->dbh; # At the beginning to die if DB does not exist.
>  
> -my ( $prefs_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> systempreferences|);
> -my ( $patrons_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> borrowers|);
> +my ( $prefs_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> systempreferences|) };
> +my ( $patrons_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> borrowers|) };
>  if ( $prefs_count or $patrons_count ) {
>      die "Database is not empty!";
>  }
> 
> You will need to apply this to koha-misc4dev first.

Can you clarify why we need to do this? Is koha-misc4dev currently broken?
Comment 11 David Cook 2020-12-03 00:39:31 UTC
(In reply to Victor Grousset/tuxayo from comment #9)
> It works! 💥💥💥
> 

This could give me my first opportunity to QA! Looking forward to it...
Comment 12 Jonathan Druart 2020-12-03 09:18:51 UTC
(In reply to David Cook from comment #10)
> (In reply to Jonathan Druart from comment #7)
> > diff --git a/populate_db.pl b/populate_db.pl
> > index 097bbda..7d07aa6 100755
> > --- a/populate_db.pl
> > +++ b/populate_db.pl
> > @@ -92,8 +92,8 @@ if (     $marcflavour ne 'MARC21'
> >  
> >  my $dbh = C4::Context->dbh; # At the beginning to die if DB does not exist.
> >  
> > -my ( $prefs_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> > systempreferences|);
> > -my ( $patrons_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> > borrowers|);
> > +my ( $prefs_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> > systempreferences|) };
> > +my ( $patrons_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> > borrowers|) };
> >  if ( $prefs_count or $patrons_count ) {
> >      die "Database is not empty!";
> >  }
> > 
> > You will need to apply this to koha-misc4dev first.
> 
> Can you clarify why we need to do this? Is koha-misc4dev currently broken?

Hum, good question. I did not spend much time on it, but we should not need it as we have the KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR flag set.
I will investigate a bit more and see if there is not something hidden behind it.
Comment 13 Jonathan Druart 2020-12-03 09:51:10 UTC
Created attachment 114136 [details] [review]
Bug 25026: Use HandleError to not raise or print error

DBIC has its own HandleError, and so it does not work to unset
RaiseError or PrintError if unsafe is not set (what we don't want here).

The idea of this patch is to overwrite the HandleError when we need it:
Either if KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR is set (misc4dev)
Or if the installer is triggered (we don't want to explode if the DB
does not exist).

There is an additional trick, when the installer is completed, we want
to restore the original behaviour, and so a disconnect is made.
Note that during the installer we want to display eventual SQL errors,
that's why we still display the errors.
Comment 14 Jonathan Druart 2020-12-03 09:53:46 UTC
The installer was broken!

This follow-up should do it.
We need to test `reset_all` and the install process. Also hit circ/pendingreserves.pl which should explode when the install is done (see the "trick" in the commit message)

Now the change in misc4dev is:
diff --git a/populate_db.pl b/populate_db.pl
index 097bbda..563cb28 100755
--- a/populate_db.pl
+++ b/populate_db.pl
@@ -92,6 +92,8 @@ if (     $marcflavour ne 'MARC21'
 
 my $dbh = C4::Context->dbh; # At the beginning to die if DB does not exist.
 
+my $HandleError = $dbh->{HandleError};
+$dbh->{HandleError} = sub { return 1 };
 my ( $prefs_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM systempreferences|);
 my ( $patrons_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM borrowers|);
 if ( $prefs_count or $patrons_count ) {


To hide the 2 errors that will be risen when the DB is empty.
Comment 15 David Cook 2020-12-04 02:27:50 UTC
I'm not sure about this change...

Why don't we just set RaiseError and PrintError in Database.pm and then deal with localizations in install.pl and misc4dev? 

For example:

"If you want to temporarily turn RaiseError off (inside a library function that is likely to fail, for example), the recommended way is like this:"
https://metacpan.org/pod/DBI#RaiseError

{
  local $h->{RaiseError};  # localize and turn off for this block
  ...
}

I'd prefer that ideally.
Comment 16 Jonathan Druart 2020-12-04 10:16:03 UTC
See the commit message, it does not work. It is what we are doing currently. Without unsafe, DBIC does not accept RaiseError turned off.

Have a look at DBIx::Class::Storage::DBI->_connect for more information.
Comment 17 David Cook 2020-12-07 00:57:16 UTC
(In reply to Jonathan Druart from comment #16)
> See the commit message, it does not work. It is what we are doing currently.
> Without unsafe, DBIC does not accept RaiseError turned off.
> 
> Have a look at DBIx::Class::Storage::DBI->_connect for more information.

I've read through the commit messages and DBIx::Class::Storage::DBI->_connect but I still don't see why it's necessary. 

It sounds like perhaps more so an issue with populate_db.pl than Koha...
Comment 18 Jonathan Druart 2020-12-11 09:47:58 UTC
The problem also exists for the install, when the tables don't exist yet.

I can try to explain again, it's as simple as: Without unsafe we cannot disable RaiseError (passing RaiseError => 0 to ->connect), because DBIC set its own HandleError (and will raise the error anyway, because it considers it's the right thing to do, unless you passed unsafe => 1).

The idea of this patch is the remove this HandleError and ignore the error in both case:
- KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR is set (to not have errors *displayed* on reset_all, ie. misc4dev/do_all_you_can_do.pl)
- we are hitting the installer for the first time

I have not retested, but I don't think misc4dev could deal with the replacement of the HandleError, $dbh is init at compilation and that's why we have KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR in the BEGIN block.
Even if we could, I don't think we should drop KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR's behaviour without deprecation notice.
Comment 19 Victor Grousset/tuxayo 2021-01-07 19:27:05 UTC
Applied the patch, reset_all => multiple errors
Was another step needed?

root@kohadevbox:kohadevbox$ reset_all
This action will erase all your data. Are you sure? (y/N) y

Running [sudo koha-shell kohadev -p -c 'PERL5LIB=/kohadevbox/koha:/kohadevbox/qa-test-tools perl /kohadevbox/misc4dev/populate_db.pl -v --opac-base-url http://kohadev.myDNSname.org:8080 --intranet-base-url http://kohadev-intra.myDNSname.org:8081 --marcflavour MARC21']...
{UNKNOWN}: DBI Exception: DBD::mysql::db selectrow_array failed: Table 'koha_kohadev.systempreferences' doesn't exist [for Statement "SELECT COUNT(*) FROM systempreferences"]  at /usr/share/perl5/DBIx/Class/Schema.pm line 1118.
	DBIx::Class::Schema::throw_exception(Koha::Schema=HASH(0x556066af02d8), "DBI Exception: DBD::mysql::db selectrow_array failed: Table '"...) called at /usr/share/perl5/DBIx/Class/Storage.pm line 113
	DBIx::Class::Storage::throw_exception(DBIx::Class::Storage::DBI::mysql=HASH(0x5560667c3cc8), "DBI Exception: DBD::mysql::db selectrow_array failed: Table '"...) called at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1501
	DBIx::Class::Storage::DBI::__ANON__("DBD::mysql::db selectrow_array failed: Table 'koha_kohadev.sy"..., DBI::db=HASH(0x556066f57c30), undef) called at /kohadevbox/misc4dev/populate_db.pl line 95

Restarting Apache httpd web server: apache2.
Restarting Koha ILS: koha-commonStopping Plack daemon for kohadev:.
Stopping Z39.50/SRU daemon for kohadev:.
Stopping Koha worker daemon for kohadev:.
Stopping Koha indexing daemon for kohadev:.
Starting Plack daemon for kohadev:.
Starting Z39.50/SRU daemon for kohadev:DBD::mysql::st execute failed: Table 'koha_kohadev.systempreferences' doesn't exist [for Statement "SELECT `me`.`variable`, `me`.`value`, `me`.`options`, `me`.`explanation`, `me`.`type` FROM `systempreferences` `me` WHERE ( `me`.`variable` = ? )" with ParamValues: 0='casauthentication'] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::st execute failed: Table 'koha_kohadev.systempreferences' doesn't exist [for Statement "SELECT `me`.`variable`, `me`.`value`, `me`.`options`, `me`.`explanation`, `me`.`type` FROM `systempreferences` `me` WHERE ( `me`.`variable` = ? )" with ParamValues: 0='caslogout'] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::st execute failed: Table 'koha_kohadev.systempreferences' doesn't exist [for Statement "SELECT `me`.`variable`, `me`.`value`, `me`.`options`, `me`.`explanation`, `me`.`type` FROM `systempreferences` `me` WHERE ( `me`.`variable` = ? )" with ParamValues: 0='casserverurl'] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::db selectall_arrayref failed: Table 'koha_kohadev.marc_subfield_structure' doesn't exist [for Statement "
        SELECT *
        FROM marc_subfield_structure
        WHERE frameworkcode = ?
        AND kohafield > ''
        ORDER BY frameworkcode,tagfield,tagsubfield
    "] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::st execute failed: Table 'koha_kohadev.authorised_values' doesn't exist [for Statement "
        SELECT DISTINCT av.*
        FROM authorised_values av
     WHERE category = ? ORDER BY category, lib, lib_opac" with ParamValues: 0='Z3950_STATUS'] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::st fetchrow_hashref failed: fetch() without execute() [for Statement "
        SELECT DISTINCT av.*
        FROM authorised_values av
     WHERE category = ? ORDER BY category, lib, lib_opac" with ParamValues: 0='Z3950_STATUS'] at /kohadevbox/koha/Koha/Database.pm line 124.
.
Starting Koha worker daemon for kohadev:.
Starting Koha indexing daemon for kohadev:.
Comment 20 Jonathan Druart 2021-01-08 09:31:14 UTC
(In reply to Victor Grousset/tuxayo from comment #19)
> Applied the patch, reset_all => multiple errors
> Was another step needed?

Yes, see comment 14.
Comment 21 David Cook 2021-01-13 22:31:26 UTC
(In reply to Jonathan Druart from comment #20)
> (In reply to Victor Grousset/tuxayo from comment #19)
> > Applied the patch, reset_all => multiple errors
> > Was another step needed?
> 
> Yes, see comment 14.

Why don't we just use the first patch here and then update populate_db.pl to trap the fatal errors like in comment 7?

populate_db.pl could be updated before even testing this patch.

Am I missing something?
Comment 22 Jonathan Druart 2021-01-14 13:43:48 UTC
(In reply to David Cook from comment #21)
> (In reply to Jonathan Druart from comment #20)
> > (In reply to Victor Grousset/tuxayo from comment #19)
> > > Applied the patch, reset_all => multiple errors
> > > Was another step needed?
> > 
> > Yes, see comment 14.
> 
> Why don't we just use the first patch here and then update populate_db.pl to
> trap the fatal errors like in comment 7?

I am not sure I understand exactly what you mean. If you don't remove/disable the error handler when the dbh is created, you will have to eval all the queries executed before the installer will create the DB.

> populate_db.pl could be updated before even testing this patch.

Yes, maybe, but I wanted it to be approved before modifying it.
Comment 23 David Cook 2021-01-14 23:43:50 UTC
(In reply to Jonathan Druart from comment #22)
> 
> I am not sure I understand exactly what you mean. If you don't
> remove/disable the error handler when the dbh is created, you will have to
> eval all the queries executed before the installer will create the DB.
> 

Yeah, that's what I meant. I think that's a reasonable thing to do. 

> > populate_db.pl could be updated before even testing this patch.
> 
> Yes, maybe, but I wanted it to be approved before modifying it.

That's understandable too. It just makes it less pleasant to test here. Not the end of the world though.
Comment 24 Jonathan Druart 2021-01-15 09:00:05 UTC
(In reply to David Cook from comment #23)
> (In reply to Jonathan Druart from comment #22)
> > 
> > I am not sure I understand exactly what you mean. If you don't
> > remove/disable the error handler when the dbh is created, you will have to
> > eval all the queries executed before the installer will create the DB.
> > 
> 
> Yeah, that's what I meant. I think that's a reasonable thing to do. 

I don't think so. It seems much more sensible to me to know it and do what's needed when the dbh is created.

It's also following the current behaviour.
Comment 25 Martin Renvoize 2021-02-02 11:19:56 UTC
Created attachment 116206 [details] [review]
Bug 25026: Turn on RaiseError

This is a follow-up for bug 22001.

The patch "Bug 22001: Set unsafe only if RaiseError is not set" proved that
unsafe => 1 and RaiseError => 0 was wrong.
We should move forward and turn RaiseError for the UI, and... see what's happening.
The change goes to the right direction, but we need to know how big is the task
to have a stable release with these parameters correctly set.

We will have a lot of fun once this patch is pushed! :)

Test plan:
0. Does not apply the patch
1. Add, somewhere in mainpage.pl
  C4::Acquisition::NewBasket();
2. restart_all
3. Hit the mainpage
=> Hum... nothing happens! However there is an error in the logs
 DBI Exception: DBD::mysql::db do failed: Column 'booksellerid' cannot be null
No basket has been created!
4. Apply the patch, restart_all
5. Hit the mainpage
=> Here we go, there was something wrong and the user is aware of that!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2021-02-02 11:19:59 UTC
Created attachment 116207 [details] [review]
Bug 25026: Use HandleError to not raise or print error

DBIC has its own HandleError, and so it does not work to unset
RaiseError or PrintError if unsafe is not set (what we don't want here).

The idea of this patch is to overwrite the HandleError when we need it:
Either if KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR is set (misc4dev)
Or if the installer is triggered (we don't want to explode if the DB
does not exist).

There is an additional trick, when the installer is completed, we want
to restore the original behaviour, and so a disconnect is made.
Note that during the installer we want to display eventual SQL errors,
that's why we still display the errors.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2021-02-02 11:21:08 UTC
This is a positive change to move us in the right direction for catching errors.

I'm sure there will be a little fallout, but I'm also confident fixes will come quickly.

As such.. I'm happy to PQA on this.. it works as I expect and is a positive move in the right direction.
Comment 28 Martin Renvoize 2021-02-02 11:56:19 UTC
Humm.. that second patch isn't working the way I expected actually.. the errors are causing script death in the installer
Comment 29 Jonathan Druart 2021-02-02 13:50:13 UTC
(In reply to Martin Renvoize from comment #28)
> Humm.. that second patch isn't working the way I expected actually.. the
> errors are causing script death in the installer

If it's about 0000-00-00 dates, then see bug 26997. We dependent on it.
Comment 30 Jonathan Druart 2021-02-02 13:50:24 UTC
(In reply to Jonathan Druart from comment #29)
> (In reply to Martin Renvoize from comment #28)
> > Humm.. that second patch isn't working the way I expected actually.. the
> > errors are causing script death in the installer
> 
> If it's about 0000-00-00 dates, then see bug 26997. We dependent on it.

* We can depend on it
Comment 31 Jonathan Druart 2021-02-09 11:15:39 UTC
Skipping QA as we must push this ASAP.
Comment 32 Jonathan Druart 2021-02-09 11:18:03 UTC
Pushed to master for 21.05.
Comment 33 Fridolin Somers 2021-02-12 14:52:58 UTC
Do you think this can be backported to 20.11.x ?
Comment 34 Jonathan Druart 2021-02-12 14:57:20 UTC
(In reply to Fridolin Somers from comment #33)
> Do you think this can be backported to 20.11.x ?

No, absolutely not! :)
Comment 35 Fridolin Somers 2021-02-19 14:02:32 UTC
(In reply to Jonathan Druart from comment #34)
> (In reply to Fridolin Somers from comment #33)
> > Do you think this can be backported to 20.11.x ?
> 
> No, absolutely not! :)

OK.
Enhancement not pushed to 20.11.x