Skip to content

Prevent crash in SQL Server's JDBC when tracing execute methods with generated keys #9321

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+32 −3
Open

Prevent crash in SQL Server's JDBC when tracing execute methods with generated keys #9321

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69211ed
Prevent crash in SQL Server JDBC when tracing execute methods with ge…
na-ji Aug 6, 2025
94d3981
remove dynamic typing when packing all the JDBC arguments
na-ji Aug 14, 2025
e84d9eb
move JDBC test on injection with generated keys to SQL Server file
na-ji Aug 14, 2025
3a324e0
remove useless constant in StatementAdvice
na-ji Aug 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions ...ation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/StatementInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ public static class StatementAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static AgentScope onEnter(
@Advice.Argument(value = 0, readOnly = false) String sql,
@Advice.AllArguments() Object[] args,
@Advice.This final Statement statement) {
// TODO consider matching known non-wrapper implementations to avoid this check
final int callDepth = CallDepthThreadLocalMap.incrementCallDepth(Statement.class);
Expand Down Expand Up @@ -127,6 +128,21 @@ public static AgentScope onEnter(
// context_info and v$session.action respectively.
// we should not also inject it into SQL comments to avoid duplication
final boolean injectTraceInComment = injectTraceContext && !isSqlServer && !isOracle;

boolean appendComment = StatementInstrumentation.appendComment;
Copy link
Contributor

Choose a reason for hiding this comment

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

Advice should not depend on the instrumentation class, because the advice bytecode will be lifted and injtected into the class that lives in the application class-loader. This happens to work here because StatementInstrumentation.appendComment is a constant which gets inlined by javac, but we should not rely on this. If someone ever changed StatementInstrumentation.appendComment to not be a constant then this advice would break and users would lose JDBC traces.

Can you remove StatementInstrumentation.appendComment from the surrounding class and replace its use here by the literal value false.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be fair, I don't really understand why we have this constant on this class. It used to be used here, and but its type was final static so it wasn't really useful.


// There is a bug in the SQL Server JDBC driver that prevents
// the generated keys from being returned when the
// SQL comment is prepended to the SQL query.
// We only append in this case to avoid the comment from being truncated.
// @see https://github.com/microsoft/mssql-jdbc/issues/2729
if (isSqlServer
&& args.length == 2
&& args[1] instanceof Integer
&& (Integer) args[1] == Statement.RETURN_GENERATED_KEYS) {
appendComment = true;
}

sql =
SQLCommenter.inject(
sql,
Expand Down
15 changes: 15 additions & 0 deletions dd-java-agent/instrumentation/jdbc/src/test/groovy/SQLServerInjectionForkedTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,19 @@ class SQLServerInjectionForkedTest extends AgentTestRunner {
// Verify that the SQL does NOT contain traceparent
assert !statement.sql.contains("traceparent")
}

def "SQL Server apend comment when getting generated keys"() {
setup:
def connection = new TestConnection(false)
def metadata = new TestDatabaseMetaData()
metadata.setURL("jdbc:microsoft:sqlserver://localhost:1433;DatabaseName=testdb;")
connection.setMetaData(metadata)

when:
def statement = connection.createStatement() as TestStatement
statement.executeUpdate(query, 1)

then:
assert statement.sql == "${query} /*${serviceInjection}*/"
}
}