Support including Module and Action in each JDBC session with Oracle JDBC

[radovanradic]

Created as draft, not sure if this approach is correct

[radovanradic]

Since there is ConnectableInterceptor that is creating custom ConnectionDefinition I thought we could use this annotation for this purpose so users can customize module and action that will be traced. Don't see other way than use this annotation for the repository that needs to be traced. Without it, we cannot obtain details about class/method being executed when connection is established.

[radovanradic]

Didn't find the way to test it, so I used it on a small app and was looking up what is being written to the V$SESSION table.
If we have repository like this

@JdbcRepository(dialect = Dialect.ORACLE)
@Connectable(traceClientInfo = true)
public interface BookRepository extends CrudRepository<Book, Long> {

    @Override
    @NonNull
    @Connectable(traceClientInfo = true, tracingAction = "INSERT")
    Book save(@NonNull Book entity);
}

Then findById method will have this in Oracle DB

SELECT sid, client_identifier, service_name, program, module, action FROM V$SESSION WHERE client_identifier = 'mn-oracle-jdbc'

and save method with customized action

Please note that when only class is annotated, then we don't get actual repository class name as expected com.example.BookRepository but we get its superclass io.micronaut.data.repository.CrudRepository and when method is annotated we get actual class name. Not sure how to resolve that, I have used executableMethod.getDeclaringType().getName() where executableMethod is ExecutableMethod<Object, Object>

[dstepanov]

Maybe we can introduce OracleConnectable and for Connectable add some kind of parameter to include a listener that will be able to react to open/close. It is similar to ConnectionSynchronization but with more lifecycles and a way to propagate the definition. (The definition might need to include AnnotationMetadata) The oracle connectable can include it's own listener and propagate the annotation configuration.

[graemerocher]

I don't think this makes sense at the annotation level. You likely just want to configure it once for your whole application. I would refactor this logic to be something that can be configured.

Potentially you can add a strategy interface ConnectionCustomizer or something that is invoked and implement the Oracle specific logic..

One or more customisers could be registered and ordered with @Ordered

[radovanradic]

I thought users might want to customize module and action with some specific naming in some cases and not just use class name/method name. This is why I thought annotation could be used and also option to enable logging for just some classes. But, I agree it wouldn't be the easiest option to configure and something global might make sense.
The other reason for using annotation + interceptor is ability to propagate method metadata because with current code we don't have a way to know which method is being executed when connection is opened. Don't know how to propagate it since it begins somewhere in TransactionOperations in most cases and then opens connection without passing method info like this in AbstractTransactionOperations:

@Override
    protected final <R> R doExecute(@NonNull TransactionDefinition definition, @NonNull TransactionCallback<C, R> callback) {
        ConnectionStatus<C> connectionStatus = connectionOperations.findConnectionStatus().orElse(null);
        if (connectionStatus == null) {
            return connectionOperations.execute(
                txConnectionDefinition(definition),
                status -> doExecute(status, definition, callback)
            );
        }
        return doExecute(connectionStatus, definition, callback);
    }

or

@NonNull
    @Override
    public TransactionStatus<C> getTransaction(TransactionDefinition definition) throws TransactionException {
        if (synchronousConnectionManager == null) {
            throw new TransactionUsageException("Synchronous connection manager not supported!");
        }
        ConnectionStatus<C> connectionStatus = connectionOperations.findConnectionStatus().orElse(null);
        Optional<T> existingTransactionStatus = findTransactionStatus();
        if (existingTransactionStatus.isPresent()) {
            T existingTransaction = existingTransactionStatus.get();
            return switch (definition.getPropagationBehavior()) {
                case REQUIRED, SUPPORTS, MANDATORY, NESTED ->
                    reuseTransaction(definition, connectionStatus, existingTransaction);
                case REQUIRES_NEW -> suspendAndOpenNewTransaction(definition, existingTransaction);
                case NOT_SUPPORTED -> suspendAndOpenNewConnection(definition, existingTransaction);
                case NEVER ->
                    throw new TransactionUsageException("Existing transaction found for transaction marked with propagation 'never'");
            };
        } else {
            return switch (definition.getPropagationBehavior()) {
                case REQUIRED, REQUIRES_NEW, NESTED -> openNewConnectionAndTransaction(definition); // Nested propagation applies only for the existing TX
                case SUPPORTS, NEVER, NOT_SUPPORTED ->
                    withNoTransactionStatus(connectionStatus, definition);
                case MANDATORY -> throw newMandatoryTx();
            };
        }
    }

[graemerocher]

I think we could use @Transcactional(name="Whatever") to the method name. If they want to customise it set the transaction name.

[radovanradic]

Still the problem persists, we must somehow populate JDBC client info parameters in ConnectionDefinition for this method

@Override
    protected Connection openConnection(ConnectionDefinition definition) {
        try {
            return dataSource.getConnection();
        } catch (SQLException e) {
            throw new CannotGetJdbcConnectionException("Failed to obtain JDBC Connection", e);
        }
    }

And ConnectionDefinition does not have information of the method being executed, without the interceptor with @Connectable or similar annotation because ConnectionDefinition is created in various places and propagated, but without method invocation context.

[graemerocher]

ok does this info need to be set before any SQL is executed?

[radovanradic]

I guess so, and it seemed that right place to do it is in DefaultDataSourceConnectionOperations method @Override protected void setupConnection(ConnectionStatus<Connection> connectionStatus) {
@dstepanov does this make sense?

[graemerocher]

Seems weird if that is the case, what if you want different actions per query

[graemerocher]

according to https://docs.oracle.com/en/database/oracle/oracle-database/19/arpls/DBMS_APPLICATION_INFO.html

The action name should usually be the name or description of the current transaction within a module

So @Transactional(name="whatever") seems to map naturally into that

[radovanradic]

Seems weird if that is the case, what if you want different actions per query

Then would do this for the method

@Override
    @NonNull
    @Connectable(traceClientInfo = true, tracingAction = "INSERT")
    Book save(@NonNull Book entity);

since @Connectable can be used to annotate class or method.

[dstepanov]

Why is this change?

[radovanradic]

Because you suggested interface methods accept ConnectionStatus and then creating it here and passing back to openConnection
Will revert this

[dstepanov]

I think it can be done without this change

[radovanradic]

Yes, using existing internal method for open and introducing new for close.

[dstepanov]

Can you introduce something like openConnectionInternal to avoid changes in the implementations?

[dstepanov]

Bind listeners to using registerSynchronization to avoid rechecks if the connection is supported

[dstepanov]

Please remove this; let it be fetched by the Oracle listener. Make ConnectionDefinition AnnotationMetadataProvider

[dstepanov]

Move to oracle listener

[dstepanov]

Add a default value to getClass().getSimpleName()

[dstepanov]

Make inner annotation of OracleConnectionClientInfo

[graemerocher]

I don't think this needs to be renamed, it is not specific to Oracle

[dstepanov]

Please avoid changes in other connection operations

[dstepanov]

This creates a memory leak by collecting all the connections. After turning the connection definition to an annotation provider you should be able to check by simple hasAnnotation.

Inject the datasource and do unwrap + instanceof to disable the listener permanently.

[radovanradic]

@EachBean(DataSource.class) was not creating this bean until added @Context. Not sure if this can cause some issues, I would prefer if it could work without @Context but not sure what is missing.

[dstepanov]

What is the point of enabled? Just remove the annotation of you don't want to have the values set

[radovanradic]

If class is annotated and maybe some methods want to be skipped. If you think it makes no sense, I will remove it.

[graemerocher]

if it is removed it makes sense for ConnectionClientInfo to become a repeatable container so you can just do

@ConnectionClientInfo(name="foo", value ="bar")
@ConnectionClientInfo(name="foo2", value ="baz")

[radovanradic]

Changed, removed enabled and added repeatable.

[sonarcloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.9% Coverage on New Code
2.8% Duplication on New Code

See analysis details on SonarQube Cloud

[dstepanov]

I would renamed the annotation to something ahorter

[graemerocher]

I would rename this to just @ClientInfo (the word connection is already in the package) and make the repeatable annotation an annotation named List that is an inner type. See https://github.com/sergiy-naumovych/Professional-Java-for-Web-Applications/blob/master/Chapter%2021/Eclipse/Spring-JPA/source/production/java/com/wrox/site/validation/NotBlank.java for an example

[radovanradic]

Then repository looks super awkward

@JdbcRepository(dialect = Dialect.ORACLE)
@ClientInfo.List
public interface OracleXEAuthorRepository extends AuthorRepository {
    @Override
    @Join(value = "books", type = Join.Type.LEFT_FETCH)
    @ClientInfo(name = "OCSID.ACTION", value = "QueryAuthorByName")
    Author queryByName(String name);
}

Please suggest something new, I have no more ideas.

[graemerocher]

why is the annotation needed on the type?

[radovanradic]

Because from type we get module and don't have to annotate each method. Then module will be repository class name and action will be method name. But, it can be added on the method as well, for example if users want to name module/action differently.
I would be ok with @ClientInfo and @ClientInfoAttr (or @ClientInfoAttribute)

[graemerocher]

then can't it be:

@JdbcRepository(dialect = Dialect.ORACLE)
@ClientInfo(name = "OCSID.MODULE", value = "SomeModule")
public interface OracleXEAuthorRepository extends AuthorRepository {
    @Override
    @Join(value = "books", type = Join.Type.LEFT_FETCH)
    @ClientInfo(name = "OCSID.ACTION", value = "QueryAuthorByName")
    Author queryByName(String name);
}

[radovanradic]

Yes, but if want to allow fallback for module attribute (to the repository class name) then we do this

@JdbcRepository(dialect = Dialect.ORACLE)
@ClientInfo
public interface OracleXEAuthorRepository extends AuthorRepository {
    @Override
    @Join(value = "books", type = Join.Type.LEFT_FETCH)
    @ClientInfo.Attribute(name = "OCSID.ACTION", value = "QueryAuthorByName")
    Author queryByName(String name);
}

and on methods we can override module or action attributes. That's how it looks like now and I think this is fine.

[dstepanov]

I don't think @ClientInfo without anything makes sense. I would expect exactly what Graeme suggested; otherwise, it needs to be some custom annotation that will transform the method/package into custom client info.

Another option would have some placeholders like MD.METHOD_NAME which we will replace at runtime.

[radovanradic]

Changed now since we can use metadata from SqlPreparedQuery when executing calls and just populate defaults if not provided in the annotations. And will be applied to all Oracle jdbc calls if datasources.<datsource-name>.enable-oracle-client-info=true

[sonarcloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
88.4% Coverage on New Code
3.0% Duplication on New Code

See analysis details on SonarQube Cloud