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

radovanradic · 2024-10-17T12:25:35Z

Created as draft, not sure if this approach is correct

...java/io/micronaut/data/connection/jdbc/operations/DefaultDataSourceConnectionOperations.java

radovanradic · 2024-10-17T12:28:55Z

data-connection/src/main/java/io/micronaut/data/connection/annotation/Connectable.java

@@ -83,4 +83,29 @@
     */
    boolean readOnly() default false;

+    /**


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 · 2024-10-17T12:34:17Z

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>

...onnection/src/main/java/io/micronaut/data/connection/interceptor/ConnectableInterceptor.java

dstepanov · 2024-10-17T14:30:30Z

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

...java/io/micronaut/data/connection/jdbc/operations/DefaultDataSourceConnectionOperations.java

...onnection/src/main/java/io/micronaut/data/connection/interceptor/ConnectableInterceptor.java

radovanradic · 2024-10-18T13:17:17Z

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 · 2024-10-18T14:01:10Z

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

radovanradic · 2024-10-18T14:50:16Z

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 · 2024-10-18T15:08:46Z

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

radovanradic · 2024-10-18T15:13:28Z

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 · 2024-10-20T15:00:51Z

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

graemerocher · 2024-10-20T15:03:11Z

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 · 2024-10-21T08:22:28Z

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.

...a/io/micronaut/data/connection/jdbc/oracle/OracleClientInfoCustomizerConnectionListener.java

.../main/java/io/micronaut/data/connection/jdbc/oracle/OracleClientInfoCustomizerCondition.java

src/main/docs/guide/dbc/jdbc/jdbcConfiguration.adoc

.../main/java/io/micronaut/data/connection/jdbc/oracle/OracleClientInfoCustomizerCondition.java

Co-authored-by: Graeme Rocher <[email protected]>

…t info connection listener

dstepanov · 2024-11-19T08:57:54Z

...java/io/micronaut/data/connection/jdbc/operations/DefaultDataSourceConnectionOperations.java

        } catch (SQLException e) {
            throw new CannotGetJdbcConnectionException("Failed to obtain JDBC Connection", e);
        }
+
+        return new DefaultConnectionStatus<>(connection, definition, true);


Why is this change?

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

I think it can be done without this change

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

dstepanov · 2024-11-19T08:59:53Z

...nection/src/main/java/io/micronaut/data/connection/support/AbstractConnectionOperations.java

    /**
     * Opens a new connection.
     *
     * @param definition The connection definition
     * @return The connection
     */
-    protected abstract C openConnection(ConnectionDefinition definition);
+    protected final C openConnection(ConnectionDefinition definition) {


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

dstepanov · 2024-11-19T09:00:25Z

...nection/src/main/java/io/micronaut/data/connection/support/AbstractConnectionOperations.java

-    protected abstract C openConnection(ConnectionDefinition definition);
+    protected final C openConnection(ConnectionDefinition definition) {
+        ConnectionStatus<C> connectionStatus = doOpenConnection(definition);
+        for (ConnectionListener<C> connectionListener : connectionListeners) {


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

dstepanov · 2024-11-19T09:02:33Z

data-connection/src/main/java/io/micronaut/data/connection/ConnectionDefinition.java

+     * @return An instance of {@link ConnectionClientInfoDetails} representing the connection client information, or null if not set.
+     * @since 4.10
+     */
+    @Nullable ConnectionClientInfoDetails connectionClientInfo();


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

dstepanov · 2024-11-19T09:04:12Z

...onnection/src/main/java/io/micronaut/data/connection/interceptor/ConnectableInterceptor.java

+     * @param appName The micronaut application name, null if not set
+     * @return The connection client info or null if not configured to be used
+     */
+    private static @Nullable ConnectionClientInfoDetails getConnectionClientInfo(AnnotationValue<io.micronaut.data.connection.annotation.ConnectionClientInfo> annotation,


Move to oracle listener

dstepanov · 2024-11-19T09:05:09Z

data-connection/src/main/java/io/micronaut/data/connection/support/ConnectionListener.java

+     *
+     * @return the name of this listener
+     */
+    String getName();


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

data-connection/src/main/java/io/micronaut/data/connection/annotation/ConnectionClientInfo.java

dstepanov · 2024-11-19T09:06:17Z

...ion/src/main/java/io/micronaut/data/connection/annotation/ConnectionClientInfoAttribute.java

+ *
+ * @since 4.10
+ */
+public @interface ConnectionClientInfoAttribute {


Make inner annotation of OracleConnectionClientInfo

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

dstepanov · 2024-11-19T09:07:03Z

data-mongodb/src/main/java/io/micronaut/data/mongodb/session/MongoConnectionOperationsImpl.java

        this.mongoClient = mongoClient;
    }

    @Override
-    protected ClientSession openConnection(ConnectionDefinition definition) {
-        return mongoClient.startSession();
+    protected ConnectionStatus<ClientSession> doOpenConnection(ConnectionDefinition definition) {


Please avoid changes in other connection operations

dstepanov · 2024-11-19T09:10:08Z

...c/main/java/io/micronaut/data/connection/jdbc/oracle/OracleClientInfoConnectionListener.java

+
+    @Override
+    public boolean supportsConnection(@NonNull ConnectionStatus<Connection> connectionStatus) {
+        return connectionSupportedMap.computeIfAbsent(connectionStatus.getConnection(), this::isOracleConnection);


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 · 2024-11-19T14:33:23Z

...c/main/java/io/micronaut/data/connection/jdbc/oracle/OracleClientInfoConnectionListener.java

+ */
+@EachBean(DataSource.class)
+@Requires(condition = OracleClientInfoCondition.class)
+@Context


@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.

...c/main/java/io/micronaut/data/connection/jdbc/oracle/OracleClientInfoConnectionListener.java

dstepanov · 2024-11-19T14:44:57Z

data-connection/src/main/java/io/micronaut/data/connection/annotation/ConnectionClientInfo.java

+    /**
+     * @return whether connection should set client info
+     */
+    boolean enabled() default true;


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

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

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")

Changed, removed enabled and added repeatable.

sonarcloud · 2024-11-19T16:44:55Z

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 · 2024-11-22T09:54:52Z

data-jdbc/src/test/java/io/micronaut/data/jdbc/oraclexe/OracleXEAuthorRepository.java

 public interface OracleXEAuthorRepository extends AuthorRepository {
    @Override
    @Join(value = "books", type = Join.Type.LEFT_FETCH)
+    @ConnectionClientInfoAttribute(name = "OCSID.ACTION", value = "QueryAuthorByName")


I would renamed the annotation to something ahorter

graemerocher · 2024-11-25T14:43:34Z

data-connection/src/main/java/io/micronaut/data/connection/annotation/ConnClientInfoAttr.java

+@Retention(RetentionPolicy.RUNTIME)
+@Repeatable(ConnClientInfo.class)
+@Connectable
+public @interface ConnClientInfoAttr {


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

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.

why is the annotation needed on the type?

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)

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); }

sonarcloud · 2024-11-25T18:01:23Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

radovanradic added 9 commits October 15, 2024 17:18

Investigation for setting connection client info

b45b532

Merge branch '4.10.x' into jdbc-clientinfo

e0b6f4a

Use Connectable interface to pass connection client tracing info

19e58f0

Merge branch '4.10.x' into jdbc-clientinfo

b8ed459

Remove unneeded changes

92902bd

Use connectable for Oracle Book repo

afb396d

Merge branch '4.10.x' into jdbc-clientinfo

a481771

Documentation, comments and code cleanup.

27bdc07

Merge branch '4.10.x' into jdbc-clientinfo

8bb94e8

radovanradic commented Oct 17, 2024

View reviewed changes

...java/io/micronaut/data/connection/jdbc/operations/DefaultDataSourceConnectionOperations.java Outdated Show resolved Hide resolved

radovanradic commented Oct 17, 2024

View reviewed changes

...onnection/src/main/java/io/micronaut/data/connection/interceptor/ConnectableInterceptor.java Outdated Show resolved Hide resolved

radovanradic requested review from dstepanov and graemerocher October 17, 2024 12:39

Rename ConnectionClientTracingInfo to ConnectionTracingInfo

7874bdf

graemerocher requested changes Oct 18, 2024

View reviewed changes

...java/io/micronaut/data/connection/jdbc/operations/DefaultDataSourceConnectionOperations.java Outdated Show resolved Hide resolved

...onnection/src/main/java/io/micronaut/data/connection/interceptor/ConnectableInterceptor.java Outdated Show resolved Hide resolved

radovanradic added 4 commits October 21, 2024 10:56

Changes per CR comments, still not there.

41fd257

Merge branch '4.10.x' into jdbc-clientinfo

8e70480

Change as suggested in PR comment

71e66de

Properly clear connection client info

7a16299

radovanradic marked this pull request as ready for review November 18, 2024 14:44

radovanradic requested review from dstepanov and graemerocher November 18, 2024 14:44

Updated comments

fdff622

graemerocher requested changes Nov 18, 2024

View reviewed changes

radovanradic and others added 5 commits November 18, 2024 16:56

Update src/main/docs/guide/dbc/jdbc/jdbcConfiguration.adoc

d56a78b

Co-authored-by: Graeme Rocher <[email protected]>

Update src/main/docs/guide/dbc/jdbc/jdbcConfiguration.adoc

c25d742

Co-authored-by: Graeme Rocher <[email protected]>

Update src/main/docs/guide/dbc/jdbc/jdbcConfiguration.adoc

f6b7f80

Co-authored-by: Graeme Rocher <[email protected]>

Update src/main/docs/guide/dbc/jdbc/jdbcConfiguration.adoc

1f9e9e9

Co-authored-by: Graeme Rocher <[email protected]>

Renamed classes as suggested and also property to enable Oracle clien…

c68b657

…t info connection listener

graemerocher approved these changes Nov 18, 2024

View reviewed changes

dstepanov requested changes Nov 19, 2024

View reviewed changes

Refactoring according to suggestions in PR comments.

06901f3

radovanradic commented Nov 19, 2024

View reviewed changes

dstepanov reviewed Nov 19, 2024

View reviewed changes

...c/main/java/io/micronaut/data/connection/jdbc/oracle/OracleClientInfoConnectionListener.java Outdated Show resolved Hide resolved

dstepanov reviewed Nov 19, 2024

View reviewed changes

Cleanup

5adc61e

radovanradic requested a review from dstepanov November 19, 2024 14:45

radovanradic added 3 commits November 19, 2024 15:53

Cache module name by the class name

f66881f

Remove enabled attribute and create repeatable annotation

5c97b15

Fixes for Sonar

da4e8a3

radovanradic added 2 commits November 20, 2024 12:08

Merge branch '4.10.x' into jdbc-clientinfo

913d9a8

Update @SInCE attributes

886b103

dstepanov approved these changes Nov 22, 2024

View reviewed changes

Rename classes to shorter names

60458ad

graemerocher requested changes Nov 25, 2024

View reviewed changes

Rename connection client info annotations

e0099b8

radovanradic requested a review from graemerocher November 25, 2024 17:17

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

Are you sure you want to change the base?

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

Conversation

radovanradic commented Oct 17, 2024

Choose a reason for hiding this comment

radovanradic commented Oct 17, 2024

dstepanov commented Oct 17, 2024

graemerocher left a comment

Choose a reason for hiding this comment

radovanradic commented Oct 18, 2024 • edited Loading

graemerocher commented Oct 18, 2024

radovanradic commented Oct 18, 2024

graemerocher commented Oct 18, 2024

radovanradic commented Oct 18, 2024

graemerocher commented Oct 20, 2024

graemerocher commented Oct 20, 2024

radovanradic commented Oct 21, 2024

Choose a reason for hiding this comment

radovanradic Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstepanov Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 19, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radovanradic Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 25, 2024

Quality Gate passed

radovanradic commented Oct 18, 2024 •

edited

Loading

radovanradic Nov 19, 2024 •

edited

Loading

dstepanov Nov 19, 2024 •

edited

Loading

radovanradic Nov 25, 2024 •

edited

Loading