Bug 25040 - Problematic current_timestamp syntax generated by DBIx::Class::Schema::Loader
Summary: Problematic current_timestamp syntax generated by DBIx::Class::Schema::Loader
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low blocker (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL: https://github.com/dbsrgits/dbix-clas...
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-02 09:17 UTC by Jonathan Druart
Modified: 2020-11-30 21:46 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06


Attachments
Bug 25040: Handle incorrect current_timestamp syntax (2.67 KB, patch)
2020-04-02 09:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25040: ALTERNATE (2.13 KB, patch)
2020-04-03 10:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25040: monkeypatch Schema::Loader for recent MariaDB (2.87 KB, patch)
2020-04-03 10:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25040: monkeypatch Schema::Loader for recent MariaDB (2.70 KB, patch)
2020-04-03 11:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25040: monkeypatch Schema::Loader for recent MariaDB (2.70 KB, patch)
2020-04-03 11:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25040: Add Sub::Override development dependancy (780 bytes, patch)
2020-04-03 11:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25040: monkeypatch Schema::Loader for recent MariaDB (2.77 KB, patch)
2020-04-03 11:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25040: Add Sub::Override development dependancy (854 bytes, patch)
2020-04-03 11:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25040: monkeypatch Schema::Loader for recent MariaDB (4.08 KB, patch)
2020-04-09 15:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25040: monkeypatch Schema::Loader for recent MariaDB (4.15 KB, patch)
2020-04-10 10:36 UTC, Nick Clemens
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-04-02 09:17:53 UTC
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()'
Comment 1 Jonathan Druart 2020-04-02 09:22:06 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
Comment 2 Jonathan Druart 2020-04-02 15:58:04 UTC
Raising severity, we need that ASAP!
Comment 3 Martin Renvoize 2020-04-03 07:28:17 UTC
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.
Comment 4 Martin Renvoize 2020-04-03 10:17:59 UTC
Created attachment 102339 [details] [review]
Bug 25040: ALTERNATE

You will need to install Sub::Override to test this.
Comment 5 Martin Renvoize 2020-04-03 10:38:52 UTC
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.
Comment 6 Martin Renvoize 2020-04-03 11:10:07 UTC
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.
Comment 7 Martin Renvoize 2020-04-03 11:24:21 UTC
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.
Comment 8 Martin Renvoize 2020-04-03 11:24:24 UTC
Created attachment 102343 [details] [review]
Bug 25040: Add Sub::Override development dependancy

This patch adds the Sub::Override dependancy to our cpanfile.
Comment 9 Martin Renvoize 2020-04-03 11:43:12 UTC
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"
Comment 10 Jonathan Druart 2020-04-03 11:55:53 UTC
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>
Comment 11 Jonathan Druart 2020-04-03 11:55:58 UTC
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>
Comment 12 Jonathan Druart 2020-04-03 11:57:45 UTC
(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!
Comment 13 Jonathan Druart 2020-04-07 10:28:32 UTC
My tests were wrong. I tested on My8, that's why I needed to apply 24161.
Comment 14 Martin Renvoize 2020-04-07 12:56:48 UTC
Back to the drawing board.. the override isn't working as expected :(
Comment 15 Martin Renvoize 2020-04-09 15:14:00 UTC
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>
Comment 16 Martin Renvoize 2020-04-09 15:15:11 UTC
Subclassing seems to have done the trick.. I just had to delve deeper into the dbic docs to understand how to call my subclass.
Comment 17 Jonathan Druart 2020-04-10 08:54:08 UTC
This time I tested it correctly and it works!
Comment 18 Martin Renvoize 2020-04-10 09:39:03 UTC
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. 🙂
Comment 19 Martin Renvoize 2020-04-10 09:39:48 UTC
S/u/I/.. silly mobile autocorrect
Comment 20 Jonathan Druart 2020-04-10 09:58:40 UTC
Upstream patch is at https://github.com/dbsrgits/dbix-class-schema-loader/pull/23
Comment 21 Nick Clemens 2020-04-10 10:36:49 UTC
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>
Comment 22 Martin Renvoize 2020-04-14 07:37:25 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 23 Joy Nelson 2020-05-04 21:39:41 UTC
backported to 19.11.x for 19.11.06
Comment 24 Lucas Gass 2020-05-13 21:30:36 UTC
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.
Comment 25 Jonathan Druart 2020-05-14 09:18:25 UTC
Lucas, it's a POD warning, you can ignore it.