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

[SDK-2758] Restore withIssuer #513

Merged
merged 2 commits into from
Sep 10, 2021
Merged

[SDK-2758] Restore withIssuer #513

merged 2 commits into from
Sep 10, 2021

Conversation

jimmyjames
Copy link
Contributor

Changes

#288 changed the signature of withIssuer(String issuer) to withIssuer(String... issuer). As discussed in #507, this is a binary incompatible change.

This change restores that binary compatibility, by introducing a new default method on the Verification interface, that simply delegates to the existing implementation.

Notes:

  • This change will cause the apiDiff check to fail, as the japicmp gradle plugin does not currently support configuring this check (see Support for overrideCompatibilityChangeParameters melix/japicmp-gradle-plugin#43)
  • Rather then configure the plugin to ignore the Verification interface, which would then never catch any issues, we'll need to bump the baseline compare version when this change is released.
  • Regarding if adding a default method to an existing interface is binary compatible, this documentation from Oracle provides a good explanation on why it should be considered binary compatible.
  • No tests were added as part of this change, as the JWTVerifierTests already contain a test that verify the behavior when calling withIssuer with a single string.
  • When this change is released, we will need to update the CHANGELOG for 3.8.0 to indicate the binary incompatible change, and note its fixed version.

@jimmyjames jimmyjames added this to the v3-Next milestone Sep 3, 2021
@jimmyjames jimmyjames requested a review from a team as a code owner September 3, 2021 20:48
@lbalmaceda lbalmaceda linked an issue Sep 7, 2021 that may be closed by this pull request
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@ebickle ebickle left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@lbalmaceda lbalmaceda merged commit fc0ecba into master Sep 10, 2021
@lbalmaceda lbalmaceda deleted the restore-with-issuer branch September 10, 2021 07:55
@jimmyjames jimmyjames modified the milestones: v3-Next, 3.18.2 Sep 16, 2021
@jimmyjames jimmyjames mentioned this pull request Sep 16, 2021
poovamraj added a commit that referenced this pull request Jan 13, 2022
* Upgrade jackson dependency

* Remove ignored files

* Update baseline compare version to 3.18.2 as mentioned in PR #513
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.8.0 introduced a breaking change
4 participants