Automatically escape type in enum values (#4295)

* automatically escape `type` in enum values

* fix Validation tests

* fix tests

apollo-compiler/api/apollo-compiler.api

@@ -192,8 +192,9 @@ public final class com/apollographql/apollo3/compiler/PackageNameGenerator$Flat
 
 public final class com/apollographql/apollo3/compiler/ReservedKeywordsKt {
 	public static final fun escapeJavaReservedWord (Ljava/lang/String;)Ljava/lang/String;
-	public static final fun escapeKotlinReservedEnumValueNames (Ljava/lang/String;)Ljava/lang/String;
 	public static final fun escapeKotlinReservedWord (Ljava/lang/String;)Ljava/lang/String;
+	public static final fun escapeKotlinReservedWordInEnum (Ljava/lang/String;)Ljava/lang/String;
+	public static final fun escapeKotlinReservedWordInSealedClass (Ljava/lang/String;)Ljava/lang/String;
 }
 
 public final class com/apollographql/apollo3/compiler/Roots {

apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/ReservedKeywords.kt

@@ -15,14 +15,25 @@ fun String.escapeJavaReservedWord() = if (this in JAVA_RESERVED_WORDS) "${this}_
 // Does nothing. KotlinPoet will add the backticks
 fun String.escapeKotlinReservedWord() = this
 
-fun String.escapeKotlinReservedEnumValueNames(): String {
+internal fun String.escapeTypeReservedWord(): String? {
   return when {
-    // https://kotlinlang.org/docs/enum-classes.html#working-with-enum-constants:~:text=properties%20for%20obtaining%20its%20name%20and%20position
+    // type is forbidden because we use it as a companion property to hold the CompiledType
+    // See https://github.com/apollographql/apollo-kotlin/issues/4293
+    "(?:type)_*".toRegex().matches(this) -> "${this}_"
+    else -> null
+  }
+}
+
+
+fun String.escapeKotlinReservedWordInEnum(): String {
+  return when {
+    // name and ordinal are forbidden because already used in Kotlin
+    // See https://kotlinlang.org/docs/enum-classes.html#working-with-enum-constants:~:text=properties%20for%20obtaining%20its%20name%20and%20position
     "(?:name|ordinal)_*".toRegex().matches(this) -> "${this}_"
-    // "header" and "impl" are added to this list because of https://youtrack.jetbrains.com/issue/KT-52315
-    this in arrayOf("header", "impl") -> "`${this}`"
-    else -> this
+    else -> escapeTypeReservedWord() ?: escapeKotlinReservedWord()
   }
 }
 
-internal fun String.isApolloReservedEnumValueName() = this == "type"
+fun String.escapeKotlinReservedWordInSealedClass(): String {
+  return escapeTypeReservedWord() ?: escapeKotlinReservedWord()
+}

apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/checkApolloReservedEnumValueNames.kt

@@ -20,32 +20,16 @@ internal fun checkApolloReservedEnumValueNames(schema: Schema): List<Issue> {
       val targetName = value.directives.findTargetName(schema)
 
       val name = targetName ?: value.name
-      when {
-        name.isApolloReservedEnumValueName() -> {
-          val message = if (targetName == null) {
-            "'$name' is a reserved enum value name, please use the @targetName directive to specify a target name"
-          } else {
-            "'$name' is a reserved enum value name, please use a different name"
-          }
-          issues.add(
-              Issue.ReservedEnumValueName(
-                  message = message,
-                  sourceLocation = value.sourceLocation
-              )
-          )
-        }
-        name in enumNames -> {
-          issues.add(
-              Issue.ReservedEnumValueName(
-                  message = "'${targetName}' is already defined in this enum, please use a different name",
-                  sourceLocation = value.sourceLocation
-              )
-          )
-        }
-        else -> {
-          // all good
-          enumNames.add(name)
-        }
+      if (name in enumNames) {
+        issues.add(
+            Issue.ReservedEnumValueName(
+                message = "'${targetName}' is already defined in this enum, please use a different name",
+                sourceLocation = value.sourceLocation
+            )
+        )
+      } else {
+        // all good
+        enumNames.add(name)
       }
     }
   }

apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/codegen/java/JavaCodegenLayout.kt

@@ -3,6 +3,7 @@ package com.apollographql.apollo3.compiler.codegen.java
 import com.apollographql.apollo3.compiler.PackageNameGenerator
 import com.apollographql.apollo3.compiler.codegen.CodegenLayout
 import com.apollographql.apollo3.compiler.escapeJavaReservedWord
+import com.apollographql.apollo3.compiler.escapeTypeReservedWord
 import com.apollographql.apollo3.compiler.ir.Ir
 
 internal class JavaCodegenLayout(
@@ -22,5 +23,5 @@ internal class JavaCodegenLayout(
 
   // We used to write upper case enum values but the server can define different values with different cases
   // See https://github.com/apollographql/apollo-android/issues/3035
-  internal fun enumValueName(name: String) = regularIdentifier(name)
+  internal fun enumValueName(name: String) = name.escapeTypeReservedWord() ?: regularIdentifier(name)
 }

apollo-compiler/src/main/kotlin/com/apollographql/apollo3/compiler/codegen/kotlin/KotlinCodegenLayout.kt

@@ -2,8 +2,10 @@ package com.apollographql.apollo3.compiler.codegen.kotlin
 
 import com.apollographql.apollo3.compiler.PackageNameGenerator
 import com.apollographql.apollo3.compiler.codegen.CodegenLayout
-import com.apollographql.apollo3.compiler.escapeKotlinReservedEnumValueNames
+import com.apollographql.apollo3.compiler.escapeKotlinReservedWordInEnum
 import com.apollographql.apollo3.compiler.escapeKotlinReservedWord
+import com.apollographql.apollo3.compiler.escapeKotlinReservedWordInSealedClass
+import com.apollographql.apollo3.compiler.escapeTypeReservedWord
 import com.apollographql.apollo3.compiler.ir.Ir
 
 internal class KotlinCodegenLayout(
@@ -24,10 +26,10 @@ internal class KotlinCodegenLayout(
   /**
    * Enum value name to use when generating enums as sealed classes
    */
-  internal fun enumAsSealedClassValueName(name: String) = regularIdentifier(name)
+  internal fun enumAsSealedClassValueName(name: String) = name.escapeKotlinReservedWordInSealedClass()
 
   /**
    * Enum value name to use when generating enums as enums
    */
-  internal fun enumAsEnumValueName(name: String) = name.escapeKotlinReservedEnumValueNames()
+  internal fun enumAsEnumValueName(name: String) = name.escapeKotlinReservedWordInEnum()
 }

apollo-compiler/src/test/graphql/com/example/enum_field/schema.graphqls

@@ -13,7 +13,7 @@ enum Gravity {
   is
 
   # a name that clashes with the generated `type` constant
-  type @targetName(name: "type_")
+  type
 
   # an enum value that clashes with the rawValue type
   String

apollo-compiler/src/test/graphql/com/example/enums_as_sealed/schema.sdl

@@ -8,5 +8,5 @@ enum Enum {
     NORTH
     SOUTH
 
-    type @targetName(name: "type_")
+    type
 }

apollo-compiler/src/test/validation/schema/reserved-enum-value-names.graphql

@@ -6,14 +6,6 @@ enum Enum {
   type
 }
 
-enum Enum2 {
-  type @targetName(name: "type")
-}
-
-enum Enum3 {
-  foo @targetName(name: "type")
-}
-
 enum Enum4 {
   foo
   bar @targetName(name: "foo")

tests/enums/build.gradle.kts

@@ -11,5 +11,5 @@ dependencies {
 
 apollo {
   packageName.set("enums")
-  sealedClassesForEnumsMatching.set(listOf(".*avity"))
+  sealedClassesForEnumsMatching.set(listOf(".*avity", "FooSealed"))
 }

tests/enums/src/main/graphql/operation.graphql

@@ -2,4 +2,6 @@ query GetEnums {
   direction
   gravity
   foo
+  fooSealed
+  fooEnum
 }

tests/enums/src/main/graphql/schema.graphqls

@@ -2,6 +2,8 @@ type Query {
   direction: Direction
   gravity: Gravity
   foo: Foo
+  fooSealed: FooSealed
+  fooEnum: FooEnum
 }
 
 enum Direction {
@@ -11,7 +13,7 @@ enum Direction {
   EAST,
   WEST,
 
-  # Clashes, must be renamed in extra.graphqls
+  # renamed in extra.graphqls
   type,
 
   # Value names should be escaped with _ suffixes when generating enums
@@ -33,7 +35,7 @@ enum Gravity {
   name,
   ordinal,
 
-  # Clashes, must be renamed in extra.graphqls
+  # renamed in extra.graphqls
   type,
 }
 
@@ -42,3 +44,13 @@ enum Foo {
   header,
   footer,
 }
+
+enum FooEnum {
+  # not renamed in extra.graphqls, will be renamed automatically
+  type,
+}
+
+enum FooSealed {
+  # not renamed in extra.graphqls, will be renamed automatically
+  type,
+}

tests/enums/src/test/kotlin/test/EnumsTest.kt

@@ -2,6 +2,8 @@ package test
 
 import enums.type.Direction
 import enums.type.Foo
+import enums.type.FooEnum
+import enums.type.FooSealed
 import enums.type.Gravity
 import org.junit.Test
 import kotlin.test.assertEquals
@@ -14,7 +16,7 @@ class EnumsTest {
     assertEquals(Direction.UNKNOWN__, Direction.safeValueOf("newDirection"))
     assertEquals(Direction.name_, Direction.safeValueOf("name"))
     assertEquals(Direction.ordinal_, Direction.safeValueOf("ordinal"))
-    assertEquals(Direction.type_, Direction.safeValueOf("type"))
+    assertEquals(Direction.type__, Direction.safeValueOf("type"))
   }
 
   @Test
@@ -24,14 +26,22 @@ class EnumsTest {
     assertEquals(Gravity.UNKNOWN__("newGravity"), Gravity.safeValueOf("newGravity"))
     assertEquals(Gravity.name, Gravity.safeValueOf("name"))
     assertEquals(Gravity.ordinal, Gravity.safeValueOf("ordinal"))
-    assertEquals(Gravity.type_, Gravity.safeValueOf("type"))
+    assertEquals(Gravity.type__, Gravity.safeValueOf("type"))
   }
 
   @Test
   fun headerAndImpl() {
     assertEquals(Foo.header.rawValue, "header")
   }
 
+  @Test
+  fun type() {
+    assertEquals(Direction.type__, Direction.safeValueOf("type"))
+    assertEquals(Gravity.type__, Gravity.safeValueOf("type"))
+    assertEquals(FooSealed.type_, FooSealed.safeValueOf("type"))
+    assertEquals(FooEnum.type_, FooEnum.safeValueOf("type"))
+  }
+
   @Test
   fun sealedClassesKnownValues() {
     // Order is important
@@ -47,7 +57,7 @@ class EnumsTest {
             Gravity.RIGHT,
             Gravity.name,
             Gravity.ordinal,
-            Gravity.type_,
+            Gravity.type__,
         ).toList(),
         Gravity.knownValues().toList()
     )