fix(#1518) Add support for displaying the response data in the Timeline and improve scripting error handling

[fredjeck]

Closes #1518

Description

Considering the following valid request which contains a scripting error:

meta {
  name: HttpBin
  type: http
  seq: 2
}

post {
  url: https://httpbin.org/post
  body: json
  auth: none
}

body:json {
  {  
      "test": "hello Bruno"  
  }  
}

vars:post-response {
  unknown_field: res.body.arguments.unknown_field
}

Bruno 1.9 and prior is showing the following response and timeline

Whereas the request has been executed successfully the message is not really self explanatory and is hard for the user to understand. In fact a scripting error occured and the user has no way to check to the root cause of the scripting error as the response is nowhere to be seen.

This PR improves error handling during requests execution so that:

• Nothing is changed when a network error occurs while executing a request
• The error message displayed in the response pane is improved when a scripting error is raised during pre, post and testing phases
• A scripting error is not anymore considered as fatal (except for pre-request scripting errors) and the response is being returned to the renderer process
• The full response body is now logged into the timeline
• Errors are also being displayed in the timeline for easing debugging

For example, the request above is now displayed as following in the timeline:

To limit the potentials lags induced by logging large requests payloads in case of errors, three new user preferences have been introduced :

• Display the response in the timeline tab (Yes/No)
• Limit the display to responses which size is lower than the provided threshold (if the user specifies 0 then the setting is ignored and all sizes are considered)
• Limit the display to responses of the corresponding content types, if empty or '*' all the content types will be displayed

Contribution Checklist:

• The pull request only addresses one issue or adds one feature.
• The pull request does not introduce any breaking changes
• I have added screenshots or gifs to help explain the change if applicable.
• I have read the contribution guidelines.
• Create an issue and link to the pull request.

[fredjeck]

For the discussion. This PR solves the root cause by displaying errors and showing the response in the timeline pane however I have the feeling this can be improved.
If the request succeeded I think it would be nice to be able to display the scripting error in the response pane - however this would require deeper change. Therefore I would suggest this PR as a firststep which would avoid user frustration.

[Its-treason]

I don't think showing the complete response here is a good idea, without any limitations. Showing bigger responses will cause some massive lag.

Dismissing the error in the response tab would probably be a better idea.

[fredjeck]

Hi @Its-treason thanks for taking the time to review my PR

One of the main reason of displaying the raw payload here is that the timeline is considered by many as the "Debugging tab", and is used to check what has been sent and received in a raw/unprocessed manner.

Working in a complex environment (i.e. with cascading gateways) I came across several scenarios where the request was in error but bruno actually received content and I was left with only very little information about the error detail - which was burried inside the response and had to revert to generating the curl command line to find out the issue.
For instance invalid Json, misbehaving APIS returning HTML errors instead of JSON errors etc..

From a UX experience, I think (I might be wrong) that the response tab should be used for successful response only as it allows for further response processing (filtering etc.).
The timeline is a good place for showing the raw network exchanges.

Maybe a good tradeoff would be to only show the unprocessed response data in case of error what do you think ?

[thekalinga]

Should responses for certain content types be ignored say images/videos/whatever (allow user to override from some config file?)

Also, allow user to set a limit on the maximum number of bytes that should be processed, if breached, response wont be processed?

May be better options?

[fredjeck]

Having a look at https://www.iana.org/assignments/media-types/media-types.xhtml#application there is no easy way of finding out if the content type is binary or not therefore we would need to support user configurable debuggable payloads (along side to a size limit).
Having a look at this solution

[fredjeck]

@thekalinga I implemented the discussed idea (PR description is updated see screenshot)
Three new settings have been added to the general preferences tab :

• Display the response in the timeline tab (Yes/No)
• Limit the display to responses which size is lower than the provided threshold (if the user specifies 0 then the setting is ignored and all sizes are considered)
• Limit the display to responses of the corresponding content types, if empty or '*' all the content types will be displayed

[thekalinga]

@fredjeck Thank you. I am not a maintainer on this project. Please note that my understanding of this codebase is extremely limited. However, I will share my comments in few days.

In the interim, here are few comments.

For safety reasons, when user specifies nothing, I tend to be more restrictive rather than less restrictive so as to not run out of memory i.e when no size limit is specified, I would want to restrict to a reasonable size but not unlimited. When they specify no content types, we would ideally want to restrict to a specific known types & if user specifies, we can use those.

Also, I see that the complete response is converted to string before being parsed. We might want to verify content type header to get the number of bytes (or) read raw bytes 1st till we reach the threshold before dropping off the buffer on floor if it overflows user specifies size. Currently we seem to read everything into a simple buffer & also assuming it to be interpreted as a string irrespective of the content-type header sent by server.

[fredjeck]

Hi @thekalinga thanks for your time - I'm also not a maintainer just a user which tries to improve a wonderful tool out of some experiences.

While I tend to agree with you regarding settings, I believe that's what defaults are made for.
I think it is better to let the user in control of his environment - placing predefined defaults which can be overridden if required rather than locking down everything and creating frustrations. That's why the new preferences set do have defaults and can be overridden (like in most software) if needed.

When it come to response parsing the network code was not changed. The data buffer is returned by the electron process along with the response size which is then used to take the decision if the content should or not be rendered, therefore the response buffer is not taken into consideration at this point.

When it comes to parsing the whole goal of the PR is to provide debugging support/insight with raw unmodified content to the user when something goes wrong. As already explained as of today in case of error with actual data returned by remote API you will not see anything and you will have to revert to Curl to check the root cause.
Therefore I voluntarily chose to display the raw unparsed response as a string.