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

Fix handling missing Spring Security on classpath on Java 8. #1552

Merged
merged 5 commits into from
Jun 23, 2021
Merged

Conversation

maciejwalkowiak
Copy link
Contributor

📜 Description

Fix handling missing Spring Security on classpath on Java 8.

💡 Motivation and Context

@ConditionalOnClass placed on methods ends up with an exception when used with Java 8. This behavior does not exist with Java 11+.

Fixes #1543
Fixes #1546

💚 How did you test it?

This bug is related with how classes are loaded, and the test with filtered class does not exactly reproduces the issue. To reproduce this issue we would need to add a separate Gradle module that has no Spring Security on the classpath. Not sure if it's worth it.

📝 Checklist

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

`@ConditionalOnClass` placed on methods ends up with an exception when used with Java 8. This behavior does not exist with Java 11+.

Fixes #1543
Fixes #1546
CHANGELOG.md Outdated Show resolved Hide resolved
@maciejwalkowiak maciejwalkowiak marked this pull request as ready for review June 23, 2021 08:50
@codecov-commenter
Copy link

Codecov Report

Merging #1552 (718520a) into main (1cdf98b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 718520a differs from pull request most recent head f762acb. Consider uploading reports for the commit f762acb to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1552   +/-   ##
=========================================
  Coverage     76.01%   76.01%           
  Complexity     1944     1944           
=========================================
  Files           192      192           
  Lines          6725     6726    +1     
  Branches        670      670           
=========================================
+ Hits           5112     5113    +1     
  Misses         1287     1287           
  Partials        326      326           
Impacted Files Coverage Δ
...io/sentry/spring/boot/SentryAutoConfiguration.java 98.21% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cdf98b...f762acb. Read the comment docs.

@marandaneto
Copy link
Contributor

@maciejwalkowiak

To reproduce this issue we would need to add a separate Gradle module that has no Spring Security on the classpath. Not sure if it's worth it.

yeah don't think so, otherwise we'd need a new gradle module for everything that loads at runtime.
a way around it would be using Gradle fixtures, https://github.com/getsentry/sentry-android-gradle-plugin/tree/main/plugin-build/src/test/resources/testFixtures/appTestProject

and then you can do things like this: https://github.com/getsentry/sentry-android-gradle-plugin/blob/main/plugin-build/src/test/kotlin/io/sentry/android/gradle/SentryPluginTest.kt#L32-L88

but not worth it either I guess, unless this type of problem happens again.

@marandaneto marandaneto merged commit 007cf3d into main Jun 23, 2021
@marandaneto marandaneto deleted the gh-1543 branch June 23, 2021 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants