InsertSetStep and similar interfaces should implement AutoCloseable

[rgoldberg]

InsertSetStep and similar interfaces should implement AutoCloseable.

DSLContext#insertInto(Table<R>) returns an InsertSetStep, which, since InsertSetStep doesn't currently implement AutoCloseable, means that I can't write the following sensible code:

try (InsertSetStep insert = ctx.insertInto(table)) {
    ...
}

[lukaseder]

Hmm, interesting. It would be easy to implement. I suspect that it would be sufficient for org.jooq.Query to extend AutoCloseable, which makes sense on its own because a Query can keep a reference to a PreparedStatement via Query.keepStatement().

What would close() do in your situation?

[rgoldberg]

It's just good practice to call close() whenever an AutoCloseable is no longer needed.

Since the implementation of InsertSetStep actually implements AutoCloseable, and since many of the other insert interfaces, like InsertValuesStepN, also extend AutoCloseable, then this change just simplifies the closing code, instead of having to cast, or track the returned interfaces from the various methods.

Query already extends AutoCloseable; InsertSetStep does not extend Query.

It also might be nice for all these interfaces to extend Query, or at least some new super-interface of Query that itself extends AutoCloseable and also includes boolean isExecutable() & int execute() that are defined in Query. I know that a plain InsertSetStep that hasn't been returned from a jOOQ method as another interface won't be executable, but, since the InsertSetStep implementation is mutable, I should be able to see if it's executable and then execute it directly from that interface, without having to establish a new variable of a different type that may or may not get populated, so I'll have to check that variable to see if it's null, etc.

I know that this is growing the scope of this issue, and it can be broken out into two; or we could just renamed it "Create & extend ExecutableQuery super-interface of Query", or some such similar title.

[lukaseder]

I was tired this morning. I suspect I hadn't looked into the right version of Query. You're right, of course. And nice observation about AutoCloseable. It might make sense to put that API on all of these Step types. On the other hand, it's not 100% clear what should be communicated to the user, this way.

It also might be nice for all these interfaces to extend Query,

That won't work well with jOOQ's DSL design strategy. It should not be possible to execute queries that are "incomplete"

or at least some new super-interface of Query that itself extends AutoCloseable

Perhaps... It could be AutoCloseableQueryPart, but right now, I don't think this new type will pull its weight.

and also includes boolean isExecutable() & int execute() that are defined in Query.

I wouldn't do that for the reasons mentioned before.

I should be able to see if it's executable and then execute it directly from that interface, without having to establish a new variable of a different type that may or may not get populated, so I'll have to check that variable to see if it's null, etc.

There is a strategic distinction between the DSL API and the model API, which we will further evolve in future versions. The DSL API is supposed to be used for "static SQL" whereas the model API is better suited for "dynamic SQL". In that sense, if you want to build very dynamic SQL statements, you might be better off with the model API, e.g. with InsertQuery

[rgoldberg]

I had heard about the model API, but hadn't yet looked into it, but now I have, and it looks like it should solve the issues that I had with the restrictiveness of the DSL API. Thanks for bringing it to my attention again.

I'm not sure what you mean by "It might make sense to put that API on all of these Step types. On the other hand, it's not 100% clear what should be communicated to the user, this way.".

Do you mean making them extend AutoCloseable, making them extend Query, or some other API?

[rgoldberg]

The Model API is much less expressive than the DSL API, so having all of its interfaces extend AutoCloseable would be very useful.

[rgoldberg]

The DSL API is so much more natural than the Model API that I'd much prefer to use the DSL.

if you're willing to add isExecutable() & execute() to all of the DSL interfaces that don't yet include them, then, in those interfaces, you could annotate the methods as @Deprecated, so that anyone who calls them will see a warning (that they can explicitly suppress) to indicate that this is unusual usage (guava frequently uses @Deprecated in a similar way).

If you're unwilling to add isExecutable() & execute() to all of the DSL interfaces that don't yet include them, if there aren't too many of such interfaces, could you just add an extra sub interface including both methods for each such interface, possibly named like ExecutableInsertSetStep & obtainable from a method like DSLContext#insertIntoExecutable(Table<R>)? You could use @Deprecated for these interfaces and/or methods, if you wish.

From what I've seen, this should only require managing a few interface methods / interfaces, since all the implementations of interfaces without isExecutable() & execute() seem to still have those methods.

[rgoldberg]

FYI, InsertValuesStep1, etc., inherit from Query, so they are AutoCloseable, and have isExecutable() & execute(). The corresponding SQL for those interfaces when they are first obtained from the DSL isn't actually executable (at least not in MySQL), just like the case for InsertSetStep, so there definitely is a precedent for interfaces inheriting from Query even when they might not be executable.

[lukaseder]

in those interfaces, you could annotate the methods as @deprecated

I think we're already discussing solutions to a problem that we haven't yet fully specified or understood - at least I haven't. We'll certainly not do any deprecation trickery, that's for sure. :) Guava may do things, but that doesn't mean that those things are good.

In all of the above discussion, you haven't yet explained why you're trying to write the following:

try (InsertSetStep insert = ctx.insertInto(table)) {
    ...
}

... rather than, for instance, the following:

try (Query insert = ctx.insertInto(table).values(...)) {
    ...
}

... or even:

Query insert = ctx.insertInto(...).values(...);
try (Query query = insert) {
    ...
}

Let's get on the same level of understanding about the problem first, before we discuss possible solutions.

Do note that my point here is that locally assigning intermediate DSL "Step" API parts is always a workaround, and never best practice. Any attempt to improve the feel of applying this workaround is the wrong way to go forward in the long run.

An acceptable solution in the long run is to work towards overall better APIs. The DSL API is designed for static SQL, and the model API is designed for dynamic SQL. Every in-between solution will deteriorate the perceived API quality, maintainablity, and usability. And already now, things aren't perfect.

FYI, InsertValuesStep1, etc., inherit from Query, so they are AutoCloseable, and have isExecutable() & execute()

That's a known issue: #3771. It will not be possible to call insertInto(TABLE).execute() in a future version of jOOQ.

[lukaseder]

After second review, I really don't think this is a reasonable thing to do. The intermediate "Step" API types do not stand on their own. As such, they should not expose any "convenient" functionality that helps using them in a way they weren't intended to be used.

Some people use them for "dynamic SQL". That is not recommended practice, although for a quick win, unfortunately, it is perceived better than resorting to the model API. This means we should fix the model API in jOOQ 4.0 to make it more suitable for such cases, not add functionality to the Step types.

Closing this as won't fix