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

adding replyTo field in http request body to submit RFQ #140

Merged
merged 16 commits into from
Feb 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
# The format is described: https://github.blog/2017-07-06-introducing-code-owners/

# These owners will be the default owners for everything in the repo.
* @mistermoe @phoebe-lew @jiyoontbd @michaelneale @KendallWeihe @ethan-tbd @tomdaffurn @diehuxx
* @mistermoe @phoebe-lew @jiyoontbd @michaelneale @KendallWeihe @ethan-tbd @tomdaffurn @diehuxx @kirahsapong

# -----------------------------------------------
# BELOW THIS LINE ARE TEMPLATES, UNUSED
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,20 @@ import okhttp3.HttpUrl.Companion.toHttpUrl
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
Expand Down Expand Up @@ -68,36 +72,71 @@ object TbdexHttpClient {
}

/**
* 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) {
Copy link
Contributor

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 called sendMessage, 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?

Copy link
Contributor Author

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), and sendMessage() for everything else? i'll give that a go.

Copy link
Contributor Author

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 the kind and throw IllegalArgumentException. i'm not sure if i like that dev ex. i suppose we could explicitly say pls 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 thoughts

Copy link
Contributor

@diehuxx diehuxx Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suppose we could explicitly say pls don't use this method if you're sending an RFQ!!11 in the ktdocs.

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 a createExchange endpoint which takes an RFQ and a replyTo. 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.

Copy link
Contributor Author

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

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)
}

/**
Expand Down Expand Up @@ -208,4 +247,25 @@ object TbdexHttpClient {
errors = errors
)
}

private fun validateMessage(message: Message) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only used in tests, can we move it into test/ directory? Or is it intended to be used by consumers of this library in some way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 rfq and replyTo (the new overloaded method)

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
Expand Up @@ -96,13 +96,14 @@ class E2ETest {

val rfqData = buildRfqData(firstOfferingId, vcJwt)
val rfq = Rfq.create(to = pfiDid, from = myDid.uri, rfqData)

rfq.sign(myDid)

println("Sending RFQ against first offering id: ${offerings[0].metadata.id}.")
println("ExchangeId for the rest of this exchange is ${rfq.metadata.exchangeId}")

try {
client.sendMessage(rfq)
client.createExchange(rfq, "https://tbdex.io/callback")
} catch (e: TbdexResponseException) {
throw AssertionError(
"Error in sending RFQ. " +
Expand All @@ -124,6 +125,7 @@ class E2ETest {
order.sign(myDid)

println("Sending order against Quote with exchangeId of ${order.metadata.exchangeId}")

try {
client.sendMessage(order)
} catch (e: TbdexResponseException) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,16 @@ class TbdexHttpClientTest {
server.enqueue(MockResponse().setResponseCode(HttpURLConnection.HTTP_ACCEPTED))

val rfq = TestData.getRfq(pfiDid.uri, TypeId.generate("offering"))
assertDoesNotThrow { TbdexHttpClient.sendMessage(rfq) }
assertDoesNotThrow { TbdexHttpClient.sendMessage(rfq, "https://tbdex.io/callback") }
}

@Test
fun `send RFQ with createExchange success via mockwebserver`() {

server.enqueue(MockResponse().setResponseCode(HttpURLConnection.HTTP_ACCEPTED))

val rfq = TestData.getRfq(pfiDid.uri, TypeId.generate("offering"))
assertDoesNotThrow { TbdexHttpClient.createExchange(rfq, "https://tbdex.io/callback") }
}

@Test
Expand All @@ -109,10 +118,49 @@ class TbdexHttpClientTest {
)

val mockResponseString = Json.jsonMapper.writeValueAsString(errorDetails)
server.enqueue(MockResponse().setBody(mockResponseString).setResponseCode(HttpURLConnection.HTTP_BAD_REQUEST))
server
.enqueue(
MockResponse()
.setBody(mockResponseString)
.setResponseCode(HttpURLConnection.HTTP_BAD_REQUEST)
)

val rfq = TestData.getRfq(pfiDid.uri, TypeId.generate("offering"))
val exception = assertThrows<TbdexResponseException> {
TbdexHttpClient.sendMessage(rfq)
}
assertEquals(1, exception.errors?.size)
assertEquals("400", exception.errors?.get(0)?.status)
}

@Test
fun `send RFQ with createExchange() fail via mockwebserver`() {
val errorDetails = mapOf(
"errors" to listOf(
ErrorDetail(
id = "1",
status = "400",
code = "INVALID_INPUT",
title = "Invalid Input",
detail = "The request input is invalid.",
source = null,
meta = null
)
)
)

val mockResponseString = Json.jsonMapper.writeValueAsString(errorDetails)
server
.enqueue(
MockResponse()
.setBody(mockResponseString)
.setResponseCode(HttpURLConnection.HTTP_BAD_REQUEST)
)

val rfq = TestData.getRfq(pfiDid.uri, TypeId.generate("offering"))
val exception = assertThrows<TbdexResponseException> { TbdexHttpClient.sendMessage(rfq) }
val exception = assertThrows<TbdexResponseException> {
TbdexHttpClient.createExchange(rfq, "https://tbdex.io/callback")
}
assertEquals(1, exception.errors?.size)
assertEquals("400", exception.errors?.get(0)?.status)
}
Expand Down Expand Up @@ -149,7 +197,9 @@ class TbdexHttpClientTest {
val mockResponseString = Json.jsonMapper.writeValueAsString(errorDetails)
server.enqueue(MockResponse().setBody(mockResponseString).setResponseCode(HttpURLConnection.HTTP_BAD_REQUEST))

assertThrows<TbdexResponseException> { TbdexHttpClient.getExchange(pfiDid.uri, alice, "exchange_1234") }
assertThrows<TbdexResponseException> {
TbdexHttpClient.getExchange(pfiDid.uri, alice, "exchange_1234")
}
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ import io.ktor.server.routing.get
import io.ktor.server.routing.post
import io.ktor.server.routing.route
import io.ktor.server.routing.routing
import tbdex.sdk.httpserver.handlers.createExchange
import tbdex.sdk.httpserver.handlers.getExchanges
import tbdex.sdk.httpserver.handlers.getOfferings
import tbdex.sdk.httpserver.handlers.submitClose
import tbdex.sdk.httpserver.handlers.submitOrder
import tbdex.sdk.httpserver.handlers.submitRfq
import tbdex.sdk.httpserver.models.ExchangesApi
import tbdex.sdk.httpserver.models.FakeExchangesApi
import tbdex.sdk.httpserver.models.FakeOfferingsApi
Expand Down Expand Up @@ -100,8 +100,8 @@ class TbdexHttpServer(private val config: TbdexHttpServerConfig) {
}

route("/exchanges") {
post("/{exchangeId}/rfq") {
submitRfq(
post("/{exchangeId}") {
createExchange(
call = call,
offeringsApi = offeringsApi,
exchangesApi = exchangesApi,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -49,7 +67,8 @@ suspend fun submitRfq(
val errorDetail = ErrorDetail(detail = "RFQ already exists.")
Copy link
Contributor

@diehuxx diehuxx Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tangential to the replyTo stuff, but can you update the catch (_: Exception) below to catch (_: NoSuchElementException) and add comment // exchangesApi.getExchange throws if no existing exchange is found. For a sec I was pretty confused why we were swallowing an error so egregiously.

Further musings: maybe this isn't the most graceful API for ExchangeApi. Personally I would return an empty list rather than throw. But that's a separate PR if we decide to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 exchangesApi.getExchange(exchangeId) does not return an exchange.

this try catch is actually hoping an NoSuchElementException is thrown when an exchange with the exchangeId is not found - which is what we want, because we want to create a new exchange

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
Expand Down Expand Up @@ -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
}
}
Loading
Loading