Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DateTime from Long.MIN_VALUE or MAX_VALUE #190

Closed
mstevens83 opened this issue Oct 6, 2014 · 15 comments
Closed

DateTime from Long.MIN_VALUE or MAX_VALUE #190

mstevens83 opened this issue Oct 6, 2014 · 15 comments

Comments

@mstevens83
Copy link

In joda-time version 2.3 both this ...

DateTime minDT = new DateTime(Long.MIN_VALUE);

... and this ...

DateTime maxDT = new DateTime(Long.MAX_VALUE);

... worked perfectly fine.

But since version 2.4 (and still in 2.5) either expression causes an ArithmeticException to be thrown:

Exception in thread "main" java.lang.ArithmeticException: Adding time zone offset caused overflow
    at org.joda.time.DateTimeZone.convertUTCToLocal(DateTimeZone.java:965)
    at org.joda.time.chrono.ZonedChronology$ZonedDateTimeField.get(ZonedChronology.java:422)
    at org.joda.time.base.BaseDateTime.<init>(BaseDateTime.java:129)
    at org.joda.time.base.BaseDateTime.<init>(BaseDateTime.java:97)
    at org.joda.time.DateTime.<init>(DateTime.java:209)
    at MinMaxTest.main(MinMaxTest.java:29)
@jodastephen
Copy link
Member

In essence, the code has been fixed to avoid overflowing the date-time. Given that, I'm not sure what the correct answer is here. Accepting those constructors would require adjusting the millisecond instant to be valid, but resulting in an object that did not match the constructor request.

@mstevens83
Copy link
Author

I see.
Well I guess that I always just assumed that because Java tends to use longs to represent time instants (e.g. System.currentTimeMillis()) , any long value would correspond to some valid point in time, and consequently Long.MIN_VALUE and Long.MAX_VALUE would represent very distant (but valid!) instants in the past and future, respectively.

Could you explain why this is not the case? I mean - disregarding the Big Bang (or Genesis, for those who are so inclined), the end of the universe (or the Armageddon, again if so inclined), and possibly Einstein's relativity - any number, no matter how big or small, of milliseconds prior or since a defined instant (i.e. epoch) should be a valid time indication, no?

Either way, if for whatever theoretical, conceptual or technical reason the range of long values that can be used to instantiate a valid DateTime or Instant instance happens to be "narrower" than [Long.MIN_VALUE, Long.MAX_VALUE] then wouldn't it be good if the DateTime and/or Instant classes had static fields which demarcate their actual bounds? For example:

// In the Instant class:
public static final long MIN_INSTANT = ...; // some value >= (but more likely >) Long.MIN_VALUE
public static final long MAX_INSTANT = ...; // some value <= (but more likely <) Long.MAX_VALUE

// In the DateTime class:
public static final DateTime MIN_VALUE = new DateTime(Instant.MIN_INSTANT);
public static final DateTime MAX_VALUE = new DateTime(Instant.MAX_INSTANT);

This would then make it easy for client code to avoid trying to construct DateTime instances from long values that are either too small or too large.
(Please forgive me if something like this already exists, I haven't found it.)

Edit: I do agree with you that making the constructor accept invalid millisecond values but then internally adjusting these (thereby producing an object that does not match the request) would not be a desirable solution. So I'd prefer the ArithmeticException over that, as long as there is a way for client code to know what the bounds for valid millisecond values are.

@mstevens83
Copy link
Author

Any new thoughts on this?

@jodastephen
Copy link
Member

The problem with allowing min/max is that they tend to result in bugs, especially in time-zone handling, where you need to increase/decrease the value by the time-zone offset.

It would be desirable to have known bounds that are tested for. I don't have any immediate plans to work on that, but a well-tested PR with sensible bounds would probably be accepted. See how JSR-310 does it in Java SE 8.

@AmazingDonut
Copy link

I am facing this problem as well. It's hard to adopt this change from a backwards compatibility perspective, since I already have a number of dates and ranges created using Long.MAX_VALUE.

Moreover, I discovered that only the DateTime constructor was affected by this change. For example, it is still possible to create an Interval with Long.MAX_VALUE, which leads to hard-to-detect, possible runtime errors.

Interval bigInterval = new Interval(0,Long.MAX_VALUE);
System.out.println(bigInterval);
System.out.println(bigInterval.getEnd());

Output:

1969-12-31T16:00:00.000-08:00/292278994-08-17T00:12:55.807-07:00
Exception thrown

org.joda.time.IllegalFieldValueException: Value 292278994 for year must be in the range [-292275054,292278993]
    at org.joda.time.field.FieldUtils.verifyValueBounds(FieldUtils.java:234)
    at org.joda.time.chrono.BasicYearDateTimeField.set(BasicYearDateTimeField.java:83)
    at org.joda.time.chrono.ZonedChronology$ZonedDateTimeField.set(ZonedChronology.java:482)
    at org.joda.time.base.BaseDateTime.<init>(BaseDateTime.java:129)
    at org.joda.time.DateTime.<init>(DateTime.java:236)
    at org.joda.time.base.AbstractInterval.getEnd(AbstractInterval.java:83)
    at org.joda.time.ReadableInterval$getEnd.call(Unknown Source)

@jodastephen
Copy link
Member

See issue #100

@jodastephen
Copy link
Member

  • I've tried capping the input at a safe value, but that causes errors all over the place.
  • I've tried extending the range of valid years, but that causes even more errors.
  • It isn't possible to add a fixed maximum valid instant because it varies by timezone.
  • I could remove the validation that was added, but that would open up a class of bug.

TLDR, no solution likely here

@mstevens83
Copy link
Author

Thanks for looking into this some more Stephen.
But what about the approach used in JSR-310/Java SE 8?
Are you sure there's no pair of (possibly "tighter") instant bounds that would work regardless of the time zone?

@jodastephen
Copy link
Member

Doing so may be possible but it would be a lot of work. It would also break compatibility with even more users. My opinion is that the only acceptable solution would be to make Long.MAX_VALUE work, however doing so looks like an equally futile task, and certainly not one I've got time for!

@AmazingDonut
Copy link

I understand your reasoning, Stephen. What's your opinion on the Interval constructor issue I reported above?

@jodastephen
Copy link
Member

Perhaps Interval should be made to match. It is certainly logical. Although as shown here, each fix can cause problems elsewhere!

@mstevens83
Copy link
Author

Do you have any new thoughts on this? This problem means I've been suck at v2.3 for a long time.
This closely relates to #297 as well.

@jodastephen
Copy link
Member

Try the min/max branch. I won't merge the changes unless I hear they are suitable.

@mstevens83
Copy link
Author

Hi Stephen,

Thanks for spending time on this!
These changes satisfy my needs, and I agree with you that it is a sensible compromise to default to UTC when handling Long.MAX_VALUE and Long.MIN_VALUE value, even when a non-UTC DateTimeZone was passed to the DateTime constructor.

I've been running a bunch of tests (I've made my own, not relying on the min/max tests I saw you added in 295f43e).
My tests covered:

All these tests passed. So if you ask me the min/max branch is ready to be merged.

By the way, when do you think we could expect a new Joda-time release with these changes? Oh and would that release also include tzdata 2015g which was released earlier this month?

Best & thanks again!

Matthias

@jodastephen
Copy link
Member

Thanks for testing. When a new release occurs it will include the latest time-zone data.

mstevens83 referenced this issue in ExCiteS/Sapelli Oct 27, 2015
…E/MIN_VALUE issue is finally fixed)

Signed-off-by: Matthias Stevens <matthias.stevens@gmail.com>
jodastephen added a commit that referenced this issue Nov 12, 2015
Bug introduced by min/max changes #190
Fixes #328
Guardiola31337 pushed a commit to Guardiola31337/joda-time that referenced this issue Feb 8, 2016
Provide some form of support for date-times up to the min/max of Long
At the margins, calculations will be slightly imperfect
UTC is used for the min/max
Fixes JodaOrg#297
Fixes JodaOrg#190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants