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

Old-style directives get lost, only new applied directives are present. #695

Closed
enriquedacostacambio opened this issue Sep 7, 2022 · 2 comments · Fixed by #708
Closed
Labels

Comments

@enriquedacostacambio
Copy link

Description

After the directive rework, directives are lost in favor of applied directives, this causes a problem with other tools that have not migrated to applied directives and are using the deprecated but still valid plain directives (such as graphql-java-extended-validation)

Imo both should be kept for compatibility purposes here: https://github.com/graphql-java-kickstart/graphql-java-tools/pull/663/files#diff-7817f33fe28a145f0c7b6bdcbbaea4cd6e7a9bed458b38db8abbab214ef76fddL169

Expected behavior

Both old directives and applied directives are kept in field definitions (and other objects).

Actual behavior

directives are lost, only applied ones are present.

Steps to reproduce the bug

package test;

import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.junit.Assert;
import org.junit.Test;

import graphql.ExecutionInput;
import graphql.GraphQL;
import graphql.kickstart.tools.GraphQLQueryResolver;
import graphql.kickstart.tools.SchemaParser;
import graphql.kickstart.tools.SchemaParserBuilder;
import graphql.schema.DataFetchingEnvironment;
import graphql.schema.GraphQLSchema;

public class IssueTest {
    static class MockQueryResolver implements GraphQLQueryResolver {
        public String mockField(DataFetchingEnvironment environment) {
            // This passes:
            Assert.assertNotNull(environment.getFieldDefinition().getAppliedDirective("mockDirective"));
            // This fails:
            Assert.assertNotNull(environment.getFieldDefinition().getDirective("mockDirective"));
            return null;
        }
    };
    
    @Test
    public void test() {
        GraphQL graphQl = graphQl(
            "directive @mockDirective on FIELD_DEFINITION",
            "type Query {",
            "    mockField: String @mockDirective",
            "}"
        );
        ExecutionInput executionInput = ExecutionInput.newExecutionInput()
            .query("{ mockField }")
            .build();
        graphQl.execute(executionInput);
    }
    
    private GraphQL graphQl(String... sdl) {
        SchemaParserBuilder builder = new SchemaParserBuilder();
        builder.schemaString(Stream.of(sdl).collect(Collectors.joining("\n")));
        SchemaParser schemaParser = builder
            .resolvers(new MockQueryResolver())
            .build();
        GraphQLSchema schema = schemaParser.makeExecutableSchema();
        return GraphQL.newGraphQL(schema).build();
    }
}
@oryan-block
Copy link
Collaborator

@enriquedacostacambio are you able to test #708 before I merge it to make sure it solves your problem?

@enriquedacostacambio
Copy link
Author

Yup, #708 solves this issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants