Skip to content

Add repeatable directives#1915

Closed
dugenkui03 wants to merge 2 commits intographql-java:masterfrom
dugenkui03:add_repeatable_directives
Closed

Add repeatable directives#1915
dugenkui03 wants to merge 2 commits intographql-java:masterfrom
dugenkui03:add_repeatable_directives

Conversation

@dugenkui03
Copy link
Copy Markdown
Contributor

@dugenkui03 dugenkui03 commented May 21, 2020

#1763

  1. Add repeatable directives;

  2. change the UniqueDirectiveNamesPerLocation to ignore repeatable directive;

  3. change the Introspection to follow the spec:

type __Directive {
  name: String!
  description: String
  locations: [__DirectiveLocation!]!
  args: [__InputValue!]!
  isRepeatable: Boolean!
}

@bbakerman bbakerman requested review from andimarek and bbakerman and removed request for andimarek May 21, 2020 23:53
bbakerman
bbakerman reviewed May 21, 2020
View reviewed changes
Comment thread src/main/java/graphql/introspection/Introspection.java
.field(newFieldDefinition()
.name("name")
.type(GraphQLString))
.type(nonNull(GraphQLString)))
Copy link
Copy Markdown
Member

@bbakerman bbakerman May 21, 2020

Choose a reason for hiding this comment

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

nice catch

bbakerman
bbakerman reviewed May 21, 2020
View reviewed changes
Comment thread src/main/java/graphql/validation/rules/UniqueDirectiveNamesPerLocation.java
String directiveName = directive.getName();
GraphQLDirective graphQLDirective = getValidationContext().getSchema().getDirective(directiveName);

if (graphQLDirective == null) continue;
Copy link
Copy Markdown
Member

@bbakerman bbakerman May 21, 2020

Choose a reason for hiding this comment

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

please use fully formed if statements

if (graphQLDirective == null) {
    continue;
}

more lines : yes - more readable : also yes

Copy link
Copy Markdown
Member

@andimarek andimarek May 22, 2020

Choose a reason for hiding this comment

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

by using the GraphQL java code formatter it will be automatically handled.

bbakerman
bbakerman reviewed May 21, 2020
View reviewed changes
Comment thread src/main/java/graphql/validation/rules/UniqueDirectiveNamesPerLocation.java
GraphQLDirective graphQLDirective = getValidationContext().getSchema().getDirective(directiveName);

if (graphQLDirective == null) continue;
if (graphQLDirective.getDefinition() != null && graphQLDirective.getDefinition().isRepeatable()) continue;
Copy link
Copy Markdown
Member

@bbakerman bbakerman May 21, 2020

Choose a reason for hiding this comment

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

I think we will want to transfer the AST isRepeatable into the runtime GraphQLDirective class.

That is inside SchemaGenerator related code we transfer these booleans over

The AST elements are for reading SDL - the GraphqlXXX class are aimed as the runtime equivalents with more logic in them and more state rules.

Copy link
Copy Markdown
Contributor Author

@dugenkui03 dugenkui03 May 22, 2020

Choose a reason for hiding this comment

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

thanks for your guidance

@bbakerman
Copy link
Copy Markdown
Member

bbakerman commented May 22, 2020

This is start to the repeatable directives. However it only handles "queries" and not the type system

Its valid to do this

   type Foo {
     secureField : String @securedGroup(name : "admins")  @securedGroup(name : "execs")
   }

So we would need repeatable directives to go into the GraphqlXXX types as well.

The challenge here is that while they expose <List> getDirectives they have a map on the builder

private final Map<String, GraphQLDirective> directives = new LinkedHashMap<>();

These would need to be fixed up - thank good we made it a list and not a map to the outside world

phew

@bbakerman bbakerman mentioned this pull request May 22, 2020
Closed
@bbakerman
Copy link
Copy Markdown
Member

bbakerman commented May 22, 2020

Thanks for this work. In order to collaborate on this (where we can add code and so could you) I am going to close this PR and use a long lived branch

See #1916

branch : support_repeatable_directives

Please make any future PRs against that branch and we can collaboratively get this up

@bbakerman bbakerman closed this May 22, 2020
@bbakerman
Copy link
Copy Markdown
Member

bbakerman commented May 22, 2020

Again this code so far is pretty good and a great start - its just that it need to be the full support to make it into the master and we may work on improving that as well outselves

@dugenkui03
Copy link
Copy Markdown
Contributor Author

dugenkui03 commented May 22, 2020

Again this code so far is pretty good and a great start - its just that it need to be the full support to make it into the master and we may work on improving that as well outselves

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbakerman bbakerman bbakerman left review comments

@andimarek andimarek andimarek left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dugenkui03 @bbakerman @andimarek