-
Notifications
You must be signed in to change notification settings - Fork 35
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
Handling Variables via JSON? #145
Comments
@russmatney I think for now parsing the JSON request yourself is the way to go. I haven't yet managed to convince myself that the |
I think we should add an example of this in the docs as people using graphql are really used to querying their API using a separate JSON object for the variables. Can I try to make a PR for this ? Also, I'm not sure if handling a JSON object of variables is just "delayed" until we can figure out a better implementation than #128 or whether there is no decent implementation possible given the way things are right now ? (if that makes sense) |
@theobat PRs are very welcome, especially doc PRs! JSON handling is delayed until someone has time to pick it up. My feeling is that the parsing should by type directed (i.e. same instance pattern matching we use for resolution) but I don't have the time in the foreseeable future to do this. It's a bit frustrating that it's stalled, but I prefer stalled over a solution I don't really believe in, especially because people can roll their own solution today (so they're not hard-blocked). Does that make sense? |
thanks for the quick answer, it does make sense. |
So I'm playing around with this and here's what I'd like to do in another PR (But planning everything ahead seems like a safer bet):
is represented in the AST exactly. I believe the argument part should be something like:
So please let me know if anyone has any suggestion on this, I'll continue my investigation anyway (and try to get the exact structure of the AST in this case). |
Points 1 and 2 sound like a good idea! I had forgotten that we're storing the |
It's very likely I'm missing something, so take all of this as lightly held, sleep-deprived opinion. I don't know why we'd go to the AST to answer questions about variable definitions, rather than on the validated query. The goal is to go from an The major downside of this approach is that have some type information that we can't get up to the type level, partly due to |
@jml yeah actually you're totally right, I came to the same conclusion myself just a few minutes ago by getting a better understanding of the true role of the AST while adding a test case on it. So this brings only two functions to the table:
|
Yup, although parseJSONVariableValues :: VariableDefinitions -> Aeson.Value -> Either VariableError GraphQL.Value |
I'm having a very hard time transforming VariableDefinitions down to a bunch of Schema.InputTypeDefinition. The reason I critically need this is that a VariableDefinition is defined in Validation.hs as:
And the AST.GType itself (which I thought was carrying enough information) is just the representation of what can be written in a query such as:
So, just a parsed version of the GraphQL input (which is completely fine, it's an AST after all). Now, here's my problem, I need the entire schema information about MyVarType in my example to build a proper VariableValues object. Which means I need something along the lines of:
Now we begin to see there's something bad for my use case in here: The schema does not directly expose InputTypes ... One could probably say it is normal, since the graphql Operations are represented as Type definitions, not InputType definitions, and their (optional) arguments are part of their definition. But it seems like traversing the entire schema every time we parse a JSON object of variables seems like a lot of work at runtime for no valid reason. What do you think ? |
I’ll try to take a look on the weekend.
…On Wed, 13 Jun 2018 at 22:07, theobat ***@***.***> wrote:
I'm having a very hard time passing VariableDefinitions down to
Schema.InputTypeDefinition. The reason I critically need this is that a
VariableDefinition is defined in Validation as:
-- | Defines a variable within the context of an operation.
--
-- See <https://facebook.github.io/graphql/#sec-Language.Variables>
data VariableDefinition
= VariableDefinition
{ variable :: Variable -- ^ The name of the variable
, variableType :: AST.GType -- ^ The type of the variable
, defaultValue :: Maybe Value -- ^ An optional default value for the variable
} deriving (Eq, Ord, Show)
type VariableDefinitions = Map Variable VariableDefinition
And the AST.GType itself (which I thought was carrying enough information)
is just the representation of what can be written in a query such as:
($myVar: MyVarType, $myListVar: [MyVarType!]!)
mirrored by :
data GType = TypeNamed NamedType
| TypeList ListType
| TypeNonNull NonNullType
deriving (Eq, Ord, Show)
newtype NamedType = NamedType Name deriving (Eq, Ord, Show)
So, just a parsed version of the GraphQL input (which is completely fine,
it's an *AST* after all).
Now, here's my problem, I need the entire schema information about
MyVarType in my example to build a proper VariableValues object. Which
means I need something along the lines of:
newtype Schema = Schema (Map Name TypeDefinition) deriving (Eq, Ord, Show)
Now we begin to see there's something bad for my use case in here: The
schema does not directly expose InputTypes ... One could probably say it is
normal, since the graphql *Operations* are represented as Type
definitions, not InputType definitions, and their (optional) arguments are
part of their definition. But it seems like traversing the entire schema
every time we parse a JSON object of variables seems like a lot of work at
runtime for no valid reason.
So either I write a function doing this traversal or, I change the schema
API a little bit to expose a way to query input types (which by the way
have the same namespace as regular types in graphql-js)
What do you think ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#145 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHq6vLJYzvjLaVq8kn4F2iNt6kUBfB3ks5t8X8UgaJpZM4Rh2LZ>
.
|
I mostly follow, but then I don't. Not sure whether it's because I'm not deep enough into the details of the project or because you're using terminology loosely. Anyway, this is where I lose you:
What's "the entire schema information" here?
I don't know what you mean by If so, that would be
OK, sure, but I'm not sure exactly what proposal you're rejecting.
I don't understand what you mean here, and I don't get why namespaces are relevant. Here's my take:
Does this help? |
Yes it's probably a lack of accuracy on my end, plus mixing things together, sorry about that. The main idea I was trying to convey (which I think you essentially understood given your answer) is that:
Now, I agree with your first point, but going from AST.GType to
So what I propose is:
Yes !! |
Sounds great! |
Is there a recent summary on what's outstanding on this? Is the intention to get a fully JSON parseable version of VariableValues? I've run into this little snag when trying to get graphql-api working with Servant. The HTTP spec looks like we need to get the VariableValues and the Query as distinct query parameters on a GET. If I follow correctly, part of the issue is that the query or schema need to be available in order to handle this parsing in a type correct fashion. Has the change been made that allows the Ambiguous types until the schema (perhaps the wrong word) can be provided? I've been trying to workaround this issues in the meantime but ran into a much more difficult problem with I found that ConstScalar's value constructors aren't exported (which won't need to be if the FromJSON instances are available along with VariableValues. I'll see if I can find some time to help move this forward. |
At this point everything is "ready" to implement a JSON -> GQL DSL. The last pull request I opened and ultimately closed was about implementing such mechanism. The FromJSON will not help you though, cause you need to compare the schema definitions with the actual values and as far as I know (I've searched for this) you just can't provide context for json decoding in aeson typeclass instances. It's fairly "easy" to implement this functionality, I've just ended up not doing it, because I think there's a problem with the way schema and values are completely separated DSLs in this implementation of graphql. |
@theobat that makes sense. What about the idea of creating an instance of FromJSON for AmbiguousVariableValues which could be reified with a (AmbiguousVariableValues -> Schema -> VariableValues)? That way we could get the raw input through the disparate mechanisms and then use the schema after most of hydration. I'm not quite sure on the details yet but from looking through the code, this shouldn't be too terrible to implement. I can't see how much duplication or otherwise it might result in either yet. Is there some way during a normal use (like an HTTP handler) to treat VariableValues as a component of the schema? Would you mind elaborating on why these being separate creates an issue? As a consumer, it seems like this separation comes about due to the way I need to receive and forward this information to the library. I'm still working through the details though. Let me know if some samples would help in anyway, and let me know if I can help with anything. I'd like to start leveraging this sooner rather than later if possible. |
Thanks much for all the work getting this GraphQL implementation together - it's no small task.
How would you recommend using this library with variables parsed from a JSON Request? It looks like #128 would accomplish that - just want to make sure this was actually not yet supported, as the path I'm on seems to require parsing the JSON (via Aeson) prior to interpreting the query.
Would love to see that PR merged soon!
The text was updated successfully, but these errors were encountered: