`NimbusJwtDecoder` should remove its setters in favor of its builders

[ThomasKasene]

This is perhaps a little controversial, but I've sometimes wondered about the possibility of converting some of the more commonly used setters to return this rather than void.

Case in point, I recently wanted to be able to do something like this:

@Bean
ReactiveJwtDecoder jwtDecoder(/* ... */) {
    return NimbusReactiveJwtDecoder
            .withIssuerLocation(...)
            .webClient(...)
            .jwtValidator(...)
            .build();
}

But I was thwarted by the limitations of the JwkSetUriReactiveJwtDecoderBuilder not allowing me to specify a jwtValidator, so I had to resort to an ugly detour via a variable:

@Bean
ReactiveJwtDecoder jwtDecoder(/* ... */) {
    var jwtDecoder = NimbusReactiveJwtDecoder
            .withIssuerLocation(...)
            .webClient(...)
            .build();
    jwtDecoder.setJwtValidator(...);
    return jwtDecoder;
}

For a little while I was considering submitting a PR to add jwtValidator(...) to the builder, but soon realized that this class contains 4 such builders, and I'd have to duplicate the functionality between them all (and then double that for the non-reactive variant.)

An option, which I believe is becoming more and more common, is to enable developers to chain setters by returning this. If the setJwtValidator(...) method I used above returned the NimbusReactiveJwtDecoder instance, I could at the very least get a compromise going:

@Bean
ReactiveJwtDecoder jwtDecoder(/* ... */) {
    return NimbusReactiveJwtDecoder
            .withIssuerLocation(...)
            .webClient(...)
            .build()
            .setJwtValidator(...);
}

It should be backwards compatible, and as an added benefit, the validation that already exists within setJwtValidator(...) will be applied so we wouldn't have to duplicate that as well.

This is just one example, but it's a common theme perhaps especially in Spring Security.

Thanks for listening to my TED talk! 😄

[marcusdacoregio]

Hi @ThomasKasene, I don't think returning this on setters is a pattern that Spring Security follows, that's why it has those with... methods.

For a little while I was considering submitting a PR to add jwtValidator(...) to the builder, but soon realized that this class contains 4 such builders, and I'd have to duplicate the functionality between them all (and then double that for the non-reactive variant.)

I did not check the code, but you are right, sometimes we have to make the exact change in multiple places.

That said, if you are willing to add a withJwtValidator you check with @sjohnr if that is something that we want, otherwise we can close this.

[ThomasKasene]

Thanks for your response!

I absolutely recognize that this is different from how things have traditionally been done (hence the "controversial" disclaimer at the top 😄 ). But I saw #13266 and figured, since usability improvements is already such a big thing, maybe it's worth enabling an arguably more modern programming style too.

Anyway, take it or leave it - I can still add a jwtValidator(...) method to the various builders if @sjohnr gives his blessing 😃

[sjohnr]

Hi @ThomasKasene, sorry for the delay. From my point of view, I see no issue with adding additional methods to the builders (and duplicating some code when necessary). However, I have not had much impact on the JwtDecoder APIs to date, so it might be nice to hear if @jzheaux has any thoughts on whether additional builder methods make sense before going too far.

Having said that, I don't think it makes sense to simply start adding chainability to setters, as this would potentially set a precedent that should be discussed more thoroughly, and further may not be desired at all. I think for consistency, focusing on the builder pattern and enhancing builders would be the preferred route.

If adding jwtValidator() to the builder(s) makes sense, you might also look at adding claimSetConverter() as well.

Regarding gh-13266, we're quite early days on that and we want to be strategic in approaching big changes. I think it's awesome to get suggestions like this though, as these conversations are great for gathering feedback and ideas!

[jzheaux]

The setters are a primary way for XML-based applications to get configured. While quite old-school, Spring Security still supports that, which means, at least for the time being, that the setters should stay there. Further, changing to this-returning setters would be a more framework-wide type of decision anyway and something that we'd likely wait on the Framework team to adopt.

Regarding a new builder method, we also don't want to introduce multiple ways to do the same thing for the sake of satisfying stylistic preferences. While I recognize that adding jwtValidator to the builder could increase developer quality-of-life in one dimension, it has the potential to reduce it in another since there would now be two ways in the same bean to provide an OAuth2TokenValidator. Which one do I pick and how do I know whether it matters?

Possibly in 7, we could make the constructor package-private, change it to take a JWTProcessor, OAuth2TokenValidator, and claims set converter, and remove the setters. Then placing it in the builder makes a little more sense. To me, to require everyone to change to this in order to upgrade doesn't seem ideal for what amounts to a stylistic difference unless we are making a broader style move away from setters, but I'm happy to keep this open to see if folks vote for something like that.

@ThomasKasene, would you be okay with changing your issue title to be "NimbusJwtDecoder should remove its setters in favor of its builders" and describe the needed changes? Then we can schedule it for 7 and see if enough folks vote for it.

[ThomasKasene]

@jzheaux I could do that if you think it's okay to repurpose this issue, or I can submit a new one with a more narrow scope so you can close this one if you want (the *JwtDecoder was only an example in this issue).

But what changes are you referring to? It seems like you got most of the required changes in your reply 😄 Or do you mean listing them in even more detail than that?