Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question/Chore: unused code in DefaultClientBuilder#build method (ClientCredentialsResolver) #1575

Open
wallind opened this issue Nov 5, 2024 · 1 comment
Assignees

Comments

@wallind
Copy link

wallind commented Nov 5, 2024

ℹ️ If you have a question, please post it on the Okta Developer Forum instead. Issues in this repository are reserved for bug reports and feature requests only.

I did originally post this on the dev forum, but they directed me here. See this post.

I'm submitting a

Question/comment about what I believe is a piece of unused code in the SDK

Background info

I believe I have found a unused/unnecessary bit of code in the okta-java-sdk and would like to bring it your attention.

In the DefaultClientBuilder#build method, if you are configuring the client for OAuth2 with privateKey, the following logic gets hit (see here in Github -> okta/okta-sdk-java -> impl/src/main/java/com/okta/sdk/impl/client/DefaultClientBuilder.java#L378):

                this.clientConfig.setClientCredentialsResolver(new DefaultClientCredentialsResolver(oAuth2ClientCredentials));

but it doesn't actually do anything from what I can tell. The getCredentialsResolver method seems to never be called subsequently...meaning this is unused and unnecessary.

From what I can tell, this is just a leftover artifact from a previous version of the code where an underlying base class required the ClientCredentialsResolver to be not null.

See here where this code was first added, and this line was discussed: #354 (comment). You can see that the BaseClient being discussed requires a DefaultDataStore, and then DefaultDataStore requires the passed in clientCredentialsResolver to be non-null.

The mentioned classes making this required were then removed/refactored in #776 ; I believe this PR should have also removed the line I'm questioning.

Expected behavior

The SDK interface (particularly the ClientBuilder) wouldn't have this red herring which can lead to lost time tracing code that ultimately leads nowhere.

What went wrong?

N/A

Steps to reproduce

N/A

SDK Version

com.okta.sdk:okta-sdk-api:19.0.1
com.okta.sdk:okta-sdk-impl:19.0.1
com.okta.spring:okta-spring-sdk:3.0.7
@arvindkrishnakumar-okta
Copy link
Contributor

Thanks for posting!

I'll take a look. We welcome pull requests, if you'd like to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants