Custom body decoding#555
Conversation
|
Lint decided to fail with versions updates for some reason on this PR. 🤷♂️ |
250dd1b to
32b2bcf
Compare
| } else if (bodyString.isBlank()) { | ||
| if (!isBodyPlainText) { |
There was a problem hiding this comment.
Can you merge this into a single condition?
There was a problem hiding this comment.
Not in the current format, but I think it makes more sense to change to when. It'll make it clearer.
| private class LiteralBodyDecoder : BodyDecoder { | ||
| override fun decodeRequest(request: Request, body: ByteString) = "Request" | ||
| override fun decodeResponse(response: Response, body: ByteString) = "Response" | ||
| } |
There was a problem hiding this comment.
I believe that all the *Decoder + all the related tests should be moved to a ChuckerInterceptor_withDecoderTest
| val pokemon = Pokemon("Pikachu", level = 99) | ||
| val body = pokemon.encodeByteString().toRequestBody("application/protobuf".toMediaType()) | ||
| val request = Request.Builder() | ||
| .url("https://postman-echo.com/post") |
There was a problem hiding this comment.
This class is getting bloated :( It's called HttpBinClient but we do a lot of other stuff inside:
downloadSampleImage(colorHex = "fff")
downloadSampleImage(colorHex = "000")
getResponsePartially()
getProtoResponse()
Can you please move this code into PostmanEchoClient and then we'll take care of refactoring the status quo?
There was a problem hiding this comment.
I didn't want to move focus from decoding in this PR. Like mentioned in "next steps" section I'd prefer to restructure it in a separate PR if you don't mind.
| @Throws(IOException::class) | ||
| public fun decodeRequest(request: Request, body: ByteString): String? |
There was a problem hiding this comment.
nit: This is more an API design question.
What we're modelling here is an operation that could have 3 outcomes: success (String), failure (IOException), skipped (null).
Have you considered different approaches (e.g. sealed classes or the Kotlin Result type)? I'm not saying that the approach here is wrong, but I was wondering if we have some benefit in modelling it differently.
There was a problem hiding this comment.
Agree with this nitpick that it might be a good idea to model the result to leave less ambiguity.
For example, we could have something like Failure instead of null in case of unsupported content.
There was a problem hiding this comment.
Yes, I considered it and I don't think it is the right choice. I don't see how it helps us in the processing pipeline. We are interested only in the first successfully decoded body.
IOException does not model a type of respons. Methods are annotated with it because tools like adapters generally can throw this type of exception so it removes form users a burden of needing to deal with it. And in Kotlin it is easy to forget since exceptions are not checked. Also, it allows us to log exceptions for the user so they have feedback information when something goes wrong. I would be either for keeping this part as is or to remove @Throws and move the responsibility to the users.
Returning null instead of some type is a nod towards libraries like Retrofit or Moshi, where factories return it when they can't handle input. I don't mind modelling it with a sealed class but I don't know if it brings anything to the table. If you feel that it would be helpful I'll change it.
There was a problem hiding this comment.
Thanks for your insights 👍 I think we're fine with the current modelling for the reasons you mentioned + is the one that has the smaller API surface
There was a problem hiding this comment.
This thread started with nit annotation, so let's leave it as it is :)
| public interface BodyDecoder { | ||
| /** | ||
| * Returns a text representation of [body] that will be displayed in Chucker UI transaction, | ||
| * or `null` if [request] cannot be handled by this decoder. [Body][body] is no longer than |
There was a problem hiding this comment.
Looks like a typo with 2 body
There was a problem hiding this comment.
I believe is actually valid KDoc where you can specify the display text for a link 🤔
There was a problem hiding this comment.
Yes, it is deliberate. It allows to link to body property and start the sentence with a capital letter.
| @Throws(IOException::class) | ||
| public fun decodeRequest(request: Request, body: ByteString): String? |
There was a problem hiding this comment.
Agree with this nitpick that it might be a good idea to model the result to leave less ambiguity.
For example, we could have something like Failure instead of null in case of unsupported content.
|
|
||
| /** | ||
| * Returns a text representation of [body] that will be displayed in Chucker UI transaction, | ||
| * or `null` if [response] cannot be handled by this decoder. [Body][body] is no longer than |
There was a problem hiding this comment.
Same, not a typo. :)
📷 Screenshots
Protobuf encoded request available in preview:
📄 Context
There is an issue – #523.
📝 Changes
I added
BodyEncoderinterface which instances can be installed inChukcerInterceptor. They are then applied when bodies are being processed. First result that is non–nullStringis used as a body in transaction.Exporting functionality was modified to accommodate for changes and export custom payloads as well.
I also edited sample to contain some custom processing. Unfortunately I did only for requests because I cannot find any public service that will echo proto data.
🚫 Breaking
No
🛠️ How to test
Run the sample, check postman echo request, check exporting.
⏱️ Next steps
ChuckerInterceptorTestneeds some splitting into smaller units and/or moving test to respective processors. Also,HttpBinClientneeds some clean-up because as mentioned some time ago it is now doing more than communicating with httpbin.Closes #523.