Skip to content

Add deprecations to IDL #275

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 27, 2016
Merged

Add deprecations to IDL #275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30d65d3
Add deprecations to IDL
cjoudrey Sep 23, 2016
e0f8f37
Add support for DEFAULT_DEPRECATION_REASON
cjoudrey Sep 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/graphql/directive.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,15 @@ class Directive
MUTATION = :MUTATION,
SUBSCRIPTION = :SUBSCRIPTION,
FIELD = :FIELD,
FIELD_DEFINITION = :FIELD_DEFINITION,
FRAGMENT_DEFINITION = :FRAGMENT_DEFINITION,
FRAGMENT_SPREAD = :FRAGMENT_SPREAD,
INLINE_FRAGMENT = :INLINE_FRAGMENT,
ENUM_VALUE = :ENUM_VALUE,
]

DEFAULT_DEPRECATION_REASON = 'No longer supported'

def initialize
@arguments = {}
end
Expand All @@ -45,3 +49,4 @@ def on_operation?

require "graphql/directive/include_directive"
require "graphql/directive/skip_directive"
require "graphql/directive/deprecated_directive"
11 changes: 11 additions & 0 deletions lib/graphql/directive/deprecated_directive.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
GraphQL::Directive::DeprecatedDirective = GraphQL::Directive.define do
name "deprecated"
description "Marks an element of a GraphQL schema as no longer supported."
locations([GraphQL::Directive::FIELD_DEFINITION, GraphQL::Directive::ENUM_VALUE])

reason_description = "Explains why this element was deprecated, usually also including a "\
"suggestion for how to access supported similar data. Formatted "\
"in [Markdown](https://daringfireball.net/projects/markdown/)."

argument :reason, !GraphQL::STRING_TYPE, reason_description, default_value: GraphQL::Directive::DEFAULT_DEPRECATION_REASON
end
2 changes: 1 addition & 1 deletion lib/graphql/schema.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class Schema
:query_analyzers, :middleware


DIRECTIVES = [GraphQL::Directive::SkipDirective, GraphQL::Directive::IncludeDirective]
DIRECTIVES = [GraphQL::Directive::SkipDirective, GraphQL::Directive::IncludeDirective, GraphQL::Directive::DeprecatedDirective]
DYNAMIC_FIELDS = ["__type", "__typename", "__schema"]
RESOLVE_TYPE_PROC_REQUIRED = -> (obj, ctx) { raise("Schema.resolve_type is undefined, can't resolve type for #{obj.inspect}") }

Expand Down
58 changes: 50 additions & 8 deletions lib/graphql/schema/printer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module Printer
# Return a GraphQL schema string for the defined types in the schema
# @param schema [GraphQL::Schema]
def print_schema(schema)
print_filtered_schema(schema, method(:is_defined_type))
print_filtered_schema(schema, lambda { |n| !is_spec_directive(n) }, method(:is_defined_type))
end

# Return the GraphQL schema string for the introspection type system
Expand All @@ -21,16 +21,20 @@ def print_introspection_schema
name "Query"
end
schema = GraphQL::Schema.define(query: query_root)
print_filtered_schema(schema, method(:is_introspection_type))
print_filtered_schema(schema, method(:is_spec_directive), method(:is_introspection_type))
end

private

def print_filtered_schema(schema, type_filter)
def print_filtered_schema(schema, directive_filter, type_filter)
directives = schema.directives.values.select{ |directive| directive_filter.call(directive) }
directive_definitions = directives.map{ |directive| print_directive(directive) }

types = schema.types.values.select{ |type| type_filter.call(type) }.sort_by(&:name)
type_definitions = types.map{ |type| print_type(type) }

[print_schema_definition(schema)].concat(type_definitions).join("\n\n")
[print_schema_definition(schema)].concat(directive_definitions)
.concat(type_definitions).join("\n\n")
end

def print_schema_definition(schema)
Expand All @@ -44,6 +48,10 @@ def print_schema_definition(schema)
BUILTIN_SCALARS = Set.new(["String", "Boolean", "Int", "Float", "ID"])
private_constant :BUILTIN_SCALARS

def is_spec_directive(directive)
['skip', 'include', 'deprecated'].include?(directive.name)
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to figure out how this compares to the JS implementation, are there ever cases that we don't want to print these out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.

I ported this from the JS implementation: https://github.com/graphql/graphql-js/blob/5e3d377e6e0881191efe02b9d40720ff0f0b5b0f/src/utilities/schemaPrinter.js#L28-L44

My understanding of the JS code is that when printing the schema, we do not want to include spec directives since they are part of the spec. We still print non-spec directives though.

When printing the introspection query, we only include spec directives.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-spec directives

Part of me wants to leave this out, since there aren't any non-spec directives yet. But ... if we left it out, I can guarantee people would be surprised not to see @defer / @stream in the query dump if I ever got around to merging that 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be surprised not to see @defer / @stream in the query dump

Just to make sure we're on the same page. You'd still see @defer and @stream on the fields, you just don't see their definitions, i.e. directive @defer.

I think the other reason they do this in the JS implementation is because directives can be specified on the schema: https://github.com/graphql/graphql-js/blob/a725499b155285c2e33647a93393c82689b20b0f/src/type/schema.js#L50-L54

We don't support that yet.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, maybe I don't understand! If graphql-ruby supports @stream and @defer, but they aren't part of the GraphQL spec, would they show up in a schema print-out or not?

Copy link
Contributor Author

@cjoudrey cjoudrey Sep 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry, I kept confusing myself here too.

There's two concepts: Directive and DirectiveDefinition.

Example @skip Directive:

query myQuery($someTest: Boolean) {
  experimentalField @skip(if: $someTest)
}

Example @skip DirectiveDefinition:

directive @skip(if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT

The is_spec_directive method is used to omit the DirectiveDefinition of spec directives from getting printed when dumping the schema.

I need to rename def print_directive(directive) to def print_directive_definition(directive) as that's not named correctly and could be a source of confusion.

In terms of Directive we're only printing the deprecated directive at the moment, see DeprecatedPrinter. We may want to generalize this in the future though.

Edit: So to answer your question about @stream and @defer, if we don't add them to is_spec_directive their definitions will get printed, but if you use GraphQL::Language::Generation to print a document representing a query that uses @defer on some selection set, it won't show up unless we print it in GraphQL::Language::Generation.


def is_introspection_type(type)
type.name.start_with?("__")
end
Expand All @@ -56,12 +64,27 @@ def print_type(type)
TypeKindPrinters::STRATEGIES.fetch(type.kind).print(type)
end

def print_directive(directive)
TypeKindPrinters::DirectivePrinter.print(directive)
end

module TypeKindPrinters
module FieldPrinter
def print_fields(type)
type.all_fields.map{ |field| " #{field.name}#{print_args(field)}: #{field.type}" }.join("\n")
module DeprecatedPrinter
def print_deprecated(field_or_enum_value)
return unless field_or_enum_value.deprecation_reason

case field_or_enum_value.deprecation_reason
when nil
''
when '', GraphQL::Directive::DEFAULT_DEPRECATION_REASON
' @deprecated'
else
" @deprecated(reason: #{field_or_enum_value.deprecation_reason.to_s.inspect})"
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure I understand, is this equivalent to three, mutually-exclusive branches, ie

case field_or_enum_value.deprecation_reason 
when nil 
  "" 
when "" 
  " @deprecated" 
else 
  " @deprecated(...)" 
end

? I guess there's a different behavior with false, but I don't think we expect false here, right ?

Copy link
Owner

@rmosolgo rmosolgo Sep 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the JS version handles the "default deprecation reason" specially, should we do the same?

https://github.com/graphql/graphql-js/pull/384/files#diff-71ba52e9c625f826d2b0df2963c8633aR170

(eg case "", GraphQL::Directive::DEFAULT_DEPRECATION_REASON ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The code is clearer with the case statement. I'll make that change.

end
end

module ArgsPrinter
def print_args(field)
return if field.arguments.empty?
"(#{field.arguments.values.map{ |arg| print_input_value(arg) }.join(", ")})"
Expand Down Expand Up @@ -105,6 +128,24 @@ def print_value(value, type)
end
end

module FieldPrinter
include DeprecatedPrinter
include ArgsPrinter
def print_fields(type)
type.all_fields.map{ |field|
" #{field.name}#{print_args(field)}: #{field.type}#{print_deprecated(field)}"
}.join("\n")
end
end

class DirectivePrinter
extend ArgsPrinter
def self.print(directive)
"directive @#{directive.name}#{print_args(directive)} "\
"on #{directive.locations.join(' | ')}"
end
end

class ScalarPrinter
def self.print(type)
"scalar #{type.name}"
Expand Down Expand Up @@ -137,8 +178,9 @@ def self.print(type)
end

class EnumPrinter
extend DeprecatedPrinter
def self.print(type)
values = type.values.values.map{ |v| " #{v.name}" }.join("\n")
values = type.values.values.map{ |v| " #{v.name}#{print_deprecated(v)}" }.join("\n")
"enum #{type.name} {\n#{values}\n}"
end
end
Expand Down
10 changes: 10 additions & 0 deletions spec/graphql/introspection/directive_type_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@
"onFragment" => true,
"onOperation" => false,
},
{
"name" => "deprecated",
"args" => [
{"name"=>"reason", "type"=>{"name"=>"Non-Null", "ofType"=>{"name"=>"String"}}}
],
"locations"=>["FIELD_DEFINITION", "ENUM_VALUE"],
"onField" => false,
"onFragment" => false,
"onOperation" => false,
},
]
}
}}
Expand Down
20 changes: 17 additions & 3 deletions spec/graphql/schema/printer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

value "FOO"
value "BAR"
value "BAZ", deprecation_reason: 'Use "BAR".'
value "WOZ", deprecation_reason: GraphQL::Directive::DEFAULT_DEPRECATION_REASON
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

end

sub_input_type = GraphQL::InputObjectType.define do
Expand Down Expand Up @@ -46,6 +48,7 @@
field :title, !types.String
field :body, !types.String
field :comments, types[!comment_type]
field :comments_count, !types.Int, deprecation_reason: 'Use "comments".'
end

query_root = GraphQL::ObjectType.define do
Expand All @@ -70,24 +73,32 @@
query: Query
}

directive @skip(if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT

directive @include(if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT

directive @deprecated(reason: String! = \"No longer supported\") on FIELD_DEFINITION | ENUM_VALUE

type __Directive {
name: String!
description: String
args: [__InputValue!]!
locations: [__DirectiveLocation!]!
onOperation: Boolean!
onFragment: Boolean!
onField: Boolean!
onOperation: Boolean! @deprecated(reason: \"Moved to 'locations' field\")
onFragment: Boolean! @deprecated(reason: \"Moved to 'locations' field\")
onField: Boolean! @deprecated(reason: \"Moved to 'locations' field\")
}

enum __DirectiveLocation {
QUERY
MUTATION
SUBSCRIPTION
FIELD
FIELD_DEFINITION
FRAGMENT_DEFINITION
FRAGMENT_SPREAD
INLINE_FRAGMENT
ENUM_VALUE
}

type __EnumValue {
Expand Down Expand Up @@ -158,6 +169,8 @@
enum Choice {
FOO
BAR
BAZ @deprecated(reason: "Use \\\"BAR\\\".")
WOZ @deprecated
}

type Comment implements Node {
Expand All @@ -173,6 +186,7 @@
title: String!
body: String!
comments: [Comment!]
comments_count: Int! @deprecated(reason: \"Use \\\"comments\\\".\")
}

type Query {
Expand Down