cpstats: make 'Response Status' serializable

[sanitec]

Fixes an issue where it wasn't possible to fetch /cpstats/data when
running the cpstats tool with Python3.

Could potentially also solve the problem by supplying a default
function for 'json.dumps', but this solution seems cleaner.

[codecov]

Codecov Report

Merging #1715 into master will decrease coverage by 1.86%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master    #1715      +/-   ##
==========================================
- Coverage   80.81%   78.94%   -1.87%     
==========================================
  Files         105      104       -1     
  Lines       13566    13552      -14     
==========================================
- Hits        10963    10699     -264     
- Misses       2603     2853     +250

[webknjaz]

@sanitec would you mind rebasing and adding a test for this?

[sanitec]

Sure, where would it be natural to put such a test, and what should it test? I couldn't find any testcases relevant for cpstats.

[webknjaz]

Since it's a tool, I'd go for https://github.com/cherrypy/cherrypy/blob/master/cherrypy/test/test_tools.py

Honestly, we're trying to get rid of unittest in favor or pytest, but still stuck with it. If you want, you can try to avoid inheriting unittest.TestCase, but it's not mandatory for now.

[jaraco]

I agree this change is the right one - rather than to have a workaround at the encoding layer. I think the solution could be more precise about what's expected/allowed for the inputs/outputs.

I know a lot of cherrypy uses tonative and other functions in the compat module, but those functions encourage a continued bimorphism for strings (bytes on Python 2 and unicode on Python 3). Instead, the code should move toward a unified approach (always unicode or bytes regardless of Python). Let's honor that intention in this change and not perpetuate the status quo. Thanks.

[jaraco]

Another thing - could you provide an example server/script that someone like myself could use to replicate the issue?

[webknjaz]

Let's honor that intention in this change and not perpetuate the status quo.

I totally agree with this, but the problem is that from __future__ import unicode_strings would require other changes in the module and I would like to keep a single logical change per PR principle here. Such changes should go to a separate refactoring PR.

[sanitec]

@jaraco to replicate:

Run (in a python3 environment):

import cherrypy
from cherrypy.lib import cpstats


if __name__ == '__main__':
    cherrypy.quickstart(
        cpstats.StatsPage(),
        config={
            '/': {'tools.cpstats.on': True}
        })

And then:
curl http://localhost:8080/data

[sanitec]

Hmm, seems like it doesn't fix the problem entirely. When running that example with the fix I get:

Traceback (most recent call last):
  File "/Users/sfosteru/src/cherrypy/cherrypy/_cprequest.py", line 627, in respond
    self._do_respond(path_info)
  File "/Users/sfosteru/src/cherrypy/cherrypy/_cprequest.py", line 686, in _do_respond
    response.body = self.handler()
  File "/Users/sfosteru/src/cherrypy/cherrypy/_cprequest.py", line 803, in __set__
    raise ValueError(self.unicode_err)
ValueError: Page handlers MUST return bytes. Use tools.encode if you wish to return unicode.

[webknjaz]

@sanitec could you please also add a test for this case under cherrypy/test/?

[sanitec]

@webknjaz I'm not really sure where to go from here.

[jaraco]

the problem is that from __future__ import unicode_strings would require other changes in the module

I didn't think I was suggesting converting unicode literals for the module - only always returning unicode for that one function.

[webknjaz]

Oh, makes sense then.