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

[Feature Request] Ability to set operationName (name) through queryDocument #30

Closed
sunwukonga opened this issue Dec 20, 2017 · 7 comments

Comments

@sunwukonga
Copy link

sunwukonga commented Dec 20, 2017

At the moment I cannot see any mechanism for setting the operationName of a Query or Mutation.

queryDocument is declared in src/GraphQL/Request/Builder.elm as:

queryDocument :
    ValueSpec NonNull ObjectType result vars
    -> Document Query result vars
queryDocument spec =
    document
        (Operation
            { operationType = queryOperationType
            , name = Nothing
            , directives = []
            , spec = spec
            }
)
{-| On a pedantic note, perhaps `name` should be renamed `operationName` for transparency. -}

I propose that this function should be named anonymousQueryDocument and another queryDocument function created with signature:

 queryDocument :
    OperationName
    -> ValueSpec NonNull ObjectType result vars
    -> Document Query result vars

where

type alias OperationName = Maybe String

The OperationName should be, alphanumeric with underscores, and not starting with a number /[_A-Za-z][_0-9A-Za-z]*/ I think this check can be deferred to a query validation step. anonymousQueryDocument could then be written as:

anonymousQueryDocument:
    -> ValueSpec NonNull ObjectType result vars
    -> Document Query result vars
anonymousQueryDocument spec = queryDocument Nothing spec

queryDocument:
    OperationName
    -> ValueSpec NonNull ObjectType result vars
    -> Document Query result vars
queryDocument operationName spec =
    document
        (Operation
            { operationType = queryOperationType
            , name = operationName
            , directives = []
            , spec = spec
            }
        )

Breaking change obviously. The alternative is a new function, maybe namedQueryDocument instead of moving the old behaviour to anonymousQueryDocument. All of the above also applies to mutationDocument.

@sunwukonga
Copy link
Author

I'd don't mind doing this and issuing a PR. I just need to know that this is a direction that you (@jamesmacaulay) want to go in.

@jamesmacaulay
Copy link
Owner

@sunwukonga I didn't provide a way to create named operations with the builder DSL because I couldn't think of a situation where you'd need a named operation in a single-operation Document. I considered expanding the DSL to allow for the creation of multi-operation documents, but decided against it because it would have involved adding a lot more complexity to the API and didn't seem worth it.

Can I ask what your use case is for this? Is there a reason why you need to be able to create named operations?

@sunwukonga
Copy link
Author

sunwukonga commented Dec 21, 2017

I am integrating Haskell's graphql-api with elm-graphql. Up untill last night, graphql-api did not accept anonymous queries that are prefixed with query. It now does, at least in my branch.

I have to admit that I'm a little uncertain why an operation would need to be named at all. By convention defined in the spec under #operation-names

You'll need these optional parts (operationName, variables) to a GraphQL operation if you want to execute something other than a query or pass dynamic variables.

and #post-request-operation

Name is only required if multiple operations are present in the query

You are correct, if there is only a single operation, there is no need to specify an operation name. However, simply restricting documents to a single operation seems a better and saner choice (which you took), given the lack of utility of the case. The above implies that mutations need naming, but again, there is no need if there is only one operation. Similarly, the need in conjunction with variables is only realized in the context of multiple operations.

However, operation names and multiple operations per document are defended at graphql github on the strength of future expansions of the spec, circa. 2015, (summarized as):

  1. Storing operations client-side (iOS, Android) in the form of .graphql files in a format that a standard parser can still handle (seems a flimsy justification).
  2. In a world where graphql servers store request documents (not implemented anywhere that I know of, and not mentioned at all in the spec), a new query could be specified with:
    {
    query: "documentID: ####"
    operationName: "someOperationNameKnownToExistInReferncedDocument"
    }
  3. Batching operations, such that the initiating operation could call other operations in the document (again, haven't seen it in the spec, or implemented; though it was mentioned in this comment by the same author github graphql).

1, 2, and 3 seem unnecessary. That means, to me at least, that operation names are not really about selecting operations unambiguously (although they can if you're silly enough to include more than one operation per document).

Therefore, the only utility for operation names (at this time) is when returning error messages, in which case they become a valuable feedback mechanism for tracking down bugs, regardless of the number of operations in the document.

Ultimately, it seems that multiple operations per document and the means to specify a particular one, seems to have been a grand architecture choice that hasn't panned out (...yet).


In conclusion, since operation names are only truly useful for tracking errors, they have utility in every query, not just in ones with the arbitrarily and unnecessarily introduced case of multiple operations in a single request.

From above:

... if you want to execute something other than a query or pass dynamic variables.

With mutations (and possibly subscriptions) ^^^^^^, an operation name seems to be expected by servers, thanks to the spec, though not because of any concrete need for them. In the context of "dynamic variables", they are also expected (again without need).

@sunwukonga
Copy link
Author

Strictly from the spec #operation-names, and interpreted succinctly:

BOTH query keyword AND query name MUST be added to any mutation or any subscription, OR to any query that passes dynamic variables.

@jamesmacaulay
Copy link
Owner

jamesmacaulay commented Dec 23, 2017

@sunwukonga okay, you've convinced me that it's worth providing at least the ability to name your operations. I will add functions for that.

However, about this statement:

BOTH query keyword AND query name MUST be added to any mutation or any subscription, OR to any query that passes dynamic variables.

I believe you've interpreted the documentation you were reading incorrectly. The "Learn GraphQL" docs are a great learning resource, but they aren't as precise as the actual spec. If you look at the section of the spec on operations and the corresponding section on validation of operations, you'll see that the operation name is always optional as long as an anonymous operation only ever occurs on its own in a single-operation document. In fact, the primary example of an operation given by the spec is a mutation with no name:

mutation {
  likeStory(storyID: 12345) {
    story {
      likeCount
    }
  }
}

@sunwukonga
Copy link
Author

This is one of those times I'm happy to be wrong; I was literally wondering how facebook engineers could be so imprecise and illogical :). The graphql.org on operation names is not just badly worded, it's wrong. I had, up to this point, made the automatic assumption that they were one-to-one. Thanks, this makes me even more sure that what I'm PR-ing on the server side with graphql-api is correct. I'll keep my eyes firmly on http://facebook.github.io/graphql/October2016/ in future.

Would you like me to rearrange these functions to provide a named version and the corresponding anon version that uses it?

Which do you prefer:

  1. [Breaking change] Add name parameter to queryDocument, and then create anonymousQueryDocument that uses queryDocument with Nothing in the name parameter?

  2. Leave the type signature of queryDocument alone and create namedQueryDocument with a name parameter. Have queryDocument call namedQueryDocument with Nothing in the name parameter?

I think that option 2 conveys the sense that queryDocument is the default, and namedQueryDocument is an extension of something that is optional, with the added advantage that any code that relies on the current signature of queryDocument will be unaffected.

@jamesmacaulay
Copy link
Owner

Resolved with #31, I will publish a minor version release today. Thanks for bringing this up, @sunwukonga!

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