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

Accept only non null value maps #1368

Merged
merged 2 commits into from
Mar 31, 2021
Merged

Accept only non null value maps #1368

merged 2 commits into from
Mar 31, 2021

Conversation

wzieba
Copy link
Contributor

@wzieba wzieba commented Mar 30, 2021

Resolves: #1366

📜 Description

This PR adds org.jetbrains.annotations.NotNull annotation for a places, where SDK could receive a Map from a client, which has a nullable type for value generic parameter.

I've also added this annotation to internal API methods and some fields to which ConcurrentHashMap is assigned.

💡 Motivation and Context

ConcurrentHashMap can't have a null for any of it's entries value, otherwise NullPointerException is thrown. By adding this NotNull annotation we:

  • raise awareness of this issue for Java clients
  • have a build-time check for Kotlin clients:
    image

💚 How did you test it?

Unfortunately, I don't know how to test it, I'm open to any suggestions.

📝 Checklist

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

🔮 Next steps

@codecov-io
Copy link

Codecov Report

Merging #1368 (2f0dad9) into main (ed6e293) will decrease coverage by 0.01%.
The diff coverage is 90.90%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1368      +/-   ##
============================================
- Coverage     75.69%   75.67%   -0.02%     
  Complexity     1856     1856              
============================================
  Files           185      185              
  Lines          6360     6361       +1     
  Branches        635      636       +1     
============================================
  Hits           4814     4814              
  Misses         1258     1258              
- Partials        288      289       +1     
Impacted Files Coverage Δ Complexity Δ
sentry/src/main/java/io/sentry/protocol/App.java 96.55% <ø> (ø) 18.00 <0.00> (ø)
...ntry/src/main/java/io/sentry/protocol/Browser.java 100.00% <ø> (ø) 8.00 <0.00> (ø)
...entry/src/main/java/io/sentry/protocol/Device.java 99.04% <ø> (ø) 66.00 <0.00> (ø)
sentry/src/main/java/io/sentry/protocol/Gpu.java 100.00% <ø> (ø) 22.00 <0.00> (ø)
.../main/java/io/sentry/protocol/OperatingSystem.java 100.00% <ø> (ø) 16.00 <0.00> (ø)
...rc/main/java/io/sentry/protocol/SentryRuntime.java 100.00% <ø> (ø) 10.00 <0.00> (ø)
sentry/src/main/java/io/sentry/protocol/User.java 100.00% <ø> (ø) 15.00 <0.00> (ø)
.../src/main/java/io/sentry/util/CollectionUtils.java 40.00% <ø> (ø) 3.00 <0.00> (ø)
.../config/EnvironmentVariablePropertiesProvider.java 92.85% <66.66%> (-7.15%) 6.00 <0.00> (ø)
sentry/src/main/java/io/sentry/Breadcrumb.java 93.75% <100.00%> (ø) 21.00 <1.00> (ø)
... and 4 more

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 ed6e293...2f0dad9. Read the comment docs.

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

thanks for the contribution @wzieba :)

@marandaneto
Copy link
Contributor

@wzieba

Unfortunately, I don't know how to test it, I'm open to any suggestions.

as we write tests in Kotlin and compile fails if passing a null value to a @NotNull param, the only way is thru reflection, it's not worth IMO, we've done at the beginning but if users are aware that they can't pass null, it's already good enough :) thanks once more.

@marandaneto marandaneto merged commit bc10b2b into getsentry:main Mar 31, 2021
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.

NPE while applying Map with nullable value to User#setOthers
3 participants