Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: EXPOSED-730 Extract TransactionManager companion methods from core module #2405

Merged
merged 4 commits into from
Feb 13, 2025

Conversation

bog-walk
Copy link
Member

@bog-walk bog-walk commented Feb 12, 2025

This PR is meant only for merge into R2DBC branch r2dbc-poc-2

Note about the diff size

  • The majority of changes in this PR is just the removal of now useless casts.

Description

Summary of the change: exposed-jdbc and exposed-r2dbc now have their own TransactionManager with companion object methods, which rely on stored data in common fields found in new exposed-core/CoreManager object.

Detailed description:

  • Why:

Current setup has common TransactionManager interface in core module, which uses its companion object to store and process internal data about registered databases and their associated managers. This means that the JDBC and R2DBC implementations would need to always cast when calls like TransactionManager.current() are made.

  • How:
    • Remove companion object from original core TransactionManager interface and rename the latter to TransactionManagerApi.
    • Create new object CoreManager to keep all original content from above.
      • Some new functions are added to allow internal use in core module of some data: currentTransaction(), currentTransactionOrNull.
      • Some new functions are meant to be called by jdbc/r2dbc implementations so data collections can remain private.
      • Replace all uses of TransactionManager... in exposed-core with CoreManager....
      • Mark this as @InternalApi
      • Add opt-in compiler option to build.gradle.kts to avoid annotating every use of CoreManager in core module
    • Rename both module managers to TransactionManager so API hasn't changed much & add their own companion objects that rely on CoreManager data
      • Add back original ThreadLocalTransactionManager to exposed-jdbc with Deprecation annotation.
      • Adjust signature of manager parameter in Database.connect() variants to account for existing deprecated class.

Type of Change

Please mark the relevant options with an "X":

  • Other - refactor

Affected databases:

  • All

Checklist

  • The build is green (including the Detekt check)
  • All public methods affected by my PR has up to date API docs

Related Issues

EXPOSED-730

@bog-walk bog-walk requested review from e5l and obabichevjb February 12, 2025 02:50
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It looks like use sites generally looks cleaner now

@@ -230,7 +230,7 @@ abstract class AbstractQuery<T : AbstractQuery<T>>(
}
if (set.source != Table.Dual || currentDialect.supportsDualTableConcept) {
append(" FROM ")
set.source.describe(TransactionManager.current(), this)
set.source.describe(CoreManager.currentTransaction(), this)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be more specific and and use CoreTransactionManager name

import java.util.concurrent.ConcurrentLinkedDeque
import java.util.concurrent.atomic.AtomicReference

private object NotInitializedManager : TransactionManagerApi {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same applied to the naming in other places

@bog-walk bog-walk force-pushed the bog-walk/split-transaction-manager branch from 1215a7e to 74f4e06 Compare February 13, 2025 18:38
…om core module

As part of the R2DBC implementation:
- Remove companion object from original interface and rename to TransactionManagerApi
- Create new object CoreManager that stores members responsible for internal processing
of databases and their associated managers
- Mark this as @internalapi
- Rename both module managers to TransactionManager & add their own companion objects
that rely on CoreManager
- Remove useless casts from everywhere
- Move transaction properties out of core so that there is no need to define
a core TM
…om core module

- Remove strange import optimization results
- Return original ThreadLocalTransactionManager into jdbc module with deprecation flag
- Refactor Database.connect() variants to account for existing deprecated class
…om core module

- Rename new object to CoreTransactionManager
- Rename private object to NotInitializedTransactionManager
@bog-walk bog-walk force-pushed the bog-walk/split-transaction-manager branch from 74f4e06 to 24978aa Compare February 13, 2025 19:48
@bog-walk bog-walk merged commit 21a19ba into r2dbc-poc-2 Feb 13, 2025
4 checks passed
@bog-walk bog-walk deleted the bog-walk/split-transaction-manager branch February 13, 2025 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants