search

auth0 / Auth0.Android

Android toolkit for Auth0 API
https://auth0.com
MIT License
208 stars 130 forks source link

Guard against NullPointerException when getting Credentials from Json #701

Closed bennycao closed 2 months ago

bennycao commented 7 months ago

Changes

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

Solves https://github.com/auth0/Auth0.Android/issues/676

Testing

Checklist

poovamraj commented 7 months ago

@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 commented 7 months ago

@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 https://github.com/auth0/Auth0.Android/issues/676#issuecomment-1685422498 .

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

poovamraj commented 7 months ago

@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 commented 6 months ago

@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 commented 6 months ago

@poovamraj Any chance we can get this looked thanks ?

bennycao commented 5 months ago

@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 commented 3 months ago

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

poovamraj commented 3 months ago

@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 commented 3 months ago

@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 ?