Descriptions as Strings in BuildFromDefinition

[cjoudrey]

I just stumbled upon this PR in graphql-js: https://github.com/graphql/graphql-js/pull/927

This proposes replacing leading comment blocks as descriptions in the schema definition language with leading strings.

Before

# This is a description
# of the `Foo` type.

type Foo implements Bar {
  one: Type
}

Now

"""
This is a description
of the `Foo` type.
"""
type Foo implements Bar {
  one: Type
}

Implementing this depends on https://github.com/rmosolgo/graphql-ruby/issues/1150.

Ref. Impl.:

• graphql/graphql-js#927 (Descriptions as strings)
• graphql/graphql-js#1102 (SDL changes)

Spec: facebook/graphql#90

[rmosolgo]

:+1: For adding it, but we shouldn't remove the old way yet. If anyone is depending on it, we want to give them a chance to update first.

[cjoudrey]

Yeah, I agree. We should be able to support both ways if my memory of that code is right.

[zzak]

I'd take a stab at this, but seems BLOCK_STRING already uses this token. So adding it would break compatibility with existing documents?

Any ideas @rmosolgo? :bow:

[rmosolgo]

So far, we only have the first part of block string support: we added it to lexer.rl, and it emits string tokens, so you can use block strings for query inputs.

But we need to use BLOCK_STRING for a second purpose: In the Schema Definition Language, descriptions may be expressed with leading block strings. Our implementation is outdated, from when the SDL was a draft. It uses preceding comment lines to build descriptions and pass them to described items (eg field, object type.

Parser-wise, what we need to do is:

• Allow string literals to come before definitions in the SDL
• Use those string literals as descriptions
• AND continue to support comment descriptions if strings are not present, for compatibility

Additionally, we need to update Schema::Printer to output descriptions in that format, with leading strings instead of comments.

Does that clarify it a bit?

• Extend the parser to allow

[zzak]

@rmosolgo Thanks for your reply!

I was wondering if we could re-use the :COMMENT token that are generated by record_comment, and then scan for a similar block string that comes before a defition. For that we would have to peek? Or we could combine definitions to a catch-all?

I'm not super familiar with ragel, but seems pretty straight forward.

[rmosolgo]

Personally, I would rather not reuse that that code because it's a bit of a hack. Comments are technically ignored tokens, so they're ignored by the parser entirely. However, since the first draft of the SDL said that comments could be used for description, we supported that (#305) with record_comment, which required making tokens into a doubly-linked list. That way, they could remain ignored by the parser, but we could still fetch descriptions by backtracking through the list of tokens.

I think if we can implement descriptions at the parser level, we could simplify the code a bit. For example, if we picked up leading strings like:

diff --git a/lib/graphql/language/parser.y b/lib/graphql/language/parser.y
index 071e57c5..65c6c9b9 100644
--- a/lib/graphql/language/parser.y
+++ b/lib/graphql/language/parser.y
@@ -295,9 +295,13 @@ rule

+  description_opt:
+      /* none */ { return nil }
+    | STRING
+
   object_type_definition:
-      TYPE name implements_opt directives_list_opt LCURLY field_definition_list RCURLY {
-        return make_node(:ObjectTypeDefinition, name: val[1], interfaces: val[2], directives: val[3], fields: val[5], description: get_description(val[0]), position_source: val[0])
+      description_opt TYPE name implements_opt directives_list_opt LCURLY field_definition_list RCURLY {
+        return make_node(:ObjectTypeDefinition, name: val[2], interfaces: val[3], directives: val[4], fields: val[6], description: (val[0] || get_description(val[1])), position_source: val[0] || val[1])
       }

Then (after a period of backward compat), we could:

• remove get_description and record_comment
• stop keeping tokens as a doubly-linked-list
• start ignoring tokens altogether (make the Ragel action a no-op)

So, although it's a bigger change now, I think it sets us up for a bit simpler system down the road.

That said, beggars can't be choosers: if you want to send a PR with the implementation you mentioned and a few tests, I'd happily accept it as a solid improvement, and then later, when we remove the comments-as-descriptions code, we can revisit those changes to the parser!

[zzak]

@rmosolgo Thank you for your explanation!

I tried to follow your instruction for description_opt, but found I was not able to retrieve the previous token as STRING inside get_description. With a bit more prodding, I might be able to figure it out -- but for now I've had to time box this to a days worth of work.

We'll have to come back to this, so I don't want to hold things up if someone else gets to it first.

My impression after this though, is that your original suggestion should work -- and we can keep compatibility with comment style descriptions.

[rmosolgo]

Huh, thanks for taking a stab at it! I took a little try with my pseudocode from above:

https://github.com/rmosolgo/graphql-ruby/commit/11c1bf7703c2c0f0794cff11d929a4cef315e3de

In the test case added there, we never enter get_description, since val[0] (which is type_definition_opt) is present. I'm hoping to bypass get_description altogether since it's a bit of weird code which was required to support comments-as-descriptions, but not strings-as-descriptions 😅

[zzak]

@rmosolgo Oh interesting, that is not far off from the patch I was working on. And the tests pass!

How about for a single-line string?

Such as:

  it "parses strings as type descriptions" do
    defn_string = <<-GRAPHQL
    """Here's a description"""
    type Thing { field: Stuff }
    GRAPHQL

    document = subject.parse(defn_string)
    type_defn = document.definitions.first
    assert_equal "Thing", type_defn.name
    assert_equal "Here's a description", type_defn.description
  end

[rmosolgo]

According to the spec, any string should be fair game. A type's description is a StringValue (src):

Description : StringValue

and a StringValue is either a "single-quoted string" or a """block string""" (src:

StringValue ::

• " StringCharacter* "
• """ BlockStringCharacter* """

So, an example like the one you posted should be supported!

[rmosolgo]

Oh, maybe I misread, your example was a block string, but it didn't begin with a newline. This is also fair game, for example, here's a block string """Cheese""": https://github.com/rmosolgo/graphql-ruby/pull/1219/files#diff-a15bdba7df1e7fbfa7e99d733d46be17L88

[zzak]

@rmosolgo That's exactly what I meant, using a block string for the description.

Both seem at least included in the spec and other implementations I've come across.

[daemonsy]

Hi, I'm helping to close out issues. From reading the discussion, it seems this is fully resolved when #1725 landed.

I'm closing the issue, let me know if anyone likes to reopen it.

[zzak]

Glad to see this land, nice work! :tada: