Skip to content

Commit

Permalink
Fix renamed shared input type bug (#584)
Browse files Browse the repository at this point in the history
* Add test for renamed shared input type bug

* Add failing test for bug

* Pick up shared type renames when only used as input type

* Update comment
  • Loading branch information
gnawf authored Aug 29, 2024
1 parent 41d1b44 commit e8d5674
Show file tree
Hide file tree
Showing 7 changed files with 449 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@ import graphql.language.FieldDefinition
import graphql.language.ImplementingTypeDefinition
import graphql.nadel.Service
import graphql.nadel.dsl.FieldMappingDefinition
import graphql.nadel.dsl.NadelHydrationDefinition
import graphql.nadel.dsl.RemoteArgumentSource
import graphql.nadel.dsl.TypeMappingDefinition
import graphql.nadel.dsl.NadelHydrationDefinition
import graphql.nadel.engine.blueprint.hydration.NadelBatchHydrationMatchStrategy
import graphql.nadel.engine.blueprint.hydration.NadelHydrationActorInputDef
import graphql.nadel.engine.blueprint.hydration.NadelHydrationActorInputDef.ValueSource.FieldResultValue
import graphql.nadel.engine.blueprint.hydration.NadelHydrationStrategy
import graphql.nadel.engine.blueprint.hydration.NadelHydrationCondition
import graphql.nadel.engine.blueprint.hydration.NadelHydrationStrategy
import graphql.nadel.engine.transform.query.NadelQueryPath
import graphql.nadel.engine.util.AnyImplementingTypeDefinition
import graphql.nadel.engine.util.AnyNamedNode
Expand Down Expand Up @@ -771,44 +771,68 @@ private class SharedTypesAnalysis(
overallParentType: AnyImplementingTypeDefinition,
underlyingParentType: GraphQLFieldsContainer,
): List<NadelTypeRenameInstruction> {
val overallOutputTypeName = overallField.type.unwrapAll().name

val underlyingField = getUnderlyingField(overallField, overallParentType, underlyingParentType)
?: return emptyList()

val renameInstruction = if (overallOutputTypeName !in serviceDefinedTypes) {
val overallOutputTypeName = overallField.type.unwrapAll().name
val underlyingOutputTypeName = underlyingField.type.unwrapAll().name
val outputTypeRenameInstruction = getTypeRenameInstructionOrNull(
overallTypeName = overallOutputTypeName,
underlyingTypeName = underlyingOutputTypeName,
serviceDefinedTypes = serviceDefinedTypes,
service = service,
)

val argumentTypeRenameInstructions = overallField.inputValueDefinitions
.mapNotNull { overallArgument ->
val underlyingArgument = underlyingField.getArgument(overallArgument.name)
getTypeRenameInstructionOrNull(
overallTypeName = overallArgument.type.unwrapAll().name,
underlyingTypeName = underlyingArgument.type.unwrapAll().name,
serviceDefinedTypes = serviceDefinedTypes,
service = service,
)
}

val overallOutputTypeDefinition = (engineSchema.getType(overallOutputTypeName) as? GraphQLFieldsContainer?)
?.definition as AnyImplementingTypeDefinition?

return listOfNotNull(outputTypeRenameInstruction) + argumentTypeRenameInstructions + (overallOutputTypeDefinition
?.let {
investigateTypeRenames(
visitedTypes,
service,
serviceDefinedTypes,
overallType = overallOutputTypeDefinition,
underlyingType = underlyingField.type.unwrapAll() as GraphQLFieldsContainer,
)
} ?: emptyList())
}

private fun getTypeRenameInstructionOrNull(
overallTypeName: String,
underlyingTypeName: String,
serviceDefinedTypes: Set<String>,
service: Service,
): NadelTypeRenameInstruction? {
return if (overallTypeName !in serviceDefinedTypes) {
// Service does not own type, it is shared
// If the name is different than the overall type, then we mark the rename
when (val underlyingOutputTypeName = underlyingField.type.unwrapAll().name) {
overallOutputTypeName -> null
when (underlyingTypeName) {
overallTypeName -> null
in scalarTypeNames -> null
else -> when (typeRenameInstructions[overallOutputTypeName]) {
else -> when (typeRenameInstructions[overallTypeName]) {
null -> error("Nadel does not allow implicit renames")
else -> NadelTypeRenameInstruction(
service,
overallName = overallOutputTypeName,
underlyingName = underlyingOutputTypeName,
overallName = overallTypeName,
underlyingName = underlyingTypeName,
)
}
}
} else {
null
}

val overallOutputType = engineSchema.getType(overallOutputTypeName)
// Ensure type exists, schema transformation can delete types, so let's just ignore it
.let { it ?: return emptyList() }
// Return if not field container
.let { it as? GraphQLFieldsContainer ?: return emptyList() }
.let { it.definition as AnyImplementingTypeDefinition }

return listOfNotNull(renameInstruction) + investigateTypeRenames(
visitedTypes,
service,
serviceDefinedTypes,
overallType = overallOutputType,
underlyingType = underlyingField.type.unwrapAll() as GraphQLFieldsContainer,
)
}

private fun getUnderlyingField(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,11 +333,11 @@ private fun transformFields(fieldDefinitions: List<FieldDefinition>): List<Field
}
.map { field ->
field.transform { fieldBuilder ->

fieldBuilder
.name(field.getUnderlyingName())
.directives(field.directives.filterNotNadelDirectives())
.type(field.type.getUnderlyingType())
.inputValueDefinitions(transformInputValueDefinitions(field.inputValueDefinitions))
}
}
.toList()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ private fun makeConstructorInvocationToExpectedServiceCall(call: TestExecutionCa
ExpectedServiceCall::query.name
add("%L = %S,\n", ExpectedServiceCall::service.name, call.service)
add("%L = %S,\n", ExpectedServiceCall::query.name, call.query.replaceIndent(" "))
add("%L = %S,\n", ExpectedServiceCall::variables.name, call.variables)
add("%L = %S,\n", ExpectedServiceCall::variables.name, writeResultJson(call.variables))
add("%L = %S,\n", ExpectedServiceCall::result.name, writeResultJson(call.result))

add("delayedResults = %M", listOfJsonStringsMember)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package graphql.nadel.tests.next.fixtures.rename

import graphql.nadel.NadelExecutionHints
import graphql.nadel.tests.next.NadelIntegrationTest

/**
* The `ConfluenceLegacyPathType` input type was renamed.
*
* In the test snapshot we ensure the variable is defined as `PathType`.
*/
class RenamedInputTypeTest : NadelIntegrationTest(
query = """
query {
me {
profilePicture {
path(type: ABSOLUTE)
}
}
}
""".trimIndent(),
services = listOf(
Service(
name = "confluence_legacy",
overallSchema = """
type Query {
me: ConfluenceLegacyUser
}
type ConfluenceLegacyUser @renamed(from: "User") {
profilePicture: ConfluenceLegacyProfilePicture
}
type ConfluenceLegacyProfilePicture @renamed(from: "ProfilePicture") {
path(type: ConfluenceLegacyPathType!): String
}
enum ConfluenceLegacyPathType @renamed(from: "PathType") {
ABSOLUTE
RELATIVE
}
""".trimIndent(),
runtimeWiring = { wiring ->
data class ProfilePicture(
val absolutePath: String,
val relativePath: String,
)

data class User(
val profilePicture: ProfilePicture,
)

wiring
.type("Query") { type ->
type
.dataFetcher("me") { env ->
User(
profilePicture = ProfilePicture(
relativePath = "/wiki/aa-avatar/5ee0a4ef55749e0ab6e0fb70",
absolutePath = "https://atlassian.net/wiki/aa-avatar/5ee0a4ef55749e0ab6e0fb70",
),
)
}
}
.type("ProfilePicture") { type ->
type
.dataFetcher("path") { env ->
val pfp = env.getSource<ProfilePicture>()!!
when (val urlType = env.getArgument<String>("type")) {
"ABSOLUTE" -> pfp.absolutePath
"RELATIVE" -> pfp.relativePath
else -> throw IllegalArgumentException(urlType)
}
}
}
},
),
),
) {
override fun makeExecutionHints(): NadelExecutionHints.Builder {
return super.makeExecutionHints()
// todo: this should be on by default
.allDocumentVariablesHint {
true
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// @formatter:off
package graphql.nadel.tests.next.fixtures.rename

import graphql.nadel.tests.next.ExpectedNadelResult
import graphql.nadel.tests.next.ExpectedServiceCall
import graphql.nadel.tests.next.TestSnapshot
import graphql.nadel.tests.next.listOfJsonStrings
import kotlin.Suppress
import kotlin.collections.List
import kotlin.collections.listOf

private suspend fun main() {
graphql.nadel.tests.next.update<RenamedInputTypeTest>()
}

/**
* This class is generated. Do NOT modify.
*
* Refer to [graphql.nadel.tests.next.UpdateTestSnapshots
*/
@Suppress("unused")
public class RenamedInputTypeTestSnapshot : TestSnapshot() {
override val calls: List<ExpectedServiceCall> = listOf(
ExpectedServiceCall(
service = "confluence_legacy",
query = """
| query (${'$'}v0: PathType!) {
| me {
| profilePicture {
| path(type: ${'$'}v0)
| }
| }
| }
""".trimMargin(),
variables = """
| {
| "v0": "ABSOLUTE"
| }
""".trimMargin(),
result = """
| {
| "data": {
| "me": {
| "profilePicture": {
| "path": "https://atlassian.net/wiki/aa-avatar/5ee0a4ef55749e0ab6e0fb70"
| }
| }
| }
| }
""".trimMargin(),
delayedResults = listOfJsonStrings(
),
),
)

/**
* ```json
* {
* "data": {
* "me": {
* "profilePicture": {
* "path": "https://atlassian.net/wiki/aa-avatar/5ee0a4ef55749e0ab6e0fb70"
* }
* }
* }
* }
* ```
*/
override val result: ExpectedNadelResult = ExpectedNadelResult(
result = """
| {
| "data": {
| "me": {
| "profilePicture": {
| "path": "https://atlassian.net/wiki/aa-avatar/5ee0a4ef55749e0ab6e0fb70"
| }
| }
| }
| }
""".trimMargin(),
delayedResults = listOfJsonStrings(
),
)
}
Loading

0 comments on commit e8d5674

Please sign in to comment.