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

Encoding None Values #118

Closed
karosc opened this issue May 3, 2024 · 6 comments · Fixed by #119
Closed

Encoding None Values #118

karosc opened this issue May 3, 2024 · 6 comments · Fixed by #119

Comments

@karosc
Copy link

karosc commented May 3, 2024

I was testing niquests to see if it is a drop-in replacement for requests for my project and found an issue. My project sometimes ends up posting data that has None values. Requests does not have an issue with this, but niquests throws a TypeError: 'NoneType' object is not iterable error.

The difference comes in how requests and niquests have their `_encode_params method. See the requests implementation and the niquests implementation

Expected Result

Expect the post complete when posting data with None values.

Actual Result

niquests throws a TypeError: 'NoneType' object is not iterable error.

Reproduction Steps

import niquests
import requests

url = "https://httpbin.org/post"
data = {'test_none': None}

# requests completes
resp = requests.post(url, data = data)
print(resp.json())

resp = niquests.post(url, data = data)
print(resp.json())

System Information

$ python -m niquests.help
{
  "charset_normalizer": {
    "version": "3.3.2"
  },
  "http1": {
    "h11": "0.14.0"
  },
  "http2": {
    "jh2": "5.0.3"
  },
  "http3": {
    "enabled": true,
    "qh3": "1.0.4"
  },
  "idna": {
    "version": "3.6"
  },
  "implementation": {
    "name": "CPython",
    "version": "3.8.18"
  },
  "niquests": {
    "version": "3.6.2"
  },
  "ocsp": {
    "enabled": true
  },
  "platform": {
    "release": "6.5.0-28-generic",
    "system": "Linux"
  },
  "system_ssl": {
    "version": "300000d0"
  },
  "urllib3.future": {
    "cohabitation_version": null,
    "version": "2.7.906"
  },
  "wassima": {
    "certifi_fallback": false,
    "enabled": true,
    "version": "1.1.1"
  }
}
@Ousret
Copy link
Member

Ousret commented May 3, 2024

Yes. It appears to be a bug.
If you are willing to, I'd accept a PR for it.

  if iterable_vs is not None:
      for v in iterable_vs:
          if v is not None:
              result.append(
                  (
                      k.encode("utf-8") if isinstance(k, str) else k,
                      v.encode("utf-8") if isinstance(v, str) else v,
                  )
              )

Adding if iterable_vs is not None: seems to fix this.

regards,

@Ousret Ousret added the bug Something isn't working label May 3, 2024
@karosc
Copy link
Author

karosc commented May 3, 2024

I'd opt to integrate the more general solution that is in requests, which just throws the object into a list if it's not iterable.

   if isinstance(vs, (str, bytes, int, float, bool))  or not hasattr(vs, "__iter__"):
       # not officially supported, but some people maybe passing ints, float or bool.
       if isinstance(vs, (str, bytes)) is False:
           iterable_vs = [str(vs)]
       else:
           iterable_vs = [vs]
   else:
       iterable_vs = vs
   for v in iterable_vs:
       if v is not None:
           result.append(
               (
                   k.encode("utf-8") if isinstance(k, str) else k,
                   v.encode("utf-8") if isinstance(v, str) else v,
               )
           )  )

or we could do this to avoid another if statement:

   if isinstance(vs, (str, bytes, int, float, bool, type(None)):
       # not officially supported, but some people maybe passing ints, float or bool.
       if isinstance(vs, (str, bytes)) is False:
           iterable_vs = [str(vs)]
       else:
           iterable_vs = [vs]
   else:
       iterable_vs = vs
   for v in iterable_vs:
       if v is not None:
           result.append(
               (
                   k.encode("utf-8") if isinstance(k, str) else k,
                   v.encode("utf-8") if isinstance(v, str) else v,
               )
           )  )

@Ousret
Copy link
Member

Ousret commented May 3, 2024

I'd opt to integrate the more general solution that is in requests

the thing is[...] requests isn't typed and allowed things that made mypy unhappy (as far as I remember).

I am open to suggestions as long as:

  • mypy is okay with it
  • the presented case is fixed
  • no regression is introduced (this one is hard to foresee as the test suite is weak on that particular spot...)

regards,

@karosc
Copy link
Author

karosc commented May 3, 2024

Thanks @Ousret, I have a fix with mypy passing. I can also implement some unit tests for this bit too. Are you interested in setting mypy to run in CI? Or are the pre-commit hooks sufficient for you?

@Ousret
Copy link
Member

Ousret commented May 4, 2024

Or are the pre-commit hooks sufficient for you?

The pre-commit includes mypy. So yes, it is sufficient.

Ousret added a commit that referenced this issue May 6, 2024
This addresses #118, inspired by the code in
[requests](https://github.com/psf/requests/blob/2d5f54779ad174035c5437b3b3c1146b0eaf60fe/src/requests/models.py#L122).

Essentially this PR will allow encoding any non-iterable object as
string. This is a general solution to the specific problem in #118.
Since `None` is not iterable, it is encoded "None". Similarly, any
non-iterable object will be encoded as `str(obj)`.

---------

Co-authored-by: TAHRI Ahmed R <Ousret@users.noreply.github.com>
@Ousret
Copy link
Member

Ousret commented May 6, 2024

It is live.

pip install niquests -U

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