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

querydsl-sql : Joda time should be optional #2025

Closed
amanteaux opened this issue Sep 27, 2016 · 11 comments · Fixed by #2643
Closed

querydsl-sql : Joda time should be optional #2025

amanteaux opened this issue Sep 27, 2016 · 11 comments · Fixed by #2643
Assignees
Milestone

Comments

@amanteaux
Copy link

I am using Java 8 so I don't want Joda time in my classpath.
Indeed it messes with the imports: if I want to use a java.time.LocalDate, I have to be careful not to import org.joda.time.LocalDate.

Currently if I try to exclude Joda time from my classpath, I cannot initialize Querydsl because in JavaTypeMapping, the CalendarType is registered and it depends on Joda time.
Moreover:

  • still in JavaTypeMapping, Joda time types are registered,
  • the Configuration class is final and reference JavaTypeMapping, so there is no way around.

In a first step, the modifications that needs to be done are:

  • Refactor AbstractDateTimeType to remove the Joda time dependency,
  • In JavaTypeMapping, do the joda time initialization the same way java 8 types are registered,
  • Mark joda time as optional in the pom.xml :)

If that is ok, I can do the change.
However, it is a breaking change: libraries and projects depending on querydsl-sql and using joda time will have to explicitly add joda time in their pom.xml, gradle etc.
So the version on which this development will be made has to be decided.

@janneri
Copy link

janneri commented Jun 2, 2020

Spring Boot will autoconfigure a formatter when joda-time is in classpath. The formatter wants a newer version of joda-time than querydsl. So to me, the joda-time dependency seems pretty useless in 2020 and it's causing problems. I would greatly appreciate this fix!

p.s. Declaring joda-time as a dependency is a trivial fix. I wouldn't worry about this as a breaking change.

@Neikius
Copy link

Neikius commented Jun 8, 2020

Spring Boot will autoconfigure a formatter when joda-time is in classpath. The formatter wants a newer version of joda-time than querydsl. So to me, the joda-time dependency seems pretty useless in 2020 and it's causing problems. I would greatly appreciate this fix!

p.s. Declaring joda-time as a dependency is a trivial fix. I wouldn't worry about this as a breaking change.

Same here. Please take the joda-time out.

@lagnat
Copy link

lagnat commented Jul 16, 2020

Another vote to address this issue. Thank you.

@digsim
Copy link

digsim commented Aug 7, 2020

Same problem here, we can't update spring-boot any longer. Would be great if we can get rid of joda-time.

@germatpreis
Copy link

Same problem here.

@jwgmeligmeyling jwgmeligmeyling removed their assignment Aug 27, 2020
@daniel-shuy
Copy link
Contributor

I've decided to take up this task.

The main issue is how to replace joda-time's DateTimeFormatter.

There are 4 alternatives:

  • Use SimpleDateFormat. The main issue with this is that SimpleDateFormat is not thread-safe and is expensive to initialize, which would require creating new instances of SimpleDateFormat for each thread using ThreadLocal, which has a performance cost.
  • Import Apache Commons Lang to use FastDateFormat, a thread-safe equivalent to SimpleDateFormat. This would however introduce Apache Commons Lang as a new transitive dependency.
  • Increase QueryDSL's Java target version to Java 8, which would allow us to use Java 8's DateTimeFormatter. This would however require dropping support for Java 6/7.
  • Use Java 8's DateTimeFormatter if Java 8 is available, else use joda-time's DateTimeFormatter if joda-time is imported, else fallback to SimpleDateFormat. This would however greatly increase the code complexity.

@lagnat
Copy link

lagnat commented Sep 5, 2020

Java 8 is my vote.

@daniel-shuy
Copy link
Contributor

daniel-shuy commented Sep 5, 2020

Java 8 is my vote.

That's my preference as well, however I would need someone from the QueryDSL team to make the decision, as it is a major change

@jwgmeligmeyling
Copy link
Member

jwgmeligmeyling commented Sep 5, 2020

We want to keep supporting Java 7 for 4.x. Java 8 API's can still be used through reflection though. As long as they are not imported, which would prevent classes from loading on Java 7.

I strongly discourage writing a Java 8 only solution, because it would just delay how soon this can be integrated, and the demand is high.

Also, lets try not to introduce commons lang as a transitive dependency, looking at the Guava discussion, it appears people really dislike dependencies shipping other dependencies.

@daniel-shuy
Copy link
Contributor

@jwgmeligmeyling agreed. I've created a PR using the SimpleDateFormat with ThreadLocal approach.

jwgmeligmeyling added a commit that referenced this issue Dec 31, 2020
[#2025] querydsl-sql : Change Joda-Time to optional
@daniel-shuy
Copy link
Contributor

Good news everyone! The upcoming QueryDSL 5.x release will require Java 8+, which allows us to remove the transitive dependency on joda-time, using the Java 8's DateTimeFormatter approach, which imo is the cleanest approach.

Many thanks to @jwgmeligmeyling for the Java 8 migration, and for reviewing and merging the PR.

@jwgmeligmeyling jwgmeligmeyling self-assigned this Jun 8, 2021
@jwgmeligmeyling jwgmeligmeyling added this to the 5.0 milestone Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants