feat(respect-graphqlname-input): GraphQLName Annotation for Input…

[agarwalaman786]

… Type Suffix Suppression

Issue Description
While migrating the capability (reviews) to the federated subgraph, it was noticed that in the older supergraph, one field InputLocalizedString was not in standard format, meaning it didn't have the "Input" suffix. However, while using the graphql-kotlin library, it was discovered that it forcefully appends the "Input" suffix in case of input types and making it as a InputLocalizedString, which is the breaking change for the end users/ customers.

Fix Description
In this fix, the idea is to respect the @GraphQLName annotation. Whatever name is provided for input types, it is expected not to modify it. If someone wants to explicitly append "input" suffix in input types, they can include it in the value of the annotation.

Is it the breaking change?
Yes, it could potentially cause a breaking change for clients who are using the @GraphQLName annotation, as it will always append "input" as a suffix. For them, when the schema is compiled, they will need to update the codebase to explicitly append "Input" as a suffix in the @GraphQLName annotation.

[dariuszkuc]

Note that this might potentially cause issues when @GraphQLName is applied on a class that is used for both input and output as their names will no longer be unique. Maybe alternative way would be to provide @GraphQLInputName or make the original annotation repeatable and accept some enum value whether it is applicable to input/output/both.

[agarwalaman786]

Note that this might potentially cause issues when @GraphQLName is applied on a class that is used for both input and output as their names will no longer be unique. Maybe alternative way would be to provide @GraphQLInputName or make the original annotation repeatable and accept some enum value whether it is applicable to input/output/both.

Thanks for your input dariuszkuc, If I have understood your suggestion correctly, is it like we don't append output as a suffix in case of output types?

And use of this @GraphQLInputName would be to to have this current logic? If yes, I am liking this approach because if modify the @GraphQLName and when the client would move onto the new version it may potentially be multiple changes for them and @GraphQLInputName would be the independent change.

Kindly help with my understanding and I can proceed accordingly.

[agarwalaman786]

Note that this might potentially cause issues when @GraphQLName is applied on a class that is used for both input and output as their names will no longer be unique. Maybe alternative way would be to provide @GraphQLInputName or make the original annotation repeatable and accept some enum value whether it is applicable to input/output/both.

Thanks for your input dariuszkuc, If I have understood your suggestion correctly, is it like we don't append output as a suffix in case of output types?

And use of this @GraphQLInputName would be to to have this current logic? If yes, I am liking this approach because if modify the @GraphQLName and when the client would move onto the new version it may potentially be multiple changes for them and @GraphQLInputName would be the independent change.

Kindly help with my understanding and I can proceed accordingly.

I took a look into this further that how we can actually support @GraphQLInputName and this is the class where that logic is to generateInputObject
https://github.com/ExpediaGroup/graphql-kotlin/blob/master/generator/graphql-kotlin-schema-generator/src/main/kotlin/com/expediagroup/graphql/generator/internal/types/generateInputObject.kt#L36

If we introduce @GraphQLInputName annotation, If we directly replace getSimpleName to getInputClassSimpleName (function which we would introduce) it could be breaking change and may force all the clients to add the GraphQLInputName annotation. I was thinking if by having if check in this generateInputObject.kt class based on annotation we can support both? If client is using GraphQLInputName then it won't append the input suffix but if using GraphQLName it would append the Input as suffix thoughts?

[samuelAndalon]

we had an internal discussion about this PR and we believe that keeping GraphQLName annotation would be the best and easiest way to deal with this (no breaking change).

Add a second argument of type enum to the GraphQLName annotation

@Target(AnnotationTarget.CLASS, AnnotationTarget.VALUE_PARAMETER, AnnotationTarget.PROPERTY, AnnotationTarget.FUNCTION)
annotation class GraphQLName(val value: String, val target: GraphQLNameTarget = GraphQLNameTarget.BOTH)

enum class GraphQLNameTarget {
    INPUT,
    OUTPUT,
    BOTH
}


@GraphQLName(value = "YourType", target = GraphQLName.INPUT)
@GraphQLName(value = "YourType", target = GraphQLName.OUTPUT)
@GraphQLName(value = "YourType", target = GraphQLName.BOTH) // default

[agarwalaman786]

we had an internal discussion about this PR and we believe that keeping GraphQLName annotation would be the best and easiest way to deal with this (no breaking change).

Add a second argument of type enum to the GraphQLName annotation

@Target(AnnotationTarget.CLASS, AnnotationTarget.VALUE_PARAMETER, AnnotationTarget.PROPERTY, AnnotationTarget.FUNCTION)
annotation class GraphQLName(val value: String, val target: GraphQLNameTarget = GraphQLNameTarget.BOTH)

enum class GraphQLNameTarget {
    INPUT,
    OUTPUT,
    BOTH
}


@GraphQLName(value = "YourType", target = GraphQLName.INPUT)
@GraphQLName(value = "YourType", target = GraphQLName.OUTPUT)
@GraphQLName(value = "YourType", target = GraphQLName.BOTH) // default

Thanks Samuel, for your input. One thing I've noticed is that currently in our enum, we're using the parameter OUTPUT. However, I believe we're not updating the output class accordingly. We don't need to modify the name of the output class because it's managed automatically by the @GraphQLName annotation. This annotation doesn't append any suffix. Are you suggesting something on output as well?

[dariuszkuc]

After thinking about it a little bit more I think there is actually much simpler solution. We already have a mechanism to restrict types to input/output only -> https://opensource.expediagroup.com/graphql-kotlin/docs/8.x.x/schema-generator/customizing-schemas/restricting-input-output so lets use that

@Throws(CouldNotGetNameOfKClassException::class)
internal fun KClass<*>.getSimpleName(isInputClass: Boolean = false): String {
    val isInputOnlyLocation = this.findAnnotation<GraphQLValidObjectLocations>().let { it != null && it.locations.size == 1 && it.locations.contains(GraphQLValidObjectLocations.Locations.INPUT_OBJECT) }

    val name = this.getGraphQLName()
        ?: this.simpleName
        ?: throw CouldNotGetNameOfKClassException(this)

    return when {
        isInputClass -> if (name.endsWith(INPUT_SUFFIX, true) || isInputOnlyLocation) name else "$name$INPUT_SUFFIX"
        else -> name
    }
}

So your use case would be solved by applying valid location on types, i.e.

@GraphQLValidObjectLocations([Locations.INPUT_OBJECT])
@GraphQLName("FooBar")
class Foo {
  // whatever
}

Can you also update the https://github.com/ExpediaGroup/graphql-kotlin/blob/master/website/docs/schema-generator/customizing-schemas/renaming-fields.md with details about the input type naming behavior.

[agarwalaman786]

After thinking about it a little bit more I think there is actually much simpler solution. We already have a mechanism to restrict types to input/output only -> https://opensource.expediagroup.com/graphql-kotlin/docs/8.x.x/schema-generator/customizing-schemas/restricting-input-output so lets use that

@Throws(CouldNotGetNameOfKClassException::class)
internal fun KClass<*>.getSimpleName(isInputClass: Boolean = false): String {
    val isInputOnlyLocation = this.findAnnotation<GraphQLValidObjectLocations>().let { it != null && it.locations.size == 1 && it.locations.contains(GraphQLValidObjectLocations.Locations.INPUT_OBJECT) }

    val name = this.getGraphQLName()
        ?: this.simpleName
        ?: throw CouldNotGetNameOfKClassException(this)

    return when {
        isInputClass -> if (name.endsWith(INPUT_SUFFIX, true) || isInputOnlyLocation) name else "$name$INPUT_SUFFIX"
        else -> name
    }
}

So your use case would be solved by applying valid location on types, i.e.

@GraphQLValidObjectLocations([Locations.INPUT_OBJECT])
@GraphQLName("FooBar")
class Foo {
  // whatever
}

Can you also update the https://github.com/ExpediaGroup/graphql-kotlin/blob/master/website/docs/schema-generator/customizing-schemas/renaming-fields.md with details about the input type naming behavior.

I gave it a try but as per my understanding for all the input classes only getSimpleName method is being applied which is having the logic to forcefully put the input suffix in input class and I feel when we will use @GraphQLValidObjectLocations([Locations.INPUT_OBJECT]) it's still mentioning that it's the input class so my logic of suffix will execute.

For instance:
This is how I have created my data class

~~@GraphQLValidObjectLocations([GraphQLValidObjectLocations.Locations.INPUT_OBJECT])~~
~~@GraphQLName("InputLocalizedString")~~
~~data class InputLocalizedString(val locale: String, val value: String)~~

But my schema is still giving me this output

~~input InputLocalizedStringInput {~~
  ~~locale: String!~~
  ~~value: String!~~
~~}~~

Where as my expected output is
input InputLocalizedString {locale: String! value: String!}

There is one existing communication, where you investigated this and found out that Input suffix is always being appended.

Could you please help to know if I am missing something?

Sure, I can update this doc but before that I am thinking to close on this PR so that I can link the new documentation if require or if we don't have any existing way.~~

[agarwalaman786]

After thinking about it a little bit more I think there is actually much simpler solution. We already have a mechanism to restrict types to input/output only -> https://opensource.expediagroup.com/graphql-kotlin/docs/8.x.x/schema-generator/customizing-schemas/restricting-input-output so lets use that

@Throws(CouldNotGetNameOfKClassException::class)
internal fun KClass<*>.getSimpleName(isInputClass: Boolean = false): String {
    val isInputOnlyLocation = this.findAnnotation<GraphQLValidObjectLocations>().let { it != null && it.locations.size == 1 && it.locations.contains(GraphQLValidObjectLocations.Locations.INPUT_OBJECT) }

    val name = this.getGraphQLName()
        ?: this.simpleName
        ?: throw CouldNotGetNameOfKClassException(this)

    return when {
        isInputClass -> if (name.endsWith(INPUT_SUFFIX, true) || isInputOnlyLocation) name else "$name$INPUT_SUFFIX"
        else -> name
    }
}

So your use case would be solved by applying valid location on types, i.e.

@GraphQLValidObjectLocations([Locations.INPUT_OBJECT])
@GraphQLName("FooBar")
class Foo {
  // whatever
}

Can you also update the https://github.com/ExpediaGroup/graphql-kotlin/blob/master/website/docs/schema-generator/customizing-schemas/renaming-fields.md with details about the input type naming behavior.

Kindly, ignore my previous comment, I got the intention will modify the code accordingly.

[dariuszkuc]

This seems to be working fine?

[agarwalaman786]

This seems to be working fine?

Yeah, this change looks good but I was wondering since currently it accepts the list, if one mentions multiple Locations, like INPUT, OUTPUT, In that case it would simply ignore, I was just wondering like previously we discussed to have the better handling for OUTPUT as well, If we want to handle it.

[dariuszkuc]

Currently we validate the location position when generating input/output types -> by default we handle the type as if it is applicable on both locations. So it is not possible to have input only type and generate output type names. When applying custom renaming rules we preserve the names for output types but are appending Input suffix automatically to input types.

If your type is applicable for both input and output you need to have some unique discriminator there - it is not possible to use same type name for both input and output type. Location discriminator is the simplest way to allow users to skip the unnecessary suffixes.

There is another potential use case that we currently don't handle -> ability to provide custom input AND output type names. In order to support that we would need separate input/output name annotations OR some repeatable annotations with some flags to distinguish between input/output. IMHO that is not a common scenario and we probably shouldn't be supporting it at this point of time.

[agarwalaman786]

Currently we validate the location position when generating input/output types -> by default we handle the type as if it is applicable on both locations. So it is not possible to have input only type and generate output type names. When applying custom renaming rules we preserve the names for output types but are appending Input suffix automatically to input types.

If your type is applicable for both input and output you need to have some unique discriminator there - it is not possible to use same type name for both input and output type. Location discriminator is the simplest way to allow users to skip the unnecessary suffixes.

There is another potential use case that we currently don't handle -> ability to provide custom input AND output type names. In order to support that we would need separate input/output name annotations OR some repeatable annotations with some flags to distinguish between input/output. IMHO that is not a common scenario and we probably shouldn't be supporting it at this point of time.

makes sense, thanks for the explanation

[agarwalaman786]

This seems to be working fine?

I validated the changes which you have made and they are working fine.

[dariuszkuc]

Addressed in #1960