put shared renamed types in unions behind a hint #541
Conversation
|
|
||
| ) { | ||
| override fun toString(): String { | ||
| return "Service{name='$name'}" |
There was a problem hiding this comment.
this is just so we don't see stuff like Service@323edf
| val typeName: String = if ( | ||
| !executionContext.hints.sharedTypeRenames(service) || | ||
| overallField.objectTypeNames.contains(overallTypeName) | ||
| ) { |
There was a problem hiding this comment.
so, if hints are off, we will always use the old behaviour, just get the overallTypeName
| overallTypeName | ||
| } else { | ||
| overallField.objectTypeNames.single { |
There was a problem hiding this comment.
that single blew up here during the rollout, handling it safer by defaulting to overallTypeName
| } | ||
| } else { | ||
| objectTypeNames.map { | ||
| executionBlueprint.getUnderlyingTypeName(service, overallTypeName = it) |
There was a problem hiding this comment.
ok, so if something is renamed but the type is declared and renamed in another service
executionBlueprint.getUnderlyingTypeName(service, overallTypeName = it) will not actually get you the underlying name as it's only looking at the types owned by service. this thing executionBlueprint.getUnderlyingTypeName(overallTypeName = it) looks at the whole overall schema tho, for every rename, there can only be 1 underlying name and 1 overall name, so it's always 1 to 1 mapping
lib/src/main/java/graphql/nadel/engine/transform/NadelTypeRenameResultTransform.kt
Outdated
Show resolved
Hide resolved
| executionBlueprint.getUnderlyingTypeName(service, overallTypeName = it) | ||
| return if (executionContext.hints.sharedTypeRenames(service)) { | ||
| objectTypeNames.map { | ||
| executionBlueprint.getUnderlyingTypeName(overallTypeName = it) |
There was a problem hiding this comment.
This… doesn't feel right.
Sorry I haven't really been following this work, but renames are technically supposed to be per service.
Tbh I don't really remember the full story, but a long time ago I wanted to implement repeated @renamed with a service argument.
| @@ -168,8 +168,14 @@ class NadelQueryTransformer private constructor( | |||
| } | |||
|
|
|||
| private fun getUnderlyingTypeNames(objectTypeNames: Collection<String>): List<String> { | |||
| return objectTypeNames.map { | |||
| executionBlueprint.getUnderlyingTypeName(service, overallTypeName = it) | |||
| return if (executionContext.hints.sharedTypeRenames(service)) { | |||
There was a problem hiding this comment.
this is the bit I missed the first time around. This is covered by the test where I added fragments that used renamed typenames
query {
nodes {
__typename
... on Issue {
id
}
... on JiraComment {
id
}
}
}I didn't realise that this thing runs every time before we send the query to the underlying
Please make sure you consider the following:
that's a follow up on #537
I put all changes that were done there behind a hint so we can have a safer rollout
also in that PR I missed one branch, fixed that and covered with a test