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

Add repeatable directives #1915

Closed

Conversation

dugenkui03
Copy link
Contributor

#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
@@ -418,16 +418,19 @@ private static String print(Object value, GraphQLInputType type) {
.name("__Directive")
.field(newFieldDefinition()
.name("name")
.type(GraphQLString))
.type(nonNull(GraphQLString)))
Copy link
Member

Choose a reason for hiding this comment

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

nice catch

String directiveName = directive.getName();
GraphQLDirective graphQLDirective = getValidationContext().getSchema().getDirective(directiveName);

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

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
Member

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.

GraphQLDirective graphQLDirective = getValidationContext().getSchema().getDirective(directiveName);

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

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
Contributor Author

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
Member

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
Copy link
Member

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
Member

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
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants