Skip to content

Replace direct servlet advice tests with instrumentation coverage #12696

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

Merged
dougqh merged 1 commit into from
Oct 2, 2026
Merged

Replace direct servlet advice tests with instrumentation coverage #12696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions ...t/jakarta-servlet-5.0/src/test/groovy/JakartaHttpServletRequestInstrumentationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,24 @@ class JakartaHttpServletRequestInstrumentationTest extends InstrumentationSpecif
suite << testSuite()
}

void 'getSession tolerates a null servlet context'() {
setup:
final module = Mock(ApplicationModule)
InstrumentationBridge.registerIastModule(module)
final session = Mock(HttpSession)
final delegate = Mock(HttpServletRequest)
final request = new CustomRequest(request: delegate)

when:
final result = request.getSession()

@chatgpt-codex-connector chatgpt-codex-connector Bot Sep 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep a test that exposes null-context advice failures

This instrumented call cannot detect the regression the deleted direct-advice test covered: GetHttpSessionAdvice uses @Advice.OnMethodExit(suppress = Throwable.class), so if the null guard is removed and the context store again throws on get(null), Byte Buddy suppresses that exception and this test still receives the session, observes getServletContext(), and sees no module calls. The equivalent javax test at JettyServlet3Test.groovy:583 has the same blind spot; retain a direct advice invocation or otherwise assert that the advice body completes past the potentially throwing context-store access.

Useful? React with 👍 / 👎.

@ygree ygree Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That reasoning overlooks the test harness: suppressed advice exceptions are recorded in InstrumentationErrors, and InstrumentationSpecification checks for those errors during cleanup. Removing the guard therefore fails the replacement tests even if the session is returned and the module remains untouched.

Starting test: getSession tolerates a null servlet context from JakartaHttpServletRequestInstrumentationTest
09:18:52.859 [Test worker] DEBUG datadog.trace.bootstrap.ExceptionLogger - Failed to handle exception in instrumentation for JakartaHttpServletRequestInstrumentationTest$CustomRequest - datadog.trace.instrumentation.servlet5.IastOptOutJakartaHttpServletRequestInstrumentation$GetHttpSessionAdvice
java.lang.NullPointerException: null
	at com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap.get(WeakConcurrentMap.java:109)
	at datadog.trace.agent.tooling.WeakMaps$Adapter.get(WeakMaps.java:69)
	at datadog.trace.bootstrap.WeakMapPerStore.get(WeakMapPerStore.java:33)
	at datadog.trace.bootstrap.FieldBackedContextStore.get(FieldBackedContextStore.java:26)
	at JakartaHttpServletRequestInstrumentationTest$CustomRequest.getSession(JakartaHttpServletRequestInstrumentationTest.groovy:515)


then:
result.is(session)
1 * delegate.getSession() >> session
1 * delegate.getServletContext() >> null

@datadog-datadog-prod-us1-2 datadog-datadog-prod-us1-2 Bot Sep 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Replacement test cannot observe suppressed advice failures

If the null-context guard regresses, the context-store access throws, but @Advice.OnMethodExit(suppress = Throwable.class) swallows that failure. The request still returns the session and the module remains untouched, so both replacement tests pass despite the regression; deleting the direct advice tests therefore removes the effective check that the guard prevents an instrumentation error.

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session

@ygree ygree Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That reasoning overlooks the test harness: suppressed advice exceptions are recorded in InstrumentationErrors, and InstrumentationSpecification checks for those errors during cleanup. Removing the guard therefore fails the replacement tests even if the session is returned and the module remains untouched.

Starting test: getSession tolerates a null servlet context from IastJettyServlet3ForkedTest
09:22:25.972 [Test worker] DEBUG datadog.trace.bootstrap.ExceptionLogger - Failed to handle exception in instrumentation for IastJettyServlet3ForkedTest$CustomRequest - datadog.trace.instrumentation.servlet3.IastOptOutHttpServletRequest3Instrumentation$GetHttpSessionAdvice
java.lang.NullPointerException: null
	at com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap.get(WeakConcurrentMap.java:109)
	at datadog.trace.agent.tooling.WeakMaps$Adapter.get(WeakMaps.java:69)
	at datadog.trace.bootstrap.WeakMapPerStore.get(WeakMapPerStore.java:33)
	at datadog.trace.bootstrap.FieldBackedContextStore.get(FieldBackedContextStore.java:26)
	at IastJettyServlet3ForkedTest$CustomRequest.getSession(JettyServlet3Test.groovy:596)

0 * module._
}

protected <E> E runUnderIastTrace(Closure<E> cl) {
final ddctx = new TagContext().withRequestContextDataIast(iastCtx)
final span = TEST_TRACER.startSpan("test", "test-iast-span", ddctx)
Expand Down
63 changes: 0 additions & 63 deletions ...et-5.0/src/test/java/datadog/trace/instrumentation/servlet5/GetHttpSessionAdviceTest.java
View file
Open in desktop

This file was deleted.

27 changes: 27 additions & 0 deletions ...entation/servlet/javax-servlet/javax-servlet-3.0/src/test/groovy/JettyServlet3Test.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import javax.servlet.annotation.WebServlet
import javax.servlet.http.HttpServlet
import javax.servlet.http.HttpServletRequest
import javax.servlet.http.HttpServletResponse
import javax.servlet.http.HttpSession

import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.CUSTOM_EXCEPTION
import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR
Expand Down Expand Up @@ -570,6 +571,32 @@ class JettyServlet3ServeFromAsyncTimeout extends JettyServlet3Test {

class IastJettyServlet3ForkedTest extends JettyServlet3TestSync {

void 'getSession tolerates a null servlet context'() {
setup:
final module = Mock(ApplicationModule)
InstrumentationBridge.registerIastModule(module)
final session = Mock(HttpSession)
final delegate = Mock(HttpServletRequest)
final request = new CustomRequest(request: delegate)

when:
final result = request.getSession()

then:
result.is(session)
1 * delegate.getSession() >> session
1 * delegate.getServletContext() >> null
0 * module._

cleanup:
InstrumentationBridge.clearIastModules()
}

private static class CustomRequest implements HttpServletRequest {
@Delegate
private HttpServletRequest request
}

@Override
Class<Servlet> servlet() {
return TestServlet3.GetSession
Expand Down
63 changes: 0 additions & 63 deletions ...et-3.0/src/test/java/datadog/trace/instrumentation/servlet3/GetHttpSessionAdviceTest.java
View file
Open in desktop

This file was deleted.

Loading