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

Adds upload_files functionality for request transport #244

Merged

Conversation

DENKweit
Copy link
Contributor

@leszekhanusz This PR requires a new package dependency: requests_toolbelt but I'm not sure where to include it. Could you add it for me? Thanks!

@leszekhanusz
Copy link
Collaborator

@leszekhanusz This PR requires a new package dependency: requests_toolbelt but I'm not sure where to include it. Could you add it for me? Thanks!

I don't have access to a pc right now but you could add it to the install_requests_requires array in setup.py

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2021

Codecov Report

Merging #244 (3697543) into master (af8f223) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #244   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         1214      1240   +26     
=========================================
+ Hits          1214      1240   +26     
Impacted Files Coverage Δ
gql/transport/requests.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af8f223...3697543. Read the comment docs.

@DENKweit
Copy link
Contributor Author

DENKweit commented Sep 28, 2021

@leszekhanusz Could you have a look and merge this PR if you're satisfied? :)

@leszekhanusz
Copy link
Collaborator

For the coverage, you could refactor:

            if self.headers is None:
                post_args["headers"] = {"Content-Type": data.content_type}
            else:
                post_args["headers"]["Content-Type"] = data.content_type

into:

            if post_args["headers"] is None:
                post_args["headers"] = {}

            post_args["headers"]["Content-Type"] = data.content_type

Copy link
Collaborator

@leszekhanusz leszekhanusz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀
Just please add a maximum version for the requests_toolbelt dependency.

@leszekhanusz
Copy link
Collaborator

Thanks, I'll merge this PR next week.

@leszekhanusz leszekhanusz mentioned this pull request Sep 28, 2021
@maaft
Copy link

maaft commented Sep 30, 2021

Hi! I noticed something strange with this PR.. The content-type header gets set correctly to "multipart/form-data" when you do an upload mutation, but when you execute another mutation/query on the same client afterwards, this header is still present.

Looking at the code, I fail to grasp how this can be the case as post_args exists only in the local context and should not have any impact after the mutation was send.. @leszekhanusz @DENKweit do you have any idea how this can happen??

@leszekhanusz
Copy link
Collaborator

leszekhanusz commented Sep 30, 2021

Indeed, post_args["headers"] is set as a reference to self.headers so if you modify it, you modify self.headers.
This is a bug.

@DENKweit
Copy link
Contributor Author

DENKweit commented Sep 30, 2021

Thank you for finding this! So, this is happening:

  • post_args["headers"] is initialized to self.headers
  • setting post_args["headers"]["Content-Type"] to foo is also modifying self.headers which gets used again during the next request

Writing a fix now.

@leszekhanusz leszekhanusz merged commit 3db7a86 into graphql-python:master Oct 6, 2021
@leszekhanusz
Copy link
Collaborator

Thanks @DENKweit, @maaft !

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 this pull request may close these issues.

4 participants