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

Updates to allow printing of Schema Directives #638

Closed
wants to merge 8 commits into from
Closed

Updates to allow printing of Schema Directives #638

wants to merge 8 commits into from

Conversation

Tyler-Gauch
Copy link

@Tyler-Gauch Tyler-Gauch commented Mar 31, 2020

Overview

I have been working to get a working Apollo Federation server set up using this library. In doing so one of the requirements of Apollo Federation is that you need to serve the printed schema via the Query._service field, however, Apollo Federation relys heavily on schema directives and the current SchemaPrinter doesn't print any schema directives although the library allows adding them.

Problem

The current SchemaPrinter doesn't support printing Schema Directives

Example

Given schema:

directive @sd on OBJECT

type Bar @sd {
  foo: String
}

type Query {
  foo: Bar
}

SchemaPrinter::doPrint(...);
will print:

directive @sd on OBJECT

type Bar {
  foo: String
}

type Query {
  foo: Bar
}

Notice the missing @sd on Bar.

Solution

I added a piece of code similar to the interfaces printing in printObject which now prints Schema Directives

Testing

Added test cases for:

  1. Schema Directive No Arguments
  2. Schema Directive String Argument
  3. Schema Directive Int Argument
  4. Schema Directive Array Argument
  5. Schema Directive with Type Arguments
  6. Schema Directive Optional Arguments
  7. Multiple Schema Directives
  8. Schema Directive on a class with an interface
  9. Schema Directive on an interface

@Tyler-Gauch
Copy link
Author

If anyone has any ideas on why the pipeline keeps failing on composer dependencies that would be great thanks.

@simPod
Copy link
Collaborator

simPod commented Apr 10, 2020

@Tyler-Gauch try again (eg. rebase on current master). I have just rebased my PR and it's all green.

@vladar
Copy link
Member

vladar commented Apr 10, 2020

Thank you for the PR!

But have you seen a discussion in #552 ? It is a bit more complicated than just printing directives. Long story short, we are waiting when this is resolved in graphql-js first to adopt the same approach.

So sadly, it is unlikely that this PR will make it into the master. I suggest following graphql/graphql-js#2020 and as soon as it is resolved - we can port the solution.

@@ -1237,4 +1237,203 @@ interfaces: [__Type!]
EOT;
self::assertEquals($introspectionSchema, $output);
}

public function testPrintSchemaDirectiveNoArgs()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function testPrintSchemaDirectiveNoArgs()
public function testPrintSchemaDirectiveNoArgs() : void

self::assertEquals($exceptedSdl, $actual);
}

public function testPrintSchemaDirectiveWithStringArgs()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function testPrintSchemaDirectiveWithStringArgs()
public function testPrintSchemaDirectiveWithStringArgs() : void

self::assertEquals($exceptedSdl, $actual);
}

public function testPrintSchemaDirectiveWithNumberArgs()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function testPrintSchemaDirectiveWithNumberArgs()
public function testPrintSchemaDirectiveWithNumberArgs() : void

self::assertEquals($exceptedSdl, $actual);
}

public function testPrintSchemaDirectiveWithArrayArgs()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function testPrintSchemaDirectiveWithArrayArgs()
public function testPrintSchemaDirectiveWithArrayArgs() : void

$schema = BuildSchema::build($exceptedSdl);
$actual = $this->printForTest($schema);

self::assertEquals($exceptedSdl, $actual);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self::assertEquals($exceptedSdl, $actual);
self::assertSame($exceptedSdl, $actual);

self::assertEquals($exceptedSdl, $actual);
}

public function testPrintSchemaDirectiveOnInterface()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function testPrintSchemaDirectiveOnInterface()
public function testPrintSchemaDirectiveOnInterface() : void

$schema = BuildSchema::build($exceptedSdl);
$actual = $this->printForTest($schema);

self::assertEquals($exceptedSdl, $actual);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self::assertEquals($exceptedSdl, $actual);
self::assertSame($exceptedSdl, $actual);

return $count > 0 ? (' ' . implode(
' ',
array_map(
static function ($directive) : string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to add some type hint for $directive?

$count = count($directives);
}

return $count > 0 ? (' ' . implode(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd invert the condition so else is not hidden at the bottom: ' ' . $count === 0 ? '' : implode(

return '';
}

if ($type->astNode instanceof ObjectTypeDefinitionNode) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

First two codnitions can be merged

@simPod
Copy link
Collaborator

simPod commented Apr 10, 2020

@vladar Should we keep these PRs / issues open + add some label like waiting for reference impl or sth?

@vladar
Copy link
Member

vladar commented Apr 10, 2020

@simPod I think it will be a separate PR anyway, so yeah, let's close this.

@vladar vladar closed this Apr 10, 2020
@Tyler-Gauch
Copy link
Author

Ahhhhhh I had not seen those other issues, good to know, thanks!

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

Successfully merging this pull request may close these issues.

3 participants