Skip to content

NIFI-14469 - Support custom fields in AccessToken#9872

Merged
exceptionfactory merged 5 commits intoapache:mainfrom
pvillard31:NIFI-14469
Apr 17, 2025
Merged

NIFI-14469 - Support custom fields in AccessToken#9872
exceptionfactory merged 5 commits intoapache:mainfrom
pvillard31:NIFI-14469

Conversation

@pvillard31
Copy link
Contributor

Summary

NIFI-14469 - Support custom fields in AccessToken

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 21

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks @pvillard31, the concept sounds good, but it looks like the implementation approach should be adjusted. I can provide more details on options if needed, but it should be possible to configure the Jackson implementation to handle the custom fields, without necessarily annotating the AccessToken class itself.

@pvillard31
Copy link
Contributor Author

Thanks for the review @exceptionfactory - I just pushed a commit where I'm instead using a custom deserializer to not add a Jackson dependency on the API. The only downside I see with this approach is that the deserializer would be needed in all bundles where an implementation requires this feature. But I guess it's ok as we should not see so many implementations.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @pvillard31.

Have you evaluated using the JsonView annotation on a subclass of AccessToken within the StandardOauth2AccessTokenProvider? Having an internal subclass of AccessToken, with an interface method annotated with JsonAnySetter seems like an approach that would work without having to implement a full Deserializer.

@pvillard31
Copy link
Contributor Author

Thanks @exceptionfactory - I was not aware of the JsonView annotation. I just pushed a commit to leverage it. Let me know if this is what you had in mind.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @pvillard31, this looks close to what I had in mind. I noted a couple adjustments to keep the mapping view internal to the implementation module, otherwise, this looks close to completion.

@pvillard31
Copy link
Contributor Author

Addressed your comments - this concept of JsonView is new to me, appreciate the guidance!

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @pvillard31, after taking another look at available options, the mix-in strategy appears to be the most concise solution, enabling a simple interface with matching method signature to apply the JsonAnySetter annotation.

I pushed that change and also renamed the Map to additionalParameters, which follows the convention of Spring Security's OAuth2AccessTokenReponse class. I also changed the Map value type from String to Object, since it could be something else, such as a number.

If these changes look good, I circle back to complete the review.

@pvillard31
Copy link
Contributor Author

Changes LGTM, thanks @exceptionfactory

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for the concurrence @pvillard31! +1 merging

@exceptionfactory exceptionfactory merged commit ad3dc61 into apache:main Apr 17, 2025
6 checks passed
TomaszK-stack pushed a commit to TomaszK-stack/nifi that referenced this pull request May 5, 2025
…ken (apache#9872)

Co-authored-by: David Handermann <exceptionfactory@apache.org>
Signed-off-by: David Handermann <exceptionfactory@apache.org>
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