Skip to content

Feat: Add support for custom scalar types [updated] #104

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

Closed

Conversation

KingDarBoja
Copy link
Contributor

Updated #33 with latest changes from master. Had to open my own branch as I don't have permission to push to source repo.

Awais Hussain and others added 10 commits May 1, 2018 11:47
Move the GQLResponseParser into the gql library.

Each time a GQL query is made by the client, the response is passed through
ResponseParser.parse() where it is processed.

Configure your own custom scalars as follows:
```
    custom_scalars = {
        'SomeCustomScalar': <ScalarClass>
    }

    gql_client = GQLClient(transport=gql_transport,
                           schema=schema,
                           custom_scalars=custom_scalars)

    gql_client.execute(...)
```
<ScalarClass> should have a .parse_value(value) function

There are a few anti-patterns I have had to include in order to support
some new functionality that we require:
- client.execute now accepts `url` and `headers` as arguments, since we
  need to be able to set these on a per request basis. Previously they were
  supplied by the transport (which gets set only once at initialization
  time).
- As a consequence, the url supplied in to the transport goes unused if a
  url is passed in to `execute()`. It is a required field so I have to pass
  in a string, but it's not the best.
The tests were copied over from a different repo and so were inconsistent
with the Star Wars schema we are using here. This PR should fix the tests
(although we are still using a remote schema rather than a local one, so
the tests run slowly becuase they need to make a http request each time).
The 'ResponseParser' name was confusing because it was too broad, and we
weren't really actually parsing the response. Hopefully this makes it
clearer that what we are actually doing it just adding support for custom
types.
Stick to the format of having a one line summary followed by more detailed
information.
Since we are now merging the headers specified in the transport with the
headers specified on a per request basis, we need to make sure the old
headers are of type `dict`, a `None` is not sufficient.
GQLServerError maps to errors which are generated on the backend server.
I.e. the server responds with a <200 OK> but an "errors" field is included
within the response. These are often user facing errors and should be
handled as such.

GQLSyntaxError maps to cases where either the query or the schema is
improperly formatted. This usually indicates a mistake in the code.
Previously we were using Python3.5+ type annotation syntax but this is not
backwards compatible so we are removing it.
@KingDarBoja KingDarBoja added the type: feature A new feature label Jun 14, 2020
@KingDarBoja KingDarBoja requested a review from leszekhanusz June 14, 2020 21:55
@coveralls
Copy link

coveralls commented Jun 14, 2020

Coverage Status

Coverage decreased (-100.0%) to 0.0% when pulling 33dc6e7 on KingDarBoja:custom-scalar-types into 8fc378d on graphql-python:master.

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.

In the client.py, the result.data should also be modified in the subscribe method of AsyncClientSession

@leszekhanusz
Copy link
Collaborator

Also why do we need to provide a class with a parse_value method ? Isn't it simpler to just provide a method directly ?

@KingDarBoja
Copy link
Contributor Author

Also why do we need to provide a class with a parse_value method ? Isn't it simpler to just provide a method directly ?

Not sure what was the reasoning behind this but probably it was done in order to keep most of the boilerplate code on its own class so any extra stuff can be added later to extend the type adapter functionality.

@KingDarBoja KingDarBoja requested a review from leszekhanusz June 22, 2020 01:45
@KingDarBoja KingDarBoja self-assigned this Jun 22, 2020
@KingDarBoja KingDarBoja added this to the Version 3.0 milestone Jun 29, 2020
@leszekhanusz
Copy link
Collaborator

leszekhanusz commented Jun 29, 2020

Some thoughts:

  • why do we limit ourselves to convert only scalars ? Could it be more useful to be also able to convert GraphQLObjects ?
  • As of now, what we have will convert scalars using only the answer from the server and the schema and does not take into account the query. This could cause problems:
    • if the same name is used as a root field for query, mutation or subscription
    • This can't work if the user is using aliases

@KingDarBoja
Copy link
Contributor Author

KingDarBoja commented Jun 30, 2020

why do we limit ourselves to convert only scalars ? Could it be more useful to be also able to convert GraphQLObjects ?

Indeed, this should support any kind of leaf types like as Enums or Objects, this PR wanted to bring the old one with the latest features of gql so I didn't bother to add the rest of types to keep the review simple.

As of now, what we have will convert scalars using only the answer from the server and the schema and does not take into account the query.

Never though about that, maybe we should perform the conversion using the AST and checking each node kind to find the correct and perform the operation on it. For this setp, there is a Visitor class on the graphql-core which let us walk over the AST and perform any changes without modifying the original AST.

@KingDarBoja KingDarBoja requested a review from Cito June 30, 2020 23:03
@leszekhanusz leszekhanusz mentioned this pull request Nov 8, 2020
@fpieper fpieper mentioned this pull request Dec 21, 2020
@leszekhanusz leszekhanusz mentioned this pull request Feb 22, 2021
@leszekhanusz
Copy link
Collaborator

leszekhanusz commented Nov 19, 2021

The proposition of @KingDarBoja to use the Visitor pattern on the document AST has been implemented in PR #256.
So I am going to close this PR as obsolete.

Thank again for your work anyway 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants