Revise the HandleCallback decorator code

[hgschmie]

• Rename "Handler" to "HandleCallbackDecorator". "Handler" is a way too generic name and there is potential for confusion with "Handle" which is a main concept in Jdbi while this change introduces a minor feature.
• Mark the added code with @Alpha annotation, as the API should not have been set in stone.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[hgschmie]

TBH, I am pretty unhappy that #2448 was merged right before the 3.40.0 release and then immediately released. This PR marks the code as @Alpha (it should have been alpha to begin with IMHO).

It is not clear to me what that code is intended to accomplish.

The way I see it, the code can do one of two things:

One use case is to select an alternate HandleCallback instead of the one supplied. That is similar to one test where within the decorator, the code tests and does not return the HandleCallback passed in but an alternate version. This is IMHO much simpler accomplished by doing

var handleCallback = someCondition()
    ? handleCallback1
    : handleCallback2;

var result = jdbi.withHandle(handleCallback);

The other use case is that the decorator is used to modify the handle:

    var decorator = new HandleCallbackDecorator() {
        @Override
        public <R, X extends Exception> HandleCallback<R, X> decorate(HandleCallback<R, X> callback) {
            return handle -> {
                handle.setReadOnly(true); // this is the handle modification code
                return callback.withHandle(handle);
            };
        }
    };

    jdbi.setHandleCallbackDecorator(decorator);

    jdbi.withHandle(...); // handle here will always be read-only

This seems to be a very complicated way to do this. The same effect can be accomplished today by doing

    var plugin = new JdbiPlugin() {
        @Override
        public Handle customizeHandle(Handle handle) throws SQLException {
            handle.setReadOnly(true);
            return handle;
        }
    };

    jdbi.installPlugin(plugin);

which is as global as the registered HandleCallbackDecorator (all handles will pass through the customizeHandle method) and accomplished the same effect.

The feature is undocumented (no examples, no adding to index.adoc). This is destined to be another of the myriad of undocumented features that were added at some time or another for a very specific use case without documentation and in a year or so, no one remembers why.

@Randgalt could you elaborate what you are trying to do? I read the description about Specialized behavior can be added for an app's entire JDBI usage without project devs needing to do anything special. This is very useful when introducing protocols such as custom retry behavior, custom connection management,[...] and I would like to understand what specific thing can not be done with a Jdbi plugin and needs this specific decoration of the handle callback?

Also, it would be good if you could contribute some documentation and examples beyond the test.

[Randgalt]

We are working with a database, CRDB, that has some unfortunate edge cases. The TL;DR is that our connection pool can get poisoned with invalid connections that cannot be discovered without making a query with the connection. Inside of a transaction this is not a problem as we have a custom JDBI TransactionHandler that allows us to notice this event and substitute a good connection for the bad one and then retry the entire operation. In non-transactions, however, we have no opportunity to do this.

This is IMHO much simpler accomplished by doing

This requires changes at every call site. The change I submitted makes it possible to hook every instance of withHandle/useHandle just as we do with the TransactionHandler. Going through our code and wrapping every call to useHandle is a non-starter. Note: we need to substitute a good connection and then retry the handler/operation.

I hope this helps.

[hgschmie]

Thanks for the explanation. There is a chance that you are actually be hit by #2446.

From you explanation and the quote, are you actually replacing the HandleCallback or are you augmenting (wrapping) it? Is this something that is related to the handle or the connection?

It would be great to see some (abstract) code example. It might be possible to solve your issues with a Jdbi Plugin; there is not a lot that you can do with that callback decorator that can not be done with a plugin. And as your do this on a per-jdbi basis, registering such a plugin with the jdbi instance should be possible.

[Randgalt]

Thanks for the explanation. There is a chance that you are actually be hit by #2446.

I don't think this affects us. We actually wrap all connections returned by our Connection pool so that we can manage them manually as needed.

It would be great to see some (abstract) code example. It might be possible to solve your issues with a Jdbi Plugin;

I can't publish our internal code but here is a sketch that shows something of what we're wanting to do.

public class ExampleJdbiPlugin
        implements JdbiPlugin, Handler
{
    private final int maxRetries;

    public ExampleJdbiPlugin(int maxRetries)
    {
        this.maxRetries = maxRetries;
    }

    @Override
    public void customizeJdbi(Jdbi jdbi)
    {
        jdbi.setHandler(this);
    }

    @Override
    public <R, X extends Exception> HandleCallback<R, X> decorate(HandleCallback<R, X> callback)
    {
        return handle -> {
            try {
                if (handle.getConnection().getAutoCommit()) {
                    return withRetry(handle, callback);
                }
            }
            catch (SQLException e) {
                throw new ConnectionException(e);
            }
            return callback.withHandle(handle);
        };
    }

    private <R, X extends Exception> R withRetry(Handle handle, HandleCallback<R, X> callback)
            throws X
    {
        int attempts = 1 + maxRetries;

        Deque<X> failures = new ArrayDeque<>();
        while (true) {
            try {
                return callback.withHandle(handle);
            }
            catch (Exception last) {
                X x = (X) last;

                failures.addLast(x);

                if (!isBadConnectionError(x)) {
                    throwAll(failures);
                }

                attempts -= 1;
                if (attempts <= 0) {
                    throwAll(failures);
                }

                fixBadConnection(handle);
            }
        }
    }
}

[hgschmie]

Thanks for the example, this is useful. It would be good to contribute some documentation (e.g for chapter 14 or chapter 19) about the callback so that this feature is properly documented.

[Randgalt]

#2463