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

Guard against NullPointerException when getting Credentials from Json #701

Merged
merged 8 commits into from
May 7, 2024

Conversation

bennycao
Copy link
Contributor

@bennycao bennycao commented Dec 6, 2023

Changes

This change adds code to guard against possible NullPointerException thrown as a result of f being null in the JsonRequiredTypeAdaptor

Solves #676

Testing

Checklist

@poovamraj
Copy link
Contributor

@bennycao I'd rather prefer us to check if the field is null here like below

if (f!= null && f.getAnnotation(JsonRequired.class) != null) {

Instead of catching the exception. It feels much cleaner and the check makes more sense. Can you try this solution out?

@bennycao
Copy link
Contributor Author

bennycao commented Dec 12, 2023

@bennycao I'd rather prefer us to check if the field is null here like below

if (f!= null && f.getAnnotation(JsonRequired.class) != null) {

Instead of catching the exception. It feels much cleaner and the check makes more sense. Can you try this solution out?

@poovamraj the question i would have is if f is null, what actually happens ? I don't have intimate enough knowledge of the code. Yes it would stop the exception but not sure what the side effect would be.
I did ask this exact question in the issue i raised #676 (comment) .

Raising an exception as i see it allows the consumer to handle it, not just side step the issue.

@poovamraj
Copy link
Contributor

@bennycao yes if the required value like expiresAt is not present for constructing the object, there will be an error thrown, but since this is fixing an issue that no one is able to reproduce, this is the only change that will introduce no new changes to the public API but at the same time solve your issue. If making this change introduces the new issue in your error logs, atleast you will be able to further debug it.

@bennycao bennycao changed the title Catch NullPointerException when getting Credentials from Json Guard against NullPointerException when getting Credentials from Json Jan 2, 2024
@bennycao
Copy link
Contributor Author

bennycao commented Jan 2, 2024

@bennycao yes if the required value like expiresAt is not present for constructing the object, there will be an error thrown, but since this is fixing an issue that no one is able to reproduce, this is the only change that will introduce no new changes to the public API but at the same time solve your issue. If making this change introduces the new issue in your error logs, atleast you will be able to further debug it.

@poovamraj i've made the update as requested.

@bennycao
Copy link
Contributor Author

@poovamraj Any chance we can get this looked thanks ?

@bennycao
Copy link
Contributor Author

bennycao commented Feb 8, 2024

@poovamraj thanks for the approval. When do we think a new release with this fix be available ?
In terms of build status, i'm not sure what is happening there. But let me know if i need to do anything. Thanks

@bennycao
Copy link
Contributor Author

bennycao commented Apr 1, 2024

@poovamraj any movement on getting this into a release ? we would like to close this out asap. Thanks

@poovamraj
Copy link
Contributor

@bennycao you can go ahead with merging the PR after ensuring all checks are passed. We can cut a patch release once this is merged.

@bennycao
Copy link
Contributor Author

bennycao commented Apr 2, 2024

@bennycao you can go ahead with merging the PR after ensuring all checks are passed. We can cut a patch release once this is merged.

@poovamraj it seems some build steps require authorizations to run. And i don't have rights to merge. Can you please authorize these ?

@auth0 auth0 deleted a comment from RMSPORTS1902 May 2, 2024
@auth0 auth0 deleted a comment from RMSPORTS1902 May 2, 2024
@stevehobbsdev stevehobbsdev merged commit 038f66a into auth0:main May 7, 2024
9 of 12 checks passed
@poovamraj poovamraj mentioned this pull request May 8, 2024
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.

4 participants