Low level interceptor with shared context

[mikymigs]

Hello,

I'm trying to migrate to Unirest 4 and replace my previous interceptor logic based on Apache Http Client (leveraging HttpRequestInterceptor and HttpResponseInterceptor). I have followed your suggestion to use the Unirest interceptor implementation. While I was able to succeed, I had to go around important drawbacks that I would like to address here and see if there can be any improvement made to the lib:

• The Apache interceptors provide an HttpContext instance in the interceptor methods. This allows to pass some context around between the request and the response interceptors. In my case I needed to pass a unique identifier that is critical for logging. Unfortunately, I couldn't find an easy way to do this with the current Unirest implementation. My solution was a bit "hacky" as I had to add a header to the request and parse the request summary with a regex in the response interceptor. It's probably bad for performances not to count that a useless header is sent to the destination of the call.

• The body provided in the response interceptor is already deserialized into a java object (by the object mapper) from the raw json response. In my case I want to log the body so I have to convert the object back to a string while taking multiple precautions about the object instance type. It would be much simpler if I could simply log the original raw response. I noticed an attribute "rawBody" but it's null, not sure if this is a bug or not but I could have probably used it if it was working. I had a similar issue with the request where the body gets parsed into a "UnibodyString" that I need to dive into to find my original json body as a string. In this case there is no "rawBody" attribute. All in all I just need a "low level" interceptor to see what is going out and what is sent back as response with as minimal processing / interfering as possible in the middle.

I hope you can consider these issues and address them in a future version of the lib. If you have better suggestions / workarounds, I'm also willing to listen :)

Thanks a lot,

[ryber]

On a shared context:

I think adding the headers explicitly on the RequestSummary is reasonable. Would that fix this for you? Then you don't have a to parse them from the toString(). a explicit context for a request would be a lot more complicated and change a lot of signatures.

On the body

The body was originally a InputStream, and as such, can only be read once. It also may not even BE a string (binary data like Files, or Protobufs). If you want to deal with the lowest level you would need to end with asString() and then map the body later to JSON later. I have thought about adding a setting that would let all responses exist as both the mapped response and as a copy of the InputStream or as a String, however this is very dangerous and if some people turned it on would easily result in OOM errors as the response is too big.

[mikymigs]

Thanks for the quick reply,

Having the headers on the RequestSummary seems like a good compromise. I would still send a useless header but at least I don't have to do the regex parsing. I do understand the constraint of having to modify the signatures not a problem. Although I wonder if it would be possible to add a context map similar to the Apache HttpContext to the RequestSummary. That way you don't expose the headers of the request in the response interceptor and the context map remains separated from the headers (in the end it does serve a different purpose). The only downside I see with this solution is that it seems that the RequestSummary has been implemented to be rather "read-only" and it might be a bit awkward to make it updatable from the request interceptor.

Regarding the body, yes I've had my fair share of issues as well on that topic with the different types of interceptors of the Java world. Usually the body can only be read once but sometimes the InputStream is "markable" (markSupported) and you can reset the read marker so that it can be read twice. Other times that was not possible but with some tricks I was able to create a new response and "re" set it after reading the original one. I understand this limitation, it is probably set to avoid impacting performances but in our case we only use this for debug purposes... Regarding the body being too big, or being binary content I would check the content type and set a maximum configurable body size after which I truncate the content. Having a default limit prevents unaware users of the code from reaching memory limits and having configuration allows to be flexible and think well about the adequate number. Finally, if the content type is not text, I would just print "[NON TEXT FORMAT]" in the logs. To wrap things up, I would very much appreciate if at minimum you could make a copy of the InputStream somehow safely available, although I would dream of a lib that can handle all the issues mentioned above ;-) There's of course no pressure on the matter, since I do have a workaround at the moment. Let's just say it's the raw expression of a need, food for thoughts that could lead to a useful feature in the future.

[ryber]

headers are exposed on the Summary.

[mikymigs]

Thank you for the quick implementation.

Unfortunately, it does not seem to work as expected. I do get the original request headers but not the one I have added manually. If I print httpResponse.getRequestSummary().asString() my header is visible in the generated string but if I call httpResponse.getRequestSummary().getHeaders(), my header is not part of the result collection.

[ryber]

Where did you add the header? I have a test here https://github.com/Kong/unirest-java/blob/24dff3f4459cc033dd5d4262c1a86acc0a84c275/unirest-bdd-tests/src/test/java/BehaviorTests/HeaderTest.java#L408-L423

how is what you are doing different from that?

[mikymigs]

In the onRequest function of the interceptor, I add a header, for example: httpRequest.getHeaders().add(key, value). Then in the onResponse function of the interceptor, I fetch the header: httpResponse.getRequestSummary().getHeaders()

I guess the difference with your test is that you don't add the headers in the context of an interceptor. In my case, I also get the headers that were added prior to the interceptor.

[ryber]

I have a fix but Maven Central is having issues again, I'll try later

[mikymigs]

No worries, thanks a lot for the fix.

[ryber]

It finally went though with a few other things. see 4.1.0

[mikymigs]

Thank you, the fix is working, I can now get the header!