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

Support hooks to add custom documentation to objects, fields, arguments and enum values #231

Closed
liqweed opened this issue Jun 4, 2019 · 11 comments · Fixed by #232
Closed
Labels
type: enhancement New feature or request

Comments

@liqweed
Copy link

liqweed commented Jun 4, 2019

We came up with a documentation technique that works well for us (syncing both development teams and our product documentation team). We're trying to resolve the relevant Markdown documentation for each object, field, argument and enum value.

SchemaGeneratorHooks is an awesome general purpose vehicle for intercepting schema generation. I think it can be very helpful in this case, but it would have to be extended to include hook methods that would allow me to augment builder objects just before calling build(). That way I can call the builders' description method and add my own documentation. Specifically I'm asking for the inclusion of the following hooks:

fun willGenerateGraphQLObject(objectName: String, builder: graphql.schema.GraphQLObjectType.Builder): graphql.schema.GraphQLObjectType.Builder = builder

fun willGenerateGraphQLField(objectName: String, fieldName: String, builder: graphql.schema.GraphQLFieldDefinition.Builder): graphql.schema.GraphQLFieldDefinition.Builder = builder

fun willGenerateGraphQLEnumType(enumName: String, builder: graphql.schema.GraphQLEnumType.Builder): graphql.schema.GraphQLEnumType.Builder = builder

fun willGenerateGraphQLEnumValue(enumName: String, enumValue: String, builder: graphql.schema.GraphQLEnumValueDefinition.Builder): graphql.schema.GraphQLEnumValueDefinition.Builder = builder

fun willGenerateGraphQLArgument(objectName: String, fieldName: String, argumentName: String, builder: graphql.schema.GraphQLArgument.Builder): graphql.schema.GraphQLArgument.Builder = builder

I added the names of the objects/fields/arguments/enums to the hooks' contexts since it's difficult to extract that information from the builders directly.

What do you think?

Thanks a lot for a fantastic library!

@liqweed liqweed added the type: enhancement New feature or request label Jun 4, 2019
@dariuszkuc
Copy link
Collaborator

You can add custom documentation using @GraphQLDescription annotation that can be added to any type. See https://github.com/ExpediaDotCom/graphql-kotlin/wiki/Fields#documenting-fields for more details.

Is there some additional functionality needed that current annotation doesn't provide?

@liqweed
Copy link
Author

liqweed commented Jun 4, 2019

GraphQL supports Markdown schema documentation. Our technique is pretty basic - we maintain a directory with Markdown files. The documentation can be as rich as Markdown allows (we use paragraphs, links, tables etc). Within those files an h1 (# ...) demarcates a beginning of a type documentation. h2 (## ...) demarcates a beginning of a field documentation. The directory structure doesn't really matter so we can organize these files as we see fit. Several types can be documented in the same file. Our documentation team uses an external tool to author the files (mostly https://typora.io/).

Maintaining documentation by passing an argument to an annotation is disadvantageous because:

  • it's very limiting. It isn't convenient to read/write anything more than pretty short string values. We don't consider that adequate documentation for our API.
  • it's maintained as part of the code. While this may be perfectly fine for many cases we find it hinders collaboration with our documentation team.
  • There can be many GraphQL API endpoints implemented using different stacks but we would like the documentation for all of them to be maintained coherently using the same documentation method.

We generate another GraphQL schema for another use case not using graphql-kotlin using the documentation technique I described above successfully and we would like to continue to document all our APIs in similar way.

We would happily contribute the above mentioned documentation technique although it's very simple. It only involves scanning a directory for Markdown files, parsing them using https://github.com/vsch/flexmark-java and providing a resolution method for types and fields.

@smyrick
Copy link
Contributor

smyrick commented Jun 5, 2019

@liqweed I see why you would want this with your dynamic documentation.

We could add a method for every GraphQLType implementation but would it make more sense if we added a hook at the the same location as didGenerateGraphQLType? This is called right before we add the type to the schema but that is not helpful for you since we don't use the modified value if you were to change it in the hook.

If we added a new hook at the same location as didGenerateGraphQLType, we can have you modify the type by passing it into a builder again and the returning it.

/**
 * Called after using reflection to generate the graphql object type but before returning it to the schema.
 * This allows for modifying the type info, like description or directives
 */
fun willAddGraphQLTypeToSchema(type: KType, generatedType: GraphQLType): GraphQLType = generatedType

Your hooks can check all the possible types

override fun willAddGraphQLTypeToSchema(type: KType, generatedType: GraphQLType): GraphQLType {
    return when (generatedType) {
        is GraphQLObjectType -> GraphQLObjectType.Builder(generatedType).description("My custom description + old one: ${generatedType.description}").build()
        else -> generatedType
    }
}

smyrick pushed a commit to smyrick/graphql-kotlin that referenced this issue Jun 6, 2019
Fixes ExpediaGroup#231

Adds a new hook that can be used to modify all the GraphQLTypes that get generated from reflection. This allows for runtime documentation or any other changes that may want to be made to types after they are fully generated
@liqweed
Copy link
Author

liqweed commented Jun 6, 2019

To add description for fields I would also need the field's name at that point. The builders don't expose the added fields for inspection or manipulation.
If I understand correctly, according to your suggestion I should traverse all the information in the builder, including its fields, and create a copy based on that - right?

@rharriso
Copy link
Contributor

rharriso commented Jun 6, 2019

Are there use cases for mutating hooks besides adding markdown docs, could it be used to implement custom directives? #88 (comment)

@smyrick
Copy link
Contributor

smyrick commented Jun 6, 2019

@liqweed You can access the name of the generatedType since at this point in the hook, it is fully generated. You can also look at the info on the KType but that is not always the most accurate if you are using this like @GraphQLName to override the type name. You don't have to traverse any builder, but if you want to edit the type, you can pass it into a new builder and edit the fields that way to produce a new generated GraphQLType


@rharriso This hook can be used at this point to add any more info to the type, not just documentation. We already have a way to do custom directives, documented here: https://github.com/ExpediaDotCom/graphql-kotlin/wiki/Schema-Directives

The comment you are referring to is a way to expose a config method or setting to provide a list of pre-configured directives instead of using the onRewireGraphQLType. There are slightly different issues.

smyrick pushed a commit that referenced this issue Jun 13, 2019
Fixes #231

Adds a new hook that can be used to modify all the GraphQLTypes that get generated from reflection. This allows for runtime documentation or any other changes that may want to be made to types after they are fully generated
@liqweed
Copy link
Author

liqweed commented Jun 13, 2019

@smyrick To make sure I follow your comment about modifying fields using this new hook, do you mean like so?

class DocumentationSchemaGeneratorHooks : SchemaGeneratorHooks {
    override fun willAddGraphQLTypeToSchema(type: KType, generatedType: GraphQLType): GraphQLType {
        return when(generatedType) {
            is GraphQLObjectType -> {
                val builder = GraphQLObjectType.Builder(generatedType)
                builder.clearFields()
                generatedType.fieldDefinitions.forEach {
                    builder.field(GraphQLFieldDefinition.Builder(it).description(resolveFieldDocumentation(generatedType.name, it.name)))
                }
                builder.description(resolveTypeDocumentation(generatedType.name)).build()
            }
            else -> generatedType
        }
    }
}

@smyrick
Copy link
Contributor

smyrick commented Jun 13, 2019

@liqweed Yep that looks correct, I know it's a few lines to parse but with some helper functions you can reuse the code for all the types, not just object types.

If that doesn't work please feel free to create a new issue with this hook

@liqweed
Copy link
Author

liqweed commented Jun 15, 2019

@smyrick Thanks, I think that would do.

Thank you for such a quick handling of the issue!
Looking forward to seeing this in an upcoming release.

@liqweed
Copy link
Author

liqweed commented Jul 25, 2019

This mechanism works well for all GraphQLTypes except for GraphQLInterfaceTypes.

The following code:

    override fun willAddGraphQLTypeToSchema(type: KType, generatedType: GraphQLType): GraphQLType {
        return when (generatedType) {
            is GraphQLInterfaceType -> GraphQLInterfaceType.newInterface(generatedType)
                .description("My resolved description").build()
            else -> generatedType
        }
    }

Produces the following error:

graphql.AssertException: All types within a GraphQL schema must have unique names. No two provided types may have the same name.
No provided type may have a name which conflicts with any built in types (including Scalar and Introspection types).
You have redefined the type 'Result' from being a 'GraphQLInterfaceType' to a 'GraphQLInterfaceType'

	at graphql.schema.GraphQLTypeCollectingVisitor.assertTypeUniqueness(GraphQLTypeCollectingVisitor.java:105)
	at graphql.schema.GraphQLTypeCollectingVisitor.visitGraphQLInterfaceType(GraphQLTypeCollectingVisitor.java:58)
	at graphql.schema.GraphQLInterfaceType.accept(GraphQLInterfaceType.java:152)
	at graphql.schema.TypeTraverser$TraverserDelegateVisitor.enter(TypeTraverser.java:71)
	at graphql.util.Traverser.traverse(Traverser.java:147)
	at graphql.schema.TypeTraverser.doTraverse(TypeTraverser.java:58)
	at graphql.schema.TypeTraverser.depthFirst(TypeTraverser.java:50)
	at graphql.schema.TypeTraverser.depthFirst(TypeTraverser.java:38)
	at graphql.schema.SchemaUtil.allTypes(SchemaUtil.java:42)
	at graphql.schema.GraphQLSchema.<init>(GraphQLSchema.java:102)
	at graphql.schema.GraphQLSchema.<init>(GraphQLSchema.java:37)
	at graphql.schema.GraphQLSchema$Builder.build(GraphQLSchema.java:411)
	at com.expedia.graphql.generator.SchemaGenerator.generate$graphql_kotlin_schema_generator(SchemaGenerator.kt:68)
	at com.expedia.graphql.ToSchemaKt.toSchema(toSchema.kt:24)
	at com.expedia.graphql.ToSchemaKt.toSchema$default(toSchema.kt:21)

@smyrick
Copy link
Contributor

smyrick commented Jul 26, 2019

@liqweed These seems like a valid error, I have created a new issue which is open for anyone to work on #281

dariuszkuc pushed a commit to dariuszkuc/graphql-kotlin that referenced this issue Aug 5, 2022
Fixes ExpediaGroup#231

Adds a new hook that can be used to modify all the GraphQLTypes that get generated from reflection. This allows for runtime documentation or any other changes that may want to be made to types after they are fully generated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

4 participants