Add specialized REST request exceptions

[Galarzaa90]

(See #269)

This is a simple attempt at implementing specialized REST request exceptions.

I initially considered making KtorRequestException a sealed class and making subclasses for the errors, but this would mean any implementation creating instances of KtorRequestException would be broken, and a catch-all subclass would be needed to handle all other cases.

I'm opening this as a draft as this is just a simple proposal that needs to be polished based on feedback received.

What is the purpose of this?

Going from this:

try {
    val role = guild.createRole {
        name = arguments.name
        reason = translate("command.group-manage.create.reason", arrayOf(member?.asMember()!!.tag))
    }
    // Success
} catch (hre: RestRequestException) {
    when (hre.status.code) {
        HttpStatusCode.Forbidden.value -> //tell user bot is missing permissions
        HttpStatusCode.BadRequest.value -> // tell user they probably used an invalid name
        else -> throw hre // do not handle
    }
}

To this:

try {
    val role = guild.createRole {
        name = arguments.name
        reason = translate("command.group-manage.create.reason", arrayOf(member?.asMember()!!.tag))
    }
    // Success
} catch (forbiddenException: ForbiddenKtorRequestException) {
    //tell user bot is missing permissions
}
catch (badRequestException: BadRequestKtorRequestException) {
    // tell user they probably used an invalid name
} 

[lukellmann]

you need to run ./gradlew apiDump and commit the changes to let the tests pass

[Galarzaa90]

you need to run ./gradlew apiDump and commit the changes to let the tests pass

Done

[DRSchlaubi]

The request handler is about to get deprecated, instead it would be great to use ktors built in response validation feature

[Galarzaa90]

The request handler is about to get deprecated, instead it would be great to use ktors built in response validation feature

I agree.

On the exceptions side, what do you guys think of the naming? Also, should I stick to these?

https://discord.com/developers/docs/topics/opcodes-and-status-codes#http-http-response-codes

[DRSchlaubi]

Sounds great, as making an exception class for every single json error code might be a bit too much, however we might be able to generate those

/cc @lukellmann

[Galarzaa90]

I definitely think we should use Ktor's response validation. However, I was checking kord's code and it would mean we have to add HttpResponseValidator to every instance of HttpClient being created.

This is a bit more entangled into the code 🤔

[DRSchlaubi]

Kord core should use the same client for everything, right?

[Galarzaa90]

Kord core should use the same client for everything, right?

You're right. I had another go at it. I was trying to add the validation to KordBuilderUrl, however, I ran into an issue, the exception classes expect dev.kord.rest.request.Request, and at this point what we have is io.ktor.client.request.DefaultHttpRequest.

internal fun HttpClient?.configure(): HttpClient {
    if (this != null) return this.config {
        defaultConfig()
    }

    val json = Json {
        encodeDefaults = false
        allowStructuredMapKeys = true
        ignoreUnknownKeys = true
        isLenient = true
    }

    return HttpClient(CIO) {
        defaultConfig()
        install(ContentNegotiation) {
            json(json)
        }
        expectSuccess = true
        HttpResponseValidator {
            handleResponseExceptionWithRequest { exception, request ->
                val clientException = exception as? ResponseException ?: return@handleResponseExceptionWithRequest
                val response = clientException.response
                if(response.isRateLimit) return@handleResponseExceptionWithRequest
                val body = response.bodyAsText()
                val discordError = if (response.isError) {
                    if (response.contentType() == ContentType.Application.Json)
                        DiscordErrorResponse.serializer().optional.deserialize(json, body)
                    else null
                } else
                    null
                throw when (response.status) {
                    HttpStatusCode.BadRequest -> BadRequestKtorRequestException(response, request, discordError)
                    HttpStatusCode.Forbidden -> ForbiddenKtorRequestException(response, request, discordError)
                    HttpStatusCode.NotFound -> NotFoundKtorRequestException(response, request, discordError)
                    HttpStatusCode.InternalServerError -> ServerErrorKtorRequestException(response, request, discordError)
                    else -> KtorRequestException(response, request, discordError)
                }
            }
        }
    }
}

[Galarzaa90]

I noticed I created this fork from 0.8.x instead of 0.9.x. But I'm seeing that 0.8.x. has commits that 0.9.x does't have.

[lukellmann]

Sounds great, as making an exception class for every single json error code might be a bit too much, however we might be able to generate those

i don't think it's worth having an exception per json error code (tbh i'm not even sure if the json error code enum is even useful)

[lukellmann]

I noticed I created this fork from 0.8.x instead of 0.9.x. But I'm seeing that 0.8.x. has commits that 0.9.x does't have.

yeah, that's because some commits got cherrypicked from one to the other but there was no merge between them, so they diverged

if you merge 0.9.x into the branch of your pr, it should be alright

[lukellmann]

or to make the history nicer, you can drop the commits before your work from the branch (e.g. with interactive rebase) and then rebase onto 0.9.x

[Galarzaa90]

Ok, I finally fixed the history. My branch is basically main with my new commits.

I'm still dealing with this conundrum though:
#835 (comment)