Migration to graphql-core-v3 #36
Conversation
There was a problem hiding this comment.
This looks fantastic thanks @KingDarBoja ! Couple of questions around why some tests were removed though.
|
Ummm, so weird, the re-added tests didn't passed although those did locally. EDIT Fixed both tests because missing period on the graphql-core error message (which got fixed on v3.1.0). |
|
@jkimbo I have no idea what's going on but removing the |
There was a problem hiding this comment.
Looks good @KingDarBoja . Minor change needed regarding the graphql-server dependency but other than that I think this is good to merge.
@jkimbo I have no idea what's going on but removing the @mark.asyncio on the asyncio test will make it pass 🤔
I think that makes sense because the pytest asyncio library assumes the function that is wrapping is an async function but the tests here create an async function inside the body and then wait for it resolve. We might need to add the pytest asyncio library back again in the future if we end up writing lots more async tests but we can cross that bridge when we come to it.
- Bump dependencies - Refactor code to use f-strings format (3.6+) - Rename public data structures BREAKING CHANGE: - Requires graphql-core-v3 - Drop support for Python 2 and below 3.6 - Remove executor check as graphql-core-v3 does not have SyncExecutor
Co-Authored-By: Jonathan Kim <[email protected]>
KingDarBoja
force-pushed
the
migrate-graphql-core-v3
branch
from
874017e
to
bed7432
Compare
KingDarBoja
added
type: maintenance
| "errors": [{"msg": "Some msg", "loc": "1:2", "pth": "some/path"}], | ||
| } | ||
| assert output.status_code == 200 | ||
| assert output.status_code == 400 |
There was a problem hiding this comment.
This seems like a subtly nontrivial change... if existing clients expect an HTTP 200 even if the errors key is present in the response, won't this potentially break them? Personally, I prefer this behavior, but it seems risky to change outright and I don't see anything that mentions the rationale for doing so.
There was a problem hiding this comment.
Good spot @wyattanderson ! @KingDarBoja can we revert the change to return a 400 status code if the response contains errors? We should always return with a 200 because there might be partial data returned.
There was a problem hiding this comment.
The changes were made at these lines so should be easier to revert.
I do prefer the 400 status code as it reflects there was an error on the request but I didn't took into account the possible side effects for those clients than expect a HTTP 200 response.
Maybe we can keep the new behaviour and put on the readme a Breaking Change Note about this as this will be part of graphql-server-core v3.
Based on @Cito branch as stated at #20 and as part of the refactoring and merge steps discussed at the Slack channel plus #34, this PR aims to make graphql-server compatible with latest graphql-core version.
@jkimbo this one is ready for review!