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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved handling of encoded/binary bodies #523

Closed
1 of 2 tasks
mattprecious opened this issue Dec 17, 2020 · 6 comments 路 Fixed by #555
Closed
1 of 2 tasks

Improved handling of encoded/binary bodies #523

mattprecious opened this issue Dec 17, 2020 · 6 comments 路 Fixed by #555
Assignees
Labels
feature request Request a new feature to be developed
Milestone

Comments

@mattprecious
Copy link

馃挕 Describe the solution you'd like

It would be great if we could optionally provide a custom decoder to handle bodies that aren't plain text. We use protocol buffers, so I would like to be able to print something useful inside Chucker instead of (encoded or binary body omitted).

馃搳 Describe alternatives you've considered

None

馃搫 Additional context

An interface with a function that takes the Request/Response and returns a string would probably be enough.

Something like this, but with better naming:

interface BodyDecoder { // Converter?
  fun decodeRequest(request: Request): String?
  fun decodeResponse(response: Response): String?
}

Or maybe you do converter factories like retrofit 馃し

馃檵 Do you want to develop this feature yourself?

  • Yes
  • No
@cortinico cortinico added the feature request Request a new feature to be developed label Dec 18, 2020
@cortinico
Copy link
Member

That's definitely and valuable extension point for Chucker 4.x

@MiSikora
Copy link
Contributor

MiSikora commented Dec 18, 2020

I was thinking about it for some time as well. The thing I haven't figured out is how we could get correct type for a decoder.

@mattprecious How would getting a concrete adapter look like for this interface? Retrofit relies on concrete types and reflection. We, unfortunately, don't have this information.

Also most likely we wouldn't expose whole Request and Response. Instead it could be some metadata and ByteString that has raw bytes as we use side streaming while the consumer pulls the bytes to their application.

@mattprecious
Copy link
Author

mattprecious commented Dec 18, 2020

Yeah, decoding with proper names and type information wouldn't be straightforward. However, it is possible to decode a proto without that information. My ideas for the steps I would take to incorporate this into our app:

  1. Print out the bytes in utf8 hex. This string can be pasted into a tool (like, self-plug: protogram) to decode into something somewhat useful. You don't get class/field/enum names, but it's still very useful information and you can manually map tag/value numbers back to useful names by opening the proto definition yourself if needed.
  2. Incorporate protogram into our debug app to save you the step of copying and pasting.
  3. If this was being heavily used and could justify the investment, we could use reflection or code-gen something based on our retrofit interface(s) and use the path to know which adapters to use. That's a lot of work, but it sounds plausible.

If we only receive the ByteString then we lose valuable information for decoding like the path, content type, response code, etc.

@MiSikora
Copy link
Contributor

MiSikora commented Dec 18, 2020

Ok, thanks for the feedback. Will think about it.

And maybe I didn't make myself clear but I wouldn't expose only ByteString. We just can't expose Response and Request because the bodies would be already consumed at the time of processing them. It would have to be some other type that has all available bytes already pulled plus metadata like response codes etc. Unless you didn't mean OkHttp types and was just speaking generally in which case I'm sorry for the confusion. :)

@MiSikora MiSikora self-assigned this Dec 18, 2020
This was referenced Jan 29, 2021
@ghost ghost added the Pending PR The resolution for the issue is in PR label Feb 10, 2021
@ghost ghost removed the Pending PR The resolution for the issue is in PR label Feb 14, 2021
@cortinico cortinico added this to the 4.0.0 milestone Feb 14, 2021
@nienienienie
Copy link

@cortinico so was it added as "extension point for Chucker 4.x"?

@cortinico
Copy link
Member

Yup this is documented here https://github.com/ChuckerTeam/chucker#decode-body-

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request a new feature to be developed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants