mixpanel / mixpanel-python

Official Mixpanel Python library.
https://mixpanel.com/help/reference/python
Other
103 stars 85 forks source link

cert_reqs unicode is not well handled by urllib3 in python2 #94

Closed hugoArregui closed 3 years ago

hugoArregui commented 3 years ago

Hello,

I understand that urllib3 doesn't support python2 anymore, so maybe this is something you are not willing to fix, but I though about reporting it since we are still in the process of migrating to python3.

This is how mixpanel creates the connection pool:

        cert_reqs = 'CERT_REQUIRED' if verify_cert else 'CERT_NONE'
        self._http = urllib3.PoolManager(
            retries=retry_config,
            timeout=urllib3.Timeout(request_timeout),
            cert_reqs=cert_reqs,
        )

but since we are importing for future:

from __future__ import absolute_import, unicode_literals

both in python2 and python3 cert_reqs is a unicode. But please also notice that for python2 (unlike python3) unicode is not an instance of str, so in urllib3:


def resolve_cert_reqs(candidate):
    """
    Resolves the argument to a numeric constant, which can be passed to
    the wrap_socket function/method from the ssl module.
    Defaults to :data:`ssl.CERT_REQUIRED`.
    If given a string it is assumed to be the name of the constant in the
    :mod:`ssl` module or its abbreviation.
    (So you can specify `REQUIRED` instead of `CERT_REQUIRED`.
    If it's neither `None` nor a string we assume it is already the numeric
    constant which can directly be passed to wrap_socket.
    """
    if candidate is None:
        return CERT_REQUIRED

    if isinstance(candidate, str):
        res = getattr(ssl, candidate, None)
        if res is None:
            res = getattr(ssl, "CERT_" + candidate)
        return res

    return candidate

candidate will not be correctly resolved.

seizethedave commented 3 years ago

Still supporting Py2, so will fix. Might happen faster with a PR, if you are interested.

hugoArregui commented 3 years ago

@seizethedave thanks a lot! I sent a PR, let me know if you want me to bump the version in the PR as well

seizethedave commented 3 years ago

Excellent, I will take care of the version stuff and try to get out a release today. Thanks for doing the PR!

seizethedave commented 3 years ago

4.8.2 is out with this change. @hugoArregui

hugoArregui commented 3 years ago

Thanks a lot @seizethedave