Mark former OIDC config class fields for removal

[michalvavrik]

This should give users impulse to migrate as we will eventually remove them, though not anytime soon.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

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

✅ 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.

[github-actions]

🎊 PR Preview a6413e3 has been successfully built and deployed to https://quarkus-pr-main-46317-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[quarkus-bot]

Status for workflow Quarkus CI

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

✅ 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.

[sberyozkin]

Hi Michal, thanks, LGTM, but I wonder, should we try to mark the whole legacy class tree deprecated and mark it for removal ? I had a quick look, as far as recall, a few more factory methods reinitializing OidcTenantConfig interface impl were needed to support some of the OIDC recorder code.
We can merge it if you'd like, but I guess we won't be able to remove anything later without deprecating classes

[michalvavrik]

I don't have firm opinon on this, but I'll share it:

Hi Michal, thanks, LGTM, but I wonder, should we try to mark the whole legacy class tree deprecated and mark it for removal ?

I deprecated OidcCommonConfig and OidcClientCommonConfig but you removed it 070bf60. As for OidcTenantConfig, we need some user-facing class that is part of API and I think it is convenient to keep this one. Yes, it implements interfaces with @ConfigMapping, however if we make changes on the @ConfigMapping interface, most often we will be able to keep it backwards compatible (keep both and deprecate the old method).

I had a quick look, as far as recall, a few more factory methods reinitializing OidcTenantConfig interface impl were needed to support some of the OIDC recorder code.

We can avoid making OidcTenantConfig fields editable even now. I left few things editable like io.quarkus.oidc.OidcTenantConfig#tenantEnabled, io.quarkus.oidc.OidcTenantConfig#authentication and io.quarkus.oidc.OidcTenantConfig#token etc. We will need a way to enable/disable tenant, but the rest of stuff that is editable was there just because I didn't want to address it back then. These PRs were big enough.

We can merge it if you'd like, but I guess we won't be able to remove anything later without deprecating classes

I think we can agree that changes in this PR should be done. My proposal is that I open a PR or two that address the fields that are still edited and one of following things happen:

• we agree that is better (I have loads of arguments TBH, but I'll write them to these PRs)
• we keep it editable and then we discuss deprecating classes again

[sberyozkin]

Hi @michalvavrik

I deprecated OidcCommonConfig and OidcClientCommonConfig but you removed it 070bf60.

The problem was, even if new interface methods were used, they were still marked as deprecated

As for OidcTenantConfig, we need some user-facing class that is part of API and I think it is convenient to keep this one. Yes, it implements interfaces with @ConfigMapping, however if we make changes on the @ConfigMapping interface, most often we will be able to keep it backwards compatible (keep both and deprecate the old method).

IMHO we should deprecate it too, we have new interface. For those few cases where we internally need to modify the tenant id or some other property we can have some specifically named factory methods which would return a new interface instance...

I think we can agree that changes in this PR should be done. My proposal is that I open a PR or two that address the fields that are still edited...

OK, thanks

[michalvavrik]

IMHO we should deprecate it too, we have new interface. For those few cases where we internally need to modify the tenant id or some other property we can have some specifically named factory methods which would return a new interface instance...

But new interface classes are not part of public API while OidcTenantConfig is and we need such a class for users, they need to be able to rely on it as API classes expose it as method parameters. I suggest that I'll proceed with avoiding modification first and once that is done, whether it is deprecated or not is a separate issue.

[michalvavrik]

The problem was, even if new interface methods were used, they were still marked as deprecated

yes, I agreed with that change, but we can't have both 🤷

[sberyozkin]

@michalvavrik Hey,

But new interface classes are not part of public API while OidcTenantConfig is and we need such a class for users, they need to be able to rely on it as API classes expose it as method parameters.

That class is not meant to be an API any more, The only reason I moved to API (it was internal long time ago) was because of TenantConfigResolver where users create it manually - and now we offer interface builders. There is nothing there that we should be recommending users to use in their own code. If some API class (I can only think of internal TenantConfigBean which is now essentially API) still uses it then we can deprecate methods which rely on it.

I suggest that I'll proceed with avoiding modification first and once that is done, whether it is deprecated or not is a separate issue.

Sure, indeed, the short term is indeed about removing all those deprecated warnings, I can help as well. And then, once we remove those deprecations, I'm quite sure there will not too many reasons remaining to keep the legacy OidcTenantConfig around undeprecated - but sure, we can discuss that later, thanks

[michalvavrik]

Agreed @sberyozkin, thanks for the context.

[sberyozkin]

Thanks @michalvavrik, ideally, at some point in the future, we'd not be required to modify both OidcTenantConfigImpl and this OidcTenantConfig every time a new property is added :-), I appreciate we need to do it now though