Add Parser support for Type System Extensions

[jorge-vasquez-2301]

closes #238

[ghostdogpr]

It looks very good! The only thing missing is the valid case where no field is provided but only directives/implements/etc. It'll make the parser a bit more complex unfortunately but we have to support all valid syntax 😄

[jorge-vasquez-2301]

@ghostdogpr Thanks for looking into this! And yeah, I totally missed the alternative cases, so I'll make sure to add them 😀

[ghostdogpr]

While this is working well, I suggest modeling the parsers more strictly after the spec. In some cases (like object) it will be a little simpler, but the main benefit is that it's easier to find which code corresponds to which spec definition When specs will evolve in the future I think it might be easier to apply the changes if the code follows it closely. What do you think?

[ghostdogpr]

I think you can reduce to 3 cases:

• fields + optional interfaces + optional directives
• no fields + directives + optional interfaces
• no fields, no directives, interfaces

[jorge-vasquez-2301]

@ghostdogpr Yeah, actually my first attempt was modelling the Parser more strictly after the specs, like this:

private def objectTypeExtensionWithOptionalInterfacesOptionalDirectivesAndFields[_: P]: P[ObjectTypeExtension] =
    P(name ~ implements.? ~ directives.? ~ "{" ~ fieldDefinition.rep ~ "}").map {
      case (name, implements, directives, fields) =>
        ObjectTypeExtension(
          name,
          implements.getOrElse(Nil),
          directives.getOrElse(Nil),
          fields.toList
        )
    }

  private def objectTypeExtensionWithOptionalInterfacesAndDirectives[_: P]: P[ObjectTypeExtension] =
    P(name ~ implements.? ~ directives).map {
      case (name, implements, directives) =>
        ObjectTypeExtension(
          name,
          implements.getOrElse(Nil),
          directives,
          Nil
        )
    }

  private def objectTypeExtensionWithInterfaces[_: P]: P[ObjectTypeExtension] =
    P(name ~ implements).map {
      case (name, implements) =>
        ObjectTypeExtension(
          name,
          implements,
          Nil,
          Nil
        )
    }


  private def objectTypeExtension[_: P]: P[ObjectTypeExtension] =
    P(
      "extend type" ~/ (
        objectTypeExtensionWithOptionalInterfacesOptionalDirectivesAndFields |
          objectTypeExtensionWithOptionalInterfacesAndDirectives |
          objectTypeExtensionWithInterfaces
      )
    )

[jorge-vasquez-2301]

However, that didn't work as expected, when running unit tests I've got failures for some cases. Here are the error messages I got (I've enabled verboseFailures temporally):

[jorge-vasquez-2301]

 extend type with interfaces and directives
[info]     Fiber failed.
[info]     A checked error was not handled.
[info]     Parsing Error: Expected document:1:1 / objectTypeExtension:1:1 / objectTypeExtensionWithOptionalInterfacesOptionalDirectivesAndFields:1:13 / "{":1:58, found "" 

[jorge-vasquez-2301]

extend type with directives
[info]     Fiber failed.
[info]     A checked error was not handled.
[info]     Parsing Error: Expected document:1:1 / objectTypeExtension:1:1 / objectTypeExtensionWithOptionalInterfacesOptionalDirectivesAndFields:1:13 / "{":1:33, found "" 

[jorge-vasquez-2301]

I've tried other combinations as well, but none of them worked for all cases, that's why I had to implement the parser rules in a different way.

[jorge-vasquez-2301]

What do you think @ghostdogpr? Maybe I'm missing something?

[ghostdogpr]

Hmm that’s strange indeed. I can’t see anything wrong with the code at first sight but tomorrow I’ll get your branch and try to figure it out in debug.

[jorge-vasquez-2301]

And, I got similar issues when I tried to define parsers for the other type extension cases

[jorge-vasquez-2301]

Awesome, thanks for your help @ghostdogpr!

[ghostdogpr]

How about sticking to the spec definitions and have:

• operations + optional directives
• no operations, directives

[jorge-vasquez-2301]

Similarly to the above case, my first version was more like the spec definitions:

private def schemaExtensionWithOptionalDirectivesAndOperations[_: P]: P[SchemaExtension] =
    P(directives.? ~ "{" ~ rootOperationTypeDefinition.rep ~ "}").map {
      case (directives, ops) =>
        val opsMap = ops.toMap
        SchemaExtension(
          directives.getOrElse(Nil),
          opsMap.get(OperationType.Query).map(_.name),
          opsMap.get(OperationType.Mutation).map(_.name),
          opsMap.get(OperationType.Subscription).map(_.name)
        )
    }

  private def schemaExtensionWithDirectives[_: P]: P[SchemaExtension] =
    P(directives).map(SchemaExtension(_, None, None, None))

  private def schemaExtension[_: P]: P[SchemaExtension] =
    P("extend schema" ~/ (schemaExtensionWithOptionalDirectivesAndOperations | schemaExtensionWithDirectives))

[jorge-vasquez-2301]

However, this didn't work for all cases, I got an error for one of the unit tests:

extend schema with directives
[info]     Fiber failed.
[info]     A checked error was not handled.
[info]     Parsing Error: Expected document:1:1 / schemaExtension:1:1 / schemaExtensionWithOptionalDirectivesAndOperations:1:15 / "{":1:30, found "" 

[jorge-vasquez-2301]

I tried other combinations that were more similar to the specs too, but none of them worked

[ghostdogpr]

@jorge-vasquez-2301 OK, I found the reason. You just need to change the ~/ into ~ in def directive. Otherwise it takes the first path and "cuts" which means it won't go back and try the other possibilities.

[jorge-vasquez-2301]

Ohhhh, didn't realize that, thanks @ghostdogpr! I'll make the adjustments

[ghostdogpr]

All good! Thanks for the contribution!