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

Exist basic scalar type in downloaded schema and got Schema errors #5126

Closed
sonatard opened this issue Jul 26, 2023 · 11 comments
Closed

Exist basic scalar type in downloaded schema and got Schema errors #5126

sonatard opened this issue Jul 26, 2023 · 11 comments

Comments

@sonatard
Copy link
Contributor

sonatard commented Jul 26, 2023

Question

スクリーンショット 2023-07-26 13 11 38

When I download the Schema with gradlew :app:downloadApolloSchema , the basic scalar type(ID, Int, Float...) exists in Schema file. Probably because of this, Android Studio is throwing an error, but how should I deal with this?

There is no problem with the operation such as building or sending requests.

Also, I sometimes write code to parse GraphQL schemas in Go, but I get the same error there and cannot parse it. Should the Go GraphQL parser be made not to throw an error, or should the schema not include a basic scalar type, which is correct?

@martinbonnin
Copy link
Contributor

Hi 👋 . This is a known issue in the GraphQL IntelliJ plugin. These types needs to be in the schema because there exists several versions of them (deprecated input fields, repeated directives, etc... have been introduced more recently). And we need to know what version is used to do proper validation.

@sonatard
Copy link
Contributor Author

Thank you! How does knowing the version affect the parser?

@martinbonnin
Copy link
Contributor

It's affecting validation and/or introspection. Take the latest builtin __InputValue type definition:

type __InputValue {
  name: String!
  description: String
  type: __Type!
  defaultValue: String
  isDeprecated: Boolean!
  deprecationReason: String
}

And write this query:

{
  __schema {
    types {
      inputFields {
        isDeprecated
      }
    }
  }
}

It will validate well for Apollo Kotlin because isDeprecated is in the schema. Now if you try to execute this against an older version of the spec that doesn't know about deprecated input fields, this query will error all the time. This is what we want to avoid by validating the queries client side.

@sonatard
Copy link
Contributor Author

I see! I didn't know that 'isDeprecated' was missing from old spec Introspection's __InputValue.

Thank you for the detailed explanation.

@sonatard
Copy link
Contributor Author

When representing a GraphQL schema using the type system definition language, all built-in scalars must be omitted for brevity.
https://spec.graphql.org/draft/#sec-Scalars.Built-in-Scalars

@martinbonnin Hi! I found this section in GraphQL Draft spec.
It seems to me that built-in scalars should not exist in the schema file. What do you think?

@sonatard sonatard reopened this Jul 27, 2023
@martinbonnin
Copy link
Contributor

Good catch, TIL! I'll remove them. It's only about builtin scalars though, not the introspection types or directives so there will be still some warnings for the time being but @BoD added a fix to the IntelliJ plugin there

@martinbonnin
Copy link
Contributor

PR here

@sonatard
Copy link
Contributor Author

Thanks 👍

@martinbonnin
Copy link
Contributor

@sonatard: for the record, I opened this PR in the GraphQL spec to explicit how introspection types should be handled in SDL. The tldr; is that it's not easy and will probably require to change to introspection and how servers declare their capabilities to other tools. You can read more in yesterday's working group meeting notes

@martinbonnin
Copy link
Contributor

martinbonnin commented Aug 4, 2023

Thinking more about this, one actionable item for us is to add to the following comment to the builtin types:

# noinspection GraphQLTypeRedefinition

At least it'll remove the warning until the spec moves forward

@martinbonnin
Copy link
Contributor

martinbonnin commented Aug 9, 2023

3 changes in 4.0.0-alpha.3:

  • scalars are not written by default in SDL
  • other types have a # noinspection GraphQLTypeRedefinition comment to ignore the warning
  • the Apollo IJ plugin itself is going to ignore the warning

I'll close this issue again as I believe there nothing more we can do in Apollo Kotlin in the current state of things. For discussions about how to handle full schemas in the GraphQL spec, see graphql/graphql-wg#1367

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants