Refactor DefaultClient from constructor-based to Builder pattern for configurable HTTP client options#910
Conversation
| private var callTimeout: Int = 0 | ||
| private var defaultHeaders: Map<String, String> = mapOf() | ||
| private var enableLogging: Boolean = false | ||
| private var logLevel: HttpLoggingInterceptor.Level = HttpLoggingInterceptor.Level.BODY |
There was a problem hiding this comment.
No need to provide the logLevel option. If the customer wants more control ,they can use a custom logging interceptor
| private var enableLogging: Boolean = false | ||
| private var logLevel: HttpLoggingInterceptor.Level = HttpLoggingInterceptor.Level.BODY | ||
| private var logger: HttpLoggingInterceptor.Logger? = null | ||
| private val interceptors: MutableList<Interceptor> = mutableListOf() |
There was a problem hiding this comment.
Lets not add option to configure interceptors . If there is a major request we can add this later .
| * by calling this method multiple times. They are invoked in the order they were added, | ||
| * after the built-in [RetryInterceptor] and before the logging interceptor. | ||
| */ | ||
| public fun addInterceptor(interceptor: Interceptor): Builder = |
| * Only takes effect if [enableLogging] is set to `true`. | ||
| * Default is [HttpLoggingInterceptor.Level.BODY]. | ||
| */ | ||
| public fun logLevel(level: HttpLoggingInterceptor.Level): Builder = |
There was a problem hiding this comment.
Lets not add this option
|
|
||
| okBuilder.addInterceptor(RetryInterceptor()) | ||
|
|
||
| interceptors.forEach { okBuilder.addInterceptor(it) } |
There was a problem hiding this comment.
Remove the custom interceptors for now
| } else { | ||
| HttpLoggingInterceptor() | ||
| } | ||
| loggingInterceptor.setLevel(logLevel) |
There was a problem hiding this comment.
#763
Check the second approach suggested. We can follow that pattern
There was a problem hiding this comment.
Updated to follow approach 2.The Builder now accepts an HttpLoggingInterceptor.Logger via .logger(myLogger)
| /** | ||
| * Internal constructor used by tests to inject SSL and Gson. | ||
| */ | ||
| @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) |
There was a problem hiding this comment.
Can't the tests use the builder now instead of this ?
There was a problem hiding this comment.
Removed the @VisibleForTesting internal constructor(...) entirely.
| .setLevel(HttpLoggingInterceptor.Level.BODY) | ||
| builder.addInterceptor(logger) | ||
| } | ||
| okHttpClient = okHttpClientBuilder.build() |
There was a problem hiding this comment.
Wouldn't this create a new client here ? Is this init block required now ?
There was a problem hiding this comment.
Cleaned up. The init block now just calls okHttpClientBuilder.build() and creates the nonRetryableOkHttpClient from it. No duplicate configuration
EXAMPLES.md
Outdated
|
|
||
| ### Logging configuration | ||
| <details> | ||
| <summary>Legacy constructor (still supported)</summary> |
There was a problem hiding this comment.
This should be termed as Deprecated instead of still supported.
Better , I think we shouldn't show the legacy one at all and show only the builder pattern one
There was a problem hiding this comment.
Removed the legacy constructor example entirely from this section.
EXAMPLES.md
Outdated
| ```kotlin | ||
| val netClient = DefaultClient.Builder() | ||
| .enableLogging(true) | ||
| .logLevel(HttpLoggingInterceptor.Level.HEADERS) // NONE, BASIC, HEADERS, or BODY (default) |
There was a problem hiding this comment.
Log level support is not added. Only the custom logger is added
Remove log level from example
There was a problem hiding this comment.
Removed logLevel from the Kotlin example.
EXAMPLES.md
Outdated
| ); | ||
| DefaultClient netClient = new DefaultClient.Builder() | ||
| .enableLogging(true) | ||
| .logLevel(HttpLoggingInterceptor.Level.HEADERS) |
There was a problem hiding this comment.
Log level support not added
There was a problem hiding this comment.
Removed logLevel from the Java example.
EXAMPLES.md
Outdated
|
|
||
| ### Set additional headers for all requests | ||
| <details> | ||
| <summary>Legacy constructor (still supported)</summary> |
There was a problem hiding this comment.
Remove the legacy one from examples
There was a problem hiding this comment.
Removed the entire legacy constructor
Details
block from the "Set additional headers" section.
EXAMPLES.md
Outdated
| </details> | ||
|
|
||
| <details> | ||
| <summary>Legacy constructor (still supported)</summary> |
EXAMPLES.md
Outdated
| ``` | ||
| </details> | ||
|
|
||
| ### Custom interceptors |
There was a problem hiding this comment.
Custom interceptors are not supported. Remove this section
There was a problem hiding this comment.
Removed the entire section.
V4_MIGRATION_GUIDE.md
Outdated
| .callTimeout(120) | ||
| .enableLogging(true) | ||
| .logLevel(HttpLoggingInterceptor.Level.HEADERS) | ||
| .addInterceptor(myCustomInterceptor) |
There was a problem hiding this comment.
Remove the addInterceptor from here
V4_MIGRATION_GUIDE.md
Outdated
| .writeTimeout(30) | ||
| .callTimeout(120) | ||
| .enableLogging(true) | ||
| .logLevel(HttpLoggingInterceptor.Level.HEADERS) |
There was a problem hiding this comment.
Remove logLevel from here
2 participants
Changes
Added:
Deprecated:
DefaultClient public constructor (DefaultClient(connectTimeout, readTimeout, defaultHeaders, enableLogging)) — deprecated with @deprecated annotation and ReplaceWith pointing to the Builder API. Still compiles and functions identically; IDE provides one-click quick-fix migration.
Usage:
References
#787 #763
Testing
Build Passed
Unit Test Passed
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors