Should None values in data be handled differently?

[johtso]

Currently, if data contains None values they are passed on to urllib.urlencode as normal. The behaviour of urllib.urlencode is to always call str() on the key/value.

This results in the following:

urllib.urlencode({'key': None})
# 'key=None'

To me this seems like odd behaviour. If anything it would make sense for it to result in 'key='.

One option would be to do something like v = v or '' before encoding the data.

Alternatively exceptions could be raised if a key or value is None.

Does this sound like something that should be changed?

[piotr-dobrogost]

Good idea but we should check for None instead of v = v or '' to support falsy values like {'key': 0}.

[piotr-dobrogost]

@johtso Strange thing. I got notification about your post which begins with Ah yes, of course, forgot about 0. What about False? if (v is None) or (v is False): v = but I don't see your post from notification...

[johtso]

Ah, I deleted it while reconsidering the problem :) Sorry for the confusion..

Translating None to '' seems to make sense, but False doesn't really have any obvious translation..

[johtso]

@kennethreitz, what do you think? :)

[kennethreitz]

This should not be happening ;)

[piotr-dobrogost]

Maybe keys with None value should not be present in the request at all?
As to True/False I think mapping them to 1/0 sounds reasonable.
This functionality could (should?) be implemented as a hook.

[johtso]

Looks like I've misunderstood things a bit.

'headers', 'cookies', 'auth', 'timeout', 'proxies', 'hooks', 'params', 'config' all have keys with None values removed from them.

data is the only kwarg that doesn't have keys with None values removed from it, due to no merging going on. Hence seeing urllib.urlencode translating them to key=None.

So the only question is, do we want to remove keys with values of None from data as we do elsewhere, leave the current behaviour (str(None)), or replace None with ''.

[kennethreitz]

The only purpose of removal upon None is because of kwarg merging when sessions are involved. This only matters for params.

I'd like to not make any changes to the behavior of urlencode of possible. No surprises that way :)

[johtso]

Yeah, I think that's probably wise. If you want keys without values in your data, convert Nones to '' before passing them to your request.

[piotr-dobrogost]

data is the only kwarg that doesn't have keys with None values removed from it, due to no merging going on.

If this is the case then I think it's wrong. The behavior should be consistent.

[francescomari]

I agree with @piotr-dobrogost, keys with None values should be removed like the other maps.

[piotr-dobrogost]

The only purpose of removal upon None is because of kwarg merging when sessions are involved.

Well, regardless of the reason it's convenient and logical.

This only matters for params.

As this issue shows this matters not only for params. Current behavior of having key=None as a payload of request does not make sense.

[foxx]

I vote that this should behave how every other browser behaves, which is to just send across an empty string. (Just spent an hour debugging this issue with James Cleary).

Cal Leeming
Simplicity Media Ltd

[foxx]

FYI - to anyone else who is having this issue, we hotfix'd it using the following:

    @staticmethod
    def _encode_params(data):
        """Encode parameters in a piece of data.

        If the data supplied is a dictionary, encodes each parameter in it, and
        returns a list of tuples containing the encoded parameters, and a urlencoded
        version of that.

        Otherwise, assumes the data is already encoded appropriately, and
        returns it twice.
        """

        if hasattr(data, '__iter__') and not isinstance(data, str):
            data = dict(data)


        if hasattr(data, 'items'):
            result = []
            for k, vs in list(data.items()):
                for v in isinstance(vs, list) and vs or [vs]:
                    # Hotfix to resolve https://github.com/kennethreitz/requests/issues/378
                    if v == None:
                        result.append((k.encode('utf-8') if isinstance(k, str) else k,
                                   ""))
                    else:
                        result.append((k.encode('utf-8') if isinstance(k, str) else k,
                                   v.encode('utf-8') if isinstance(v, str) else v))

            return result, urlencode(result, doseq=True)
        else:
            return data, data

[piotr-dobrogost]

@foxx If you want to send empty string as a value of some param you should set its value to empty string. Why should requests send key='' when you pass key=None and not skip this param altogether?
btw, using == when comparing with None is bad style.

[foxx]

Why should requests send key='' when you pass key=None and not skip this param altogether?

Guess my reply is, why SHOULDN'T it? It's a 50/50 split, and really comes down to the decision of the core maintainer I guess.

using == when comparing with None is bad style.

You're right, I probably should have used "is None" - I'll patch our side - thanks.

[piotr-dobrogost]

It's not 50/50 split if you take into consideration some other aspects. In case of params requests already deletes keys with value set to None so for sake of being uniform here it should delete them too.

[Lukasa]

Seven months later, resolved by #805!

[kennethreitz]