In DBIx::Class, inflating and deflating ends up with a different value

[skington]

Suppose we have a DBIx::Class column defined as "timestamp without time zone":

  DB<10> x $result->result_source->{_columns}{$column}
0  HASH(0x55f4cd1f5790)
   '_ic_dt_method' => 'timestamp_without_timezone'
   '_inflate_info' => HASH(0x55f4cd1f5f28)
      'deflate' => CODE(0x55f4cd1f5c28)
         -> &DBIx::Class::InflateColumn::DateTime::__ANON__[/opt/perl/5.20.3/lib/site_perl/5.20.3/DBIx/Class/InflateColumn/DateTime.pm:187] in /opt/perl/5.20.3/lib/site_perl/5.20.3/DBIx/Class/InflateColumn/DateTime.pm:182-187
      'inflate' => CODE(0x55f4cd1f56d0)
         -> &DBIx::Class::InflateColumn::DateTime::__ANON__[/opt/perl/5.20.3/lib/site_perl/5.20.3/DBIx/Class/InflateColumn/DateTime.pm:181] in /opt/perl/5.20.3/lib/site_perl/5.20.3/DBIx/Class/InflateColumn/DateTime.pm:169-181
   'data_type' => 'timestamp without time zone'
   'default' => SCALAR(0x55f4cb3afc40)
      -> 'NOW()'
   'retrieve_on_insert' => 1
   'timezone' => 'UTC'

And suppose we decide to provide a "new" column value, which is in fact the exact same as the current value (e.g. because a web form allowed a user to update the table contents, and they changed some form fields but left others untouched).

A couple of misfeatures in DateTime::Format::Pg will end up with the resulting deflated value being different from what was stored in the database, and the column being marked as dirty, even though no actual change would happen. This in turn can cause code to run that checks whether columns should be allowed to be modified, database triggers to fire etc.

If you say

    $result->$column($result->$column)

that ends up saying

DBIx::Class::InflateColumn::set_inflated_column(/opt/perl/5.20.3/lib/site_perl/5.20.3/DBIx/Class/InflateColumn.pm:195):
195:	    $self->set_column($col, $self->_deflated_column($col, $value));    

and deep in the guts of DBIx::Class we end up with this:

DBIx::Class::InflateColumn::DateTime::_flate_or_fallback(/opt/perl/5.20.3/lib/site_perl/5.20.3/DBIx/Class/InflateColumn/DateTime.pm:197):
197:	  my $preferred_method = sprintf($method_fmt, $info->{ _ic_dt_method });
198:	  my $method = $parser->can($preferred_method) || sprintf($method_fmt, 'datetime');

Unfortunately, because of a typo or misunderstanding, the right method isn't called:

  DB<13> x $preferred_method
0  'format_timestamp_without_timezone'
  DB<14> x $method
0  'format_datetime'
  DB<15> x $parser->can($preferred_method)
0  undef
  DB<16> x $parser->can('format_timestamp_without_time_zone')
0  CODE(0x55d28b65f7a0)
   -> &DateTime::Format::Pg::format_timestamp in /opt/perl/5.20.3/lib/site_perl/5.20.3/DateTime/Format/Pg.pm:746-759

DBIx::Class is expecting "timezone" with no underscore, but DateTime::Pg's method is time_zone with an underscore.

An additional problem is that for typical datetimes, the following code is used:

DateTime::Format::Pg::format_timestamptz(/opt/perl/5.20.3/lib/site_perl/5.20.3/DateTime/Format/Pg.pm:794):
794:	    return $dt->ymd('-').' '.$dt->hms(':').
795:	      _format_fractional($dt).
796:	      _format_time_zone($dt);

and _format_fractional returns nanoseconds:

668:	  my $ns = shift->nanosecond;
669:	  return $ns ? sprintf(".%09d", "$ns") : ''

As a result of all this, the old and new values are subtly different:

DBIx::Class::Row::set_column(/opt/perl/5.20.3/lib/site_perl/5.20.3/DBIx/Class/Row.pm:943):
943:	  $new_value = $self->store_column($column, $new_value);
  DB<20> x $old_value, $new_value
0  '2021-03-09 12:53:54.710061'
1  '2021-03-09 12:53:54.710061000+0000'

Now, in Postgres-land, this is no change at all for a column defined as "timestamp without time zone":

# select column from table where id=2;
          column           
----------------------------
 2021-03-09 12:53:54.710061
(1 row)

# update table set column='2021-03-09 12:53:54.710061000+0000' where id=2;
UPDATE 1
# select column from table where id=2;
          created           
----------------------------
 2021-03-09 12:53:54.710061
(1 row)

So there's no value in DateTime::Pg providing nanoseconds. Microseconds are enough.

[lestrrat]

I haven't been using Perl or DBIx::Class for a loooooooong time. I'm willing to make changes, but I would need to be able to check the results. I realize from the description this might be hard, but can you create a minimal test case?

[skington]

I realize from the description this might be hard, but can you create a minimal test case?

That's going to be hard, given that a naive implementation would start with "first install postgres and DBIx::Class".

At $WORK, our current patch is

sub DateTime::Format::Pg::format_timestamp_without_timezone {
    my ($self, $dt, %params) = @_;
    my $formatted_datetime = $self->format_timestamp_without_time_zone($dt, %params);
    $formatted_datetime =~ s/ (?<= [.] \d{6} ) ( \d{3} ) $//x;
    return $formatted_datetime;
};

but that's just saying "remove the time zone and the last three digits of the nanoseconds (to turn them into microseconds)", it's nothing like you'd want to see in a test.

The simplest one to test is "Postgres documents that it stores times at microsecond resolution, so don't output nanoseconds". I see there are some other open issues relating to nanoseconds as well. As for the method name, it might be simplest to just test that both names work, and leave untested that, as a result, DBIx::Class will call the correct method name.

I didn't know how responsive you'd be to issues, so I just documented the problem I found and left it at that, but I'm happy to work out a proper pull request if it's likely that there'd be a revised version on the CPAN sometime soon.

[lestrrat]

@skington re: nanosecond issues, I merged a PR and at least the outstanding issues went away.

re: names, it's been that way since the very beginning, before I inherited this module, so well, let me just express my "meh" for DBIx::Class just assuming things. Anyways, I initially was unsure if a rename was a good thing, but I have been working on other languages for too long, I forgot you could easily create aliases in Perl. Let me do that.

re: being responsive. you are right to be worried, as I only found this issue by chance. (I don't understand why Github doesn't notify me anywhere prominent)

[lestrrat]

Okay, so https://github.com/lestrrat-p5/DateTime-Format-Pg/tree/topic/timezone_suffix has a simple alias for parse_timestamp_with_timezone.

For the nanosecond issue, I think that's something you can create a test case for, so I skipped it for the time being (I didn't check, but perhaps it got fixed by #17).

Please check if this works, and let me know

[skington]

Too late to check anything tonight, but I'll have a look over the next few days. Thanks!