Skip Menu |
 

This queue is for tickets about the DBIx-Class CPAN distribution.

Report information
The Basics
Id: 99334
Status: open
Priority: 0/
Queue: DBIx-Class

People
Owner: Nobody in particular
Requestors: matthew [...] jenika.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: DBIx::Class::InflateColumn::Datetime bug with SQL Server
Date: Mon, 6 Oct 2014 14:18:23 -0700
To: bug-DBIx-Class [...] rt.cpan.org
From: Matthew McGillis <matthew [...] jenika.com>
Download (untitled) / with headers
text/plain 940b
The following table definition works fine for mysql: package Test::DB::Result::TestTable; __PACKAGE__->load_components(qw/Core InflateColumn::DateTime/); __PACKAGE__->table('testtable'); __PACKAGE__->add_columns('id','attribute'); __PACKAGE__->add_column('date'=>{data_type => 'date'}); __PACKAGE__->set_primary_key('date','id'); The date stored in the database is %Y-%m-%d However when I try to use the same table with SQL Server I get the following error: DBIx::Class::InflateColumn::DateTime::_flate_or_fallback(): Error while inflating '2014-05-23' for date on Test::DB::Result::TestTable=HASH(0x2c281b8): Your datetime does not match your pattern. at .../share/perl5/DBIx/Class/Storage/DBI/MSSQL.pm line 204 at .... When I look in MSSQL.pm it seems to only support: my $datetime_format = '%Y-%m-%d %H:%M:%S.%3N'; # %F %T my $smalldatetime_format = '%Y-%m-%d %H:%M:%S'; The version of DBIx::Class I'm using is 0.082801.
Subject: Re: [rt.cpan.org #99334] DBIx::Class::InflateColumn::Datetime bug with SQL Server
Date: Mon, 06 Oct 2014 23:21:24 +0200
To: bug-DBIx-Class [...] rt.cpan.org
From: Peter Rabbitson <ribasushi [...] cpan.org>
Download (untitled) / with headers
text/plain 1010b
On 10/06/2014 11:18 PM, Matthew McGillis via RT wrote: Show quoted text
> Mon Oct 06 17:18:34 2014: Request 99334 was acted upon. > Transaction: Ticket created by matthew@jenika.com > Queue: DBIx-Class > Subject: DBIx::Class::InflateColumn::Datetime bug with SQL Server > Broken in: (no value) > Severity: (no value) > Owner: Nobody > Requestors: matthew@jenika.com > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=99334 > > > > The following table definition works fine for mysql: > > package Test::DB::Result::TestTable; > > __PACKAGE__->load_components(qw/Core InflateColumn::DateTime/);
^^ these two are reversed. Because Core contains all the "database interaction" logic, the IC::DT part has no chance to fire. Please try with __PACKAGE__->load_components(qw/InflateColumn::DateTime Core/); and let me know if your problem goes away. On a side note - one or two versions down the road this will be a compile time exception as to protect the innocent.
Subject: Re: [rt.cpan.org #99334] DBIx::Class::InflateColumn::Datetime bug with SQL Server
Date: Mon, 6 Oct 2014 14:28:33 -0700
To: bug-DBIx-Class [...] rt.cpan.org
From: Matthew McGillis <matthew [...] jenika.com>
Download (untitled) / with headers
text/plain 241b
Show quoted text
> Please try with > __PACKAGE__->load_components(qw/InflateColumn::DateTime Core/); > and let me know if your problem goes away.
Changing the order of the load_components had no impact on the error message. Still get the same error message.
Subject: Re: [rt.cpan.org #99334] DBIx::Class::InflateColumn::Datetime bug with SQL Server
Date: Mon, 06 Oct 2014 23:44:59 +0200
To: bug-DBIx-Class [...] rt.cpan.org
From: Peter Rabbitson <ribasushi [...] cpan.org>
Download (untitled) / with headers
text/plain 922b
On 10/06/2014 11:28 PM, Matthew McGillis via RT wrote: Show quoted text
> Queue: DBIx-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=99334 > >
>> Please try with >> __PACKAGE__->load_components(qw/InflateColumn::DateTime Core/); >> and let me know if your problem goes away.
> Changing the order of the load_components had no impact on the error message. Still get the same error message.
My apologies I didn't read your message to the end after seeing the initial error. Indeed the current formatter does not seem to support bare YMD (there is no format/parse_date methods registered in the file you already looked at). Could you please submit a patch adding the correct pieces? Normally I would ask you to add a test as well, but in this case the MSSQL tests are so convoluted that I rather shuffle them around a bit when I receive your code patch which works for you in production. Thanks in advance!
Subject: Re: [rt.cpan.org #99334] DBIx::Class::InflateColumn::Datetime bug with SQL Server
Date: Mon, 6 Oct 2014 15:36:19 -0700
To: bug-DBIx-Class [...] rt.cpan.org
From: Matthew McGillis <matthew [...] jenika.com>
Download (untitled) / with headers
text/plain 3.1k
Well I don't have a patch and this is dev code that I'm working on. Having said that I tried a simple test of just replacing: #my $datetime_format = '%Y-%m-%d %H:%M:%S.%3N'; # %F %T my $datetime_format = '%Y-%m-%d'; Not at all what you want in the big picture but just to see if it resolves things for me. The above change then produces other warnings/errors (that I never see in mysql) like: DBIx::Class::Storage::DBI::_gen_sql_bind(): DateTime objects passed to search() are not supported properly (InflateColumn::DateTime formats and settings are not respected.) See "Formatting DateTime objects in queries" in DBIx::Class::Manual::Cookbook. To disable this warning for good set $ENV{DBIC_DT_SEARCH_OK} to true at .... and then DBIx::Class::Storage::DBI::Cursor::next(): DBI Exception: DBD::ODBC::st fetch failed: [FreeTDS][SQL Server]Data truncated (SQL-01004) [for Statement "SELECT me.id, me.attribute, me.date FROM testtable me WHERE ( ( date = ? AND me.id = ? ) )" with ParamValues: 1='2014-05-23T00:00:00', 2='11'] at ... Not sure why things work so nicely and transparently with mysql and not SQL Server. I could fix the above using the information in the Cookbook but I'm not inclined to add wrappers in all my code that has to then also take into account which DB it is using. The beauty of InflateColumn::Datetime with mysql is it just works I never had to worry about the Datetime objects passed to its searches. Just my guess but some place on the mysql side we have more flexible date/time processors so it is accepting more values. On mysql I have never gotten the above object warning so I'm also not clear what is resolving that item transparently for mysql either. If I work through the code to come up with a patch I will let you know but not sure it is a priority I may just change the schema to use datetime values and if that makes everything happy be done with it, although it would not be what is desired just one of those work arounds for now. I have a feeling this won't actually fix everything given the above warnings/errors. But, I will pass along any modification if make any. On Oct 6, 2014, at 2:45 PM, Peter Rabbitson via RT wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=99334 > > > On 10/06/2014 11:28 PM, Matthew McGillis via RT wrote:
>> Queue: DBIx-Class >> Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=99334 > >>
>>> Please try with >>> __PACKAGE__->load_components(qw/InflateColumn::DateTime Core/); >>> and let me know if your problem goes away.
>> Changing the order of the load_components had no impact on the error message. Still get the same error message.
> My apologies I didn't read your message to the end after seeing the > initial error. > > Indeed the current formatter does not seem to support bare YMD (there is > no format/parse_date methods registered in the file you already looked > at). Could you please submit a patch adding the correct pieces? > > Normally I would ask you to add a test as well, but in this case the > MSSQL tests are so convoluted that I rather shuffle them around a bit > when I receive your code patch which works for you in production. > > Thanks in advance!
Subject: Re: [rt.cpan.org #99334] DBIx::Class::InflateColumn::Datetime bug with SQL Server
Date: Mon, 6 Oct 2014 17:45:44 -0700
To: bug-DBIx-Class [...] rt.cpan.org
From: Matthew McGillis <matthew [...] jenika.com>
Did some more digging. This is what I found that is so far working for me in the DBIx::Class code. 1) To fix the error about date format not supported Comment out this in: DBIx::Class::Storage::DBI::MSSQL #__PACKAGE__->datetime_parser_type ( # 'DBIx::Class::Storage::DBI::MSSQL::DateTime::Format' #); The DBIx::Class::Storage::DBI::MSSQL::DateTime::Format doesn't appear to really be different from the default other than it is dummer. 2 To fix the warning message about DateTime object usage in searches. Note: I have never seen this because I'm usually not using the current DBIx:Class code the version of code I'm generally using does not generate this warning. The DBIx::Class 0.082801 code has: if ( ! $ENV{DBIC_DT_SEARCH_OK} and $op eq 'select' and first { length ref $_->[1] and blessed($_->[1]) and $_->[1]->isa('DateTime') } @$bind ) { carp_unique 'DateTime objects passed to search() are not supported ' . 'properly (InflateColumn::DateTime formats and settings are not ' . 'respected.) See "Formatting DateTime objects in queries" in ' . 'DBIx::Class::Manual::Cookbook. To disable this warning for good ' . 'set $ENV{DBIC_DT_SEARCH_OK} to true' } I added this just before the above as a quick fix for better transparent support of DateTime objects in queries I expect someone familiar with the code can do a much better job of this but this seems to work for me (working out a generic api for any InflateColumn seems to me like it would make since): $bind = [ map { if (ref($_->[1]) eq 'DateTime') { if (exists($_->[0]{sqlt_datatype})) { if ($_->[0]{sqlt_datatype} eq 'date') { $_->[1]=$self->datetime_parser->format_date($_->[1]); } elsif ($_->[0]{sqlt_datatype} eq 'datetime') { $_->[1]=$self->datetime_parser->format_datetime($_->[1]); } # add other formats if we support them ?time? maybe. } } $_; } @$bind];
Subject: Re: [rt.cpan.org #99334] DBIx::Class::InflateColumn::Datetime bug with SQL Server
Date: Tue, 07 Oct 2014 09:19:29 +0200
To: bug-DBIx-Class [...] rt.cpan.org
From: Peter Rabbitson <ribasushi [...] cpan.org>
Download (untitled) / with headers
text/plain 4.4k
On 10/07/2014 02:45 AM, Matthew McGillis via RT wrote: Show quoted text
> Queue: DBIx-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=99334 > > > Did some more digging. > > This is what I found that is so far working for me in the DBIx::Class code. > > 1) To fix the error about date format not supported > > Comment out this in: DBIx::Class::Storage::DBI::MSSQL > > #__PACKAGE__->datetime_parser_type ( > # 'DBIx::Class::Storage::DBI::MSSQL::DateTime::Format' > #); > > The DBIx::Class::Storage::DBI::MSSQL::DateTime::Format doesn't appear to really be different from the default other than it is dummer.
This is incorrect. The default formatter has no concept of microseconds - if you tried to execute the test suite with the above change you'd have noticed that t/inflate/datetime_mssql.t started failing. The proper fix is to add a sub format_date { ... } and sub parse_date { ... }with the appropriate code in them. Show quoted text
> > 2 To fix the warning message about DateTime object usage in searches. > > Note: I have never seen this because I'm usually not using the current DBIx:Class code the version of code I'm generally using does not generate this warning. > > The DBIx::Class 0.082801 code has: > > if ( > ! $ENV{DBIC_DT_SEARCH_OK} > and > $op eq 'select' > and > first { > length ref $_->[1] > and > blessed($_->[1]) > and > $_->[1]->isa('DateTime') > } @$bind > ) { > carp_unique 'DateTime objects passed to search() are not supported ' > . 'properly (InflateColumn::DateTime formats and settings are not ' > . 'respected.) See "Formatting DateTime objects in queries" in ' > . 'DBIx::Class::Manual::Cookbook. To disable this warning for good ' > . 'set $ENV{DBIC_DT_SEARCH_OK} to true' > } > > I added this just before the above as a quick fix for better transparent support of DateTime objects in queries I expect someone familiar with the code can do a much better job of this but this seems to work for me (working out a generic api for any InflateColumn seems to me like it would make since): > > $bind = [ map { > if (ref($_->[1]) eq 'DateTime') { > if (exists($_->[0]{sqlt_datatype})) { > if ($_->[0]{sqlt_datatype} eq 'date') { > $_->[1]=$self->datetime_parser->format_date($_->[1]); > } elsif ($_->[0]{sqlt_datatype} eq 'datetime') { > $_->[1]=$self->datetime_parser->format_datetime($_->[1]); > } > # add other formats if we support them ?time? maybe. > } > } > $_; > } @$bind]; >
Let's step back and think about this. There is a rather verbose warning in place, in one of the more widely used modules on CPAN. If there indeed was a way to solve the problem *completely*, a solution (no matter how contrived) would have been put in place instead of this warning. The reason your proposal is not acceptable is that any column_info metadata that drives how exactly the given times are interpreted are not consulted. So now everyone who used the 'timezone' option of IC::DT is going to start getting incorrect values shoved into the database. The fundamental problem stems from the entire InflateColumn infrastructure being misdesigned in a way that makes it not suitable for use in a search() context. In fact this is the oldest open bug in the queue: https://rt.cpan.org/Ticket/Display.html?id=43075. The real answer (which is currently blocked by lack of tuits) is to add an InflateColumn replacement, and use it for row-data-independent inflators (like DT inflation) instead. At the rate of current dev this feature is unlikely to come in before next spring. Note that I am giving you the above verbose explanation not as an "intimidation tool", but simply as a better reason why the second part of your report is not acceptable for mainlining. For the time being the only official way to use DT in search()es remains the one described here: https://metacpan.org/pod/release/RIBASUSHI/DBIx-Class-0.082801/lib/DBIx/Class/Manual/Cookbook.pod#Formatting-DateTime-objects-in-queries Your original issue of lacking a "date" inflator/deflator is however indeed fixable. Or as you noted earlier - switching the datatype to "datetime" is a quick workaround for this (I will still fix the date situation when I get to it, likely next maj, release in a months or so). Cheers and thanks for looking into this ;)
Subject: Re: [rt.cpan.org #99334] DBIx::Class::InflateColumn::Datetime bug with SQL Server
Date: Tue, 7 Oct 2014 09:30:33 -0700
To: bug-DBIx-Class [...] rt.cpan.org
From: Matthew McGillis <matthew [...] jenika.com>
Download (untitled) / with headers
text/plain 941b
Show quoted text
> This is incorrect. The default formatter has no concept of microseconds > - if you tried to execute the test suite with the above change you'd > have noticed that t/inflate/datetime_mssql.t started failing. The proper > fix is to add a sub format_date { ... } and sub parse_date { ... }with > the appropriate code in them.
What I would suggest then is simply copy and paste the DateTime::Format::MySQL information into the definition of DBIx::Class::Storage::DBI::MSSQL::DateTime::Format and adjust its DateTime::Format::Builder to include the missing time format. That way your DBIx::Class::Storage::DBI::MSSQL::DateTIme::Format will be a little smarter than DateTime::Format::MySQL. Show quoted text
> The real answer > (which is currently blocked by lack of tuits) is to add an InflateColumn > replacement, and use it for row-data-independent inflators (like DT > inflation) instead.
Agree the InflateColumn needs rework to support all this.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.