From 7b15f9002a263799c04d92d6278e4c4764b53e47 Mon Sep 17 00:00:00 2001 From: Michael Reed <1062477+haywood@users.noreply.github.com> Date: Mon, 9 Dec 2024 09:40:51 -0500 Subject: [PATCH] fix: Improve ok() helper function (#1542) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I'm hesitant to change this function too much, since we don't know what kind of odd dependencies on its current behavior might exist. However, adding exception chaining and surfacing the original error message seems like a risk worth taking. - Eliminate duplicated version in AuthSession by delegating to a shared implementation defined on the SDKResponse companion object. - For the SDKError case: - Set Error.message from SDKError.message - Set Error.casue from SDKError.cause - Also ran `ktlint -F` on the files I changed so that `gradle check` would pass the linting stage - Also added kotlin-ci.yml to run `./gradlew jar` on kotlin changes Fixes #1539 🦕 --- .github/workflows/kotlin-ci.yml | 63 ++++ kotlin/src/main/com/looker/rtl/AuthSession.kt | 109 +++--- kotlin/src/main/com/looker/rtl/Transport.kt | 330 ++++++++++-------- 3 files changed, 288 insertions(+), 214 deletions(-) create mode 100644 .github/workflows/kotlin-ci.yml diff --git a/.github/workflows/kotlin-ci.yml b/.github/workflows/kotlin-ci.yml new file mode 100644 index 000000000..870e21fc0 --- /dev/null +++ b/.github/workflows/kotlin-ci.yml @@ -0,0 +1,63 @@ +# This workflow uses actions that are not certified by GitHub. +# They are provided by a third-party and are governed by +# separate terms of service, privacy policy, and support +# documentation. +# This workflow will build a Java project with Gradle and cache/restore any dependencies to improve the workflow execution time +# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-java-with-gradle + +name: Kotlin CI + +on: + pull_request: + paths: + - kotlin/** + - .github/workflows/gradle.yml + + push: + branches: [ "main" ] + paths: + - kotlin/** + - .github/workflows/gradle.yml + +# TODO(#1544): Also run tests +jobs: + build: + + runs-on: ubuntu-latest + permissions: + contents: read + + steps: + - uses: actions/checkout@v4 + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + java-version: '17' + distribution: 'temurin' + - name: Setup Gradle + uses: gradle/actions/setup-gradle@af1da67850ed9a4cedd57bfd976089dd991e2582 # v4.0.0 + - name: Build with Gradle Wrapper + run: ./gradlew jar + working-directory: kotlin + + dependency-submission: + + runs-on: ubuntu-latest + permissions: + contents: write + + steps: + - uses: actions/checkout@v4 + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + java-version: '17' + distribution: 'temurin' + - run: cd kotlin + + # Generates and submits a dependency graph, enabling Dependabot Alerts for all project dependencies. + # See: https://github.com/gradle/actions/blob/main/dependency-submission/README.md + - name: Generate and submit dependency graph + uses: gradle/actions/dependency-submission@af1da67850ed9a4cedd57bfd976089dd991e2582 # v4.0.0 + with: + build-root-directory: kotlin diff --git a/kotlin/src/main/com/looker/rtl/AuthSession.kt b/kotlin/src/main/com/looker/rtl/AuthSession.kt index 70b4332f5..67f1e4c81 100644 --- a/kotlin/src/main/com/looker/rtl/AuthSession.kt +++ b/kotlin/src/main/com/looker/rtl/AuthSession.kt @@ -30,14 +30,11 @@ open class AuthSession( open val apiSettings: ConfigurationProvider, open val transport: Transport = Transport(apiSettings), ) { - var authToken: AuthToken = AuthToken() private var sudoToken: AuthToken = AuthToken() var sudoId: String = "" - /** - * Abstraction of AuthToken retrieval to support sudo mode - */ + /** Abstraction of AuthToken retrieval to support sudo mode */ fun activeToken(): AuthToken { if (sudoToken.accessToken.isNotEmpty()) { return sudoToken @@ -45,9 +42,7 @@ open class AuthSession( return authToken } - /** - * Is there an active authentication token? - */ + /** Is there an active authentication token? */ open fun isAuthenticated(): Boolean { val token = activeToken() if (token.accessToken.isBlank()) return false @@ -56,8 +51,8 @@ open class AuthSession( /** * Add authentication data to the pending API request - * @param[init] Initialized API request properties * + * @param[init] Initialized API request properties * @return The updated request properties */ fun authenticate(init: RequestSettings): RequestSettings { @@ -69,13 +64,11 @@ open class AuthSession( return init.copy(headers = headers) } - fun isSudo(): Boolean { - return sudoId.isNotBlank() && sudoToken.isActive() - } + fun isSudo(): Boolean = sudoId.isNotBlank() && sudoToken.isActive() /** - * Retrieve the current authentication token. If there is no active token, performs default login to retrieve the - * token. + * Retrieve the current authentication token. If there is no active token, performs default + * login to retrieve the token. */ open fun getToken(): AuthToken { if (!isAuthenticated()) { @@ -84,9 +77,7 @@ open class AuthSession( return activeToken() } - /** - * Reset the authentication session - */ + /** Reset the authentication session */ fun reset() { sudoId = "" authToken.reset() @@ -95,13 +86,14 @@ open class AuthSession( /** * Activate the authentication token for the API3 or sudo user + * * @param[sudoId] If provided, impersonates the user specified */ - fun login(sudoId: String = ""): AuthToken = doLogin(sudoId) /** - * Logout the active user. If the active user is impersonated , the session reverts to the API3 user. + * Logout the active user. If the active user is impersonated , the session reverts to the API3 + * user. */ fun logout(): Boolean { if (isAuthenticated()) { @@ -110,14 +102,7 @@ open class AuthSession( return false } - fun 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!!") - } - } + fun ok(response: SDKResponse) = SDKResponse.ok(response) private fun sudoLogout(): Boolean { var result = false @@ -140,37 +125,39 @@ open class AuthSession( val client_secret = "client_secret" val config = apiSettings.readConfig() val clientId = - unQuote(System.getProperty("${apiSettings.environmentPrefix}_CLIENT_ID") ?: config[client_id]) + unQuote( + System.getProperty("${apiSettings.environmentPrefix}_CLIENT_ID") + ?: config[client_id], + ) val clientSecret = - unQuote(System.getProperty("${apiSettings.environmentPrefix}_CLIENT_SECRET") ?: config[client_secret]) - val params = mapOf( - client_id to clientId, - client_secret to clientSecret, - ) + unQuote( + System.getProperty("${apiSettings.environmentPrefix}_CLIENT_SECRET") + ?: config[client_secret], + ) + val params = mapOf(client_id to clientId, client_secret to clientSecret) val body = UrlEncodedContent(params) - val token = ok( - transport.request( - HttpMethod.POST, - "$apiPath/login", - emptyMap(), - body, - ), - ) + val token = + ok( + transport.request( + HttpMethod.POST, + "$apiPath/login", + emptyMap(), + body, + ), + ) authToken = token } if (sudoId.isNotBlank()) { val token = activeToken() - val sudoToken = transport.request( - HttpMethod.POST, - "/login/$newId", - ) { requestSettings -> - val headers = requestSettings.headers.toMutableMap() - if (token.accessToken.isNotBlank()) { - headers["Authorization"] = "Bearer ${token.accessToken}" + val sudoToken = + transport.request(HttpMethod.POST, "/login/$newId") { requestSettings -> + val headers = requestSettings.headers.toMutableMap() + if (token.accessToken.isNotBlank()) { + headers["Authorization"] = "Bearer ${token.accessToken}" + } + requestSettings.copy(headers = headers) } - requestSettings.copy(headers = headers) - } this.sudoToken = ok(sudoToken) } return activeToken() @@ -178,19 +165,21 @@ open class AuthSession( private fun doLogout(): Boolean { val token = activeToken() - val resp = transport.request(HttpMethod.DELETE, "/logout") { - val headers = it.headers.toMutableMap() - if (token.accessToken.isNotBlank()) { - headers["Authorization"] = "Bearer ${token.accessToken}" + val resp = + transport.request(HttpMethod.DELETE, "/logout") { + val headers = it.headers.toMutableMap() + if (token.accessToken.isNotBlank()) { + headers["Authorization"] = "Bearer ${token.accessToken}" + } + it.copy(headers = headers) } - it.copy(headers = headers) - } - val success = when (resp) { - is SDKResponse.SDKSuccessResponse<*> -> true - is SDKResponse.SDKErrorResponse<*> -> false - else -> false - } + val success = + when (resp) { + is SDKResponse.SDKSuccessResponse<*> -> true + is SDKResponse.SDKErrorResponse<*> -> false + else -> false + } if (sudoId.isNotBlank()) { sudoId = "" sudoToken.reset() diff --git a/kotlin/src/main/com/looker/rtl/Transport.kt b/kotlin/src/main/com/looker/rtl/Transport.kt index 2806e67d3..b068272b5 100644 --- a/kotlin/src/main/com/looker/rtl/Transport.kt +++ b/kotlin/src/main/com/looker/rtl/Transport.kt @@ -85,63 +85,73 @@ sealed class SDKResponse { } /** An error representing an issue in the SDK, like a network or parsing error. */ - data class SDKError(val message: String, val cause: Exception) : SDKResponse() { + data class SDKError( + val message: String, + val cause: Exception, + ) : SDKResponse() { val type: String = "sdk_error" } inline fun getOrThrow(): V = when (this) { - is SDKResponse.SDKSuccessResponse<*> -> - checkNotNull(value as? V) { - if (value == null) { - "Expected value of type ${V::class}, but was null" - } else { - "Expected value of type ${V::class}, but was ${value::class}" - } - } - - is SDKResponse.SDKErrorResponse<*> -> - throw LookerApiException( - method, - path, - statusCode, - statusMessage, - responseHeaders, - responseBody, - ) - - is SDKResponse.SDKError -> throw cause + is SDKResponse.SDKSuccessResponse<*> -> + checkNotNull(value as? V) { + if (value == null) { + "Expected value of type ${V::class}, but was null" + } else { + "Expected value of type ${V::class}, but was ${value::class}" + } + } + is SDKResponse.SDKErrorResponse<*> -> + throw LookerApiException( + method, + path, + statusCode, + statusMessage, + responseHeaders, + responseBody, + ) + is SDKResponse.SDKError -> throw cause } - + companion object { const val ERROR_BODY = "error_body" + + /** + * Response handler that throws an error on error response, returns success result on + * success + */ + @Deprecated( + "This method throws java.lang.Error, which is not recommended for use in application code. Please use SDKResponse.getOrThrow() instead.", + ) + fun 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 + is SDKResponse.SDKError -> throw Error(response.message, response.cause) + } + } } } /** Thrown when a Looker API call returns an error. */ data class LookerApiException( - val method: HttpMethod, - val path: String, - val statusCode: Int, - val statusMessage: String, - val responseHeaders: HttpHeaders, - val responseBody: String, + val method: HttpMethod, + val path: String, + val statusCode: Int, + val statusMessage: String, + val responseHeaders: HttpHeaders, + val responseBody: String, ) : Exception() { - override val message = "$method $path $statusCode: $statusMessage" + override val message = "$method $path $statusCode: $statusMessage" } /** Response handler that throws an error on error response, returns success result on success */ @Deprecated( - "This method throws java.lang.Error, which is not recommended for use in application code. Please use SDKResponse.getOrThrow() instead." + "This method throws java.lang.Error, which is not recommended for use in application code. Please use SDKResponse.getOrThrow() instead.", ) -fun 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!!") - } -} +fun ok(response: SDKResponse) = SDKResponse.ok(response) enum class HttpMethod { GET, @@ -152,10 +162,13 @@ enum class HttpMethod { HEAD, } -enum class HttpTransports(val label: String) { +enum class HttpTransports( + val label: String, +) { APACHE("Apache HTTP Client"), JAVA_NET("Native Java HTTP Client"), - // URL_FETCH("Google App Engine HTTP Client"), TODO: App Engine support? Requires App Engine SDK. + // URL_FETCH("Google App Engine HTTP Client"), TODO: App Engine support? Requires App Engine + // SDK. // KTOR("Kotlin based HTTP Client") TODO: Add ktor transport wrapper. Do we need this? } @@ -181,6 +194,7 @@ interface TransportOptions { interface ConfigurationProvider : TransportOptions { fun isConfigured(): Boolean + fun readConfig(): Map } @@ -198,13 +212,14 @@ private val utcFormat by lazy { DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm: fun encodeParam(value: Any?): String { val utf8 = "utf-8" - var encoded = if (value is ZonedDateTime) { - value.toOffsetDateTime().format(utcFormat) - } else if (value is Date) { - value.toInstant().atZone(ZoneOffset.UTC).format(utcFormat) - } else { - "$value" - } + var encoded = + if (value is ZonedDateTime) { + value.toOffsetDateTime().format(utcFormat) + } else if (value is Date) { + value.toInstant().atZone(ZoneOffset.UTC).format(utcFormat) + } else { + "$value" + } try { val decoded = URLDecoder.decode(encoded, utf8) if (encoded == decoded) { @@ -226,19 +241,23 @@ fun encodeValues(params: Values = emptyMap()): String { .joinToString("&") } -fun addQueryParams(path: String, params: Values = emptyMap()): String { +fun addQueryParams( + path: String, + params: Values = emptyMap(), +): String { if (params.isEmpty()) return path val qp = encodeValues(params) return "$path?$qp" } -/** Returns a [HttpRequestInitializer] prepared with the provided requestSettings */ +/** Returns a [HttpRequestInitializer] prepared with the provided requestSettings */ fun customInitializer( options: TransportOptions, requestSettings: RequestSettings, -): HttpRequestInitializer { - return HttpRequestInitializer { request -> // Timeout is passed in as seconds +): HttpRequestInitializer = + HttpRequestInitializer { request -> + // Timeout is passed in as seconds val timeout = (options.timeout * 1000) request.connectTimeout = timeout request.readTimeout = timeout @@ -247,33 +266,32 @@ fun customInitializer( request.followRedirects = true // set headers - request.headers = HttpHeaders().also { - requestSettings.headers.forEach { (k, v) -> - it.set(k, v) - } - } + request.headers = + HttpHeaders().also { requestSettings.headers.forEach { (k, v) -> it.set(k, v) } } } -} - -open class Transport(val options: TransportOptions) { +open class Transport( + val options: TransportOptions, +) { private val apiPath = "${options.baseUrl}/api/${options.apiVersion}" /** * Create the correct http request path + * * @param path Relative or absolute path * @param queryParams query string arguments (if any) * @param authenticator optional authenticator callback for API requests - * @return a fully qualified path that is the base url, the api path, or a pass through request url + * @return a fully qualified path that is the base url, the api path, or a pass through request + * url */ fun makeUrl( path: String, queryParams: Values = emptyMap(), - authenticator: Authenticator? = null, // TODO figure out why ::defaultAuthenticator is matching when it shouldn't - ): String { - return if (path.startsWith("http://", true) || - path.startsWith("https://", true) - ) { + authenticator: Authenticator? = + null, // TODO figure out why ::defaultAuthenticator is matching when it + // shouldn't + ): String = + if (path.startsWith("http://", true) || path.startsWith("https://", true)) { "" // full path was passed in } else { if (authenticator === null) { @@ -282,30 +300,26 @@ open class Transport(val options: TransportOptions) { apiPath } } + addQueryParams(path, queryParams) - } open fun getAllTrustingVerifiers(): Pair { // NOTE! This is completely insecure and should ONLY be used with local server instance // testing for development purposes - val tm: X509TrustManager = object : X509TrustManager { - override fun getAcceptedIssuers(): Array { - return arrayOfNulls(0) + val tm: X509TrustManager = + object : X509TrustManager { + override fun getAcceptedIssuers(): Array = arrayOfNulls(0) + + @Throws(CertificateException::class) + override fun checkClientTrusted( + certs: Array?, + authType: String?, + ) {} + + @Throws(CertificateException::class) + override fun checkServerTrusted( + certs: Array?, + authType: String?, + ) {} } - - @Throws(CertificateException::class) - override fun checkClientTrusted( - certs: Array?, - authType: String?, - ) { - } - - @Throws(CertificateException::class) - override fun checkServerTrusted( - certs: Array?, - authType: String?, - ) { - } - } val trustAllCerts = arrayOf(tm) val sslContext = SSLContext.getInstance("SSL") sslContext.init(null, trustAllCerts, SecureRandom()) @@ -316,7 +330,8 @@ open class Transport(val options: TransportOptions) { return Pair(sslSocketFactory, hostnameVerifier) } - /** Given [TransportOptions], selects the requested [HttpTransport]. + /** + * Given [TransportOptions], selects the requested [HttpTransport]. * * Will disable SSL certificate verification iff [TransportOptions.verifySSL] is false. */ @@ -331,10 +346,7 @@ open class Transport(val options: TransportOptions) { val clientBuilder = ApacheHttpTransport.newDefaultHttpClientBuilder().disableCookieManagement() if (!options.verifySSL) { - val sslBuilder = SSLContextBuilder().loadTrustMaterial(null) { - _, _ -> - true - } + val sslBuilder = SSLContextBuilder().loadTrustMaterial(null) { _, _ -> true } val sslSocketFactory = SSLConnectionSocketFactory(sslBuilder.build()) clientBuilder .setSSLHostnameVerifier(NoopHostnameVerifier.INSTANCE) @@ -343,7 +355,6 @@ open class Transport(val options: TransportOptions) { ApacheHttpTransport(clientBuilder.build()) } - HttpTransports.JAVA_NET -> { if (!options.verifySSL) { val (sslSocketFactory, hostnameVerifier) = getAllTrustingVerifiers() @@ -375,66 +386,80 @@ open class Transport(val options: TransportOptions) { customInitializer(options, finalizedRequestSettings) val requestFactory: HttpRequestFactory = transport.createRequestFactory(requestInitializer) - val httpContent: HttpContent? = when (body) { - // the body has already been prepared as HttpContent - is HttpContent -> body - // body is a raw string to be converted to a byte array - is String -> ByteArrayContent("application/x-www-form-urlencoded", body.toByteArray()) - // body is a data class to be serialized as JSON or null - else -> { - // TODO: Consider using JsonHttpContent() - if (body != null) { - ByteArrayContent(Json.MEDIA_TYPE, GSON.toJson(body).toByteArray()) - } else { - null + val httpContent: HttpContent? = + when (body) { + // the body has already been prepared as HttpContent + is HttpContent -> body + // body is a raw string to be converted to a byte array + is String -> + ByteArrayContent( + "application/x-www-form-urlencoded", + body.toByteArray(), + ) + // body is a data class to be serialized as JSON or null + else -> { + // TODO: Consider using JsonHttpContent() + if (body != null) { + ByteArrayContent(Json.MEDIA_TYPE, GSON.toJson(body).toByteArray()) + } else { + null + } } } - } - val request: HttpRequest = requestFactory.buildRequest( - finalizedRequestSettings.method.toString(), - GenericUrl(finalizedRequestSettings.url), - httpContent, - ).setSuppressUserAgentSuffix(true) - -// TODO get overrides parameter to work without causing compilation errors in UserSession -// overrides: TransportOptions? = null): SDKResponse { -// overrides?.let { o -> -// if (options.verifySSL != o.verifySSL || options.timeout != o.timeout) { -// // need an HTTP client with custom options -// client = customClient(o) -// } -// } - - val sdkResponse = try { - val response = request.execute() - if (response.content == null) { - return SDKResponse.SDKSuccessResponse(null) - } - val rawResult: T = when (T::class) { - // some responses may be a string (e.g. query results in `csv` format) - String::class -> - response.content.bufferedReader().use(BufferedReader::readText) as T - // TODO(https://github.com/looker-open-source/sdk-codegen/issues/1341): - // add streaming support. Currently, `stream` methods read the entire response. - ByteArray::class -> response.content.readBytes() as T - // most responses are JSON - else -> response.parseAs(T::class.java) + val request: HttpRequest = + requestFactory + .buildRequest( + finalizedRequestSettings.method.toString(), + GenericUrl(finalizedRequestSettings.url), + httpContent, + ).setSuppressUserAgentSuffix(true) + + // TODO get overrides parameter to work without causing compilation errors in UserSession + // overrides: TransportOptions? = null): SDKResponse { + // overrides?.let { o -> + // if (options.verifySSL != o.verifySSL || options.timeout != o.timeout) { + // // need an HTTP client with custom options + // client = customClient(o) + // } + // } + + val sdkResponse = + try { + val response = request.execute() + if (response.content == null) { + return SDKResponse.SDKSuccessResponse(null) + } + val rawResult: T = + when (T::class) { + // some responses may be a string (e.g. query results in `csv` + // format) + String::class -> + response.content + .bufferedReader() + .use(BufferedReader::readText) as + T + // TODO(https://github.com/looker-open-source/sdk-codegen/issues/1341): + // add streaming support. Currently, `stream` methods read the + // entire response. + ByteArray::class -> response.content.readBytes() as T + // most responses are JSON + else -> response.parseAs(T::class.java) + } + SDKResponse.SDKSuccessResponse(rawResult) + } catch (e: HttpResponseException) { + SDKResponse.SDKErrorResponse( + "$method $path $ERROR_BODY: ${e.content}", + method, + path, + e.statusCode, + e.statusMessage, + e.headers, + e.content, + ) + } catch (e: Exception) { + SDKResponse.SDKError(e.message ?: "Something went wrong", e) } - SDKResponse.SDKSuccessResponse(rawResult) - } catch (e: HttpResponseException) { - SDKResponse.SDKErrorResponse( - "$method $path $ERROR_BODY: ${e.content}", - method, - path, - e.statusCode, - e.statusMessage, - e.headers, - e.content, - ) - } catch (e: Exception) { - SDKResponse.SDKError(e.message ?: "Something went wrong", e) - } return sdkResponse } @@ -452,9 +477,7 @@ open class Transport(val options: TransportOptions) { val provisionalHeaders = options.headers.toMutableMap() var auth = authenticator ?: ::defaultAuthenticator - if (path.startsWith("http://", true) || - path.startsWith("https://", true) - ) { + if (path.startsWith("http://", true) || path.startsWith("https://", true)) { // if a full path is passed in, this is a straight fetch, not an API call // so don't authenticate auth = ::defaultAuthenticator @@ -468,15 +491,13 @@ data class SDKErrorDetailInfo( var message: String, var field: String, var code: String, - @SerializedName("documentation_url") - var documentationUrl: String, + @SerializedName("documentation_url") var documentationUrl: String, ) data class SDKErrorInfo( var message: String, var errors: List, - @SerializedName("documentation_url") - var documentationUrl: String, + @SerializedName("documentation_url") var documentationUrl: String, ) fun parseSDKError(msg: String): SDKErrorInfo { @@ -486,7 +507,8 @@ fun parseSDKError(msg: String): SDKErrorInfo { info?.let { val payload = info.value result = GSON.fromJson(payload, SDKErrorInfo::class.java) - // Ignore the linter suggestion to replace `.isNullOrEmpty()` with `.isEmpty()` because it's *wrong* + // Ignore the linter suggestion to replace `.isNullOrEmpty()` with `.isEmpty()` because it's + // *wrong* if (result.errors.isNullOrEmpty()) { result.errors = listOf() }