Add comprehensive directive support to DSL module

[kasbaker]

This PR adds GraphQL directive support to the DSL module, addressing the feature request in #479 where users requested the ability to use directives like @skip and @include with the DSL module.

New Features:

• DSLDirective class: Represents GraphQL directives with argument validation and AST generation
• DSLDirectable mixin: Provides reusable .directives() method for all DSL elements that support directives
• DSLFragmentSpread class: Represents fragment spreads with their own directives, separate from fragment definitions
• Executable directive location support on query, mutation, subscription, fields, fragments, inline fragments, fragment spreads, and variable definitions (spec)
• Automatic schema resolution: Fields automatically use their parent schema for custom directive validation
• Fallback on builtin directives: Built-in directives are still available if a schema is not available to validate against

The implementation follows the October 2021 GraphQL specification for executable directive locations and maintains backward compatibility with existing DSL code. Users can now use both built-in directives (@skip, @include) and custom schema directives across all supported GraphQL locations.

Additional Changes:

• Added tmp_path_as_home fixture so that AWS Appsync tests in test_cli.py don't try to use the real ~/.aws directory

Closes #479

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (7fb869a) to head (0edd164).
⚠️ Report is 37 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##            master      #563    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           38        40     +2     
  Lines         2908      3231   +323     
==========================================
+ Hits          2908      3231   +323     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[leszekhanusz]

That's really great, many thanks!

Please check the few commits I made.

Set ast_field in init in DSLFragment and DSLFragmentSpread:
For this one, I modified this for consistency and to fix the mypy ignore. I don't really remember why we allowed to change a Fragment name after it had been created (there is a test for it), maybe we should deprecate this (I don't see the point).

I also simplified the inheritance hierarchy. Please check the diagram and tell me if this is ok.

Now, if you could please:

• put the tmp_path_as_home fixture in another PR and explain a bit what the issue is (I don't use aws much)
• add this feature to the docs (docs/advanced/dsl_module.rst)

[kasbaker]

That's really great, many thanks!

Please check the few commits I made.

Set ast_field in init in DSLFragment and DSLFragmentSpread: For this one, I modified this for consistency and to fix the mypy ignore. I don't really remember why we allowed to change a Fragment name after it had been created (there is a test for it), maybe we should deprecate this (I don't see the point).

I also simplified the inheritance hierarchy. Please check the diagram and tell me if this is ok.

Now, if you could please:

• put the tmp_path_as_home fixture in another PR and explain a bit what the issue is (I don't use aws much)
• add this feature to the docs (docs/advanced/dsl_module.rst)

@leszekhanusz, I made a few commits for the following:

• Moved the tmp_path_as_home bit to a different branch (I'll open a PR for this later)
• Added a custom directive for variable definitions and a note about how variable definitions need to be static
• Updated docs/advanced/dsl_module.rst with directive usage

A couple more notes:

When I was updating the test, I saw that if you use the DSL to add a variable as a directive argument on a variable definition it is possible to serialize a string with print_ast that cannot be parsed into a GraphQLRequest, and instead
raises GraphQLSyntaxError(self._lexer.source, variable_token.start, f"Unexpected variable '${var_name}' in constant value."). It seems like we should do some error handling in the DSL module to stop users from being able to serialize invalid GraphQL, but it seemed more effort than it was worth because graphql-core does stop us from actually generating a request. There is a note on line 1377-8 of tests/test_dsl.py related to this issue.

I also feel like it is a bit awkward to need to specify schema=ds for directives some of the time, but not always. Unfortunately, GraphQL Directives and GraphQL Types can share the same names, so building them off the schema the way DSLType does with DSLSchema.__getattr__ would not work. However, I think there may be a way to do something magic with DSLSchema.__call__ to get the directives to inherit the schema in a similar way to the way types do. Maybe something like this?

class DSLSchema:
    def __init__(self, schema: GraphQLSchema):
        self._schema: GraphQLSchema = schema
        self._get_def = self._schema.get_type

    def __call__(self, get_directive: bool: = False) -> "DSLSchema":
        if get_directive:
            self._get_def = self._schema.get_directive
        else:
            self._get_def = self._schema.get_type

    def __getattr__(self, name: str) -> Union["DSLType", "DSLDirective"]:
        ast_def = self._get_def(name)
        ... # validate and construct the obj
        self._get_def = self._schema.get_type  # reset to default behavior
        return obj

This seems kind of complicated and might be confusing to use, so I'm not sure if you feel like it's a good idea or not.

[leszekhanusz]

What about using another class for Schema directives (named DSLSchemaDirective):

You'll use it like this:

ds = DSLSchema(StarWarsSchema)
di = DSLSchemaDirective(StarWarsSchema)

query = DSLQuery(ds.Query.hero.select(ds.Character.name)).directives(
    di.customDirective
)

[kasbaker]

What about using another class for Schema directives (named DSLSchemaDirective):

You'll use it like this:

ds = DSLSchema(StarWarsSchema)
di = DSLSchemaDirective(StarWarsSchema)

query = DSLQuery(ds.Query.hero.select(ds.Character.name)).directives(
    di.customDirective
)

Yes, that would work and I thought about that option too. Since directives aren’t super common and executable ones are mostly used on fields it seems simpler to just add it as an argument than deal with another class.

I think it’s good to discuss this now before the interface is locked in and I’m open to making changes if you feel strongly about them.

[leszekhanusz]

Well, I agreed with you when you said that it is a bit awkward to need to specify schema=ds for schema directives some of the time, but not always, so I feel that this latest option seems better.

It is a bit more consistent with how the dsl code is currently working. Using attributes of DSLSchemaDirective for all directives from the schema and using DSLDirective directly for builtin directives is a bit similar to using attributes of DSLSchema for schema types and fields, and using DSLMetaField for buildin fields. I feel there is less code-smell this way.

[kasbaker]

Yes, I agree that the need to specify schema=ds is a code-smell. I am thinking that DSLSchema currently behaves like a factory for DSLType using the __getattr__ method. Basically, what we want is another factory that constructs DSLDirective instead of DSLType.

I would like to make a change and here are a few details I am considering:

1. Should DSLSchemaDirective be a completely separate class from DSLSchema?

class DSLSchemaDirective:
    def __init__(self, schema: GraphQLschema):
        ... # same as `DSLSchema.__init__`, OR if we want to support 
        # builtin directives without schema, then schema could be optional

    def __getattr__(name: str) -> "DSLDirective":
        ... # similar to `DSLSchema.__getattr__`, but constructs directives instead

• Pros: Simplest solution, no issues with complex types
• Cons: Duplicated code due to similarity to DSLSchema

2. Should DSLSchemaDirective inherit from DSLSchema?

class DSLSchemaDirective(DSLSchema):
    def __getattr__(name: str) -> "DSLDirective":
        ... # override behavior to get directive_def and construct DSLDirective

• Pros: Less repeated code, don't need to re-define DSLSchema and its __init__
• Cons: Overriding __getattr__ violates Liskov Substitution Principle

3. Should DSLSchema.__init__ take a default argument changing the type of factory it is?

class FactoryType(StrEnum):  # need to install backport for python < 3.11
    TYPE: "TYPE":
    DIRECTIVE: "DIRECTIVE" 

class DSLSchemaDirective(DSLSchema):
    def __init__(self, schema: GraphQLSchema, factory_type: FactoryType = FactoryType.TYPE):
        ... # set attributes to change `__getattr__` behavior
    def __getattr__(name: str) -> Union["DSLType", "DSLDirective"]:
        ... # different behavior depending on factory type enum

• Pros: Less repeated code, don't need to re-define __init__ or __getattr__
• Cons: Changing __getattr__ to return Union["DSLType", "DSLDirective"] will make mypy complain

4. Should we extend DSLSchema so that DSLSchema.__call__("@") returns DSLSchemaDirective?

class DSLSchemaDirective:
    def __init__(self, schema: GraphQLschema | None = None):
        ... # builtin directives without schema, 

    def __getattr__(name: str) -> "DSLDirective":
        ... # similar to `DSLSchema.__getattr__`, but constructs directives instead

class DSLSchema:
    @methodtools.lru_cache(maxsize=1)
    def __call__(directive: Literal["@"]) -> "DSLSchemaDirective":
        return DSLSchemaDirective(schema=self._schema)

# usage:
ds = DSLSchema(schema)
query = DSLQuery(ds.Query.hero.select(ds.Character.name)).directives(
    ds("@").customDirective
)

• Pros: Looks like GraphQL because we use "@", no type issues, no extra imports
• Cons: Feels like magic

I am leaning towards option 4 because it includes the simplicity of option 1, but with some syntactic sugar on top of it so users don't need to create two separate schema objects.

[kasbaker]

If you are open to extending DSLSchema with __call__, it could also be used to generate things like DSLMetaField, DSLInlineFragment and DSLFragment without needing the extra imports.

Examples:

ds = DSLSchema(schema)


# "__schema", "__type", "__typename" ->  DSLMetaField
ds.Query.hero.select(ds.Character.name, ds("__typename"))
"""
hero {
  name
  __typename
}
"""


# "..." -> DSLInlineFragment
ds.Query.hero.args(episode=6).select(
    ds.Character.name,
    ds("...").on(ds.Droid).select(ds.Droid.primaryFunction),
    ds("...").on(ds.Human).select(ds.Human.homePlanet),
)
"""
hero(episode: JEDI) {
  name
  ... on Droid {
    primaryFunction
  }
  ... on Human {
    homePlanet
  }
}
"""

# "fragment" -> DSLFragment
name_and_appearances = (
    ds("fragment").NameAndAppearances.on(ds.Character)
    .select(ds.Character.name, ds.Character.appearsIn)
)
ds.Query.hero.select(name_and_appearances)
"""
fragment NameAndAppearances on Character {
  name
  appearsIn
}

{
  hero {
    ...NameAndAppearances
  }
}
"""

# "@" -> DSLDirective
ds.Query.hero.select(ds.Character.name)).directives(
    ds("@").customDirective(value="foo)
)
"""
hero {
  name @customDirective(value: "foo")
}
"""

[leszekhanusz]

That's really interesting.

If we're going with the "magic" __call__ option, why not put the directive name directly in the call argument:

ds.Query.hero.select(ds.Character.name)).directives(
    ds("@customDirective")(value="foo)
)

Then the DSLSchemaDirective class is not needed anymore.

ds("...") and ds("__typename") are nice, but I don't really like ds("fragment").NameAndAppearances, because it introduces another get_attr type of method which is not really nice with reserved Python keywords.

Why not then use instead use ds("fragment NameAndAppearances")

Then the __call__ method:

• will return a DSLDirective if starting with "@"
• will return a DSLInlineFragment if "..."
• will return a DSLFragment if it starts with "fragment "
• will return a DSLMetaField if it is __typename and other meta fields.

[leszekhanusz]

After reflection, having the __call__ method returning a union of multiple types might cause mypy errors because mypy does not know exactly what type is returned and will complain.

We could use overloads to indicate which type is returned in which case, but with the proposed implementation above, there is no way to distinguish between fragments and directives (both accepting custom strings starting with "@" or "fragment ")

So I propose that for fragments, we use instead ds("fragment", "NameAndAppearances"), so we have "fragment" here as a Literal which can be used in the overloaded method.

The __call__ method of DSLSchema will then look something like this:

    @overload                                                                                                           
    def __call__(                                                                                                       
        self,                                                                                                           
        shortcut: Literal["__typename", "__schema", "__type"],                                                          
    ) -> DSLMetaField: ...  # pragma: no cover                                                                          
                                                                                                                        
    @overload                                                                                                           
    def __call__(                                                                                                       
        self,                                                                                                           
        shortcut: Literal["..."],                                                                                       
    ) -> DSLInlineFragment: ...  # pragma: no cover                                                                     
                                                                                                                        
    @overload                                                                                                           
    def __call__(                                                                                                       
        self,                                                                                                           
        shortcut: Literal["fragment"],                                                                                  
        name: str,                                                                                                      
    ) -> DSLFragment: ...  # pragma: no cover                                                                           
                                                                                                                        
    @overload                                                                                                           
    def __call__(                                                                                                       
        self,                                                                                                           
        shortcut: Any,                                                                                                  
    ) -> DSLDirective: ...  # pragma: no cover                                                                          
                                                                                                                        
    def __call__(                                                                                                       
        self, shortcut: str, name: Optional[str] = None                                                                 
    ) -> Union[DSLDirective, DSLFragment, DSLInlineFragment, DSLMetaField]:                                             
                                                                                                                        
        if shortcut[0] == "@":                                                                                          
            return DSLDirective(shortcut[1:], dsl_schema=self)                                                          
        elif shortcut.startswith("__"):                                                                                 
            return DSLMetaField(shortcut)                                                                               
        elif shortcut == "...":                                                                                         
            return DSLInlineFragment()                                                                                  
        elif shortcut == "fragment":                                                                                    
            return DSLFragment(name)                                                                                    
                                                                                                                        
        raise ValueError(f"Unsupported shortcut: {shortcut}")

[kasbaker]

@leszekhanusz I just pushed a refactor where I enabled DSLDirective to be created by DSLSchema.__call__("@someDirective") and updated the docs for it. The argument for __call__ is currently name, but I can change it to shortcut easily. Let's confirm what we want to do with the interface before I change it though.

I do see what you are saying about how mypy will treat the shortcut argument for __call__. It is easier to annotate so mypy will match a Literal["..."] or Literal["__type"], but in the case of "fragment someFragment" or "@someDirective" the type hint isn't so straightforward. Another way of thinking of it is that the inline fragment "..." doesn't fit the pattern because it doesn't have a name. For example, these overloads seem logical to me from a type analysis point of view:

    @overload
    def __call__(
        self,
        shortcut: Literal["__"],
        name: Literal["typename", "schema", "type"],
    ) -> DSLMetaField: ...  # pragma: no cover

    @overload
    def __call__(
        self,
        shortcut: Literal["..."],
    ) -> DSLInlineFragment: ...  # pragma: no cover

    @overload
    def __call__(
        self,
        shortcut: Literal["fragment"],
        name: str,
    ) -> DSLFragment: ...  # pragma: no cover

    @overload
    def __call__(
        self,
        shortcut: Literal["@"],
        name: str,
    ) -> DSLDirective: ...  # pragma: no cover

    def __call__(
        self, shortcut: str, name: str = ""
    ) -> Union[DSLDirective, DSLFragment, DSLInlineFragment, DSLMetaField]:
        if shortcut == "@":
            return DSLDirective(name, dsl_schema=self)
        elif shortcut.startswith("__"):
            return DSLMetaField(shortcut + name)
        elif shortcut == "...":
            return DSLInlineFragment()
        elif shortcut == "fragment":
            return DSLFragment(name)

        raise ValueError(f"Unsupported shortcut: {shortcut}")

However, this means that we have the slightly more awkward syntax of

metafield = ds("__", "typename")
directive = ds("@", "someDirective")

This one reason why I originally suggested defining __getattr__ on DSLDirective in a similar way to how it works with DSLType. However, I agree that it is simpler to just create the object directly in DSLSchema.__call__ than deal with the __getattr__ magic and name conflict stuff.

One thing to think about in terms of extension is that the shortcut argument could also be used for other graphql keywords that we want to validate with the schema. This could help with the issue of schema validation of nested types discussed here #355.

For example, instead of this:

good_query_dsl = dsl.dsl_gql(
    dsl.DSLQuery(
        ds.Query.continents(
            filter={
                'code': {'eq': 'AN'}
            }
        ).select(
            ds.Continent.code,
            ds.Continent.name,
        )
    )
)

We could instead allow this (or dynamically generate it):

good_query_dsl = dsl.dsl_gql(
    dsl.DSLQuery(
        ds.Query.continents(
            filter=ds("input", "ContinentFilterInput").args(
                code=ds("input", "StringQueryOperatorInput").args(eq="AN")
            )
        ).select(
            ds.Continent.code,
            ds.Continent.name,
        )
    )
)

Which is more verbose, but contains explicit GraphQL input type information that makes it simpler to validate nested arguments.

If we adopt a syntax like this, then the architecture follows a pattern where the existing syntax ds.__getattr__("someType") (ds.someType) is a shortcut for ds("type", "someType")

I think we need to balance convenience and simplicity. Please let me know what you think about this idea!

[kasbaker]

@leszekhanusz I think I fixed the mypy and and linter errors, but I haven't set up CI to run on my fork yet, so hopefully it passes. I also annotated DSLSchema.__call__ with the same overloads you suggested.

[leszekhanusz]

@kasbaker Thanks again for this great PR. I'll let you open another one for the syntaxic sugar __call__ method.

We'll continue the discussion there, but after reflection:

• I think we could remove the generation of DSLFragment from DSLSchema.__call__. Typing ds("fragment", "FragmentName") does not feel really easier than DSLFragment("FragmentName") and it introduces another argument just for that.
• regarding your ds("input") proposal for argument validation, I would instead prefer that the types are recursively inferred from the provided attributes if that's possible. I'll see if I can do something about this.