SNOW-270639 pyjwt major bump and compatibility

[jameslamb]

pyjwt 2.0.0 was released about a month ago. In that release, the behavior of jwt.encode() changed. It now returns a string instead of bytes, which is what caused the bug reported in #586 .

This PR proposes removing any version requirements on pyjwt in snowflake-connector-python. I think with the minor changes proposed in this PR, it's possible for snowflake-connector-python to work with both pyjwt 1.x and pyjwt 2.x.

Changes in this PR

• makes the one use of jwt.encode() in this project compatible with pyjwt 1.x and 2.x
• removes version constraint on pyjwt

How this improves snowflake-connector-python

This change would make snowflake-connector-python easier to install alongside other packages, because it relaxes a constraint placed on a popular and central package (pyjwt).

I think this could be a small step towards alleviating some of the dependency pain expressed in #284.

Notes for reviewers

I looked for uses of pyjwt in this project with git grep -E "jwt\.". I think that the uses of get_unverified_header() and .decode() will work without modification, with both 1.x and 2.x.

get_unverified_header() was not changed between versions.

2.0.1 (https://github.com/jpadilla/pyjwt/blob/3993ce1d3503b58cf74699a89ba9e5c18ef9b556/jwt/api_jws.py)

def get_unverified_header(self, jwt):
    """Returns back the JWT header parameters as a dict()
    Note: The signature is not verified so the header parameters
    should not be fully trusted until signature verification is complete
    """
    headers = self._load(jwt)[2]
    self._validate_headers(headers)

    return headers

def _validate_headers(self, headers):
    if "kid" in headers:
        self._validate_kid(headers["kid"])

def _validate_kid(self, kid):
    if not isinstance(kid, str):
        raise InvalidTokenError("Key ID header parameter must be a string")

1.7.1 (https://github.com/jpadilla/pyjwt/blob/b65e1ac6dc4d11801f3642eaab34ae6a54162c18/jwt/api_jws.py)

def get_unverified_header(self, jwt):
    """Returns back the JWT header parameters as a dict()
    Note: The signature is not verified so the header parameters
    should not be fully trusted until signature verification is complete
    """
    headers = self._load(jwt)[2]
    self._validate_headers(headers)

    return headers

def _validate_headers(self, headers):
    if 'kid' in headers:
        self._validate_kid(headers['kid'])

def _validate_kid(self, kid):
    if not isinstance(kid, string_types):
        raise InvalidTokenError('Key ID header parameter must be a string')

.decode() in 2.x still supports algorithm RS512

https://github.com/jpadilla/pyjwt/blob/9dca8eaa0ec43bcf54ac645872078ecdb0ca8d96/jwt/algorithms.py#L80

Thanks for your time and consideration.

[sfc-gh-mkeller]

Thank you for making the code work with both the older and the newer version of pyjwt.
Please address my one comment and then I can kick of tests for you 🚀

[sfc-gh-mkeller]

Suggested change

	'pyjwt',
	'pyjwt<3.0.0',

Let's add this, so that we take a look at changelog upon the next major release and verify that the API changes don't affect us before approving it, please!

[jameslamb]

In my opinion, major-version ceilings are a good practice for applications, but should be avoided for libraries unless you know for sure that your library is incompatible with that new version.

Imagine that you merge this PR and release snowflake-connector-python today, and someone installs it. Tomorrow, pyjwt 3.0.0 comes out.

Here's how that could play out, based on whether or not snowflake-connector-python is compatible with pyjwt 3.0.0.

without a ceiling

• incompatible --> need to pin to older pyjwt or open an issue asking for a new snowflake-connector-python release
• compatible --> no need to pin pyjwt, no action required by snowflake

with a ceiling

• incompatible --> need to pin to older pyjwt or open an issue asking for a new snowflake-connector-python release
• compatible --> need to pin to older pyjwt or open an issue asking for a new snowflake-connector-python release

Not putting a ceiling in place gives this project's users a chance of not needing to upgrade snowflake-connector-python if they require pyjwt (which might come from them requiring some other package that requires pyjwt>3.0.0).

[jameslamb]

all that said...if you read that and disagree, I'd be happy to follow your suggestion and add the ceiling

[sfc-gh-mkeller]

I agree with you James and this is something that many customers have been asking for (there are many other issues open with this request already).

Short answer of why this is that historically our library has been treated as an application, so now many of our customers demand that we minimize the possibility of errors, but this is only possible if we control dependency versions.

I wasn't here when this decision was made, so I can't comment on the specifics of why, but if I had to take a guess our library broke way too often. I have definitely seen this myself many times. For example this new pyjwt release had the possibility of breaking customer deployments as you found. Not once did we have to get online over the weekends to pin against certain newly released dependencies and do an unscheduled release of the connector to fix things. Over the years this lead to the state we are in now. I'm trying to make a better effort to keep things up to date, but I'd lying if I said that I'm on top of everything, this is why we introduced dependabot. It kicks off tests every dependency bump before actually moving our pin.

My other view is that if there are people that would like our library with less tightly coupled dependencies, they can really easily build it for themselves. This is the specific reason why I have introduced PEP-517 build and I was hoping that having production grade build openly documented in our GitHub actions would further aid users of this group.

I hope that this insight makes sense to you and that you find it to make at least some sense, that being said please add the next major pin, we should test at least breaking public API changes.

[jameslamb]

This is the specific reason why I have introduced PEP-517 build and I was hoping that having production grade build openly documented in our GitHub actions would further aid users of this group.

Can you link me to the docs on this? I wasn't aware of an alternative build of snowflake-connector-python that has looser constraints on dependencies.

I don't see an alternative wheel / sdist tarball at https://pypi.org/project/snowflake-connector-python/#files or on the latest release. That isn't mentioned in https://docs.snowflake.com/en/user-guide/python-connector-install.html or https://docs.snowflake.com/en/user-guide/python-connector-dependencies.html.

please add the next major pin

Sure, thanks for hearing me out. Added in e8ff9a2

[sfc-gh-mkeller]

I'm sorry, I didn't mean that we release that version. What I meant is that if you want to have a build like that then you could patch our setup.py file and copy what we do here:

snowflake-connector-python/.github/workflows/build_test.yml

Line 87 in e5e4979

build-manylinux:

and that would yield you the same caliber of wheels that we release on PyPi with your patched dependency pins.

[sfc-gh-mkeller]

Thank you @jameslamb