DateTime::Duration::in_units('days') should not return completely wrong answers for durations over 1 month

[deven]

Currently, DateTime::Duration::in_units('days') will only return the days portion of the DateTime::Duration object, completely ignoring the number of months that may also be stored. Although this behavior is documented, the caveat is very easy to miss. This method should not return completely wrong answers like this.

Here is an example script which demonstrates the problem, and also shows that DateTime::delta_days() will return the correct number of days (albeit losing the negative sign), unlike DateTime::subtract_datetime():

#!/usr/bin/env perl

use strict;
use warnings;

use Data::Dump qw[dump];
use DateTime;

print "\$DateTime::VERSION = $DateTime::VERSION\n\n";

my $date1 = DateTime->new(year => 2022, month => 8, day => 9);
print "\$date1 = \"$date1\"\n\n";

my $date2 = DateTime->new(year => 2022, month => 6, day => 17);
print "\$date2 = \"$date2\"\n\n";

my $duration = $date2 - $date1;
print "\$duration = \$date2 - \$date1 = \$date2->subtract_datetime(\$date1) = " . dump($duration) . "\n\n";

print "\$duration->in_units(\"days\") = " . $duration->in_units('days') . "\n\n";

print "\$date2->delta_days(\$date1)->in_units(\"days\") = " . $date2->delta_days($date1)->in_units("days") . "\n";

Here is the output generated by the above script with DateTime version 1.58:

$DateTime::VERSION = 1.58

$date1 = "2022-08-09T00:00:00"

$date2 = "2022-06-17T00:00:00"

$duration = $date2 - $date1 = $date2->subtract_datetime($date1) = bless({
  days => -22,
  end_of_month => "preserve",
  minutes => 0,
  months => -1,
  nanoseconds => 0,
  seconds => 0,
}, "DateTime::Duration")

$duration->in_units("days") = -22

$date2->delta_days($date1)->in_units("days") = 53

Returning -22 for $duration->in_units("days") when the correct duration is -53 days is clearly wrong. Several better options come to mind:

• Ideally, extend DateTime::Duration to be able to store the total number of days, in addition to the breakdown of months and days, so that this call could correctly return -53. (This obviously can't work in cases where a duration was constructed directly as months and days only, but it would solve the problem for durations created by subtracting DateTime objects.)
• die with an error because this duration is over a month, "months" was not in the list of units passed to in_units(), and there's no way to know the exact number of days which would be correct. This is arguably the right thing to do if the correct value cannot be calculated, but it might cause backwards-compatibility issues.
• return undef for the same reasons, preferably with a warning message generated. This could also cause backwards-compatibility issues, but likely to a lesser degree.
• Make a best-effort guess at converting the months to days, using an approximation of either 30.5 days/month or perhaps 30.4375 days/month ((366 + 365*3) / 12), and rounding to the nearest integer. This might return either -52 or -53 here, which might not be exactly correct in all cases, but it would always be close to the correct answer, and it's much better to return a slightly wrong answer of -52 than a very wrong answer of -22. This should be the most backwards-compatible option.

[deven]

Note: Even if the decision is to die (or generate a warning) in this scenario, the method should still silently return the correct answer if the number of months stored in the DateTime::Duration object is zero, either because it was created by delta_days() or because the duration is less than a month. It certainly shouldn't die or warn if -22 is actually the correct answer!

[n1vux]

i sympathize with the objection,

• i haven't liked this aspect of this interface either;
• it won't return 41 days yet won't object that it's just discarded 1 month.
• It acts like a Remainder function without nomenclature that feels Remainder-ish.

But i'm unclear if any change is safe (even a Carp instead of a Croak), as this arguably buggy behavior may by now be relied upon ?

To really fix this API would probably be majorly backwards-incompatible , so should probably be a major version bump and big warnings in CHANGES and README.md etc ...

I'm fingerpainting that it could be phased

• n+1: Carp if acting as a remainder with "deprecated, will be fatal in next release"
• n+2: Croak when loss of data, as forewarned;
• n+3: returns 41 not 10 or 11 or ... unless ('months', 'days') requested

(This could still seriously break an app that migrated from $n to $n+3 after many years of non-maintenance! Here's hoping they have regression tests )

[deven]

But i'm unclear if any change is safe (even a Carp instead of a Croak), as this arguably buggy behavior may by now be relied upon ?

I can certainly imagine that existing code might be relying on this to return some numeric value, and croaking or returning undef might potentially cause breakage. However, the existing documentation explicitly says that "Smaller units are computed from what remains after taking away the larger units given", which means $dur->in_units('days') should be returning the total number of days, not a remainder of days. Yes, there is a documented caveat: "The last example demonstrates that there will not be any conversion between units which don't have a fixed conversion rate." While this caveat does explain why the current implementation happens to return a remainder of days, that doesn't justify relying on this misbehavior.

To really fix this API would probably be majorly backwards-incompatible , so should probably be a major version bump and big warnings in CHANGES and README.md etc ...

I'm fingerpainting that it could be phased

• n+1: Carp if acting as a remainder with "deprecated, will be fatal in next release"
• n+2: Croak when loss of data, as forewarned;
• n+3: returns 41 not 10 or 11 or ... unless ('months', 'days') requested

(This could still seriously break an app that migrated from $n to $n+3 after many years of non-maintenance! Here's hoping they have regression tests )

This approach is predicated on the presumption that relying on $dur->in_units('days') to return a remainder is a reasonable thing for users to do, just because that's what it happens to return in the current implementation. My assertion is that it is not reasonable for a user to rely on this behavior, because it's contrary to the definition of this API call in the first place. Moreover, it's far more likely that the user actually expected the defined behavior of the API call (not a remainder), so maintaining bug-for-bug compatibility really isn't warranted here. What is warranted is to continue returning a number.

To minimize backwards-compatibility issues, my suggestion would be to alter the definition of this API call to return a "best effort" approximation in circumstances where a fixed conversion rate does not exist, and replace the current caveat with clear documentation of the exact approximation algorithm. For example, the conversion from years to days could be defined as 365.2425 days/year, taking leap days into account, and the conversion from days to seconds could defined as 86,400 seconds/day, explicitly ignoring leap seconds. All the other conversions are obvious: 12 months/year, 7 days/week, etc.

Fractional values of any unit (e.g. partial days) could be converted down into smaller units all the way down to nanoseconds and added to any duration values stored in those smaller units, and then the in_units() API call could either truncate all units smaller than the smallest one requested (which I'm guessing is probably what it does now), or perhaps to convert all smaller units back into a fractional value for the smallest unit actually requested and then explicitly round to the nearest integer.

Ideally, I would also suggest extending the internals of DateTime::Duration to allow the exact correct values to be returned instead of approximations, for durations created by subtracting DateTime objects. Approximations would still be necessary for durations explicitly constructed more generically such as DateTime::Duration->new(months => 1) where the particular dates in question are unknown, but the exact number of total days can always be calculated accurately when subtracting two specific known dates.

[deven]

On a separate note, the fact that DateTime::Duration->new(months => 1, days => 11)->days returns 4 instead of 11 seems counter-intuitive to me. Personally, I would've found it more intuitive if ->days returned 11 and something else like ->wdays returned 4. Obviously, the API is already established this way so it's too late to change this, but perhaps adding a new ->mdays method for abs(($dur->in_units('days', 'months'))[0]) (returning 11 in this case) might be useful?

[autarch]

Thanks for this bug report and the good discussion.

I don't think doing any sort of guesstimation of the return values (like a month is 30.5 days) is on the table. This seems like the most likely thing to break existing code, since it wouldn't even cause a warning, much less an exception.

If we want to make it possible to turn months into days, the right way to do this is by using a DateTime as a base for these calculations. The DateTime::Format::Duration module does this. It might be possible to add some new methods to DateTime::Duration around this. I think we'd want to construct a new type of Duration class, maybe called DateTime::Duration::WithBase:

my $dur = $dt1->delta_md($dt2)->with_base($dt1);

We could even consider adding a set of new methods to DateTime itself:

my $dur = $dt->delta_md_with_base($dt)

This Duration::WithBase could then return simple but correct answers for questions like "how many days does this duration represent?"

As far as other changes like making in_units carp or croak, I'm not too keen on this. There's a lot of code out there using DateTime. I suspect a lot of it is in semi-maintenance mode, where people might upgrade libraries but not pay a ton of attention to warnings coming from it.

Adding new methods and classes is much safer. It fixes the problem for people who care without any risk of breaking existing code.

[autarch]

Adding new methods and classes is much safer. It fixes the problem for people who care without any risk of breaking existing code.

I was thinking about this a bit more and there is one big question with adding a new Duration class. How do the DateTime methods that accept a DateTime::Duration work with this new Duration class?

I think this should just work as long as the new Duration class implements the deltas method and returns something reasonable for it. That said, it's a little weird to pass a Duration with a base DateTime to a method called on a different DateTime. But maybe that's ok?

[n1vux]

little weird to pass a Duration with a base DateTime to a method called on a different DateTime. But maybe that's ok?

That's what one would need to do if one needs to offset other DateTime's by the same amount, so not unreasonable to support.

The new alternative Duration class could either be duck-type compatible (supporting deltas) or you could create an abstract base class for both of them that has a must-implement commitment for deltas , to have an ISA relationship (instead of having to do can deltas for typechecking).

[autarch]

I think for max compatibility we'd want DateTime::Duration::WithBase to subclass DateTime::Duration. That way anything that checks ->isa('DateTime::Duration') will keep working.

[deven]

I don't think doing any sort of guesstimation of the return values (like a month is 30.5 days) is on the table. This seems like the most likely thing to break existing code, since it wouldn't even cause a warning, much less an exception.

You're talking about the risk of breaking existing already-broken code. How would this break the code any worse than it already is? Any code which would be affected by this change is already broken and this would be likely to make it less broken than it already is.

I understand that adding a warning after the fact is potentially detrimental. But why on earth would you want to continue returning 20 as a completely wrong answer just because you can't be certain whether 50 or 51 is the correct answer? Returning either 50 or 51, on any arbitrary basis, is clearly better than returning 20. Such an approximation may not be returning a guaranteed correct answer, but the existing implementation is already returning wrong answers! If the result is subject to potential error anyhow, why not at least minimize that potential error to a day or two, rather than an unlimited error range that could easily be off by months or years?

This Duration::WithBase could then return simple but correct answers for questions like "how many days does this duration represent?"

This is approximately what I had in mind, some sort of duration with knowledge of the actual date(s) involved so that it can return correct answers. Is there a reason it needs to be a different class though? Why not just have different behavior based on whether the reference date(s) have been set on the object?

As far as other changes like making in_units carp or croak, I'm not too keen on this. There's a lot of code out there using DateTime. I suspect a lot of it is in semi-maintenance mode, where people might upgrade libraries but not pay a ton of attention to warnings coming from it.

This is part of the reason I suggested the "best effort" approximation, because any existing code that would be affected is already broken but likely not being properly maintained, and the maintainers are probably not even aware of the issue. Replacing a completely wrong answer with a close approximation is likely to make such code work better, not worse. It's not a perfect solution, but it's a reasonable compromise.

Adding new methods and classes is much safer. It fixes the problem for people who care without any risk of breaking existing code.

What is the value of maintaining bug-for-bug compatibility for code which is already undeniably invalid code according to the existing documentation?

[n1vux]

sigh.

While i get your point that broken code is broken, it's possible someone is using the current broken API with understanding of what it actually does.

Changing the behavior of broken code in production that may have been worked-around is still frowned upon in IT shops.

This isn't like CPR where worrying about breaking ribs isn't a concern since broken ribs is better than dead. (And if you don't break ribs, you might not be doing it right.) Some script in a production job may well be still breathing despite this borked API.

So at a minimum it would need very loud INCOMPATIBLE CHANGE verbiage.

(Since DateTime::Duration is not a dual-life Core module, at least there isn't the dreaded possibility that an OS upgrade slips a new version of the module in with an updated Perl5, for those shops that foolishly use OS /usr/bin/perl for production jobs Noooooo.)

[jhannah-mm]

FWIW, at $dayjob the biggest Perl project I support uses DateTime 714 times, codebase written from 2009-present. Whatever it's been doing since 2009 is Correct(tm) from our perspective since we "worked around" any non-intuitive behavior years ago. Please don't stop us from continually updating perl and DateTime by introducing 714 "bugs" we would need to "fix." :)

[autarch]

I'm pretty sure I already wrote this in some other issue somewhere, but I'll say it again here ...

Sadly, I think that most of the Perl code that will ever be written already exists. So because of that, I think backwards compatibility has a much higher priority than fixing poorly designed APIs (note that fixing bugs is something else entirely). The situation was different 10-20 years ago, when the benefit of a backwards incompatible change could be enjoyed by more people than were upset at the breakage.

[deven]

While i get your point that broken code is broken, it's possible someone is using the current broken API with understanding of what it actually does.

Changing the behavior of broken code in production that may have been worked-around is still frowned upon in IT shops.

I understand where you're coming from, and this viewpoint makes sense in general.

In this particular instance, it seems incredibly unrealistic to me. Do you truly believe there's anyone out there who (1) is aware of the subtle nuance that in_units('days') is actually invalid code, (2) understands that this API call incorrectly returns a month remainder in days instead of a total number of days as documented, (3) wants this behavior for some reason, (4) doesn't want the number of months along with the remainder in days, and (5) intentionally puts code into production using this in_units('days') to get only the month remainder in days, despite understanding that in_units('months', 'days') is the correct way to get that remainder from the API if that's truly what they want? I'd love to meet this hypothetical person!

On the other hand, I opened this ticket because I encountered in_units('days') in real-world production code, written by someone unaware of that subtle nuance, where the intent and expectation was that it was returning the total number of days as documented, not a month remainder in days. Your example is hypothetical; this is an actual real-world example.

[deven]

FWIW, at $dayjob the biggest Perl project I support uses DateTime 714 times, codebase written from 2009-present. Whatever it's been doing since 2009 is Correct(tm) from our perspective since we "worked around" any non-intuitive behavior years ago. Please don't stop us from continually updating perl and DateTime by introducing 714 "bugs" we would need to "fix." :)

714 times? That number is very precise! I'm guessing you actually scanned the codebase to get that count, am I right?

Just for the sake of curiosity, if you wouldn't mind scanning the codebase again, it would be very interesting to hear (1) how many times your codebase actually calls the in_units() method, (2) how many of those calls have "days" as the only argument, and (3) if there are any, does the code actually expect a month remainder in days instead of a total number of days?

[jhannah-mm]

714 times? That number is very precise! I'm guessing you actually scanned the codebase to get that count, am I right?

Yup! ✗ ack DateTime | wc -l

Just for the sake of curiosity, if you wouldn't mind scanning the codebase again, it would be very interesting to hear (1) how many times your codebase actually calls the in_units() method

14

(2) how many of those calls have "days" as the only argument

2

and (3) if there are any, does the code actually expect a month remainder in days instead of a total number of days?

In one case it's doing this:

sub flatten_to_seconds {
  my ($duration) = @_;
  my $years       = $duration->in_units('years');
  my $days        = $duration->in_units('days');
  my $minutes     = $duration->in_units('minutes');
  my $nanoseconds = $duration->in_units('nanoseconds');
  # Ha. DateTime is technically correct, that this CAN'T be calculated.
  # It depends on leap seconds and all kinds of craziness.
  # But we really don't care about those errors for this report.
  # So squash a duration, close enough.
  my $seconds =
    $years * 365 * 24 * 60 * 60 +
    $days * 24 * 60 * 60 +
    $minutes * 60 +
    $nanoseconds / 1000 / 1000000;
  ...

Is that a case where my horrible code has always been broken and I never realized it?

In the other case someone else wrote this in 2012:

my $days_rem = ($dt_final_midnight->delta_days($dt_most_recent_midnight)->in_units("days"));

I don't know, off-hand, whether or not changing the behavior of in_units("days") would "break", "fix", or be irrelevant for that line of code. I'd have to figure out the possible inputs that flow into that in our environment.