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

generator: support optional directive arguments #1830

Open
dariuszkuc opened this issue Aug 30, 2023 · 1 comment
Open

generator: support optional directive arguments #1830

dariuszkuc opened this issue Aug 30, 2023 · 1 comment
Labels
module: generator Issue affects the schema generator and federation code type: enhancement New feature or request

Comments

@dariuszkuc
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
Directives are defined as annotations which works great in many cases but we are also constrained by the JVM limitations. Currently JVM does not support null value for annotation attribute so it is not possible to define directive with nullable arguments by using annotation only approach. While we can create custom directive definition using willGenerateDirective hook, annotation arguments cannot be null so it is not possible to apply the annotation with nullable attributes.

We cannot use the existing OptionalInput approach either as JVM has constraints on annotation attributes - attributes can only be a primitive value, String, Enum, Class, Annotation or an array of any of the above. Furthermore, we cannot just recreate generic OptionalInput as an annotation because of A) lack of inheritance support in annotations and B) generic annotation attributes are not supported.

Describe the solution you'd like
Schema generator should provide support for creating and applying directives with nullable arguments. Ideally we wouldn't need any custom hooks to do it and everything would be derived from annotation attributes.

Describe alternatives you've considered

--- Alternative # 1 ---

We could potentially create specific wrappers for each supported attribute type (e.g. @OptionalStringArg(val value = "", val defined: Boolean = false)).

Drawbacks:

  • we would need to create wrappers for each possible type + their array representation
  • since generics are not supported we wouldn't be able to support enums nor custom annotations

--- Alternative # 2 ---

Since we can provide our own directive definition using hooks, we could potentially introduce other hooks to allow for filtering directive arguments , e.g.

// new hook
fun isValidDirectiveArgumentValue(directiveName: String, argumentName: String, value: Any?): Boolean = true

// example usage
override fun isValidDirectiveArgumentValue(directiveName: String, argumentName: String, value: Any?): Boolean {
    if (directiveName == LINK_DIRECTIVE_NAME && argumentName == "as" && value.toString().isBlank()) {
        return false
    }
    return super.isValidDirectiveArgumentValue(directiveName, argumentName, value)
}

Drawbacks:

  • still require manually providing custom directive definition in willGenerateDirective
  • directives could be renamed so passed directiveName would have to be the original one before the renames
  • you could potentially filter out required arguments leading to invalid applied directive in the schema (it would fail the validation so I guess it is fine?)

--- Alternative # 3 ---

OR we could create a hook to transform final applied directive (currently hooks only execute around generation of directive definitions), e.g.

// new hook
fun didGenerateAppliedDirective(directiveInfo: DirectiveMetaInformation, directive: GraphQLAppliedDirective): GraphQLAppliedDirective = directive

// example usage
override fun didGenerateAppliedDirective(directiveInfo: DirectiveMetaInformation, directive: GraphQLAppliedDirective): GraphQLAppliedDirective {
    if (directiveInfo.effectiveName == LINK_DIRECTIVE_NAME) {
        val asArg = directive.getArgument("as")
        if (asArg.getValue<String>().isEmpty()) {
            // `as` argument was not specified so we need to remove it from arguments
            // since there is no API to do it we have to manually remove ALL and re-add args
            return directive.transform {
                val urlArg = directive.getArgument("url")
                val importArg = directive.getArgument("import")

                it.clearArguments()
                it.argument(urlArg)
                it.argument(importArg)
            }
        }
    }
    return super.didGenerateAppliedDirective(directiveInfo, directive)
}

Drawbacks:

  • still require manually providing custom directive definition in willGenerateDirective
  • transformation logic is much more complex than the simple filtering in alternative # 2 (isValidDirectiveArgumentValue hook) but it is a very flexible approach

Additional context
Example use case from Apollo Federation spec - @link directive defines single required argument

directive @link(url: String!, as: String, import: [link__Import]) repeatable on SCHEMA

but we cannot apply the directive without providing values for all optional fields, i.e. we cannot represent the basic link when there are no imports and namespace is derived. Ideally we would like to be able to specify just

@link(url: "https://myspecs.dev/foo/1.0")

Since this is not possible, currently users will always have to provide ALL optional values

# `as` namespace should default to the spec name from the url
@link(url: "https://myspecs.dev/foo/1.0", as: "foo", imports: [])
@dariuszkuc dariuszkuc added the type: enhancement New feature or request label Aug 30, 2023
dariuszkuc added a commit to dariuszkuc/graphql-kotlin that referenced this issue Sep 11, 2023
Adds support for [Federation v2.5](https://www.apollographql.com/docs/federation/federation-versions#v25).

Adds new `willApplyDirective`/`didApplyDirective` hooks that can be used to customize transformation of directive definition to applied directive. JVM does not support nested arrays as annotation arguments so we need to apply custom hooks to generate valid `@requiresScopes` directive. New hooks can also be used to filter out default arguments (ExpediaGroup#1830).

New federation directives

- `@authenticated` - indicates that target element is only accessible to the authenticated supergraph users
- `@requiresScopes` - indicates that target element is only accessible to the authenticated supergraph users with the appropriate JWT scopes
dariuszkuc added a commit to dariuszkuc/graphql-kotlin that referenced this issue Sep 11, 2023
Adds support for [Federation v2.5](https://www.apollographql.com/docs/federation/federation-versions#v25).

Adds new `willApplyDirective`/`didApplyDirective` hooks that can be used to customize transformation of directive definition to applied directive. JVM does not support nested arrays as annotation arguments so we need to apply custom hooks to generate valid `@requiresScopes` directive. New hooks can also be used to filter out default arguments (ExpediaGroup#1830).

New federation directives

- `@authenticated` - indicates that target element is only accessible to the authenticated supergraph users
- `@requiresScopes` - indicates that target element is only accessible to the authenticated supergraph users with the appropriate JWT scopes
dariuszkuc added a commit to dariuszkuc/graphql-kotlin that referenced this issue Sep 11, 2023
Adds support for [Apollo Federation v2.5](https://www.apollographql.com/docs/federation/federation-versions#v25).

Adds new `willApplyDirective`/`didApplyDirective` hooks that can be used to customize transformation of directive definition to applied directive. JVM does not support nested arrays as annotation arguments so we need to apply custom hooks to generate valid `@requiresScopes` directive. New hooks can also be used to filter out default arguments (ExpediaGroup#1830).

New federation directives

- `@authenticated` - indicates that target element is only accessible to the authenticated supergraph users
- `@requiresScopes` - indicates that target element is only accessible to the authenticated supergraph users with the appropriate JWT scopes

Note: due to the potential conflict on directive names we will no longer auto import federation directives. New directives will be auto-namespaced to the target spec. For backwards compatibility, we will continue auto-importing directives up to Federation version 2.3.
samuelAndalon pushed a commit that referenced this issue Sep 12, 2023
### 📝 Description

Adds support for [Apollo Federation
v2.5](https://www.apollographql.com/docs/federation/federation-versions#v25).

Adds new `willApplyDirective`/`didApplyDirective` hooks that can be used
to customize transformation of directive definition to applied
directive. JVM does not support nested arrays as annotation arguments so
we need to apply custom hooks to generate valid `@requiresScopes`
directive. New hooks can also be used to filter out default arguments
(#1830).

New federation directives

- `@authenticated` - indicates that target element is only accessible to
the authenticated supergraph users
- `@requiresScopes` - indicates that target element is only accessible
to the authenticated supergraph users with the appropriate JWT scopes

Note: due to the potential conflict on directive names we will no longer
auto import federation directives. New directives will be
auto-namespaced to the target spec. For backwards compatibility, we will
continue auto-importing directives up to Federation version 2.3.

### 🔗 Related Issues
#1830
@dariuszkuc
Copy link
Collaborator Author

This is partially resolved by #1839. Keeping this issue open to track potential usability improvements in the future.

@dariuszkuc dariuszkuc added the module: generator Issue affects the schema generator and federation code label Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: generator Issue affects the schema generator and federation code type: enhancement New feature or request
Development

No branches or pull requests

1 participant