Add OidcTenantConfig builder

[michalvavrik]

• closes Migrate OIDC-based extensions from config classes to @ConfigMapping #39185 (as this is final part, the deprecations and removals will take a long time)
• provide a builder for OidcTenantConfig class
• deprecate usage of OidcTenantConfig class fields and setters
• recommend using interface methods instead
• newly I also added deprecations on fields and inner classes of deprecated abstract classes OidcCommonConfig and OidcClientCommonConfig because my IDE doens't provide hints when these fields are used from the OidcTenantConfig instance and I want users to be warned; it is bit verbose though
• as I was already touching everything, I made some neglibible refactoring as well (!optional.isPresent() -> optional.isEmpty), not much though

These changes will allow us in the future in this order (with long breaks):

1. mark fields for removal
2. make sure users can't modify OidcTenantConfig directly, either:

• make it only modifiable from OIDC runtime config
• or better, we will make it immutable:

3. further:

• fields will be final and private (maybe a record?)
• users will use implemented interface methods to retrieve values
• OidcTenantConfig will only be possible to build with the builder by users
• drop extends to former config classes and only leave implements for the interfaces
• OidcTenantConfig class will be final

4. stop copying properties between builder and OidcTenantConfig as we can build the tenant config directly

This way, the API for users is in the io.quarkus.oidc package, more specifically:

• io.quarkus.oidc.OidcTenantConfig
• io.quarkus.oidc.OidcTenantConfigBuilder

[github-actions]

🙈 The PR is closed and the preview is expired.

[sberyozkin]

@michalvavrik Hi Michal, thanks, going forward, after this PR is merged, what happens when a new property has to be added, while the deprecated OidcTenantConfig is still around ? I'm assuming that only ConfigMapping and the corresponding builder has to be updated, and if users who work with TenantConfigProvider would like to use a new property then they can only get it by using a builder, right ?

[michalvavrik]

after this PR is merged, what happens when a new property has to be added, while the deprecated OidcTenantConfig is still around ?

1. I didn't deprecate this class. It is omnipresent including RoutingContext etc. It is unnecessary, we can keep it without issues. I think there are 2 ways it can go:

• when all the field and parent class deprecations are removed, we can turn it into final class and remove private class OidcTenantConfigImpl from the builder. it will make things bit more efficient, we will also be able a lot of copying of properties etc. (more effective) [it would require moving some logic further to SR Config customizer)
• we can keep it mutable but only from the Quarkus OIDC runtime package so that users only works with API, but we can keep changing things
• update: I just realized there is a third way - we could turn it into the interface and use it in the @ConfigMapping

2. If you add a new property, you are adding a new method to the interface. And OidcTenantConfig implements this interface, therefore you are guaranteed to be forced to update this config class appropriately. We can't forget otherwise compilation fails.

I'm assuming that only ConfigMapping and the corresponding builder has to be updated

yes; the former config class group OidcTenantConfig stiill implements this interface, but you will not add public field for this field. This way, you will force users to use the builder.

and if users who work with TenantConfigProvider would like to use a new property then they can only get it by using a builder, right ?

they will need to use the builder if they want to create the OidcTenantConfig with this new property, but they can still do this:

OidcTenantConfig tenantConfig = OidcTenantConfig.builder().sergeysNewProperty(1).build();
if (1 == tenantConfig.sergeysNewProperty()) { // do stuff

This can be hard to understand, I can answer further questions to explain what I really mean.

[sberyozkin]

@michalvavrik

after this PR is merged, what happens when a new property has to be added, while the deprecated OidcTenantConfig is still around ?

I didn't deprecate this class. It is omnipresent including RoutingContext etc. It is unnecessary, we can keep it without issues. I think there are 2 ways it can go:

• when all the field and parent class deprecations are removed, we can turn it into final class and remove private class OidcTenantConfigImpl from the builder. it will make things bit more efficient, we will also be able a lot of copying of properties etc. (more effective) [it would require moving some logic further to SR Config customizer)
  * we can keep it mutable but only from the Quarkus OIDC runtime package so that users only works with API, but we can keep changing things
  * update: I just realized there is a third way - we could turn it into the interface and use it in the @ConfigMapping

I've looked at the code and I see new io.quarkus.oidc.OidcTenantConfig() is deprecated, the io.quarkus.oidc.OidcTenantConfig class itself should indeed not be deprecated as ultimately, it will be replaced by io.quarkus.oidc.runtime.OidcTenantConfig.

Should we mark though all the deprecated io.quarkus.oidc.OidcTenantConfig constructors, properties, for removal ?

and if users who work with TenantConfigProvider would like to use a new property then they can only get it by using a builder, right ?

they will need to use the builder if they want to create the OidcTenantConfig with this new property, but they can still do this:

OidcTenantConfig tenantConfig = OidcTenantConfig.builder().sergeysNewProperty(1).build();
if (1 == tenantConfig.sergeysNewProperty()) { // do stuff

This can be hard to understand, I can answer further questions to explain what I really mean.

Sorry, I'm not sure what is the point that you are trying to highlight, I guess, once the OidcTenantConfig is ready, one can check its properties like you showed ?

[michalvavrik]

I've looked at the code and I see new io.quarkus.oidc.OidcTenantConfig() is deprecated, the io.quarkus.oidc.OidcTenantConfig class itself should indeed not be deprecated as ultimately, it will be replaced by io.quarkus.oidc.runtime.OidcTenantConfig.

exactly, fields, setters and constructors are deprecated. I also deprecated getter in favor of the interface methods, but that one is not vital

Should we mark though all the deprecated io.quarkus.oidc.OidcTenantConfig constructors, properties, for removal ?

I'd like that. I didn't mark them for removal because I tried to be conservative. So if you are fine with marking them for removal, we can do that. We could also wait for a release or two. java.lang.Deprecated#forRemoval javadoc says Indicates whether the annotated element is subject to removal in a future version. The default value is false. which I though was bit too soon, but nothing is forcing us to remove it.

Sorry, I'm not sure what is the point that you are trying to highlight, I guess, once the OidcTenantConfig is ready, one can check its properties like you showed ?

I tried to show that you can add new property without breaking things. Sorry if that was obvious.

[sberyozkin]

OidcTenantConfig tenantConfig = OidcTenantConfig.builder().sergeysNewProperty(1).build();
if (1 == tenantConfig.sergeysNewProperty()) { // do stuff

Do you mean that works because it implements the interface ? It is fine, I don't really expect users do it, they will create OidcTenantConfig in dynamic resolvers only

[michalvavrik]

OidcTenantConfig tenantConfig = OidcTenantConfig.builder().sergeysNewProperty(1).build();
if (1 == tenantConfig.sergeysNewProperty()) { // do stuff

Do you mean that works because it implements the interface ? It is fine, I don't really expect users do it, they will create OidcTenantConfig in dynamic resolvers only

That is what I meant as well.

[sberyozkin]

What should we do though about users directly accessing io.quarkus.oidc.OidcTenantConfig to check some of its properties, as in the closed issue #44077, by injecting TenantConfigBean and checking some io.quarkus.oidc.OidcTenantConfig individual properties ?

I wonder if we should only keep new io.quarkus.oidc.OidcTenantConfig() deprecated (the other constructor can be made private instead of deprecated I think, users don't use it), but keep all the getters non-deprecated (if some individual fields do not have getters and users will get deprecated warnings when accessing fields then I can add temp getters) ?

As far as for removal is concerned, I can do it myself a bit later, not critical for this PR

[michalvavrik]

What should we do though about users directly accessing io.quarkus.oidc.OidcTenantConfig to check some of its properties, as in the closed issue #44077, by injecting TenantConfigBean and checking some io.quarkus.oidc.OidcTenantConfig individual properties ?

For example, if you do tenantConfigBean.getDefaultTenant().oidcConfig().authentication.userInfoRequired.orElse(false) (which I found in one of the tests inside Quarkus main repo), you can rewrite it to the tenantConfigBean.getDefaultTenant().oidcConfig().authentication().userInfoRequired().orElse(false). This approach is not deprecated, I have planned to keep it.

I wonder if we should only keep new io.quarkus.oidc.OidcTenantConfig() deprecated (the other constructor can be made private instead of deprecated I think, users don't use it), but keep all the getters non-deprecated (if some individual fields do not have getters and users will get deprecated warnings when accessing fields then I can add temp getters) ?

Users don't have any reason to instantiate this class with new io.quarkus.oidc.OidcTenantConfig() because they need to use the builder. Problem with constructor is that it will limit us in future changes (which might never happen, these are just my thoughts!):

• we can't ever make this class the interface and avoid some copying between @ConfigMapping
• we can't make it record that implements @ConfigMapping groups and is created directly by the builder

If you believe we should have a shortcut for the OidcTenantConfig.builder().build(), then maybe we can add a factory method that hides how this class is constructed and exact implementation. What about OidcTenantConfig.newInstance() ?

the other constructor can be made private instead of deprecated I think, users don't use it

agreed about the other constructor, again I tried to be conservative, but it was there only for one release (3.17), I'll push that

As far as for removal is concerned, I can do it myself a bit later, not critical for this PR

+1, I was just writing sed command to do that, but I will leave it to you for later.

[michalvavrik]

And about the getters, some return deprecated sub-classes that we could drop, so it is better if users use the new interface methods. Otherwise we need to copy & paste between this classes.

[sberyozkin]

@michalvavrik OK, thanks, please only hide the helper OidcTenantConfig constructor which was only added in 3.17 to facilitate the transition to config mappings, and there will be time to tune a few things before 3.18 is out by the end of Jan :-)

Thanks very much for taking care of this massive restructuring

[michalvavrik]

@michalvavrik OK, thanks, please only hide the helper OidcTenantConfig constructor which was only added in 3.17 to facilitate the transition to config mappings, and there will be time to tune a few things before 3.18 is out by the end of Jan :-)

done

Thanks very much for taking care of this massive restructuring

yeah, this took a lot of patience, thanks for the review as well

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit b0aeefe.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit b0aeefe.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.