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

WSGIHeaderDict fails to get item if it's present and None in original dict #1452

Open
Andrey-mp opened this issue Sep 9, 2024 · 5 comments

Comments

@Andrey-mp
Copy link

Andrey-mp commented Sep 9, 2024

get key with not None value

>>> import bottle
>>> x = bottle.WSGIHeaderDict({"HTTP_X": "y", "HTTP_Z": None})
>>> print(x.get("x"))
y

get absent key

>>> print(x.get("w"))
None

get key with None value

>>> print(x.get("z"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.6/_collections_abc.py", line 660, in get
    return self[key]
  File "/root/work/build/debug/config/vnc_openstack/.tox/py36/lib/python3.6/site-packages/bottle.py", line 2331, in __getitem__
    val = val.decode('utf8')
AttributeError: 'NoneType' object has no attribute 'decode'
Andrey-mp added a commit to Andrey-mp/bottle that referenced this issue Sep 9, 2024
if key has None value then .get(key) throws an Exception

fixes bottlepy#1452
@defnull
Copy link
Member

defnull commented Sep 10, 2024

Header values in WSGI environment dicts should never be None as far as I know. Empty headers are represented by an empty string and missing headers should not be in the environ dictionary at all. How did you end up with that situation? Maybe your WSGI server implementation or a middleware is buggy?

@Andrey-mp
Copy link
Author

I'm also suspcting that WSGI headers can't have None values when they come to this code.
But here we have a sort of "Functional Unit Tests" which are not so simple and fails exactly on this issue. I've spent some time to dig into that legacy code and fails to find exact way how particular header is coming with None value.

@defnull
Copy link
Member

defnull commented Sep 10, 2024

Hm. Problem is, returning None (as in #1453) would just move the bug into application code, because applications would expect string values. Throwing KeyError is also bad because the key is defined and "Z" in request.headers returns True. The only sensible way would be to return an empty string, but wouldn't that be a lie? The client did not send this (empty) header, or it would have a valid value.

I think failing is the correct way to handle this, so you notice bugs in a WSGI server or middleware and have a chance to fix it. If you need a workaround, you could wrap bottle into a WSGI middleware that fixes those headers, or install a before_request hook that directly manipulates request.environ (bypassing WSGIHeaderDict) .

@Andrey-mp
Copy link
Author

In general (and it was a reason why I found it) - this breaks previous behaviour. With version<0.13 those UT work well. If I try same code with old version then I get None value for 'z' key.
Also I think that it's not good to have fail in the code which is caused by external data. I would prefer to check anything input data and raise exception consciouslyю

In terms of http headers - user may pass just a key without a value and it means that value is empty string. So it's also reasonable soluction.

tf-gerrit pushed a commit to OpenSDN-io/tf-controller that referenced this issue Sep 11, 2024
new version has issue in get method
bottlepy/bottle#1452

Change-Id: I84ae81757a09c4b204afa26e09ea7df1b0415aa4
@defnull
Copy link
Member

defnull commented Sep 11, 2024

this breaks previous behaviour.

Yes and no. Yes the behavior changed, but this is not a 'breaking change' because Bottle expects a WSGI compliant environment dictionary and if that expectation is not met, you enter 'undefined behavior' territory. Changing undefined behavior or fixing buggy behavior can happen anytime, even during patch releases. Relying on undefined behavior is actually a bug in itself and your tests revealed that bug. You can and should fix that on your side. If the bug only exists in your test framework, that's even better. No need to fix your actual application and push a new release, just fix the tests.

This reminds me of https://xkcd.com/1172/. The old behavior was wrong, because returning None breaks the "all headers are strings" contract. Throwing KeyError breaks the dict API contract. Returning an empty string would lie about the presence of a header value where there is none. Crashing is actually the correct behavior in this situation.

tf-gerrit pushed a commit to OpenSDN-io/tf-controller that referenced this issue Oct 5, 2024
new version has issue in get method
bottlepy/bottle#1452

Change-Id: I84ae81757a09c4b204afa26e09ea7df1b0415aa4
(cherry picked from commit 23c6e6c)
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

Successfully merging a pull request may close this issue.

2 participants