|
Description
Jonathan Druart
2020-04-02 09:17:53 UTC
Created attachment 102277 [details] [review] Bug 25040: Handle incorrect current_timestamp syntax There is an incorrect current_timestamp syntax generated by DBIx::Class::Schema::Loader Caught by the changes made to Koha::Object->store by bug 23463 (especially 9c383aa286fee5a29c6f084873f2eb6644bad64f) 154 elsif ( not defined $self->$col 155 && $columns_info->{$col}->{datetime_undef_if_invalid} ) 156 { 157 # timestamp 158 $self->_result()->set_column($col => $columns_info->{$col}->{default_value}); 159 } If a timestamp not null column does not have a value when store is called, the default value is set. The expected default value is \"current_timestamp"(`git grep current_timestamp Koha/Schema/Result`) but for some reasons DBIx::Class::Schema::Loader sometimes generates "current_timestamp()" (certainly depending on versions of DBIx::Class::Schema::Loader and DBMS MySQL vs MariaDB) It ends up with an error on insert/update: DBD::mysql::st execute failed: Incorrect datetime value: 'current_timestamp()' This should be a temporary patch, the root of the problem should be found and correct fixed. With this patch we want people to continue working with Koha until we investigate. Test plan: Apply this change: @ Koha/Schema/Result/Aqorder.pm:374 @ __PACKAGE__->add_columns( { data_type => "timestamp", datetime_undef_if_invalid => 1, - default_value => \"current_timestamp", + default_value => "current_timestamp()", is_nullable => 0, }, "rrp", and run: use Koha::Acquisition::Orders; use t::lib::TestBuilder; my $builder = t::lib::TestBuilder->new; my $order = $builder->build_object({ class => 'Koha::Acquisition::Orders' }); $order->timestamp(undef)->store; Without this patch you get an error Raising severity, we need that ASAP! This highlights a difference in recent MariaDB versions and I intend to submit a patch to DBIx::Class::Schema::Loader::DBI::mysql to account for it. To summarise, in the mysql drive for Schema::Loader there is an 'eq' match looking for 'current_timestamp' to add in special handling for it. But, MariaDB now returns 'current_timestamp()' from the DESCRIBE call and as such the 'eq' no longer matches and we fallback to just putting in whatever we are returned from the DB call. Created attachment 102339 [details] [review] Bug 25040: ALTERNATE You will need to install Sub::Override to test this. Created attachment 102340 [details] [review] Bug 25040: monkeypatch Schema::Loader for recent MariaDB Recent versions of MariaDB changed the output of 'DESCRIBE' for timestamp columns with defaults from `CURRENT_TIMESTAMP` to `current_timestamp()`. As such the code inside DBIx::Class::Schema::Loader which catches such cases and outputs `\"current_timestamp"` as a sensible cross platform default is missed and this leads of inconsistent class files and bugs with out default lookup code in Koha::Objects. This patch serves as a backport of the code I have submitted upstream such that out developers can continue to use update_dbix_class_files.pl to build their schema classes from the database and regardless of their db server version get a consistently correct output. You will need Sub::Override (libsub-override-perl) after this patch to use update_dbix_class_files. Created attachment 102341 [details] [review] Bug 25040: monkeypatch Schema::Loader for recent MariaDB Recent versions of MariaDB changed the output of 'DESCRIBE' for timestamp columns with defaults from `CURRENT_TIMESTAMP` to `current_timestamp()`. As such the code inside DBIx::Class::Schema::Loader which catches such cases and outputs `\"current_timestamp"` as a sensible cross platform default is missed and this leads of inconsistent class files and bugs with out default lookup code in Koha::Objects. This patch serves as a backport of the code I have submitted upstream such that out developers can continue to use update_dbix_class_files.pl to build their schema classes from the database and regardless of their db server version get a consistently correct output. You will need Sub::Override (libsub-override-perl) after this patch to use update_dbix_class_files. Created attachment 102342 [details] [review] Bug 25040: monkeypatch Schema::Loader for recent MariaDB Recent versions of MariaDB changed the output of 'DESCRIBE' for timestamp columns with defaults from `CURRENT_TIMESTAMP` to `current_timestamp()`. As such the code inside DBIx::Class::Schema::Loader which catches such cases and outputs `\"current_timestamp"` as a sensible cross platform default is missed and this leads of inconsistent class files and bugs with out default lookup code in Koha::Objects. This patch serves as a backport of the code I have submitted upstream such that out developers can continue to use update_dbix_class_files.pl to build their schema classes from the database and regardless of their db server version get a consistently correct output. You will need Sub::Override (libsub-override-perl) after this patch to use update_dbix_class_files. Created attachment 102343 [details] [review] Bug 25040: Add Sub::Override development dependancy This patch adds the Sub::Override dependancy to our cpanfile. Testplan: 0/ Prior to applying this patchset run the update_dbix_class_files.pl script against a very new version of MariaDB - Note that \"current_timestamp2 will get replaced by "current_timestamp()" in the schema classes 1/ Apply the patch and install Sub::Override 2/ Run update_dbix_class_files.pl and note that we now stick to \"current_timestamp" Created attachment 102344 [details] [review] Bug 25040: monkeypatch Schema::Loader for recent MariaDB Recent versions of MariaDB changed the output of 'DESCRIBE' for timestamp columns with defaults from `CURRENT_TIMESTAMP` to `current_timestamp()`. As such the code inside DBIx::Class::Schema::Loader which catches such cases and outputs `\"current_timestamp"` as a sensible cross platform default is missed and this leads of inconsistent class files and bugs with out default lookup code in Koha::Objects. This patch serves as a backport of the code I have submitted upstream such that out developers can continue to use update_dbix_class_files.pl to build their schema classes from the database and regardless of their db server version get a consistently correct output. You will need Sub::Override (libsub-override-perl) after this patch to use update_dbix_class_files. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 102345 [details] [review] Bug 25040: Add Sub::Override development dependancy This patch adds the Sub::Override dependancy to our cpanfile. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> (In reply to Martin Renvoize from comment #9) > Testplan: > > 0/ Prior to applying this patchset run the update_dbix_class_files.pl script > against a very new version of MariaDB - Note that \"current_timestamp2 will > get replaced by "current_timestamp()" in the schema classes > 1/ Apply the patch and install Sub::Override > 2/ Run update_dbix_class_files.pl and note that we now stick to > \"current_timestamp" Note that is not enough for me, I had to modify the structure of a table to make sure the file would have been generated. For instance with patches from bug 24161, that modifies aqorders, @ Koha/Schema/Result/Aqorder.pm:362 @ __PACKAGE__->add_columns( { data_type => "timestamp", datetime_undef_if_invalid => 1, - default_value => "current_timestamp()", + default_value => \"current_timestamp", is_nullable => 0, }, See also commit message from comment 1. Thanks Martin! My tests were wrong. I tested on My8, that's why I needed to apply 24161. Back to the drawing board.. the override isn't working as expected :( Created attachment 102645 [details] [review] Bug 25040: monkeypatch Schema::Loader for recent MariaDB Recent versions of MariaDB changed the output of 'DESCRIBE' for timestamp columns with defaults from `CURRENT_TIMESTAMP` to `current_timestamp()`. As such the code inside DBIx::Class::Schema::Loader which catches such cases and outputs `\"current_timestamp"` as a sensible cross platform default is missed and this leads of inconsistent class files and bugs with out default lookup code in Koha::Objects. This patch serves as a backport of the code I have submitted upstream such that out developers can continue to use update_dbix_class_files.pl to build their schema classes from the database and regardless of their db server version get a consistently correct output. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Subclassing seems to have done the trick.. I just had to delve deeper into the dbic docs to understand how to call my subclass. This time I tested it correctly and it works! Yeah for RTFM.. turned out subclassing was built right in to dbic, u was just doing it wrong the first couple of times I tried. Really glad that final patch is working and it even drops the Sub::Override dependancy I thought we needed to introduce. [U+1F642] S/u/I/.. silly mobile autocorrect Upstream patch is at https://github.com/dbsrgits/dbix-class-schema-loader/pull/23 Created attachment 102696 [details] [review] Bug 25040: monkeypatch Schema::Loader for recent MariaDB Recent versions of MariaDB changed the output of 'DESCRIBE' for timestamp columns with defaults from `CURRENT_TIMESTAMP` to `current_timestamp()`. As such the code inside DBIx::Class::Schema::Loader which catches such cases and outputs `\"current_timestamp"` as a sensible cross platform default is missed and this leads of inconsistent class files and bugs with out default lookup code in Koha::Objects. This patch serves as a backport of the code I have submitted upstream such that out developers can continue to use update_dbix_class_files.pl to build their schema classes from the database and regardless of their db server version get a consistently correct output. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Nice work everyone! Pushed to master for 20.05 backported to 19.11.x for 19.11.06 Not to sure about backporting this one to 19.05.x. When I run the QA tool I get: FAIL Koha/Schema/Loader/mysql.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 OK spelling OK valid Leaving it out of 19.05.x for now. Lucas, it's a POD warning, you can ignore it. |