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

Got invalid value wrong interpretation #12

Closed
syrusakbary opened this issue Oct 4, 2018 · 4 comments
Closed

Got invalid value wrong interpretation #12

syrusakbary opened this issue Oct 4, 2018 · 4 comments
Assignees

Comments

@syrusakbary
Copy link
Member

syrusakbary commented Oct 4, 2018

Code in graphql-core
https://github.com/graphql-python/graphql-core/blob/master/graphql/execution/values.py#L71-L76

Code in graphql-core-next:
https://github.com/graphql-python/graphql-core-next/blob/master/graphql/execution/values.py#L96-L99

Graphql-core next leaks the inner Python representation of an object.

In general, after reviewing a lot of tests and code, there is a lot of usage of repr when it should be used just for debugging, not for uniform error messages.

@Cito
Copy link
Member

Cito commented Oct 5, 2018

GraphQL.js uses the inspect function to print values in error messages which also leaks some details like function names. Maybe we should add our own inspect function to pyutils and use it everywhere for that purpose? Then we can easily modify what we expose in all error messages in a central location. I'd also like to stay close to GraphQL.js.

@syrusakbary
Copy link
Member Author

That would be great!

@Cito Cito self-assigned this Oct 5, 2018
@Cito
Copy link
Member

Cito commented Oct 5, 2018

I'll get that into the next release then.

@Cito
Copy link
Member

Cito commented Oct 5, 2018

The problem with the approach in graphql-core of simply JSON dumping values is that the actual error message is not shown when JSON dumping is not possible, instead you get a JSON error. Demonstration:

import graphql

schema = graphql.build_ast_schema(graphql.parse(
    "schema { query: T } input I { n: Int! } type T { s(i: I!): String }"))
query = "query ($i: I!) { s(i: $i) }"
result = graphql.graphql(schema, query, variables={'i': object()})

print(result.errors)

This gives a TypeError("Object of type 'object' is not JSON serializable") instead of a GraphQLError("Variable '$i' got invalid value...; Expected type I to be a dict.").

That's why I resorted to repr(). The custom inspect() function will not reveal as much as repr().

Cito added a commit that referenced this issue Oct 22, 2018
We don't want to leak too much of the inner representation
of Python objects in the client-facing error messages.
@Cito Cito closed this as completed Oct 22, 2018
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

No branches or pull requests

2 participants