-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Custom body decoding #555
Custom body decoding #555
Conversation
Lint decided to fail with versions updates for some reason on this PR. π€·ββοΈ |
250dd1b
to
32b2bcf
Compare
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.
Great work @MiSikora π Really excited to see this coming!
The code looks good on my end, I left mostly minor comments and nits.
library/src/main/java/com/chuckerteam/chucker/internal/support/RequestProcessor.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/ResponseProcessor.kt
Outdated
Show resolved
Hide resolved
} else if (bodyString.isBlank()) { | ||
if (!isBodyPlainText) { |
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.
Can you merge this into a single condition?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that all the *Decoder
+ all the related tests should be moved to a ChuckerInterceptor_withDecoderTest
sample/src/main/java/com/chuckerteam/chucker/sample/HttpBinClient.kt
Outdated
Show resolved
Hide resolved
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Yup let's follow up on this
@Throws(IOException::class) | ||
public fun decodeRequest(request: Request, body: ByteString): String? |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a typo with 2 body
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 believe is actually valid KDoc where you can specify the display text for a link π€
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same typo with 2 body
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.
Same, not a typo. :)
π· Screenshots
Protobuf encoded request available in preview:
π Context
There is an issue β #523.
π Changes
I added
BodyEncoder
interface which instances can be installed inChukcerInterceptor
. They are then applied when bodies are being processed. First result that is nonβnullString
is 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
ChuckerInterceptorTest
needs some splitting into smaller units and/or moving test to respective processors. Also,HttpBinClient
needs some clean-up because as mentioned some time ago it is now doing more than communicating with httpbin.Closes #523.