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

Make JWT constants final values #604

Merged
merged 5 commits into from
Aug 31, 2022
Merged

Make JWT constants final values #604

merged 5 commits into from
Aug 31, 2022

Conversation

poovamraj
Copy link
Contributor

@poovamraj poovamraj commented Aug 22, 2022

Changes

We have made all our constants as final values as this would allow us to use them at compile time. This should have been the right way from the beginning as these are constants. This will be a breaking change where we make the fields final so they cannot be mutated. But this behaviour is not expected from the library users, hence we should be able to proceed

References

#603

Checklist

@poovamraj
Copy link
Contributor Author

The build is failing because the fields were not final before. This would mean that these values could be changed by the library user. Hence the API Diff job is throwing an error.

But in reality, these values should not be changed and this wouldn't be a breaking change. (Even if users were changing the values of the variables which is highly unlikely) This should be the right way ahead.

@jimmyjames
Copy link
Contributor

jimmyjames commented Aug 25, 2022

As the apiDiff build check indicates, this technically is a breaking change. It's also a bug that these fields are not marked final - as the fields correspond to the registered claims defined in RFC 7519, they should not be mutable. So, this fix is a good one.

As @poovamraj noted it's not intended that these fields be modified, and such anticipated use is not expected. That said we will need to mark this PR as breaking, clarify the PR description, and note so on the changelog so anyone who may be relying on this bug will be aware of changes required when this is released.

@jimmyjames jimmyjames added this to the v4-Next milestone Aug 25, 2022
jimmyjames
jimmyjames previously approved these changes Aug 29, 2022
@poovamraj poovamraj merged commit fbe8efc into master Aug 31, 2022
@poovamraj poovamraj deleted the making-constants-final branch August 31, 2022 22:32
@jimmyjames jimmyjames modified the milestones: v4-Next, 4.1.0 Sep 9, 2022
@jimmyjames jimmyjames mentioned this pull request Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants