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

Add sentry-servlet-jakarta module #1987

Merged
merged 3 commits into from
Apr 21, 2022
Merged

Add sentry-servlet-jakarta module #1987

merged 3 commits into from
Apr 21, 2022

Conversation

adinauer
Copy link
Member

📜 Description

Add sentry-servlet-jakarta module to support jakarta.servlet-api which has been moved from javax.servlet-api.

💡 Motivation and Context

Partially fixes #1789
Spring and Spring Boot require more investigation: #1984

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

testImplementation(Config.TestLibs.kotlinTestJunit)
testImplementation(Config.TestLibs.mockitoKotlin)
testImplementation(Config.TestLibs.awaitility)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

pulling in spring-boot-starter-test:3.0.0-M2 would have required Java 17

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

}
}

fun mockRequest(url: String, method: String = "GET", headers: Map<String, Any?> = emptyMap()): HttpServletRequest {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a minimalistic replacement for MockHttpServletRequestBuilder and MockHttpServletRequest from spring-test which I didn't want to pull in to avoid Java 17.


verify(fixture.hub).addBreadcrumb(
check { it: Breadcrumb ->
assertEquals("/some-uri", it.getData("url"))
Copy link
Member Author

Choose a reason for hiding this comment

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

This assertion differs from the one for sentry-servlet as we simply set requestURI to the full url including protocol and host there which is not what getRequestURI usually returns according to https://docs.oracle.com/javaee/5/api/javax/servlet/http/HttpServletRequest.html#getRequestURI()

Copy link
Contributor

@maciejwalkowiak maciejwalkowiak left a comment

Choose a reason for hiding this comment

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

Looks good. Have you considered adding a sample project so that we can easily verify that it actually works on servlet container supporting Jakarta EE?

@adinauer
Copy link
Member Author

Have you considered adding a sample project so that we can easily verify that it actually works on servlet container supporting Jakarta EE?

Going to add it

@@ -24,6 +24,7 @@ targets:
maven:io.sentry:sentry-spring:
maven:io.sentry:sentry-spring-boot-starter:
maven:io.sentry:sentry-servlet:
maven:io.sentry:sentry-servlet-jakarta:
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing it on this PR will fail the publish release since it does not exist under https://github.com/getsentry/sentry-release-registry yet

Copy link
Member Author

Choose a reason for hiding this comment

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

Created a PR for adding it there: getsentry/sentry-release-registry#69

@adinauer adinauer merged commit be0ebe0 into 6.x.x Apr 21, 2022
@adinauer adinauer deleted the feat/jakarta-servlet branch April 21, 2022 08:27
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.

3 participants