Skip to content
New issue

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

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

Already on GitHub? # to your account

Add bundled signature of unsafe (default time zone) APIs of Joda-Time library #121

Open
leventov opened this issue Jun 9, 2017 · 7 comments

Comments

@leventov
Copy link

leventov commented Jun 9, 2017

No description provided.

@uschindler
Copy link
Member

We did this for Java 8's java.time API. I think signatures should look similar. But as with ICU4J, it is hard to maintain the signatures for those ever-changing libraries. It would be better to have them on Maven central separately and just use Maven coordinates to refer to them from builds. It is possible with forbiddenapis to host signatures on Maven Central, it is just somebody needed who maintains them.

I am looking forward to remove commons-io signatures for the same reason, but I have no plan yet.

@hakanai
Copy link

hakanai commented Jul 20, 2017

We use Joda Time as well, and have our own list of these already. Here's what we've found so far. :)

Note that Joda Time also has a really insidious date formatting API where you build the formatter by calling withZone, withLocale, etc., a misuse which is impossible to detect using forbidden-apis. (I haven't yet checked whether java.time is any better.)

@defaultMessage Uses default time zone
org.joda.time.DateTime#<init>()
org.joda.time.DateTime#<init>(long)
org.joda.time.DateTime#<init>(java.lang.Object)
org.joda.time.DateTime#<init>(int, int, int, int, int)
org.joda.time.DateTime#<init>(int, int, int, int, int, int)
org.joda.time.DateTime#<init>(int, int, int, int, int, int, int)
org.joda.time.DateTime#now()
org.joda.time.Instant#toDateTime()
org.joda.time.Instant#toMutableDateTime() 
org.joda.time.Instant#toMutableDateTimeISO() 
org.joda.time.base.AbstractInstant#toDateTimeISO()
org.joda.time.base.AbstractInstant#toDateTime()
org.joda.time.base.AbstractInstant#toMutableDateTime() 
org.joda.time.base.AbstractInstant#toMutableDateTimeISO() 
org.joda.time.LocalDateTime#<init>()
org.joda.time.LocalDateTime#<init>(long)
org.joda.time.LocalDateTime#<init>(java.lang.Object)
org.joda.time.LocalDateTime#now()
org.joda.time.LocalDateTime#fromDateFields(java.util.Date)
org.joda.time.LocalDateTime#toDate()
org.joda.time.LocalDateTime#toDateTime()
org.joda.time.LocalDate#<init>()
org.joda.time.LocalDate#<init>(long)
org.joda.time.LocalDate#<init>(java.lang.Object)
org.joda.time.LocalDate#fromDateFields(java.util.Date)
org.joda.time.LocalDate#now()
org.joda.time.LocalDate#toDate()
org.joda.time.LocalDate#toDateTime(org.joda.time.LocalTime)
org.joda.time.LocalDate#toDateTimeAtCurrentTime()
org.joda.time.LocalDate#toDateTimeAtStartOfDay()
org.joda.time.LocalDate#toInterval()
org.joda.time.LocalTime#<init>()
org.joda.time.LocalTime#<init>(long)
org.joda.time.LocalTime#<init>(java.lang.Object)
org.joda.time.LocalTime#fromDateFields(java.util.Date)
org.joda.time.LocalTime#now()
org.joda.time.LocalTime#toDateTimeToday()

@defaultMessage Doesn't handle edge cases where the start of day isn't midnight.
org.joda.time.LocalDate#toDateTimeAtMidnight()       
org.joda.time.DateMidnight

@uschindler
Copy link
Member

uschindler commented Jul 20, 2017

Java 8's time API is similar, but the types are much better and a LocalDateTime is different from a ZonedDateTime and you cannot accidentally convert between them. The only pain points that use default timezone are in jdk-unsafe-1.8 signatures already.

IMHO, JodaTime is separate and maybe the signatures should be published as separate Maven Central artifacts. I am planning to do the same with commons-io signatures. As there are many different versions of joda.time out there, it is hard to maintain all this inside forbidden-apis. So I am thinking about opening separate github repos to maintain those external libs.

FYI, the Maven Plugin already allows to "configure" signatures using their Maven coordinates. If you have a signatures file on Central you can use it in your build by just listing it. For Gradle you can do the same by adding another dependency type and refer to it using FileCollections or URLs.

@leventov
Copy link
Author

@trejkaz there are also two Interval constructors

@hakanai
Copy link

hakanai commented Jul 20, 2017

There are 10... I don't use that class so I can't say which ones are bad yet.

@leventov
Copy link
Author

The most complete list to my knowledge is

@defaultMessage Uses default time zone
org.joda.time.DateTime#<init>()
org.joda.time.DateTime#<init>(long)
org.joda.time.DateTime#<init>(java.lang.Object)
org.joda.time.DateTime#<init>(int, int, int, int, int)
org.joda.time.DateTime#<init>(int, int, int, int, int, int)
org.joda.time.DateTime#<init>(int, int, int, int, int, int, int)
org.joda.time.DateTime#now()
org.joda.time.Instant#toDateTime()
org.joda.time.Instant#toMutableDateTime()
org.joda.time.Instant#toMutableDateTimeISO()
org.joda.time.base.AbstractInstant#toDateTimeISO()
org.joda.time.base.AbstractInstant#toDateTime()
org.joda.time.base.AbstractInstant#toMutableDateTime()
org.joda.time.base.AbstractInstant#toMutableDateTimeISO()
org.joda.time.LocalDateTime#<init>()
org.joda.time.LocalDateTime#<init>(long)
org.joda.time.LocalDateTime#<init>(java.lang.Object)
org.joda.time.LocalDateTime#now()
org.joda.time.LocalDateTime#fromDateFields(java.util.Date)
org.joda.time.LocalDateTime#toDate()
org.joda.time.LocalDateTime#toDateTime()
org.joda.time.LocalDate#<init>()
org.joda.time.LocalDate#<init>(long)
org.joda.time.LocalDate#<init>(java.lang.Object)
org.joda.time.LocalDate#fromDateFields(java.util.Date)
org.joda.time.LocalDate#now()
org.joda.time.LocalDate#toDate()
org.joda.time.LocalDate#toDateTime(org.joda.time.LocalTime)
org.joda.time.LocalDate#toDateTimeAtCurrentTime()
org.joda.time.LocalDate#toDateTimeAtStartOfDay()
org.joda.time.LocalDate#toInterval()
org.joda.time.LocalTime#<init>()
org.joda.time.LocalTime#<init>(long)
org.joda.time.LocalTime#<init>(java.lang.Object)
org.joda.time.LocalTime#fromDateFields(java.util.Date)
org.joda.time.LocalTime#now()
org.joda.time.LocalTime#toDateTimeToday()
org.joda.time.Interval#<init>(long, long)
org.joda.time.Interval#<init>(java.lang.Object)
org.joda.time.Interval#parse(java.lang.String)
org.joda.time.Interval#parseWithOffset(java.lang.String)

@defaultMessage Doesn't handle edge cases where the start of day isn't midnight.
org.joda.time.LocalDate#toDateTimeAtMidnight()
org.joda.time.DateMidnight

@hakanai
Copy link

hakanai commented Jul 29, 2017

Looking at Interval a bit more, if we say that the (long, long) overload is bad, we mean that it's bad because we didn't provide a time zone when calling:

new Interval(startMillis, endMillis)

But hang on, new Instant(long) isn't forbidden so you can just wrap the two:

new Interval(new Instant(startMillis), new Instant(endMillis))

Now you have "non-forbidden" code but which hasn't actually solved the problem - there is still no time zone being passed in.

My guess is that, because AbstractInstant has a getZone method, you think that it's safe because the zone from the instant will be used. But that one subclass, Instant, represents the concept of a point in time without the notion of a time zone. So you end up with what looks like a contradiction. (getZone is there, but happens to return UTC. Why is it even on the interface? That is the mystery, really.)

So to resolve that, I would prohibit (Instant, Instant) as well. It's very odd that there isn't another overload which takes (Instant, Instant, DateTimeZone) or (Instant, Instant, Chronology), but you can still call the (long, long, DateTimeZone) one and end up with a correct time zone.

The other 4 which take an Instant on one side and not the other - I'm not sure about those yet, because it's more stuff we don't use. Maybe one of those things forces you to provide the time zone at creation-time, which would then not be a problem.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

No branches or pull requests

3 participants