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

Merge dropwizard-java8 addon into mainline #1365

Merged
merged 1 commit into from Dec 5, 2015

Conversation

Projects
None yet
3 participants
@joschi
Member

joschi commented Nov 28, 2015

dropwizard-java8 used to provide support for Java 8 classes like Optional<T> and the Java Date and Time API (JSR-310) for Dropwizard. Given that the baseline for Dropwizard is now Java 8, those additions can be maintained in the mainline repository.

The only backward-incompatible change is in the io.dropwizard.auth.Authenticator interface which is now using java.util.Optional instead of the equivalent class from Google Guava.

Closes #962

@joschi joschi added the improvement label Nov 28, 2015

@joschi joschi added this to the 1.0.0 milestone Nov 28, 2015

Merge dropwizard-java8 addon into mainline
dropwizard-java8 used to provide support for Java 8 classes like `Optional<T>` and
the Java Date and Time API (JSR-310) for Dropwizard. Given that the baseline for
Dropwizard is now Java 8, those additions can be maintained in the mainline repository.

The only backward-incompatible change is in the `io.dropwizard.auth.Authenticator` interface
which is now using `java.util.Optional` instead of the equivalent class from Google Guava.

Closes #962
@jplock

This comment has been minimized.

Member

jplock commented Dec 3, 2015

This looks good to me, thanks for all the work in integrating this! Any other feedback @dropwizard/committers ?

@arteam

This comment has been minimized.

Member

arteam commented Dec 5, 2015

@joschi, what do you think about creating dropwziard-jdbi-guava-optional module with Guava's
Optional support instead of mixing it with Java8's Optional? These classes basically duplicate each other, and I don't think many folks use them together in the same codebase. It would be great to let users choose what Optional support to use. I think we should encourage users to switch to Java8's Optional, because it is shipped with JRE, is a more flexible type and will probably supress Guava's Optional in future. Folks, who for some reasons don't want to switch, will have to enable an additional module and use a custom DBIFactory. It could be implemented somehting like that:

public class GuavaOptionalDBIFactory extends DBIFactory {

    @Override
    public DBI build(Environment environment, PooledDataSourceFactory configuration,
                     ManagedDataSource dataSource, String name) {
        final DBI dbi = super.build(environment, configuration, dataSource, name);
        dbi.registerArgumentFactory(new GuavaOptionalArgumentFactory(
                                    configuration.getDriverClass()));
        dbi.registerContainerFactory(new GuavaOptionalContainerFactory());

        final Optional<TimeZone> timeZone = databaseTimeZone();
        dbi.registerArgumentFactory(new GuavaOptionalJodaTimeArgumentFactory(timeZone));
        dbi.registerArgumentFactory(new GuavaOptionalLocalDateArgumentFactory());
        dbi.registerArgumentFactory(new GuavaOptionalLocalDateTimeArgumentFactory());
        dbi.registerArgumentFactory(new GuavaOptionalInstantArgumentFactory(timeZone));
        dbi.registerArgumentFactory(new GuavaOptionalOffsetTimeArgumentFactory(timeZone));
        dbi.registerArgumentFactory(new GuavaOptionalZonedTimeArgumentFactory(timeZone));

        return dbi;
    }
}

What do you think? If such modularity is too controversial, I think we could at least move Guava's Optional into a separate package in dropwizard-jdbi.

@arteam

This comment has been minimized.

Member

arteam commented Dec 5, 2015

I am also a bit of worried about mixing Joda-Time and JSR 310 support. Unfortunately, I don't see a way how could we modularize the support of these libraries. Removing Joda-Time support in favour of JSR 310 is also not an option. I believe, for the time being, we have to force users to import Joda-Time, even if they don't use it.

@joschi

This comment has been minimized.

Member

joschi commented Dec 5, 2015

@arteam In my opinion Dropwizard should "just work" with Google Guava and Joda-Time additionally to the APIs provided by JDK 8 and higher (JSR-310 and Optional).
Swapping out the support for those libraries (which are by the way being used throughout Dropwizard itself) doesn't gain us anything in my opinion and will just put the burden to add additional dependencies on our users.
Once we got rid of Google Guava or Joda-Time from the Dropwizard core components, we can move support for them from dropwizard-jdbi into separate modules, but for now I'd like to keep it.

Backward compatibility with older Dropwizard versions isn't exactly required for Dropwizard 1.0.0, but it's a nice feature. 😉

arteam added a commit that referenced this pull request Dec 5, 2015

Merge pull request #1365 from joschi/jdk8-issue-962
Merge dropwizard-java8 addon into mainline

@arteam arteam merged commit 79b17dc into dropwizard:master Dec 5, 2015

@arteam

This comment has been minimized.

Member

arteam commented Dec 5, 2015

Ok, fair enough. The rest changes look good to me. Thanks for your hard work!

@joschi

This comment has been minimized.

Member

joschi commented Dec 5, 2015

@jplock @arteam Thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment