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

[RFC] Make root query operation type optional #631

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

victorandree
Copy link

@victorandree victorandree commented Oct 23, 2019

This change would make the root query operation type optional, consistent with mutation and subscription. In this proposal, schema introspection would still work as before, by allowing a "default" query type if one is not defined, exposing only the implicit fields __schema and __types (in response to PR feedback).

The change is worded so that a "schema must define at least one root operation type.". See below for motivation and alternatives.

See #490 for background on this issue.

Motivation

Not all GraphQL APIs need a query interface but do fine with just mutation or subscription. This becomes especially apparent for micro services. Such services are forced to expose dummy query fields, with real world examples such as _, version, helloWorld, or dontQueryMe.

Alternative: Allow root query operation type to be empty

Instead of making the root query operation type optional, it can be allowed to not have any fields.

This has already been proposed (see #606), but would have wider impacts than just making query optional. A benefit of this approach is that schema introspection wouldn't need any new treatment.

Allowing for only the query root, or others, to be empty would require validating that type in the context of how it's used, which seems strange to me (e.g. if a type is only allowed to be empty if it's called Query or is used in schema { query: Query }).

Alternative: Don't require any root type

A schema which only defines types without any operations could be useful in some scenarios, where a schema in service A is perhaps stitched or used as the basis for another. To support this, one could drop the requirement "A schema must define at least one root operation type."

I think this would be esoteric and counter to existing requirements enforcing a "useful" schema (for example requiring composite types not to be empty).

@benjie
Copy link
Member

benjie commented Oct 23, 2019

Currently the spec states:

The schema introspection system is accessible from the meta‐fields __schema and __type which are accessible from the type of the root of a query operation.

We can confirm that this is the case in graphql-js at least by creating the schema:

type Query {
  query: Query
}

and running a query such as:

{
  query {
    query {
      __schema {
        queryType {
          name
        }
      }
    }
  }
}

So schema introspection is linked to the the root query type. If we want to separate schema introspection (__schema, __type(...)) from type introspection (__typename) such that schema introspection does not require a Query type I believe the spec changes necessary to make this the case would be more significant. This would also be a breaking change, as presumably queries like the above would no longer work.

@IvanGoncharov IvanGoncharov added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Oct 23, 2019
@IvanGoncharov
Copy link
Member

I agree with @benjie simplest solution would be to just always add __type and __schema to Query type. It actually makes a lot of sense since we expose __Schema inside introspection but doesn't reference it anywhere inside introspection.
So instead of three magic fields, we will have only __typename and it makes sense to keep it magic since it is available on every object.
Also since we extended introspection types in the past and planning to continue doing that in the future it means there is a non-zero chance that we will extend __schema, __type or add completely new introspection fields to Query.

This makes the root query operation type optional like the other types.

Schema introspection is unaffected. The `__schema` and `__type` fields were
already described as "implicit" and not part of the defined root query
operation type. A schema without a root query type therefore only supports
these queries.

Fixes graphql#490
@victorandree
Copy link
Author

Thanks! I've updated the proposed changes in response to your feedback.

I wasn't initially keen on generating a stub Query type if one isn't defined, but agree that this is more backwards compatible.

__schema and __type aren't included if you do { __type(name: "Query") { fields { name } } }, so this is effectively allowing an empty root Query type, even if it's only implicit. You'd have to change the schema validation logic for this case, I think.

@victorandree
Copy link
Author

victorandree commented Apr 1, 2020

I'm sad to say I've left this hanging for too long. Having thought a bit more about it, I think I'm actually pivoting back to allowing an empty root Query type to support this case.

  • Since introspection is tied to the root query type, we should keep it that way. There must always be a root query type. This maintains backwards compatibility and makes sense: A GraphQL schema can always be queried for its introspection, so there aren't any true "mutation only" GraphQL schemas.1
  • I think it would be preferable to "magically" attach these fields to the root query type defined by the schema writer, rather than "magically" creating an (almost) empty root query type.
  • To me, the most formal argument is that creating an introspection-only Query type requires naming that type. While "Query" is already a semi-reserved/magical name, I think creating types/fields without the __ reserved prefix is best avoided.
  • __schema and __type don't currently show up as fields on the root Query type. However it's created, the introspection-only Query type should then be considered "empty".

TL;DR: I think it would be a smaller change to allow defining an empty root Query type, than creating one if not already defined.

I think there's a strong case for loosening the empty object type restriction, in general, to support placeholder types during development, primarily.

The use of "magical" obviously reflects a value judgment on my part.

1 Introspection queries aren't always allowed (e.g. https://github.com/helfer/graphql-disable-introspection/blob/master/index.js), but the schema still has the types.

@benjie
Copy link
Member

benjie commented Apr 1, 2020

@victorandree Completely agree. I think we should allow for empty input and output objects in general to allow for easier schema evolution. For example, in my GraphQL APIs we follow the Relay Mutation Input Object Specification (read why here); but sometimes a mutation doesn't require any inputs at first and doesn't have any outputs yet; e.g.:

# IDEAL SCENARIO - not currently valid GraphQL

input MyMutationInput {}

type MyMutationPayload {}

type Mutation {
  myMutation(input: MyMutationInput!): MyMutationPayload
}

But later we may wish to evolve that without having to deprecate the old mutation:

input MyMutationInput {
  size: Int = 1
}

type MyMutationPayload {
  numberAffected: Int
}

type Mutation {
  myMutation(input: MyMutationInput!): MyMutationPayload
}

Being able to evolve an API in this way (and maintain consistency during design) is highly desirable. This potentially also encourages allowing empty selection sets, but for now you can solve this with __typename:

# This mutation would be valid against both GraphQL schemas above.

mutation MyMutation($input: MyMutationInput = {}) {
  myMutation(input: $input) {
    __typename
  }
}

Since we don't currently have empty types, we either have to add a stub field to both input and payload:

# clientMutationId is a field required by old Relay Legacy, but has
# been phased out. However it solves this problem well because it is
# available on both input and output, making our types non-empty.

input MyMutationInput {
  clientMutationId: String
}

type MyMutationPayload {
  clientMutationId: String
}

type Mutation {
  myMutation(input: MyMutationInput!): MyMutationPayload
}

or have a versioned/alternative field, and have the original field break the schema mutation conventions:

input MyMutationV2Input {
  size: Int = 1
}

type MyMutationV2Payload {
  numberAffected: Int
}

type Mutation {
  # Original mutation, doesn't adhere to same conventions as other mutations :(
  myMutation: Boolean
    @deprecated(reason: "Use `myMutationV2` instead")

  # Replacement mutation, now supports input and payload
  myMutationV2(input: MyMutationV2Input!): MyMutationV2Payload
}

Neither of which are particularly desirable.

@victorandree
Copy link
Author

victorandree commented Apr 1, 2020

@benjie I'm in the same camp, obviously, and am thinking about resurrecting my defunct #606 PR, taking into consideration also #674.

This discussion is now a bit all over the place, now, so it might be best to write up a proper RFC document.

@benjie
Copy link
Member

benjie commented Apr 1, 2020

@victorandree I think an RFC is the right way to go. CC me when you raise it, I'm happy to help editing/etc. I'm not sure I have the resources currently to champion another thing though!

@huehnerlady
Copy link

Hi, is there any update on this?
We have an API now that includes just a mutation and we now run into the issue linked above .

@victorandree
Copy link
Author

victorandree commented Jan 14, 2021

@huehnerlady I regret I haven't really touched this since my comment last April. It wasn't meant as an April Fool's, but...

  • As for the approach outlined in this PR, I doubt making the root query type optional is viable. It's required for introspection.
  • However, if GraphQL schemas allowed empty object type definitions, one could declare an empty root query type. This would be effectively the same (as it being optional).
  • Allowing empty object type definitions has other consequences, most of which are positive in my opinion. The syntax already supports it; empty object type definitions are only disallowed by validation rules. Implementation wise, it is seemingly a relatively small change to disable the relevant validation rules (e.g. https://github.com/graphql/graphql-js/blob/edbe218579889d6c08c6d1942f35218dbad47d54/src/type/validate.js#L267).
  • The approach to allow empty objects wasn't outright rejected by the GraphQL WG, but it's semantically a pretty sweeping change. There's a justified concern that it opens up to some anti-patterns.

I'm afraid I don't have the bandwidth to champion this, though, otherwise I would have. Unless someone else believes in the approach outlined here, this PR should probably be closed. The other issues referenced in this PR describe prior art.

@huehnerlady
Copy link

@victorandree thank you for your wuick response. is there any other issue where the empty type definition is discussed? One of the 2 discussions was closed ant the other seemed to have a different focus (on actual query objects).

Many thanks

@victorandree
Copy link
Author

victorandree commented Jan 14, 2021

@huehnerlady I'd say #568 is the main issue discussing empty types. #606 was an attempt by me at allowing empty type definitions. That PR references #490, #568, and #300 as potential use cases thereof. #490 is the issue that brought you here (since you want a mutation only schema). #674 is related.

@benjie
Copy link
Member

benjie commented Jan 15, 2021

(Just a quick note to say that I'm still particularly interested in this topic, maybe not this particular route (you can see my opinions above) but in solving this issue. However I've got too many other spec changes on my plate right now to champion anything else 😬 Definitely interested in revisiting it in the summer if you've not had time to do so yourself by then @victorandree. i.e. don't close it yet 😉 )

@yordis
Copy link

yordis commented Jun 29, 2023

Hi there. Do you have any opportunity to revisit this?

Maybe the lib/framework authors must define a query, but it could make defining the user space optional. Some lib/framework authors are waiting for guidelines on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants