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.

Sign up for GitHub

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

Refactor Kotlin RTL #384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactor Kotlin RTL #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions kotlin/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,30 @@ Verify authentication works and that API calls will succeed with code similar to
```kotlin
import com.looker.rtl.ApiSettings;
import com.looker.rtl.AuthSession;
import com.looker.rtl.SdkResult;
import com.looker.rtl.ok;
import com.looker.sdk.LookerSDK;

val localIni = "./looker.ini"
val settings = ApiSettings.fromIniFile(localIni, "Looker")
val session = AuthSession(settings)
val sdk = LookerSDK(session)
// Verify minimal SDK call works
val me = sdk.ok<User>(sdk.me())
val me = sdk.me().ok()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the ok() concept. Presumably this is used in the other SDKs as well for a short circuit to a more robust analysis.


/// continue making SDK calls
val users = sdk.ok<Array<User>>(sdk.all_users())
val result = sdk.all_users()
when (result) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extended the example here to show what you can do if you want to check for errors.

is SdkResult.SuccessResponse<List<User>> -> {
result.body.forEach { user -> print(user.name) }
}
is SdkResult.FailureResponse<com.looker.sdk.Error> -> {
log(result.body.message)
}
is SdkResult.Error -> {
log(result.error.message)
}
}
```

### More examples
Expand Down
68 changes: 48 additions & 20 deletions kotlin/src/main/com/looker/rtl/APIMethods.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,36 +33,64 @@ open class APIMethods(val authSession: AuthSession) {

val authRequest = authSession::authenticate

fun <T> ok(response: SDKResponse): T {
when (response) {
is SDKResponse.SDKErrorResponse<*> -> throw Error(response.value.toString())
is SDKResponse.SDKSuccessResponse<*> -> return response.value as T
else -> throw Error("Fail!!")
}
}

inline fun <reified T> get(path: String, queryParams: Values = mapOf(), body: Any? = null): SDKResponse {
return authSession.transport.request<T>(HttpMethod.GET, path, queryParams, body, authRequest)
inline fun <reified TSuccess, reified TFailure> get(
path: String,
queryParams: Values = mapOf(),
body: Any? = null
): SdkResult<TSuccess, TFailure> {
return authSession.transport.request<TSuccess, TFailure>(
HttpMethod.GET, path, queryParams, body, authRequest
)
}

inline fun <reified T> head(path: String, queryParams: Values = mapOf(), body: Any? = null): SDKResponse {
return authSession.transport.request<T>(HttpMethod.HEAD, path, queryParams, body, authRequest)
inline fun <reified TSuccess, reified TFailure> head(
path: String,
queryParams: Values = mapOf(),
body: Any? = null
): SdkResult<TSuccess, TFailure> {
return authSession.transport.request<TSuccess, TFailure>(
HttpMethod.HEAD, path, queryParams, body, authRequest
)
}

inline fun <reified T> delete(path: String, queryParams: Values = mapOf(), body: Any? = null): SDKResponse {
return authSession.transport.request<T>(HttpMethod.DELETE, path, queryParams, body, authRequest)
inline fun <reified TSuccess, reified TFailure> delete(
path: String,
queryParams: Values = mapOf(),
body: Any? = null
): SdkResult<TSuccess, TFailure> {
return authSession.transport.request<TSuccess, TFailure>(
HttpMethod.DELETE, path, queryParams, body, authRequest
)
}

inline fun <reified T> post(path: String, queryParams: Values = mapOf(), body: Any? = null): SDKResponse {
return authSession.transport.request<T>(HttpMethod.POST, path, queryParams, body, authRequest)
inline fun <reified TSuccess, reified TFailure> post(
path: String,
queryParams: Values = mapOf(),
body: Any? = null
): SdkResult<TSuccess, TFailure> {
return authSession.transport.request<TSuccess, TFailure>(
HttpMethod.POST, path, queryParams, body, authRequest
)
}

inline fun <reified T> put(path: String, queryParams: Values = mapOf(), body: Any? = null): SDKResponse {
return authSession.transport.request<T>(HttpMethod.PUT, path, queryParams, body, authRequest)
inline fun <reified TSuccess, reified TFailure> put(
path: String,
queryParams: Values = mapOf(),
body: Any? = null
): SdkResult<TSuccess, TFailure> {
return authSession.transport.request<TSuccess, TFailure>(
HttpMethod.PUT, path, queryParams, body, authRequest
)
}

inline fun <reified T> patch(path: String, queryParams: Values = mapOf(), body: Any? = null): SDKResponse {
return authSession.transport.request<T>(HttpMethod.PATCH, path, queryParams, body, authRequest)
inline fun <reified TSuccess, reified TFailure> patch(
path: String,
queryParams: Values = mapOf(),
body: Any? = null
): SdkResult<TSuccess, TFailure> {
return authSession.transport.request<TSuccess, TFailure>(
HttpMethod.PATCH, path, queryParams, body, authRequest
)
}

fun encodeURI(value: String): String {
Expand Down
37 changes: 11 additions & 26 deletions kotlin/src/main/com/looker/rtl/AuthSession.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,6 @@ open class AuthSession(
return false
}

fun <T> ok(response: SDKResponse): T {
@Suppress("UNCHECKED_CAST")
when (response) {
is SDKResponse.SDKErrorResponse<*> -> throw Error(response.value.toString())
is SDKResponse.SDKSuccessResponse<*> -> return response.value as T
else -> throw Error("Fail!!")
}
}

private fun sudoLogout(): Boolean {
var result = false
if (isSudo()) {
Expand Down Expand Up @@ -148,20 +139,17 @@ open class AuthSession(
append(client_secret, clientSecret)
}
)
val token = ok<AuthToken>(
transport.request<AuthToken>(
HttpMethod.POST,
"$apiPath/login",
mapOf(),
body
)
)
authToken = token
authToken = transport.request<AuthToken, Any?>(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Any?s here seem a bit crufty, but most clients won't see this. Actually, I need to double-check that this works correctly at runtime and not just at compile, since I don't know what will happen when we invoke response.receive<Any?>(). Maybe this should be Unit.

HttpMethod.POST,
"$apiPath/login",
mapOf(),
body
).ok()
}

if (sudoId.isNotBlank()) {
val token = activeToken()
val sudoToken = transport.request<AuthToken>(
val sudoToken = transport.request<AuthToken, Any?>(
HttpMethod.POST,
"/login/$newId"
) { requestSettings ->
Expand All @@ -171,26 +159,23 @@ open class AuthSession(
}
requestSettings.copy(headers = headers)
}
this.sudoToken = ok(sudoToken)
this.sudoToken = sudoToken.ok()
}
return activeToken()
}

private fun doLogout(): Boolean {
val token = activeToken()
val resp = transport.request<String>(HttpMethod.DELETE, "/logout") {
val resp = transport.request<String, Any?>(HttpMethod.DELETE, "/logout") {
val headers = it.headers.toMutableMap()
if (token.accessToken.isNotBlank()) {
headers["Authorization"] = "Bearer ${token.accessToken}"
}
it.copy(headers = headers)
}

val success = when (resp) {
is SDKResponse.SDKSuccessResponse<*> -> true
is SDKResponse.SDKErrorResponse<*> -> false
else -> false
}
val success = resp.success

if (sudoId.isNotBlank()) {
sudoId = ""
sudoToken.reset()
Expand Down
4 changes: 2 additions & 2 deletions kotlin/src/main/com/looker/rtl/OAuthSession.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ class OAuthSession(override val apiSettings: ConfigurationProvider, override val
}

fun requestToken(body: Values): AuthToken {
val response = this.transport.request<AccessToken>(
val response = this.transport.request<AccessToken, Any?>(
HttpMethod.POST,
"/api/token",
mapOf(),
body
)
val token = this.ok<AccessToken>(response)
val token = response.ok()
this.authToken.setToken(token)
return this.authToken
}
Expand Down
104 changes: 104 additions & 0 deletions kotlin/src/main/com/looker/rtl/SdkResult.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
package com.looker.rtl

import io.ktor.client.call.receive
import io.ktor.client.response.HttpResponse
import io.ktor.http.isSuccess
import kotlinx.coroutines.runBlocking

class FailureResponseError(val result: SdkResult<*, *>) : Exception()

interface SdkResponse<T> {
val success: Boolean
val statusCode: Int
val method: HttpMethod
val path: String
fun body(): T
}

sealed class SdkResult<out TSuccess, out TFailure> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add some more comments to these new classes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you're just augmenting the existing pattern of an sdk method returns a "meta" object that you can get the actual SDK response type from (on success) with .body() or use .ok() to return that or throw. And this structure seems reasonable to me.

My question is at a higher design level for you and @jkaster - searching through some pretty popular python sdks (github, azure, google cloud sdks) - the idiomatic approach for these python sdks is for a method to behave as if the sdk.ok() (or response.ok() here) method is automatically called (i.e. the sdk method either raises or returns the TSuccess object directly)

For python I'd actually propose adding a method on the SDK response types that exposes the success/status_code/method/path/raw_body metadata (and for the error case throwing an exception that has this same metadata access method).

Is it ok from our design perspective that the python sdk behaves so differently? We could make it behave like the others but then I think we lose the idiomatic nature that other popular python sdk libraries are following.

thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely simpler if the sdk response is always just TSuccess or an exception. I guess since my motivation for this was better error handling, we could limit the extra functionality to just the error path. That would limit the client's ability to introspect on the response in the success case, but maybe we don't care about that. Also, I remember @jkaster talking about how he didn't want to throw in the failure/error case until you actually went to get the value. Something about working better in an async flow, so maybe we should look at what node or something else natively async does here.

I'd be up for a quick group chat about this topic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to participate - I'll let you and our fearless leader @jkaster coordinate :-)

abstract val success: Boolean
abstract val method: HttpMethod
abstract val path: String

data class SuccessResponse<TSuccess>(
val response: HttpResponse,
override val method: HttpMethod,
override val path: String,
private val body: TSuccess
) : SdkResponse<TSuccess>, SdkResult<TSuccess, Nothing>() {
override val success: Boolean = true
override val statusCode: Int = response.status.value

override fun body(): TSuccess = body

inline fun <reified T> bodyAs(): T {
return runBlocking { response.receive<T>() }
}
}

data class FailureResponse<TFailure>(
val response: HttpResponse,
override val method: HttpMethod,
override val path: String,
private val body: TFailure
) : SdkResponse<TFailure>, SdkResult<Nothing, TFailure>() {
override val success: Boolean = false
override val statusCode: Int = response.status.value

override fun body(): TFailure = body

inline fun <reified T> bodyAs(): T {
return runBlocking { response.receive<T>() }
}
}

data class Error(
val error: Throwable,
override val method: HttpMethod,
override val path: String
) : SdkResult<Nothing, Nothing>() {
override val success: Boolean = false
}

companion object {
inline fun <reified TSuccess, reified TFailure> response(
response: HttpResponse,
method: HttpMethod,
path: String
): SdkResult<TSuccess, TFailure> {
try {
if (response.status.isSuccess()) {
val body = runBlocking { response.receive<TSuccess>() }
return SuccessResponse<TSuccess>(response, method, path, body)
} else {
val body = runBlocking { response.receive<TFailure>() }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pondering this some more, I'm wondering if the FailureResponse case should actually defer on receiving the data until requested. The issue I'm running into while thinking this through is that I want to be able to deal with a single API being able to return multiple different types of error response. In that case, we're going to have to do some preliminary parsing or some try/catch(ParseError) for the different types. With ktor, you can only do response.receive() a single time, so that's not going to work.

I'm thinking that maybe we do response.receive<ByteArray>() or response.receive<String>() to get the "raw" data, and then parse that into whatever type is requested. Of course that'll mean that we have to manage our own object mapper in the RTL instead of using ktor's registered one. All around, it ends up being a lot more to manage here in the RTL layer, but I'm not seeing a good way around that.

Looking at the error responses and some of the libraries like Jackson or kotlinx.serialization, it would be nice to have a discriminator field to use to select the type to parse into, but our API doesn't really provide that.

Just brain dumping into this comment for further discussion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, it will be a comfort python isn't the only language with a separate [de]serialization layer. I always thought it was unfair that the other languages just somehow got their models/types hydrated for free from the transport.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated feedback

  • the Kotlin SDK is now using GSON to ignore serializing null values into the request payload (so far, no complaints from the few users we have of that SDK, and it resolves some use cases with some endpoints).
  • It also appears that being able to deserialize a JSON payload to a Kotlin type outside of the request/response flow is a desirable feature as well, so we may want to separate that out into an independent function

return FailureResponse<TFailure>(response, method, path, body)
}
} catch (ex: Exception) {
return error(ex, method, path)
}
}

fun <TSuccess, TFailure> error(
error: Throwable,
method: HttpMethod,
path: String
): SdkResult<TSuccess, TFailure> {
return SdkResult.Error(error, method, path)
}
}
}

fun <TSuccess> SdkResult<TSuccess, *>.ok(): TSuccess {
return when (this) {
is SdkResult.SuccessResponse<TSuccess> -> {
this.body()
}
is SdkResult.FailureResponse<*> -> {
throw FailureResponseError(this)
}
is SdkResult.Error -> {
throw this.error
}
}
}
Loading