Support Advanced Usages #33

DaleSeo · 2020-11-06T02:10:39Z

What kind of change does this PR introduce?

fix #26 fix #32

What is the current behavior?

There's no way to specify additional options to configure the underlying requests library for advanced usages.

What is the new behavior?

A new optional parameter options is added to the constructor and the execute method to support advanced usages like Custom Authentication, SSL Cert Verification and Client Side Certificates.

Does this PR introduce a breaking change?

No

DaleSeo · 2020-11-06T02:28:05Z

@steve148 Do you have any idea why is the discrepancy with Black? Maybe, due to different versions being used?

Local

Github Actions

sreerajkksd · 2020-11-06T04:06:48Z

python_graphql_client/graphql_client.py

-                self.endpoint,
-                json=request_body,
-                headers=self.__request_headers(headers),
+                self.endpoint, json=request_body, headers={**self.headers, **headers},


options should be passed here as well. right ?

aiohttp support auth argument.
https://docs.aiohttp.org/en/stable/client_reference.html

Good point but I didn't add the options parameter to the exectue_async method intentionally in this PR to limit the scope to configuring the requests library. To expand the scope to cover the aiohttp library, I think we should consider that these two libraries can take options through the different parameter names. In case of auth, they use the same parameter name but aiohttp uses veryfy_ssl while requests uses verify for SSL cert verification. So, I'd like to tackle this separately after giving it some consideration unless you need this feature right now.

sreerajkksd

https://docs.aiohttp.org/en/stable/client_reference.html

options should be passed to aiohttp session as well. Otherwise, looks good.

sreerajkksd · 2020-11-06T04:24:07Z

@DaleSeo latest version of black gave me this.

DaleSeo · 2020-11-06T13:08:24Z

@DaleSeo latest version of black gave me this.

Yeah, I'm still trying to figure out why my VSCode auto-formats differently from the command line 😞 .

masudias

Looks good to me! Thank you so much for the changes!!
Can you please update the README and provide an example of how we can turn off SSL verification and provide auth to our request? That would help other developers to find the options easily.
Again, thanks so much!

DaleSeo · 2020-11-07T15:52:20Z

@sreerajkksd Fixed the formatting issues by updating pre-commit hooks so that the same latest version of Black is used by the pre-commit, on my terminal and IDE.

➜  python-graphql-client git:(support-options) ✗ pre-commit autoupdate 
Updating https://github.com/psf/black ... [INFO] Initializing environment for https://github.com/psf/black.
updating stable -> 20.8b1.
Updating https://gitlab.com/pycqa/flake8 ... [INFO] Initializing environment for https://gitlab.com/pycqa/flake8.
updating  -> 3.8.4.

DaleSeo · 2020-11-07T15:58:13Z

@masudias Thanks for the feedback. I've added the Advanced Usage section to README. Hope it helps clarify how to use this library. Please feel free to improve the docs if you want it to be more clear.

steve148 · 2020-11-08T15:12:25Z

python_graphql_client/graphql_client.py

    """Class which represents the interface to make graphQL requests through."""

-    def __init__(self, endpoint: str, headers: dict = None):
+    def __init__(self, endpoint: str, headers: dict = {}, options: dict = {}):


Do you have a preference for options over **kwargs here? I think storing it on the instance as options still makes sense.

So this function would turn into:

def __init__(self, endpoint: str, headers: dict = {}, **kwargs: int): ... self.options = kwargs

And the execute function changes to:

def execute( self, query: str, variables: dict = None, operation_name: str = None, headers: dict = None, **kwargs: int, ): ... result = requests.post( ... **{self.options, **kwargs} )

@steve148 I considered the following three approaches.

verify: bool, cert: Union[str, Sequence[str]], auth: ...

options: dict = {}

**kwargs: Any

I decided on the second option because the first option wasn't scalable and the third option, I would say, felt too flexible/generous and potentially, will get in our way when we want to add other parameters in the future.
I found the second option most balanced and explicit but I don't feel strongly about it.
Please let me know if you see any upsides in using **kwargs in this case.

Option number 1 is definitely too rigid, we definitely don't want to be maintaining an exact replica of the arguments allowed by the different libraries we use under the hood.

I personally feel like **kwargs is a better choice of options since **kwargs is the more commonly used pattern for creating flexible functions in python. I would rather use the more common pattern over one that we come up with just for this project.

From my view, execute is just a nice wrapper overtop of requests.post. The distinguishing arguments to execute are query, variables, and operation_name. Looking at it now, headers isn't related to graphql but instead comes from the requests library. This will cause a breaking change in this function's interface, but headers should probably be made part of the **kwargs in the future after this change lands.

Either way, I think going forward we should split arguments into two groups:

Arguments like query which are related to graphql and the functionality this project provides. These arguments should be defined individually in each function (aka def execute(query: str, ...):

Arguments like verify which are related to the underlying communication protocol and aren't specific to graphql or this library. These arguments should be defined as a group in **kwargs so the interface is flexible to changes to the underlying library.

@steve148 That makes sense. I updated the API to use **kwargs as you suggested. I'm not sure how we can also take headers through **kwargs in the future. Would it be easy to determine which argument should be treated as headers and which ones should not when everything is passed together?

🙏 thanks @DaleSeo! I'm probaably thinking a few too many steps ahead when I mention headers being part of **kwargs. One core piece of functionality we do have right now is being able to set headers on the class instance which gets used by all calls. This was pretty useful to us and probably isn't something we want to get rid of.

My thinking would be you could still set headers the class instantation and in the method call.

client = GraphqlClient(..., headers={ 'shared_key': 'shared_value' }) client.execute(..., headers={'another_key': 'another_value'})

And then the HTTP call would be a merge of the two dictionaries

requests.post(..., headers={'shared_key': 'shared_value', 'another_key': 'another_value'})

I think something like the following would achieve this idea https://stackoverflow.com/questions/20656135/python-deep-merge-dictionary-data. At the same time, this is starting to get complex and may not align with the goal of this project to be a simple graphql client.

README.md

steve148 · 2020-11-09T15:20:43Z

README.md

+
+### Custom Authentication
+
+```py


TIL py can be used as shorthand for python for markdown code blocks 🤯

haha. I've also updated the other places to use py instead of python ~~to save us a little bit of typing~~ just for consistency.

Co-authored-by: Lennox Stevenson <lennox.stevenson@mail.utoronto.ca>

DaleSeo added the enhancement New feature or request label Nov 6, 2020

DaleSeo requested review from steve148 and xkludge November 6, 2020 02:10

DaleSeo self-assigned this Nov 6, 2020

DaleSeo force-pushed the support-options branch 2 times, most recently from 5767d6c to 3852c55 Compare November 6, 2020 02:20

support options parameter

8a6da93

DaleSeo force-pushed the support-options branch from 3852c55 to 8a6da93 Compare November 6, 2020 02:37

This was referenced Nov 6, 2020

Support auth for requests #26

Closed

Turning off SSL verification for API requests #32

Closed

sreerajkksd reviewed Nov 6, 2020

View reviewed changes

sreerajkksd suggested changes Nov 6, 2020

View reviewed changes

masudias reviewed Nov 6, 2020

View reviewed changes

DaleSeo added 3 commits November 7, 2020 10:36

pre-commit autoupdate

333acfa

fix formatting issues

031b75b

add Advnaced Usage to README.md

5af59e2

masudias approved these changes Nov 7, 2020

View reviewed changes

steve148 reviewed Nov 8, 2020

View reviewed changes

use **kargs

3421a40

steve148 reviewed Nov 9, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

steve148 reviewed Nov 9, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

steve148 reviewed Nov 9, 2020

View reviewed changes

DaleSeo and others added 3 commits November 9, 2020 12:21

Update README.md

e163504

Co-authored-by: Lennox Stevenson <lennox.stevenson@mail.utoronto.ca>

Update README.md

541059f

Co-authored-by: Lennox Stevenson <lennox.stevenson@mail.utoronto.ca>

use py for syntax highlighting

903f3d3

DaleSeo requested a review from steve148 November 9, 2020 17:29

steve148 approved these changes Nov 9, 2020

View reviewed changes

DaleSeo merged commit 8c8483e into master Nov 9, 2020

steve148 deleted the support-options branch November 10, 2020 14:39

DaleSeo mentioned this pull request Nov 22, 2020

CI runs pre-commit #37

Merged


		### Custom Authentication

		```py

Support Advanced Usages #33

Support Advanced Usages #33

Uh oh!

Conversation

DaleSeo commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Uh oh!

DaleSeo commented Nov 6, 2020

Uh oh!

sreerajkksd Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sreerajkksd Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

DaleSeo Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sreerajkksd left a comment

Choose a reason for hiding this comment

Uh oh!

sreerajkksd commented Nov 6, 2020

Uh oh!

DaleSeo commented Nov 6, 2020

Uh oh!

masudias left a comment

Choose a reason for hiding this comment

Uh oh!

DaleSeo commented Nov 7, 2020

Uh oh!

DaleSeo commented Nov 7, 2020

Uh oh!

steve148 Nov 8, 2020

Choose a reason for hiding this comment

Uh oh!

DaleSeo Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve148 Nov 8, 2020

Choose a reason for hiding this comment

Uh oh!

DaleSeo Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve148 Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steve148 Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

DaleSeo Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DaleSeo commented Nov 6, 2020 •

edited

Loading

sreerajkksd Nov 6, 2020 •

edited

Loading

DaleSeo Nov 6, 2020 •

edited

Loading

DaleSeo Nov 8, 2020 •

edited

Loading

DaleSeo Nov 8, 2020 •

edited

Loading

DaleSeo Nov 9, 2020 •

edited

Loading