search

crossbario / autobahn-python

WebSocket and WAMP in Python for Twisted and asyncio
https://crossbar.io/autobahn
MIT License
2.47k stars 763 forks source link

Formatting bug in new release of Autobahn #1566

Closed itamarst closed 2 years ago

itamarst commented 2 years ago

From Tahoe-LAFS test suite:

Traceback (most recent call last):
  File "/home/itamarst/Devel/tahoe-lafs/venv/lib64/python3.7/site-packages/twisted/web/server.py", line 227, in process
    self.render(resrc)
  File "/home/itamarst/Devel/tahoe-lafs/venv/lib64/python3.7/site-packages/twisted/web/server.py", line 292, in render
    body = resrc.render(self)
  File "/home/itamarst/Devel/tahoe-lafs/venv/lib64/python3.7/site-packages/autobahn/twisted/resource.py", line 175, in render
    protocol.dataReceived(data)
  File "/home/itamarst/Devel/tahoe-lafs/venv/lib64/python3.7/site-packages/autobahn/twisted/websocket.py", line 348, in dataReceived
    self._dataReceived(data)
  File "/home/itamarst/Devel/tahoe-lafs/venv/lib64/python3.7/site-packages/autobahn/websocket/protocol.py", line 1243, in _dataReceived
    self.consumeData()
  File "/home/itamarst/Devel/tahoe-lafs/venv/lib64/python3.7/site-packages/autobahn/websocket/protocol.py", line 1272, in consumeData
    self.processHandshake()
  File "/home/itamarst/Devel/tahoe-lafs/venv/lib64/python3.7/site-packages/autobahn/websocket/protocol.py", line 2740, in processHandshake
    self.sendServerStatus()
  File "/home/itamarst/Devel/tahoe-lafs/venv/lib64/python3.7/site-packages/autobahn/websocket/protocol.py", line 3182, in sendServerStatus
    self.sendHtml(_SERVER_STATUS_TEMPLATE.format(redirect, __version__))
builtins.KeyError: '\n            color'

If you look at _SERVER_STATUS_TEMPLATE, you will see that it is both:

  1. Using CSS styles with { }
  2. Using {} with Python's .format() method.

This combination doesn't work. The solution, I believe, is that CSS curly brackets should be quoted so .format() doesn't break it, e.g.

         a, a:visited, a:hover {{
            color: #fff;
         }}
oberstet commented 2 years ago

thanks for letting us know! fwiw, the regression was introduced in v22.4. here https://github.com/crossbario/autobahn-python/pull/1547

before it was using plain old %s based escaping

https://github.com/crossbario/autobahn-python/blob/5890ca3cfcd1ee3dbe830add0e2a73ad82043ae9/autobahn/websocket/protocol.py#L2476 https://github.com/crossbario/autobahn-python/blob/5890ca3cfcd1ee3dbe830add0e2a73ad82043ae9/autobahn/websocket/protocol.py#L3147

however, now it should escape the curly braces as you hint ..

oberstet commented 2 years ago

on a side note, even though we do run flake8 as part of CI, we don't run mypy (not for autobahn, but we run it for crossbar). however, while mypy catches a lot more than flake8, it wouldn't caught this one:

cpy39_1) (base) oberstet@intel-nuci7:~/scm/crossbario/autobahn-python$ mypy autobahn/websocket/protocol.py
autobahn/websocket/protocol.py:61: error: Skipping analyzing "txaio": module is installed, but missing library stubs or py.typed marker
autobahn/websocket/protocol.py:61: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
autobahn/websocket/protocol.py:2564: error: "WebSocketServerProtocol" has no attribute "log"
Found 2 errors in 1 file (checked 1 source file)

those 2 reported errors are bogus, this is all good ..

itamarst commented 2 years ago

I bet pylint would catch it, you could setup pylint with approved list of lints starting with this one.

oberstet commented 2 years ago

if I restarted again, one of the first things would be add CI for flake8, mypy and pylint, so the code grows 100% compliant from the beginning. well. out of curiosity, I tried pinning down the problem using pylint, but it doesn't identify the issue. pylint has a looot to complain rgd code style (and fwiw, sure, I agree .. mostly), but the only error it identifies is bogus:

(cpy39_1) (base) oberstet@intel-nuci7:~/scm/crossbario/autobahn-python$ pylint -d C0103,C0301,E1101,W0201,C0411,R1702,R1705,C0123,R0915,C0325,R0913,C0209,W0212,R0912,R0902,R0914,R0911,R0902,W0613,R1710,W0703,R0903,R0205,C0116,R0201,R1714,W0702,R1711,W0702,W0612,R1711,R0904,R1703,W0511,C0302,C0114,C0115,W0125   autobahn/websocket/protocol.py
************* Module autobahn.websocket.protocol
autobahn/websocket/protocol.py:990:48: E0203: Access to member 'trackTimings' before its definition line 991 (access-member-before-definition)

------------------------------------------------------------------
Your code has been rated at 9.97/10 (previous run: 9.97/10, +0.00)

(cpy39_1) (base) oberstet@intel-nuci7:~/scm/crossbario/autobahn-python$ pylint -E -d E1101 autobahn/websocket/protocol.py
************* Module autobahn.websocket.protocol
autobahn/websocket/protocol.py:990:48: E0203: Access to member 'trackTimings' before its definition line 991 (access-member-before-definition)
(cpy39_1) (base) oberstet@intel-nuci7:~/scm/crossbario/autobahn-python$

pylint doesn't understand the attribute access guard:

https://github.com/crossbario/autobahn-python/blob/2e2ee5f9775ed312db699f5c55fc0488311735a5/autobahn/websocket/protocol.py#L990

oberstet commented 2 years ago

fixed via https://github.com/crossbario/autobahn-python/pull/1567