Refactor Kotlin RTL #384

kalenp · 2020-11-19T01:55:13Z

The goals of this PR are twofold:

Improve the handling of errors
Improve the typing of responses

This mostly revoles around replacing SDKResponse with
SdkResult<TSuccess, TFailure>. This allows us to track the type
information with the SdkResult and easily get out the expected type on
success or failure.

For now, all errors are expected to be of type com.looker.sdk.Error,
even though we do have some methods which return more precise errors.
If for some reason you need to escape the default typing, such as to get
a finer Error type, you can use bodyAs<T>() on both SuccessResponse
and FailureResponse.

For clients, this commit keeps the concept of the ok() method that
returns the success type or throws an error. This moved from being a
method on the sdk itself to an extension function on the SdkResult,
so you can do val foo: Foo = sdk.get_foo().ok(). If you want to do
more precise handling, SdkResult is a sealed class, so you can use a
when statement to check for the various type (SuccessResonse,
FailureResponse, Error) and get smart casting to access the appropriate
types and Response metatdata.

The goals of this PR are twofold: - Improve the handling of errors - Improve the typing of responses This mostly revoles around replacing SDKResponse with SdkResult<TSuccess, TFailure>. This allows us to track the type information with the SdkResult and easily get out the expected type on success or failure. For now, all errors are expected to be of type com.looker.sdk.Error, even though we do have some methods which return more precise errors. If for some reason you need to escape the default typing, such as to get a finer Error type, you can use `bodyAs<T>()` on both SuccessResponse and FailureResponse. For clients, this commit keeps the concept of the `ok()` method that returns the success type or throws an error. This moved from being a method on the sdk itself to an extension function on the SdkResult, so you can do `val foo: Foo = sdk.get_foo().ok()`. If you want to do more precise handling, SdkResult is a `sealed` class, so you can use a `when` statement to check for the various type (SuccessResonse, FailureResponse, Error) and get smart casting to access the appropriate types and Response metatdata.

kalenp

Pushing this up to get some feedback while I do a final cleanup pass.

kalenp · 2020-11-19T01:56:12Z

kotlin/README.md

 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()


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

kalenp · 2020-11-19T01:56:45Z

kotlin/README.md


 /// continue making SDK calls
-val users = sdk.ok<Array<User>>(sdk.all_users())
+val result = sdk.all_users()
+when (result) {


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

kalenp · 2020-11-19T01:58:37Z

kotlin/src/main/com/looker/rtl/AuthSession.kt

-                )
-            )
-            authToken = token
+            authToken = transport.request<AuthToken, Any?>(


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.

kalenp · 2020-11-19T01:59:56Z

kotlin/src/main/com/looker/rtl/SdkResult.kt

+    fun body(): T
+}
+
+sealed class SdkResult<out TSuccess, out TFailure> {


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

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?

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.

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

kalenp · 2020-11-19T02:00:32Z

kotlin/src/main/com/looker/rtl/Transport.kt

        method: HttpMethod,
        path: String,
        queryParams: Values = mapOf(),
        body: Any? = null,
        noinline authenticator: Authenticator? = null
-    ): SDKResponse {
-        // TODO get overrides parameter to work without causing compilation errors in UserSession


Do I need to preserve these TODOs?

We need to functionality that TODO was for, but not necessarily in that place

kalenp · 2020-11-19T02:01:38Z

packages/sdk-codegen/src/kotlin.gen.ts

@@ -58,6 +58,8 @@ export class KotlinGen extends CodeGen {
  needsRequestTypes = false
  willItStream = true

+  tFailure = 'com.looker.sdk.Error'


I used the fully qualified name here, since Kotlin also defines an Error class. Maybe the API response type should be clarified to use something that doesn't conflict.

We rename the spec's Error class for Swift, which is less flexible. We may need to do the same kind of thing for the Kotlin generator.

kalenp · 2020-11-19T02:03:18Z

packages/sdk-codegen/src/kotlin.gen.ts

  }

  streamCall(indent: string, method: IMethod) {
    const request = this.useRequest(method) ? 'request.' : ''
+    const methodName = this.it(method.httpMethod.toLowerCase())
+    const tSuccess = 'ByteArray'


For streams, we always return ByteArray for TSuccess. I kept com.looker.sdk.Error for TFailure, but maybe that should be ByteArray as well? Or maybe the streaming methods should also do deserialization? I don't know much about how we're handling streaming methods in the other SDKs, so I tried to change as little here as possible.

The kotlin "streaming" support was a total hack just to support binary payload downloading. Same is true for the Swift SDK. We still need real streaming functionality.

kalenp · 2020-11-24T01:21:08Z

kotlin/src/main/com/looker/rtl/SdkResult.kt

+                    val body = runBlocking { response.receive<TSuccess>() }
+                    return SuccessResponse<TSuccess>(response, method, path, body)
+                } else {
+                    val body = runBlocking { response.receive<TFailure>() }


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.

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.

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

kalenp requested review from jkaster, brianmadden and brln-looker November 19, 2020 01:55

kalenp commented Nov 19, 2020

View reviewed changes

kalenp commented Nov 24, 2020

View reviewed changes

Base automatically changed from master to main February 9, 2021 18:32

kabat87 approved these changes Jan 6, 2022

View reviewed changes

drstrangelooker force-pushed the main branch 4 times, most recently from 9474788 to 5f9930c Compare February 4, 2022 02:33

kalenp requested a review from a team as a code owner March 12, 2024 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Kotlin RTL #384

Refactor Kotlin RTL #384

kalenp commented Nov 19, 2020

kalenp left a comment

kalenp Nov 19, 2020

kalenp Nov 19, 2020

kalenp Nov 19, 2020

kalenp Nov 19, 2020

joeldodge79 Nov 19, 2020

kalenp Nov 19, 2020

joeldodge79 Nov 24, 2020

kalenp Nov 19, 2020

jkaster Nov 24, 2020

kalenp Nov 19, 2020

jkaster Nov 24, 2020

kalenp Nov 19, 2020

jkaster Nov 24, 2020

kalenp Nov 24, 2020

joeldodge79 Nov 24, 2020

jkaster May 4, 2021

Refactor Kotlin RTL #384

Are you sure you want to change the base?

Refactor Kotlin RTL #384

Conversation

kalenp commented Nov 19, 2020

kalenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Updated feedback