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

SONARPY-2342 Fix NPE when metaclassFQN resolved to null during a PythonType to Descriptor conversion #2160

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

maksim-grebeniuk-sonarsource
Copy link
Contributor

@maksim-grebeniuk-sonarsource maksim-grebeniuk-sonarsource commented Nov 15, 2024

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. As mentioned, I'm a bit annoyed that we keep getting surprised by these cases. It should not happen IMO.

@@ -132,6 +132,7 @@ private static Descriptor convert(String moduleFqn, String parentFqn, String sym
var metaclassFQN = type.metaClasses()
.stream()
.map(metaClass -> typeFqn(moduleFqn, metaClass))
.filter(Objects::nonNull)

Choose a reason for hiding this comment

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

This here is yet again an example of pitfalls when using streams this way.
You missed this edge case during implementation, and I missed it during review. The coverage was 100%, yet we experienced an NPE after merging.
I think we need to make progress and I don't want to refactor just for the sake of it (maybe typeFqn should return Optional?), but I must admit I'm a bit annoyed that we fall into similar traps so often.

@guillaume-dequenne-sonarsource guillaume-dequenne-sonarsource merged commit 56d18dd into master Nov 15, 2024
13 checks passed
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.

2 participants