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

The SDK does not handle error conditions properly #1539

Closed
haywood opened this issue Dec 2, 2024 · 1 comment · Fixed by #1540 or #1542
Closed

The SDK does not handle error conditions properly #1539

haywood opened this issue Dec 2, 2024 · 1 comment · Fixed by #1540 or #1542
Assignees
Labels

Comments

@haywood
Copy link
Collaborator

haywood commented Dec 2, 2024

  1. The SDKErrorResponse type does not provide explicit access to the HTTP status code or other standard metadata
  2. The ok() function throws java.lang.Error, which should not be thrown by JVM application code
  3. The ok() function does not chain the cause of a client-side exception or include its error message

Source of the SDKErrorResponse class for convenience:

    /** An erroring SDK call. */
    data class SDKErrorResponse<T>(
        /** The error object returned by the SDK call. */
        val value: T,
    ) : SDKResponse() {
        /** Whether the SDK call was successful. */
        val ok: Boolean = false
    }

Source of the ok function for convenience

/**
 * Response handler that throws an error on error response, returns success result on success
 */
fun <T>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!!")
    }
}
@haywood haywood self-assigned this Dec 2, 2024
@github-actions github-actions bot added need triage p3 Priority 3 labels Dec 2, 2024
@haywood haywood closed this as completed in 18add63 Dec 2, 2024
drstrangelooker pushed a commit that referenced this issue Dec 2, 2024
- Provide HTTP request and response details on SDKErrorResponse.
- Provide the cause of SDKError for chaining and re-throwing
- Add LookerApiException, which includes the same request/response
information as SDKErrorResponse.
- Mark ok() as Deprecated, because it throws java.lang.Error, which is
not something application code should do.
- Add SDKResponse::getOrThrow() instance method, patterned after
Kotlin's built-in Result type, which handles SDKErrorResponse by
throwing the newly introduced LookerApiException

Fixes #1539  🦕
@haywood haywood reopened this Dec 3, 2024
@haywood
Copy link
Collaborator Author

haywood commented Dec 3, 2024

Re-opening for follow-up improvement to the ok() helper method

haywood added a commit that referenced this issue Dec 9, 2024
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 🦕
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant