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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verbose misses response body #282

Closed
avkonst opened this issue Aug 19, 2020 · 11 comments
Closed

Verbose misses response body #282

avkonst opened this issue Aug 19, 2020 · 11 comments

Comments

@avkonst
Copy link

avkonst commented Aug 19, 2020

I have got the following test spec:

tests:
-   name: auth
    verbose: all
    url: /api/sessions
    method: POST
    data: "asdsad"
    status: 200

which has got data not a proper json on purpose. The response results in 400 instead of 200 code. Verbose is set to all, but it still does not print the response body, although detects non empty content:

... #### auth ####
> POST http://localhost:7000/api/sessions
> user-agent: gabbi/1.40.0 (Python urllib3)

< 400 Bad Request
< content-length: 48
< date: Wed, 19 Aug 2020 06:11:24 GMT

✗ gabbi-runner.input_auth

FAIL: gabbi-runner.input_auth
        Traceback (most recent call last):
          File "/usr/lib/python3/dist-packages/gabbi/case.py", line 94, in wrapper
            func(self)
          File "/usr/lib/python3/dist-packages/gabbi/case.py", line 143, in test_request
            self._run_test()
          File "/usr/lib/python3/dist-packages/gabbi/case.py", line 550, in _run_test
            self._assert_response()
          File "/usr/lib/python3/dist-packages/gabbi/case.py", line 188, in _assert_response
            self._test_status(self.test_data['status'], self.response['status'])
          File "/usr/lib/python3/dist-packages/gabbi/case.py", line 591, in _test_status
            self.assert_in_or_print_output(observed_status, statii)
          File "/usr/lib/python3/dist-packages/gabbi/case.py", line 654, in assert_in_or_print_output
            self.assertIn(expected, iterable)
          File "/usr/lib/python3/dist-packages/testtools/testcase.py", line 417, in assertIn
            self.assertThat(haystack, Contains(needle), message)
          File "/usr/lib/python3/dist-packages/testtools/testcase.py", line 498, in assertThat
            raise mismatch_error
        testtools.matchers._impl.MismatchError: '400' not in ['200']
----------------------------------------------------------------------

The expected behavior:

  • response body is printed to the stdout alongside the response headers.
@cdent
Copy link
Owner

cdent commented Aug 19, 2020

You're using a version of gabbi that is about 2.5 years old. If you're able to upgrade and try the same test that would be helpful in trying to trace where the problem is. I'm pretty sure the version won't make a difference, but I'd like to remove that variable.

Note that if no content-type is provided, or a content-type for which gabbi does not have a ContentHandler , then gabbi will not send a body. You can see that in your paste above because the request body is empty. If you need to be able to send other content-types there are ways to subclass ContentHandler (see: https://gabbi.readthedocs.io/en/latest/handlers.html )

Also could you tell me more about the server you are making your requests against? I've tried against a few different servers (both python and go-based, using both modern gabbi and the version you're running) and I'm unable to replicate the problem you are seeing.

One thing that would be worth trying is replicating the request using curl in verbose mode to show the output gabbi should be showing. That will help to narrow things down. I think the curl would look something like this (including empty body based on no ContentHandler):

curl -v http://localhost:7000/api/sessions -X POST

I hope we can work this out, but I need a bit more info to make any progress.

@avkonst
Copy link
Author

avkonst commented Aug 19, 2020

I can not reproduce it with the latest version.
PS: I was not sure how to install gabbi-run. I have got ubuntu 20.04 which is recent. I run the gabbi-run command and it suggested 'apt install gabbi-run'. So here I got what I got. Would it be better to update the ubuntu/debian repo with the latest version?

@cdent
Copy link
Owner

cdent commented Aug 19, 2020

Would it be better to update the ubuntu/debian repo with the latest version?

That's something that's beyond my control. Which version the various linux distros choose to package is up to them and not something I'm involved with. If you'd like to see them be more up to date, opening a bug with them may help.

The linux distros being out of date with regard to Python packages is a fairly common problem, big enough that people tend to install the Python packages they use regularly via pip and not use the version packaged by the distro.

@avkonst
Copy link
Author

avkonst commented Aug 19, 2020

no worries. thanks

@avkonst avkonst closed this as completed Aug 19, 2020
@avkonst avkonst reopened this Aug 19, 2020
@avkonst
Copy link
Author

avkonst commented Aug 19, 2020

Sorry, I was wrong. I can reproduce the issue with the latest. As far as I can see it does not print the response body if a server does not set Content-type header.

@avkonst
Copy link
Author

avkonst commented Aug 19, 2020

It also prints the following error if response content-type is "application/json" but the response body is just plain text:

< 401 Unauthorized
< content-length: 63
< content-type: application/json
< date: Wed, 19 Aug 2020 23:32:07 GMT
E gabbi-runner.input_auth

ERROR: gabbi-runner.input_auth
        Expecting value: line 1 column 1 (char 0)
----------------------------------------------------------------------
Ran 2 tests in 0.006s

@avkonst
Copy link
Author

avkonst commented Aug 19, 2020

I think it should apply the default content-type if the response does not contain it. It should be UTF-8 string by default, as far as I remember.

@cdent
Copy link
Owner

cdent commented Aug 20, 2020

Thanks, those are good clues as to what's going wrong.

I think you're right: If it can't figure out what to show it should try to dump a string. I'll look into what it will take to do that. Will try to get to it tomorrow, but it may be a few days.

@cdent
Copy link
Owner

cdent commented Aug 20, 2020

I've started experimenting with this, and there's two cases that we need to decide how to get the right behavior:

  1. When a content-type is set on the response, and it it is determined to be "binary" then the verbose routines very intentionally to do print the response body, as it is assumed that it will not be readable and will likely be long. I'd prefer that this remain the case. I am able to make so that if no content-type header is set on the response then the body will be printed. Good enough?
  2. The "Expecting value..." error can happens as part of the response decoding when content-type is application/json but the body is not valid JSON. This needs to remain true. What this means is that if the response body is "foo" and verbose is set to all then the changes being implemented will allow that "foo" to be printed in the verbose output but the "Excepting value" error will still happen, because gabbi will still try to decode the response it thinks is JSON. Okay?

cdent added a commit that referenced this issue Aug 20, 2020
When the response has no content-type and verbose is `true`
gabbi was not displaying the response body. This is because
when no content-type is known, it was assuming a binary
content-type. Now it assumes text/plain.

This comes with the risk that in some rare cases verbosity
might spew data all over the screen if there is a very big
or very binary response when no content-type is set. With
luck this will encourage servers to send a content-type.

An external test, in the form of test-verbosity.sh is added.
This was one of several ways of verifying the output.

Addresses #282
@avkonst
Copy link
Author

avkonst commented Aug 20, 2020

I think about this way (likely similar to yours view):

  • Verbose printing of a body (by request via a config or command line option) and validation of the body content (as per the test spec) are 2 different things.
  • Sure, if the test spec expects json and coming body can not be deserialised as json, than it is a test failure. Keep in mind that many servers respond with json without setting the content type at all - and it is perfectly valid, especially for servers with very narrow bandwidth deployment, where extra headers are removed on purpose.
  • Verbose printing of a body should follow the same principles for requests and responses
  • Verbose printing of binary data can be omitted (better with a message saying it was omitted) or can be printed as hexbytes text (maybe add a configuration option for this to tune the desired behavior)
  • Verbose printing of long responses (binary or text) can / should be truncated (maybe add a configuration option for the max length)
  • Verbose printing should print the body regardless if the content type is set, if it is set correctly, if the body valid or invalid structure or if it corresponds to the content type declaration
  • Verbose printing should not alter the response body, eg. if a json response is not pretty-printed, it is better to keep it as it was in the response, but a config option could change this behaviour to activate pretty printing of json/yml/etc...

This is how I think about it. Pick what you think is useful.

cdent added a commit that referenced this issue Aug 25, 2020
When the response has no content-type and verbose is `true`
gabbi was not displaying the response body. This is because
when no content-type is known, it was assuming a binary
content-type. Now it assumes text/plain.

This comes with the risk that in some rare cases verbosity
might spew data all over the screen if there is a very big
or very binary response when no content-type is set. With
luck this will encourage servers to send a content-type.

An external test, in the form of test-verbosity.sh is added.
This was one of several ways of verifying the output.

Addresses #282
@cdent
Copy link
Owner

cdent commented Aug 25, 2020

Thanks for the comments. For the time being I've gone with a relatively simple fix in #285 which will be released as version 2.0.4 here in a few minutes. As things move along it might make sense to incorporate some of your other ideas.

One thing I don't want to do is automatically decode or detect content-type without a content-type header. Gabbi has always been opinionated about using content-type effectively and encouraging people to write APIs that do so. As gabbi has grown to sometimes be used to test things which the tester has no control over, his has proven a bit more difficult, so it might make sense in the future to change this, but I don't want to make that change without due consideration.

@cdent cdent closed this as completed Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants