http-server: submitOrder and submitClose
#119
Conversation
phoebe-lew
requested review from
mistermoe,
jiyoonie9,
michaelneale,
KendallWeihe,
ethanwlee and
tomdaffurn
as code owners
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #119 +/- ##
==========================================
+ Coverage 77.51% 78.54% +1.03%
==========================================
Files 26 30 +4
Lines 556 662 +106
Branches 47 63 +16
==========================================
+ Hits 431 520 +89
- Misses 100 110 +10
- Partials 25 32 +7
|
| } catch (e: CallbackError) { | ||
| call.respond(e.statusCode, ErrorResponse(e.details)) | ||
| return | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
i am seeing this when writing the GET implementations as well - currently there doesn't seem to be a way to have the callback fail so we can check that the exception's thrown correctly and correct http codes returned in the call.respond(..
httpserver/src/main/kotlin/tbdex/sdk/httpserver/models/FakeOfferingsApi.kt
Show resolved
Hide resolved
| * @param callback The optional callback function to be executed after a successful close submission. | ||
| */ | ||
| @Suppress("TooGenericExceptionCaught", "MaxLineLength") | ||
| suspend fun submitClose( |
There was a problem hiding this comment.
Throughout the web5-kt codebase we've chosen to have exceptions be types, with the name of the exception signaling what the error was. An example of this is https://github.com/TBD54566975/web5-kt/blob/48ce6764163b32714be7d8282bfad0678f49debb/credentials/src/main/kotlin/web5/sdk/credentials/ExceptionDeclarations.kt#L10
The rationale of such approach was discussed in decentralized-identity/web5-kt#144
Is this something that makes sense to adopt in tbdex-kt?
If so, I'd suggest implementing as shown below, using a custom plugin in ktor:
val ExceptionsPlugin = createApplicationPlugin(name = "ExceptionsPlugin") {
println("ExceptionsPlugin is installed!")
on(CallFailed) { call, cause ->
when(cause) {
is MyCustomException -> {
val errorDetail = ErrorDetail(detail = cause.message.orEmpty())
val errorResponse = ErrorResponse(listOf(errorDetail))
call.respond(HttpStatusCode.BadRequest, errorResponse)
}
else -> {
println("Unknown error: ${cause.message}")
call.respond(HttpStatusCode.BadRequest, ErrorResponse(listOf(ErrorDetail(detail = cause.message.orEmpty()))))
}
}
}
}The plugin can be installed in the application by doing the following within the configure method.
app.install(ExceptionsPlugin)Personally, I'd advocate for this approach, as it decouples the translation from Exceptions to ErrorResponse types into a single place (the plugin). This makes it very easy to reuse Exceptions. Additionally, it ensures that the server always responds with ErrorResponse, even in the event of what unexpected exceptions being thrown (though perhaps that's caught elsewhere in the ktor framework!).
There was a problem hiding this comment.
Oh awesome, thanks! Yeah I think we are aligning on something similar in tbdex-js too, so it would make sense to adopt here.
There was a problem hiding this comment.
Created an issue for it: https://github.com/orgs/TBD54566975/projects/29/views/1?pane=issue&itemId=48150122
httpserver/src/main/kotlin/tbdex/sdk/httpserver/handlers/SubmitClose.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| try { | ||
| callback.invoke(call, MessageKind.close, null) |
There was a problem hiding this comment.
Callbacks are another case for which I would suggest using the plugin approach. We used a similar approach when defining Webhooks in ssi-service. A middleware-esque approach. The advantage is code decoupling and separation of concerns, which allows for code to be more easily modified in the future.
There was a problem hiding this comment.
Is this the right reference for how plugins work? https://ktor.io/docs/plugins.html
There was a problem hiding this comment.
Yes. I was looking at https://ktor.io/docs/custom-plugins.html more specifically
httpserver/src/test/kotlin/tbdex/sdk/httpserver/handlers/SubmitCloseTest.kt
Outdated
Show resolved
Hide resolved
* adding getofferings and getexchanges req handler * adding getofferings and getexchanges req handler * adding tests * Fix test * adding docs * Fix imports --------- Co-authored-by: Phoebe Lew <[email protected]>
closes #117
closes #26
closes #27