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

printSchema loses @directive information #869

Closed
jzimmek opened this issue May 19, 2017 · 14 comments
Closed

printSchema loses @directive information #869

jzimmek opened this issue May 19, 2017 · 14 comments

Comments

@jzimmek
Copy link

jzimmek commented May 19, 2017

Hi, the printSchema function in "graphql/utilities/schemaPrinter" seems to forget the directives on field definitions.

import {buildSchema} from  "graphql/utilities/buildASTSchema"
import {printSchema} from  "graphql/utilities/schemaPrinter"

const schema = buildSchema(`
  type Person {
    id: ID!
    name: String @skip(if: true)
  }
  type Query {
    people: [Person]
  }
`)

console.info(printSchema(schema))

current output:

type Person {
  id: ID!
  name: String
}

type Query {
  people: [Person]
}

expected output:

type Person {
  id: ID!
  name: String @skip(if: true)
}
type Query {
  people: [Person]
}

Tested against graphql version: 0.9.6

I already digged a bit deeper into schemaPrinter and it seems that there is some special handling for directives going on:

1.) printSchema filters directives down to "skip", "include" and "deprecated"
2.) @ deperected seems to be handled

This is maybe better addressed in a separate issue, but I would like to add my own custom directives and get them also properly handled by printSchema.

Would you consider a PR for proper handling of spec directives as well as custom ones?

@jzimmek jzimmek changed the title printSchema looses some @directive information printSchema loses @directive information May 19, 2017
@leebyron
Copy link
Contributor

This is correct behavior. Directives are artifacts of the GraphQL language, and not properties of a GraphQL schema. While directives are allowed to be used in the GraphQL schema language, they exist as language-level metadata which different tools may interpret in different ways.

For example, as you pointed out @deprecated is a directive which the schema generation can use to fill in the actual field deprecation information in the schema. Tools like graph.cool or other code generation service often use these directives to describe what kind of resolvers or data storage to use when treating this document as a source of truth.

However the opposite is not the case. Since a schema is an in-memory data structure artifact, and not simply a representation of the GraphQL language, it does not have a concept of directives on its own. Though utilities like printSchema(), which produce GraphQL language from a GraphQL schema, may use directives to express other concepts, the one that it currently uses is @deprecated.

However to your example, @skip applied to a type definition has no semantic meaning in GraphQL (in fact, if you ran validate() on this document, it would likely point this out), and thus is not used when producing the GraphQL schema. When you then use printSchema() to produce the text again, since the directive had no semantic meaning and wasn't used, it doesn't appear in the printed output.

@mrjj
Copy link

mrjj commented Nov 9, 2017

IMO lacking of mechanism to transmit any metadata in schema (even descriptions are stripped off) is a serious flaw and actually its hard for me to understand described harm.
Directives are the closest apple, but the real need is to have payload.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Nov 10, 2017

@mrjj Agree, but this issue is beyond the scope of graphql-js and should be handled in GraphQL spec. Here is the issue about adding it to the spec: graphql/graphql-spec#300

@mrjj
Copy link

mrjj commented Nov 10, 2017

@IvanGoncharov the issue that during 2 last years of development resources are investing in parser, but not in language and seeing parser and schema description language as a different things with different policy is completely shizophenic.

@jzimmek

This is correct behavior. Directives are artifacts of the GraphQL language, and not properties of a GraphQL schema.
I hope you will never meet this situation in third-party project:

  • we need anonymous lambda in our language
  • anonymous lambdas are highly supported
  • but they are not working, compiler just ignoring them
  • right, because they are supported by language syntax and reference parser standards but completely contradicts our language standards, so we've invested resources to hardcode restrictions on language constructions that could be potentially used to imitate anonymous lambdas.
  • but we really need anonymous lamdas....

Every day students write a ten new parsers even with formal grammar description support and other bells and whistles. They have no value.

Value is in maintaining industry standards that are comfortable to use, have working reference implementation, extendable and through this making integration and development easy. I don't expect that e.g. Brotobufs or SOAP or RDP or whatever extensions being defined in standards will be intentionally restricted in reference implementation to make it non-extandable by no reason.

Standards covering e.g. JSONSchema or Thrift are not separated on syntax and language, they are just JSONSchema or Thrift standards and every implementation just implementing this standard from 1% to 200% and reference implementation is you know reference for this.

@bhoriuchi
Copy link

bhoriuchi commented Mar 19, 2018

If you want to reproduce the definition along with the directives you can just print all the astNodes.

import { print, isSpecifiedScalarType, isSpecifiedDirective } from 'graphql';

export function printSchemaWithDirectives(schema) {
  const str = Object
    .keys(schema.getTypeMap())
    .filter(k => !k.match(/^__/))
    .reduce((accum, name) => {
      const type = schema.getType(name);
      return !isSpecifiedScalarType(type)
        ? accum += `${print(type.astNode)}\n`
        : accum;
    }, '');

  return schema
    .getDirectives()
    .reduce((accum, d) => {
      return !isSpecifiedDirective(d)
        ? accum += `${print(d.astNode)}\n`
        : accum;
    }, str + `${print(schema.astNode)}\n`);
}

@vicary
Copy link

vicary commented Aug 16, 2019

@bhoriuchi Great snippets, do you think it's possible to also convert comment descriptions?

I was using printSchema(buildSchema(schemas), { commentDescription: true }) to convert our Apollo compatible schemas into the pathetic AppSync compatible version. It went well until we tried their subscriptions via @aws_subscribe(mutation: [...]).

AWS AppSync has their own custom directives, but their GraphQL engine is also ancient. It does not support type extension, uses comment descriptions, hiccups left and right...

@bhoriuchi
Copy link

@vicary I'm not sure as I have no experience with AppSync. The code I provided relies on the native graphql-js print function but you can get the description from the astNode and add it in if its not being printed. https://github.com/graphql/graphql-js/blob/master/src/language/ast.js#L540

@vicary
Copy link

vicary commented Aug 17, 2019

@bhoriuchi Thanks! I wanted description of type defs instead of directive defs, but I get the idea.

I also noticed that all of my extend type ... is missing, is it possible to combine all type extensions as if they are one type def?

@vicary
Copy link

vicary commented Aug 18, 2019

Ended up digging too deep and creating appsync-schema-converter myself.

@jeffersonaguilar95
Copy link

jeffersonaguilar95 commented Jun 7, 2022

I've fixed this issue with the method printschemawithdirectives from graphql-tools library. I hope this helps! 😄

@tekkeon
Copy link

tekkeon commented Mar 27, 2023

@bhoriuchi Thanks! I wanted description of type defs instead of directive defs, but I get the idea.

I also noticed that all of my extend type ... is missing, is it possible to combine all type extensions as if they are one type def?

Hey @vicary, how did you end up fixing this issue in your appsync-schema-converter where it removes "extend type"? I was actually planning to create an AppSync converter myself, but specifically for subgraphs with Apollo Federation. Maybe something that could be built into your package instead though!

@vicary
Copy link

vicary commented Mar 27, 2023

@mkossoris I am not familiar with federation, let me know which particular feature you'd like to see in appsync-schema-converter I'll make it happen.

My team is using my converter in production for a couple of years now, it should be stable enough for most cases.

@tekkeon
Copy link

tekkeon commented Mar 27, 2023

@vicary I went ahead and created an issue in your GitLab repo here: https://gitlab.com/vicary/appsync-schema-converter/-/issues/2

@vicary
Copy link

vicary commented Apr 9, 2023

@mkossoris I've made a MR !11 to include types for Apollo Federation Subgraphs. Let's test it out and I'll release a new minor.

OT: I am also reviving GQty the GraphQL client with upcoming plans. The DX really beats everything else in the market, check it out!

Screen_Recording_2023-04-07_at_20.24.16.mov

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

8 participants