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

Transaction Names should be truncated when too long. #264

Closed
nhajratw opened this issue Mar 16, 2023 · 3 comments
Closed

Transaction Names should be truncated when too long. #264

nhajratw opened this issue Mar 16, 2023 · 3 comments
Labels
type: enhancement A general enhancement
Milestone

Comments

@nhajratw
Copy link
Contributor

Bug Report

Current Behavior

When using r2dbc-mssql via spring-r2dbc with @Transactional, the name of the transaction is the FQ class name. This cannot be overridden:

https://docs.spring.io/spring-framework/docs/current/reference/html/data-access.html#transaction-declarative-attransactional-settings

Currently, you cannot have explicit control over the name of a transaction, where 'name' means the transaction name that appears in a transaction monitor, if applicable (for example, WebLogic’s transaction monitor), and in logging output. For declarative transactions, the transaction name is always the fully-qualified class name + . + the method name of the transactionally advised class. For example, if the handlePayment(..) method of the BusinessService class started a transaction, the name of the transaction would be: com.example.BusinessService.handlePayment.

r2dbc-mssql is correctly limiting the transaction name to 32 characters per https://learn.microsoft.com/en-us/sql/t-sql/language-elements/begin-transaction-transact-sql?view=sql-server-ver16#arguments

As such, we end up with this exception:

Exception in thread "DefaultDispatcher-worker-1" org.springframework.transaction.CannotCreateTransactionException: Could not open R2DBC Connection for transaction
	at ...
	at java.base/java.lang.Thread.run(Thread.java:833)
	Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@2800d129, Dispatchers.Default]
Caused by: java.lang.IllegalArgumentException: Transaction names must contain only characters and numbers and must not exceed 32 characters
	at io.r2dbc.mssql.util.Assert.isTrue(Assert.java:106)
	at io.r2dbc.mssql.MssqlConnection.lambda$beginTransaction$1(MssqlConnection.java:110)
	at io.r2dbc.mssql.MssqlConnection.lambda$useTransactionStatus$15(MssqlConnection.java:425)
	at reactor.core.publisher.FluxDefer.subscribe(FluxDefer.java:46)
	... 59 more

Steps to reproduce

Any class like the following will reproduce the problem:

package com.company.name.here

@Service
class SomeServiceClass() {

    @Transactional
    suspend fun theMethod() {
        ...
    }
}

the transaction name here will be com.company.name.here.SomeServiceClass.theMethod which is > 32 chars an errors.

Possible Solution

Truncate the beginning of the name so that you end up with the last 32 characters (excluding punctuation): i.e. here.SomeServiceClass.theMethod

@anassder
Copy link

MSSQL truncates transaction's name automatically if it exceed 32 characters, so we can simply show warning instead of throwing exception.

@nhajratw
Copy link
Contributor Author

agreed -- that could work as well. The reason i suggested truncating the beginning is that if you have two different methods:

com.company.name.here.SomeServiceClass.theMethod1
com.company.name.here.SomeServiceClass.theMethod2

Then truncating the beginning would give both transactions the same name of com.company.name.here.SomeServic, which would make it impossible to differentiate when looking at logs.

@mp911de mp911de added the type: enhancement A general enhancement label Mar 28, 2023
@mp911de mp911de added this to the 0.9.1.RELEASE milestone Mar 28, 2023
@mp911de
Copy link
Member

mp911de commented Mar 28, 2023

Happy to include a truncation. We might not log this event but rather silently accept the fact that we're truncating the transaction name.

The issue originates from a default behavior where you (do/can) not specify the transaction name. Hence we can just remove the obstacle that causes transactions to fail. A natural consequence of truncating is information loss. If the data beforehand would be correct, then we would not run into that issue.

mp911de pushed a commit that referenced this issue Mar 28, 2023
Signed-off-by: Nayan Hajratwala <nayan@chikli.com>
[resolves #264][#265]
mp911de added a commit that referenced this issue Mar 28, 2023
Return substring from start, instead from the middle of the string to avoid invalid characters being used at the start of the string.

Reformat code.

[#264][resolves #265]

Signed-off-by: Mark Paluch <mpaluch@vmware.com>
mp911de added a commit that referenced this issue Mar 28, 2023
Return substring from start, instead from the middle of the string to avoid invalid characters being used at the start of the string.

Reformat code.

[#264][resolves #265]

Signed-off-by: Mark Paluch <mpaluch@vmware.com>
mp911de pushed a commit that referenced this issue May 25, 2023
Signed-off-by: Nayan Hajratwala <nayan@chikli.com>
[resolves #264][#265]
mp911de added a commit that referenced this issue May 25, 2023
Return substring from start, instead from the middle of the string to avoid invalid characters being used at the start of the string.

Reformat code.

[#264][resolves #265]

Signed-off-by: Mark Paluch <mpaluch@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants