Add lambda-friendly callback interfaces

[christophercurrie]

Using JDBI with Java 8 lambdas is great until you want a callback that doesn't return a value. You can't cast the lambda to VoidHandleCallback because lambdas can only be cast to interfaces. It's an annoyance, not an obstacle, but one that's bugged me often enough that I want a better experience, and it's pretty easy to add.

This change adds functional interfaces that Java 8 can infer when the lambda does not return. It does not add any Java 8 dependencies.

Objections?

[brianm]

I am generally a fan of this, let me look at the details!

[brianm]

Adding methods to IDBI and TransactionConsumer break compatibility with other implementations of them. I suspect no one has ever implemented TransactionHandler outside the core, but I caught grief for changing IDBI in the past. I am willing, but want to poll the mailing list before doing it.

@christophercurrie can you ping the mailing list for feelings on adding to IDBI before we make a decision?

[stevenschlansker]

IMO this is a positive change; I want to make sure it merges cleanly and makes sense in the context of the ever-looming Java 8 JDBI3. I can take a closer look at that aspect in a few days. I suspect the answer is that there are no issues and we will effectively revert this change, it will then be a source-level incompatibility to change to e.g. Consumer<Handle> or whatever the equivalent is.

I also think the concept of IDBI is bad, people should generally not be mocking JDBI at that interface level. Makes for extremely brittle tests. But that ship sailed and maybe we have to appease those users anyway.

[christophercurrie]

@brianm Done.

[christophercurrie]

Bump?

[stevenschlansker]

I'll merge this shortly if there are no objections.

[christophercurrie]

Thanks!

[stevenschlansker]

Do you want a release?

[brianm]

Yes

On Tue, Jul 14, 2015 at 1:06 PM, Steven Schlansker <notifications@github.com

wrote:

Do you want a release?

—
Reply to this email directly or view it on GitHub
#156 (comment).

[stevenschlansker]

@brianm Maven no longer supports Java 6. So I cannot build JDBI anymore :(

[steven@Anesthetize:~/code/jdbi](master↑13|✔)% export JAVA_HOME=/Library/Java/JavaVirtualMachines/1.6.0_33-b03-424.jdk/Contents/Home
[steven@Anesthetize:~/code/jdbi](master↑13|✔)% mvn clean install                                                                    
Exception in thread "main" java.lang.UnsupportedClassVersionError: org/apache/maven/cli/MavenCli : Unsupported major.minor version 51.0
    at java.lang.ClassLoader.defineClass1(Native Method)

When will you give up on JDK6? :P

[christophercurrie]

This can be worked around with stupid bootclasspath tricks. I can create a new PR for that if it would help :/

[hgschmie]

That has been in place for a long time.

Install a JDK6, set an environment variable "JDK6_HOME" to point at that installation (JDK6_HOME=$(/usr/libexec/java_home -version 1.6), use a JDK7 to run maven, but it will build with JDK6.

As the travis build does.

[hgschmie]

https://github.com/jdbi/jdbi/blob/master/pom.xml#L350-L369

[hgschmie]

If someone finds time, you can dig into the toolchains (https://maven.apache.org/guides/mini/guide-using-toolchains.html) which should make this painless (and the error messages a bit less obscure). I may do that when I am back from Europe (early August).

[stevenschlansker]

Released 2.63

[arteam]

Great!

[osi]

I just updated to 2.63 to get this change, and I'm getting compilation errors saying:

Error:(101, 19) java: reference to withHandle is ambiguous
both method withHandle(org.skife.jdbi.v2.tweak.HandleCallback) in org.skife.jdbi.v2.DBI and method withHandle(org.skife.jdbi.v2.tweak.HandleConsumer) in org.skife.jdbi.v2.DBI match

I can fix it by adding a cast that indicates the first method is what I want, but this did end up being backwards incompatible. And it made my Java8 code messier by adding casts.

This only appeared to be necessary when I had code that was in the form of:

dbi.withHandle(handle -> handle.createQuery().list())

If the lambda was in the form of

dbi.withHandle(handle -> {
  return handle.createQuery().list();
});

then the compiler correctly inferred the SAM that has a return value

[stevenschlansker]

What was the actual code that triggers the error?

[osi]

dbi.withHandle(handle -> handle.createQuery(SNAPSHOT_SQL)
                .define(NameStrategies.STATEMENT_TYPE, "select")
                .define(NameStrategies.STATEMENT_NAME, "snapshot")
                .bind("date", date.minusTwo().toDateTimeAtStartOfDay())
                .map(fooMapper)
                .list())

[stevenschlansker]

Actually, that's an interesting point. @christophercurrie why did you introduce HandleConsumer when there is already HandleCallback, which seems to be OK as a FunctionalInterface? Just to remove the return type?

[brianm]

I can see why a consumer interface is handy, but we should have a different
name for the method to avoid just this. Let's roll back this change ASAP,
cut a new release, purge the broken one from oss.s.o, and change the
consume oriented methods to be something like dbi.useHandle(...): void or
such.

-Brian

On Tue, Jul 21, 2015 at 10:04 AM, Steven Schlansker <
notifications@github.com> wrote:

Actually, that's an interesting point. @christophercurrie
https://github.com/christophercurrie why did you introduce
HandleConsumer when there is already HandleCallback, which seems to be OK
as a FunctionalInterface? Just to remove the return type?

—
Reply to this email directly or view it on GitHub
#156 (comment).

[christophercurrie]

Sorry this ended up not working transparently; I made a mistake of only testing with block lambdas rather than single statement lambdas.

@stevenschlansker Yes, the point was simply to remove the need for spurious return null; statements in the lambda functions. As I said, an annoyance and not an obstacle. Mea culpa for not testing both use cases before submitting the PR. I would have added test cases if I knew of a way to test using Java 8 but still compile for Java 6.

[stevenschlansker]

Is there a quick / good fix that doesn't involve reverting the change? Or should we back off and reconsider?

[osi]

rename withHandle(HandleConsumer) so it isn't ambiguous to the compiler. That's the only conflict I encountered.

[christophercurrie]

@osi is correct. If you want to revert first to get a valid build out quickly, that seems reasonable.

[christophercurrie]

FWIW, it looks like there have been bugs in the JVM in this space, so I'd be curious which specific update of Java 8 that @osi is using.

That said, I just updated from 1.8.0_31 to 1.8.0_51, and it creates a different problem that expresses itself in the same way. The solution in this case is to specify the parameter type.

dbi.withHandle((Handle handle) -> handle.createQuery().list());

With the parameter type in place, the lambda can be changed from void to returning, and Java 8 doesn't have a problem figuring out which one you want. Sadly, though, as-is this change still breaks code that worked before the change.

I can do the legwork to prep the revert and/or the replacement if desired.

[stevenschlansker]

If you can get the replacement ready today, I guess that's preferable to a revert? I can cut a release.

[christophercurrie]

Even more frustratingly, adding braces without a parameter type makes the compiler happy:

dbi.withHandle(handle -> { return handle.createQuery().list(); });

I get really tweaked when simple, apparently unnecessary changes to user code cause compiler issues. I was hoping that simply updating the compiler version would make the problem go away, but no such luck.

Yes, I can get a PR to fix the issue today.

[osi]

I'm using 1.8.0_45. I fixed my code by adding a cast before the lambda to HandleCallback so the compiler knows which variant I meant.

[stevenschlansker]

Released as 2.63.1