-
Notifications
You must be signed in to change notification settings - Fork 140
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
Added the ability to specify the JSON encoder #25
base: master
Are you sure you want to change the base?
Added the ability to specify the JSON encoder #25
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just a bystander here, but this change looks good to me. My comments aren't actual issues, just suggestions.
@@ -142,10 +143,10 @@ def get_response(self, request, data, show_graphiql=False): | |||
def json_encode(self, request, d, show_graphiql=False): | |||
pretty = self.pretty or show_graphiql or request.args.get('pretty') | |||
if not pretty: | |||
return json.dumps(d, separators=(',', ':')) | |||
return json.dumps(d, separators=(',', ':'), cls=self.json_encoder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to continue using json.dumps
here rather than the encoder directly?
return self.json_encoder(separators=(',' ':')).encode(d)
I believe they're functionally equivalent but the .encode()
route reduces the dependency on the builtin json
module, since there are several json libraries that could be reasonably used: http://artem.krylysov.com/blog/2015/09/29/benchmark-python-json-libraries/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert on json encoding/decoding in Python. What you've said sounds great but I'd rather leave this to someone in a better position to comment.
@@ -33,6 +33,7 @@ class GraphQLView(View): | |||
graphiql_template = None | |||
middleware = None | |||
batch = False | |||
json_encoder = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also include a json_decoder
member? Roughly equivalent change with the json.loads
-> self.json_decoder(…).decode(s)
a couple places below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree, I've just updated the branch with json_decoder support.
3 similar comments
@syrusakbary This PR looks good, is anything preventing it from merging? |
Hi @erydo @Mleonard87, Thanks for taking time for making this PR. I think the serialization/deserialization is better to be assumed by the scalar type itself rather than the transport formatting language ( However I might be missing some of your context here. Would be great if you could provide an example of a Scalar type that could not be created without the "customize json serialization class" approach. |
@syrusakbary I think you're mostly right, but a couple considerations:
Error serialization doesn't go through the Scalar system, though I think pretty much everything else does. |
For error serialization, you can provide a custom error function But then the solution for that is not a custom decoder for |
I have just stumbled upon this, I am having the same need. We have a SQLAlchemy model that is using a custom class called 'MVStringBag', it's a bag of translateable strings with a default. It inherits from sa.STRING. This custom type is used in several SQLAlchemy' Model Classes to provide 'automatic' multi-language strings in our API:
It allows you to send a key to it, specifying the wanted language and, if that's not available, it'll return the default string.
To generate the GraphQL Classes, I am using the Meta: feature you guys provide to automatically plug all of the Model classes' attributes to GraphQL Scalars and, since MVStringBag inherits from String, it'll be (correctly) a graphene.String (or a graphene.JSONString, in what i have seen in the wild) on the GraphQL side. which is the final type I expect to return to the API, so that translation is totally transparent and the api returns a simple and standard string. But this class cannot be serialized because it's not one of the default types. So passing a custom JSONEncoder would be really useful. |
json.dumps()
allows for a JSON encoder to be specified by thecls
argument. This PR allows for a JSONEncoder to be passed through when constructing the GraphQLView object.Supporting this PR allows for serialization of custom types which can be particular useful when defining custom graphene scalar types.