mirumee / ariadne

Python library for implementing GraphQL servers using schema-first approach.
https://ariadnegraphql.org
BSD 3-Clause "New" or "Revised" License
2.21k stars 179 forks source link

SchemaDirectiveVisitor not called when extending objects. #344

Open wapiflapi opened 4 years ago

wapiflapi commented 4 years ago

It seems like custom directives do not work on extend type Object, this might be by design but I could not find anywhere in the Ariadne docs or GraphQL specs something that confirms this.

Here is a simple test case:

from ariadne import QueryType, gql, make_executable_schema, SchemaDirectiveVisitor
from ariadne.asgi import GraphQL

class TestDirective(SchemaDirectiveVisitor):
    def visit_object(self, object_):
        print(f"Visiting object {object_}")
        return object_

type_defs = gql("""
    directive @test on OBJECT

    type Query {
        foo: String!
    }

    extend type Query @test {
        bar: String!
    }
""")

query = QueryType()
schema = make_executable_schema(type_defs, query, directives={"test": TestDirective})
app = GraphQL(schema, debug=True)

Testing using uvicorn --debug test:app.

I expect Visiting object Query to be printed but it looks like the SchemaDirectiveVisitor is not called when extending objects. (But it works as expected when doing type Query @test.)

If this is not a bug, it would make sense to at least output a warning or error that using directives on extended objects is not supported.

Looking at the code (which I do not know well), I suspect that build_and_extend_schema(ast_document) is called before SchemaDirectiveVisitor.visit_schema_directives(schema, directives) and that it somehow loses the directives in the process of extending?

To be honest I expected the directives to be applied before the schema is extended, but that's just my gut feeling.

rafalp commented 4 years ago

Looks like build_and_extend_schema is not adding extension's type directives to extended type. Our schema extension logic that lives in build_and_extend_schema could be, well, extended to handle this.

wapiflapi commented 4 years ago

Question before I start looking into code for this,

directive @test on OBJECT

type Query {
    foo: String!
}

extend type Query @test {
    bar: String!
}

extend type Query @test {
    spam: String!
}

What do we expect in this case? Should the SchemaDirectiveVisitor be called twice for Query? Before or after it was extended twice? Should this be an error? called only once on the final object (after it has been extended)

I can't find a good answer for the expected behavior from the GraphQL specs, if anyone has an opinion / source regarding this that would be helpful.

rafalp commented 4 years ago

At situations like this the first question to be asked is what you would expect to happen here. Your opinion will be as good as anyone's :)

Personally I'm thinking that it shouldn't be applied more than once (because it does same thing), but this reasoning goes out of window when you introduce arguments to directives:

extend type Query @test(arg: "A") {
    bar: String!
}

extend type Query @test(arg: "B") {
    spam: String!
}

What should happen then? Perhaps directive should have access to GraphQL type containing only the fields defined on extend? But this sound like plenty of complexity for directives. And what about directives trying to modify type itself, eg. adding or removing type fields?

patrys commented 4 years ago

What Would Apollo Do?™

rafalp commented 4 years ago

@patrys we know that because our implementation is port of apollo-server's ;)