docusign / docusign-esign-python-client

The Official DocuSign Python Client Library used to interact with the eSign REST API. Send, sign, and approve documents using this client. https://www.docusign.com/devcenter
MIT License
97 stars 88 forks source link

set_oauth_host_name function contain code that will never execute. #137

Open sqlldev opened 2 years ago

sqlldev commented 2 years ago

https://github.com/docusign/docusign-esign-python-client/blob/330a500f29e91366766fb8def4947a28e4bcdff3/docusign_esign/client/api_client.py#L810

Correct me if I'm wrong, but this code will never execute. I would like to use this functionality :(

It's not a blocker or any critical bug, because I can pass oauth_host_name to ApiClient constructor. Just letting you know this code looks strange.

geoffpdevsup commented 2 years ago

I work in developer support at DocuSign, and as I recall our customers used to be confused by setting this up in demo, missing the step of setting the oauth host, and ending up with the oath request being sent to production. That is the why we raise an error now instead of setting a default value. I have posted a bug to cleanup the unexecuted code.

geoffpdevsup commented 2 years ago

This bug is filed as [DCM-7258] Unexecuted Code in python API client.

deckar01 commented 2 years ago

There is an internal method that is still expecting this empty call to implicitly define the oauth server.

https://github.com/docusign/docusign-esign-python-client/blob/26ec78e/docusign_esign/client/api_client.py#L839-L852

Silently defaulting to production is bad, but it can be fixed without discarding the convenience. The underlying problem with the original code was that the else branch unconditionally used the production server. It could require the base path to match production to make it safe and convenient. If the base path is not defined, it should error. If the base path doesn't have a corresponding auth server, it should error also.


A slightly related topic: The way oauth_host_name is required across ApiClient methods is inconsistent.

The first 2 methods that require the positional arg can't be fixed without a major version bump (or 2 depending up your deprecation policies). I'm not sure what the maintainers' stance will be on set-then-call, optional-kwarg, or required-arg, but I would recommend settling on one and make sure all the methods use that pattern in the next (few?) major release(s?). generate_access_token has a bug and should validate the auth server in a patch.