Skip to content

Comments

Consistent object type names without instruction#714

Draft
gnawf wants to merge 1 commit intomasterfrom
consistent-object-type-names-without-instruction
Draft

Consistent object type names without instruction#714
gnawf wants to merge 1 commit intomasterfrom
consistent-object-type-names-without-instruction

Conversation

@gnawf
Copy link
Collaborator

@gnawf gnawf commented Feb 17, 2026

While testing #713 I found this bug. See comments for details.

Fixes bug where objectTypeNames is empty for renames
Copy link
Collaborator Author

@gnawf gnawf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add a test, but src/main/ code changes should be done.


return NadelTransformFieldResult(
newField = objectTypesNoRenames
.takeIf(List<GraphQLObjectTypeName>::isNotEmpty)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dumb Kotlin-isms

val objectsWithoutRename = overallField.objectTypeNames
.asSequence()
.filterNot { it in renameInstructions }
.toHashSet()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for some reason in NadelRenameTransform the objectsWithoutRename is calculated inside isApplicable which leads to a bug when the ExecutableNormalizedField.objectTypeNames has been edited by another transform prior to NadelRenameTransform.transformField running

state: State,
transformServiceExecutionContext: NadelTransformServiceExecutionContext?,
): NadelTransformFieldResult {
val objectTypeNamesWithoutRename = field.objectTypeNames.filter { it !in state.instructionsByObjectTypeNames }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it here to be consistent with all other impls, this fixes the bug I found.

field: ExecutableNormalizedField,
executionBlueprint: NadelOverallExecutionBlueprint,
): List<ExecutableNormalizedField> {
val setOfFieldObjectTypeNames = field.objectTypeNames.toSet()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is stupid, field.objectTypeNames is already a Set<String>

So we're just cloning the Set here, and we don't even edit it etc, we just do Set.contains check

@github-actions
Copy link

github-actions bot commented Feb 17, 2026

Test Results

  615 files  +  615    615 suites  +615   1m 5s ⏱️ + 1m 5s
2 032 tests +2 032  1 508 ✅ +1 508  523 💤 +523  1 ❌ +1 
2 040 runs  +2 040  1 516 ✅ +1 516  523 💤 +523  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 720ee01. ± Comparison against base commit ac893ee.

♻️ This comment has been updated with latest results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant