Consider Refactor JWTAuthContextInfoProvider

[radcortez]

We should consider to clean up JWTAuthContextInfoProvider.

• Provide static names for configs
• Replace Optionals with default values
• Maybe remove all the CDI injection and call Config API programatically? Might even get some performance improvement

[sberyozkin]

Provide static names for configs

What will it give us ?

Replace Optionals with default values

for 3.0.0 this can indeed be done where possible

Maybe remove all the CDI injection and call Config API programatically

Not sure it is worth it, it is initialized once, and not affecting the requests. Ease of testing may be, but we can just easily add few more static factory methods. There are cons and pros but we do it everywhere really, in Quarkus, here, elsewhere, I'd rather us spending time on something else...

[sberyozkin]

I think your idea of delaying the conversion was very interesting, this is where we can definitely get some minor performance improvement

[ejba]

Hi, can I help you with this task?

[ejba]

Hi guys! I don't know if you still interest on doing this but I have taken the chance with that (just for fun).

[radcortez]

Thanks @ejba :)

Let us have a look!

[ejba]

It's not ready for review yet, still have to do some tests. Also, I am not sure that I understood your idea.

[ejba]

@radcortez I will need you help here, if you don't mind. :)

Maybe remove all the CDI injection and call Config API programatically

Once CDI injection removed, where should it call the Config API? In getters? How can I measure this expected performance improvement?

[sberyozkin]

@ejba I'm not sure this needs to be done, if it needs to be done at all.

Provide static names for configs

As I said in the other PR, I'm finding it easy to look at the concrete property name

Replace Optionals with default values

This will be good for 3.0.0 otherwise it can break some Thorntail code which checks Optional getters

Maybe remove all the CDI injection and call Config API programatically

As commented above, I'm not sure it is needed. The performance gains if any will be negligible, using the programmatic API is more verbose.

[sberyozkin]

Hi @ejba @radcortez,
May be we can start with getting rid of optional now, but I'd really do it for MP JWT 1.2 branch, Thorntail does check a few optional getters, but some other extensions might do it too. Having an Optional with a default value is obviously not cool :-), there is some legacy code there, I kept copying and pasting for awhile too before @MikeEdgar has stopped me :-), but IMHO this clean up can go to the MP JWT 1.2 branch only and will be in the master in a couple of months or so anyway...
I'd be reluctant though to start replacing Inject, no doubt it adds a bit of extra cost but any Quarkus application has so many more Injects everywhere so saving on a few of them in smallrye-jwt, given it is done only once here, won't save us much IMHO :-)

Thanks

[ejba]

Hi @sberyozkin, thanks for your feedback.

I have been working on this but didn't the PR because I felt a bit lost with the description once I don't have the big picture of the impacts and consequences behind it. Could you have a look at what I did? If you see something that worths the trouble, I can extract that and make a dedicated PR of it. link

[sberyozkin]

Hi @ejba
Thanks for another massive effort, but I'm sorry, the only thing that IMHO we should start from right now is to replace something like

@Inject
@ConfigProperty(name = "smallrye.jwt.token.schemes", defaultValue = BEARER_SCHEME)
private Optional<String> tokenSchemes;

with

@Inject
@ConfigProperty(name = "smallrye.jwt.token.schemes", defaultValue = BEARER_SCHEME)
private String tokenSchemes;

against https://github.com/smallrye/smallrye-jwt/tree/mpjwt12.

IMHO the pervasive changes to do with the constants and removing the injection altogether are not required at this point of time.

Thanks

[ejba]

Ok, I can do that.
What will happen with the getters? Do you plan to keep the return type as optional?

[sberyozkin]

@ejba Hi, sorry, missed it, no, lets get rid of them as well for all the properties which have a default value

[sberyozkin]

Hey, I'll do a quick update early next year to remove the Optionals where the default values are available.

[sberyozkin]

Hey @radcortez @ejba Let me close this one - #379 did some clean up so I suppose we can resolve it and target some concrete improvements with the new issues :-), thanks

[sberyozkin]

Somehow #379 did not get in onto RC2 so I'll make RC3 early next week - it shows Merged but I might've accidentally just closed it without merging - this is just a clean up so not a big problem

[sberyozkin]

that was merged again