jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
189 stars 55 forks source link

javax.sql dependence #483

Closed gavinking closed 10 months ago

gavinking commented 10 months ago

433 provided a very convenient API and addressed a major point of lack-of-portability for JPA usage in the SE environment.

But literally ten minutes after merging the PR, @lukasj and I realized that there's something we need to consider more carefully here, that possibly justifies rolling back the PR.

Essentially, JPA 3.1 has only one SPI which depends on the module java.sql, and in principle that dependence could be removed fairly straightforwardly. But #433 bakes in a much harder dependence to java.sql that in its present form could never be removed.

Now, that's not a disaster, since java.sql is part of the JDK after all, and almost every implementation of JPA has that dependence, and so indeed it's quite difficult to decide how much importance we should place on this.

But it's indeed true that the new APIs in #433 are some sort of layer-breaker, and it seems that there is still one implementation of JPA based on non-JDBC datasources, so arguably this new API pollutes EntityManager with operations that aren't guaranteed to be available.

So what should we do here?

My third option is to change the signature of those methods to:

<C> void runWithConnection(ConnectionConsumer<C> action);
<C,T> T callWithConnection(ConnectionFunction<C,T> function)

where we've removed the direct dependence on JDBC, at the expense of a loss of type safety by making the connection type generic and provider-specific.

Usage with JDBC would now look like:

em.runWithConnection((Connection conn) -> {
    conn.prepareCall("blah blah").execute();
});

which is not a disaster.

Thoughts?

gavinking commented 10 months ago

PR #484 implements the idea proposed above. It's not terrible, IMO. WDYT?