-
Notifications
You must be signed in to change notification settings - Fork 3
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
adding replyTo
field in http request body to submit RFQ
#140
Changes from 14 commits
30101d1
ae4713a
d7b968c
ba08a73
65de422
fcff6e1
8f14f01
729854b
26e49f3
6d86b59
ae0d434
b63a0ab
2eed618
e790f32
b074ee3
9c27b39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,16 +5,20 @@ | |
import okhttp3.MediaType.Companion.toMediaType | ||
import okhttp3.OkHttpClient | ||
import okhttp3.Request | ||
import okhttp3.RequestBody | ||
import okhttp3.RequestBody.Companion.toRequestBody | ||
import okhttp3.Response | ||
import tbdex.sdk.httpclient.models.CreateExchangeRequest | ||
import tbdex.sdk.httpclient.models.ErrorDetail | ||
import tbdex.sdk.httpclient.models.Exchange | ||
import tbdex.sdk.httpclient.models.GetExchangesFilter | ||
import tbdex.sdk.httpclient.models.GetOfferingsFilter | ||
import tbdex.sdk.httpclient.models.TbdexResponseException | ||
import tbdex.sdk.protocol.Validator | ||
import tbdex.sdk.protocol.models.Message | ||
import tbdex.sdk.protocol.models.MessageKind | ||
import tbdex.sdk.protocol.models.Offering | ||
import tbdex.sdk.protocol.models.Rfq | ||
import tbdex.sdk.protocol.serialization.Json | ||
import tbdex.sdk.protocol.serialization.Json.jsonMapper | ||
import web5.sdk.dids.Did | ||
|
@@ -68,36 +72,71 @@ | |
} | ||
|
||
/** | ||
* Sends a message to the PFI. | ||
* Sends a message to the PFI. You can also use this message to create an exchange without a replyTo URL. | ||
* | ||
* @param message The [Message] object containing the message details to be sent. | ||
* @throws TbdexResponseException for request or response errors. | ||
* @param message The message to send. | ||
* | ||
* @throws TbdexResponseException for response errors. | ||
*/ | ||
fun sendMessage(message: Message) { | ||
Validator.validateMessage(message) | ||
message.verify() | ||
validateMessage(message) | ||
|
||
val pfiDid = message.metadata.to | ||
val exchangeId = message.metadata.exchangeId | ||
val kind = message.metadata.kind | ||
|
||
val pfiServiceEndpoint = getPfiServiceEndpoint(pfiDid) | ||
val url = "$pfiServiceEndpoint/exchanges/$exchangeId/$kind" | ||
val url: String = if (kind == MessageKind.rfq) { | ||
"$pfiServiceEndpoint/exchanges/$exchangeId" | ||
} else { | ||
"$pfiServiceEndpoint/exchanges/$exchangeId/$kind" | ||
} | ||
|
||
val body = Json.stringify(message).toRequestBody(jsonMediaType) | ||
val body: RequestBody = Json.stringify(message).toRequestBody(jsonMediaType) | ||
|
||
val request = Request.Builder() | ||
.url(url) | ||
.addHeader("Content-Type", JSON_HEADER) | ||
.post(body) | ||
.build() | ||
val request = buildRequest(url, body) | ||
|
||
println("attempting to send message to: ${request.url}") | ||
println("Attempting to send $kind message to: ${request.url}") | ||
|
||
val response: Response = client.newCall(request).execute() | ||
if (!response.isSuccessful) { | ||
throw buildResponseException(response) | ||
} | ||
executeRequest(request) | ||
} | ||
|
||
/** | ||
* Send RFQ message and include a replyTo URL for the PFI to send a callback to. | ||
* | ||
* @param message The message to send (is of type RFQ) | ||
* @param replyTo The callback URL for PFI to send messages to. | ||
* | ||
* @throws TbdexResponseException for response errors. | ||
* | ||
*/ | ||
fun sendMessage(message: Rfq, replyTo: String) { | ||
validateMessage(message) | ||
|
||
val pfiDid = message.metadata.to | ||
val exchangeId = message.metadata.exchangeId | ||
|
||
val pfiServiceEndpoint = getPfiServiceEndpoint(pfiDid) | ||
val url = "$pfiServiceEndpoint/exchanges/$exchangeId" | ||
|
||
val body: RequestBody = Json.stringify(CreateExchangeRequest(message, replyTo)) | ||
.toRequestBody(jsonMediaType) | ||
|
||
val request = buildRequest(url, body) | ||
|
||
println("Attempting to send Rfq message to: ${request.url}") | ||
|
||
executeRequest(request) | ||
} | ||
|
||
/** | ||
* Aliased method for sendMessage(Rfq, String) to create an exchange by sending an RFQ with a replyTo URL. | ||
* | ||
* @param message The message to send (is of type RFQ) | ||
* @param replyTo The callback URL for PFI to send messages to. | ||
*/ | ||
fun createExchange(message: Rfq, replyTo: String) { | ||
sendMessage(message, replyTo) | ||
} | ||
|
||
/** | ||
|
@@ -208,4 +247,25 @@ | |
errors = errors | ||
) | ||
} | ||
|
||
private fun validateMessage(message: Message) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice |
||
Validator.validateMessage(message) | ||
message.verify() | ||
} | ||
|
||
private fun buildRequest(url: String, body: RequestBody): Request { | ||
val request = Request.Builder() | ||
.url(url) | ||
.addHeader("Content-Type", JSON_HEADER) | ||
.post(body) | ||
.build() | ||
return request | ||
} | ||
|
||
private fun executeRequest(request: Request) { | ||
val response: Response = client.newCall(request).execute() | ||
if (!response.isSuccessful) { | ||
throw buildResponseException(response) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package tbdex.sdk.httpclient.models | ||
|
||
import tbdex.sdk.protocol.models.Rfq | ||
|
||
/** | ||
* Data class used to type request body to create exchange via TbdexHttpServer. | ||
* | ||
* @property rfq Rfq tbdex message received. | ||
* @property replyTo Optional URL to be included in the request to create exchange. | ||
*/ | ||
class CreateExchangeRequest( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is only used in tests, can we move it into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we do use this in Tbdexhttpclient#sendMessage method where i'm passing in val body: RequestBody = Json.stringify(CreateExchangeRequest(message, replyTo))
.toRequestBody(jsonMediaType) |
||
val rfq: Rfq, | ||
val replyTo: String? = null | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ import tbdex.sdk.protocol.models.Message | |
import tbdex.sdk.protocol.models.MessageKind | ||
import tbdex.sdk.protocol.models.Offering | ||
import tbdex.sdk.protocol.models.Rfq | ||
import tbdex.sdk.protocol.serialization.Json | ||
import java.net.URL | ||
|
||
/** | ||
* Handles the submission of a Request for Quote (RFQ) through the TBDex API. | ||
|
@@ -27,18 +29,34 @@ import tbdex.sdk.protocol.models.Rfq | |
* @param callback An optional callback function to be invoked after processing the RFQ. | ||
*/ | ||
@Suppress("TooGenericExceptionCaught", "SwallowedException") | ||
suspend fun submitRfq( | ||
suspend fun createExchange( | ||
call: ApplicationCall, | ||
offeringsApi: OfferingsApi, | ||
exchangesApi: ExchangesApi, | ||
callback: SubmitCallback? | ||
) { | ||
val message: Rfq? | ||
|
||
val replyTo: String? | ||
try { | ||
message = Message.parse(call.receiveText()) as Rfq | ||
val requestBody = call.receiveText() | ||
|
||
val jsonNode = Json.jsonMapper.readTree(requestBody) | ||
val rfqJsonString = jsonNode["rfq"].toString() | ||
|
||
message = Message.parse(rfqJsonString) as Rfq | ||
// sets replyTo field to null if it's not present in the jsonNode. | ||
// without .takeIf { !it.isNull }, jsonNode["replyTo"].asText() will set replyTo as "null" string. | ||
replyTo = jsonNode["replyTo"].takeIf { !it.isNull }?.asText() | ||
|
||
} catch (e: Exception) { | ||
val errorDetail = ErrorDetail(detail = "Parsing of TBDex message failed: ${e.message}") | ||
val errorDetail = ErrorDetail(detail = "Parsing of TBDex createExchange request failed: ${e.message}") | ||
val errorResponse = ErrorResponse(listOf(errorDetail)) | ||
call.respond(HttpStatusCode.BadRequest, errorResponse) | ||
return | ||
} | ||
|
||
if (replyTo != null && !isValidUrl(replyTo)) { | ||
val errorDetail = ErrorDetail(detail = "replyTo must be a valid URL") | ||
val errorResponse = ErrorResponse(listOf(errorDetail)) | ||
call.respond(HttpStatusCode.BadRequest, errorResponse) | ||
return | ||
|
@@ -49,7 +67,8 @@ suspend fun submitRfq( | |
val errorDetail = ErrorDetail(detail = "RFQ already exists.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tangential to the Further musings: maybe this isn't the most graceful API for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed - yea, this try catch is a bit confusing! in this request handler to create exchange, we want to make sure that this try catch is actually hoping an as you said, we should probably return an empty list if an exchange with the exchangeId is not found instead of throwing an exception. |
||
call.respond(HttpStatusCode.Conflict, ErrorResponse(listOf(errorDetail))) | ||
return | ||
} catch (_: Exception) { | ||
} catch (_: NoSuchElementException) { | ||
// exchangesApi.getExchange throws if no existing exchange is found | ||
} | ||
|
||
val offering: Offering | ||
|
@@ -84,4 +103,20 @@ suspend fun submitRfq( | |
} | ||
|
||
call.respond(HttpStatusCode.Accepted) | ||
} | ||
|
||
/** | ||
* Checks if a string is a valid URL. | ||
* | ||
* @param replyToUrl The string to be checked. | ||
* @return boolean indicating whether the string is a valid URL. | ||
*/ | ||
@Suppress("SwallowedException") | ||
fun isValidUrl(replyToUrl: String): Boolean { | ||
return try { | ||
URL(replyToUrl) | ||
true | ||
} catch (e: Exception) { | ||
false | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jiyoontbd what do you think about having a separate
fun createExchange(rfq: RFQ, replyTo: String?)
function for creating an exchange?The reason being, there are now two functions that can be called to create an exchange. This one can accept messages of any type (including RFQ), but does not support
replyTo
URL. The other is also calledsendMessage
, but you can actually only send an RFQ, not just any message type.You would essentially have one client method for creating an exchange, which can have a
replyTo
or not (optional), and a different method for all subsequent messages.Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea. so you're saying we'd call
createExchange()
for sending RFQ (with or without replyTo), andsendMessage()
for everything else? i'll give that a go.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wish we could type guard for
sendMessage()
to prevent RFQ from being passed in as message in kotlin. so without the type guard, i'd have to check thekind
and throw IllegalArgumentException. i'm not sure if i like that dev ex. i suppose we could explicitly saypls don't use this method if you're sending an RFQ!!11
in the ktdocs.i'm leaning towards keeping the overloaded
sendMessage()
, would love to hear @diehuxx 's thoughtsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really prefer not to resort to that. IMO as long as we're leveraging the type system to enforce correct message types and options at compile-time, I can be flexible about how we name methods.
If I were writing this, I would keep the overloaded
sendMessage
AND add acreateExchange
endpoint which takes an RFQ and areplyTo
. Admittedly, my ruby background is showing in this suggestion, as aliasing methods is common in ruby.Feel free to take this idea or not. As long as the type system can enforce our choices, @jiyoontbd I trust your judgement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm good with that. the code editor should show the difference between the two. commit here