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

Fix abc.GitHubAPI.graphql() to accept response content types lacking spaces #122

Merged
merged 13 commits into from
May 27, 2020
Merged

Fix abc.GitHubAPI.graphql() to accept response content types lacking spaces #122

merged 13 commits into from
May 27, 2020

Conversation

caseyduquettesc
Copy link
Contributor

@caseyduquettesc caseyduquettesc commented May 12, 2020

Background

Closes #121

As stated in the issue, Github enterprise server sets a response header equal to application/json;charset=utf-8 (note: without the space separating the type and character encoding).

The graphql method in the ABC asserts the content type header equals application/json; charset=utf-8 (note: with space).

The failing assertion prevents the method from successfully returning.

Change

  • Removed the content type assertion from the graphql response handling
  • Replaced the response data decoding with a call to sansio to parse the content-type from the header and load the json based on that.
    • It felt kind of dirty copying this logic out of sansio._decode_body() but it did have a little bit of v3 specific logic in there. Maybe a _decode_gql_body() is needed?
  • Added a new explicit exception when the graphql response content type is unknown to gidgethub, GraphQLResponseTypeError.

Test Plan

Added three tests to prevent regressions and test the new exception. I appended the original issue number just in case anyone is curious about the context of the tests. If that's not your style, I can remove it.

  • test_response_content_type_parsing_gh121
  • test_unknown_response_content_type_gh121
  • test_no_response_content_type_gh121

@caseyduquettesc caseyduquettesc marked this pull request as ready for review May 12, 2020 01:23
@brettcannon brettcannon self-requested a review May 12, 2020 16:52
Copy link
Collaborator

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Looking good, but I think some error conditions should raise and another case where it shouldn't. 😄

gidgethub/__init__.py Outdated Show resolved Hide resolved
gidgethub/__init__.py Outdated Show resolved Hide resolved
gidgethub/abc.py Outdated Show resolved Hide resolved
gidgethub/abc.py Outdated Show resolved Hide resolved
gidgethub/abc.py Outdated Show resolved Hide resolved
tests/test_abc.py Outdated Show resolved Hide resolved
tests/test_abc.py Outdated Show resolved Hide resolved
tests/test_abc.py Outdated Show resolved Hide resolved
tests/test_abc.py Outdated Show resolved Hide resolved
tests/test_abc.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Getting down to formatting, so almost there!

gidgethub/abc.py Outdated
Comment on lines 247 to 248
if not response_data:
raise GraphQLException("Response contained no data", response_data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe move this up as it's a bigger issue than trying to get the content-type.

tests/test_abc.py Show resolved Hide resolved
tests/test_abc.py Outdated Show resolved Hide resolved
tests/test_abc.py Outdated Show resolved Hide resolved
tests/test_abc.py Outdated Show resolved Hide resolved
tests/test_abc.py Outdated Show resolved Hide resolved
@caseyduquettesc
Copy link
Contributor Author

@brettcannon Looks like httpx released a new version (0.12.1 -> 0.13.1) and the type for the AyncClient headers changed. Should I address it here? I wanted to check with you before changing the scope of the PR. Although I confess that to avoid changing the ABC, the only solution I can see is to cast the headers passed in to _client.request().

Looks like it changed from typing.Dict[typing.AnyStr, typing.AnyStr] to Dict[StrOrBytes, StrOrBytes], with the problem being they switched from AnyStr to a Union.

0.13.1: https://github.com/encode/httpx/blob/ebc9c2936503cbff979658a70530ce22775bd11a/httpx/_types.py#L40
0.12.1: https://github.com/encode/httpx/blob/23486b5438c8c142c430aba702d61144ec2e901b/httpx/_models.py#L68

@brettcannon
Copy link
Collaborator

What I don't get is why is Mapping[str, str] not acceptable as a covariant of Mapping[Union[str, bytes], Union[str, bytes]]?

The two options I can think of are:

  1. type cast
  2. ignore that single point with a # type: ignore flag by breaking the call up into multiple lines

I don't love either, but I will accept either as well.

@caseyduquettesc
Copy link
Contributor Author

I'm going to go with an ignore I think because as they make clear in their readme, the library is still in development and the types could very well change again.

Copy link
Collaborator

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

I just realized there are no docs for the new exception. Last thing and then I can merge this.

@brettcannon brettcannon changed the title Fix the graphql method to accept content types without spaces Fix abc.GitHubAPI.graphql() to accept response content types lacking spaces May 26, 2020
@caseyduquettesc
Copy link
Contributor Author

I just realized there are no docs for the new exception. Last thing and then I can merge this.

Good call. I missed that.

@brettcannon brettcannon merged commit d9eae33 into gidgethub:master May 27, 2020
@brettcannon
Copy link
Collaborator

Thanks for all the work, @caseyduquettesc !

@caseyduquettesc caseyduquettesc deleted the casey/content-type branch May 27, 2020 19:06
@caseyduquettesc
Copy link
Contributor Author

@brettcannon coincidentally httpx fixed the types in 0.13.2 😁, but it's now causing the master builds to fail because the type ignore is extraneous

encode/httpx@3721a78

@brettcannon
Copy link
Collaborator

Ah, that's why it's suddenly failing! 👍

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.

GraphQL response content-type header assertion failing
2 participants