Scalar mapping and adapter configuration improvements #3779

BoD · 2022-01-10T19:47:10Z

Related to #3748.

For now here is a design document with possible changes.

martinbonnin · 2022-01-13T18:14:22Z

design-docs/Scalar adapter configuration.md

+sealed interface AdapterInitializer
+
+// The adapter will be instantiated in the generated code
+class SingletonAdapterInitializer(val qualifiedName: String): AdapterInitializer
+
+// The adapter will be used as-is (it's an object or a public val)
+class NoArgConstructorAdapterInitializer(val qualifiedName: String): AdapterInitializer
+
+// The adapter will be looked up in the `customScalarAdapters` parameter (same as current behavior)
+object RuntimeAdapterInitializer: AdapterInitializer


I know I'm the one who suggested this initially but I think we should avoid Kotlin APIs (like sealed classes) in the Gradle plugin API.

Mainly for Groovy users + because we use R8 to relocate all of the Kotlin-stdlib, this feels dangerous. I'd suggest something like this instead:

class Service { fun mapScalar(graphQLName: String, kotlinName: String) fun mapScalarUsingKotlinSingleton(graphQLName: String, kotlinName: String, singletonName: String) fun mapScalarUsingKotlinConstructor(graphQLName: String, kotlinName: String, constructorName: String) fun mapScalarToKotlinLong(graphQLName: String) fun mapScalarToKotlinInt(graphQLName: String) fun mapScalarToKotlinString(graphQLName: String) fun mapScalarToKotlinBoolean(graphQLName: String) fun mapScalarToKotlinMap(graphQLName: String) fun mapScalarToJavaLong(graphQLName: String) fun mapScalarToJavaInteger(graphQLName: String) fun mapScalarToJavaString(graphQLName: String) fun mapScalarToJavaBoolean(graphQLName: String) fun mapScalarToJavaMap(graphQLName: String) }

Agreed!
Plus I actually find the syntax easier to use 👍

Shouldn't mapScalarUsingKotlinSingleton / mapScalarUsingKotlinConstructor be mapScalarUsingSingleton / mapScalarUsingConstructor?

Shouldn't mapScalarUsingKotlinSingleton / mapScalarUsingKotlinConstructor be mapScalarUsingSingleton / mapScalarUsingConstructor?

I was wondering the same. Singleton is a Kotlin concept so it will not work with Java but maybe we can throw a nice error if that happens?

I was assuming this would work also in Java with static fields:

public class MyAdapters { public static final Adapter<MyDate> DATE_ADAPTER = new Adapter<MyDate>() {...} }

mapScalarUsingSingleton("Date", "com.example.MyDate", "com.example.MyAdapters.DATE_ADAPTER")

But now I'm realizing maybe the name "singleton" is not the most accurate?

Maybe we can "just" ask for a verbatim code block?

apollo { mapScalarUsingAdapter("Date", "com.example.MyDate", "com.example.MyAdapters.DATE_ADAPTER") mapScalarUsingAdapter("Date", "com.example.MyDate", "com.example.MyAdapterSingleton") mapScalarUsingAdapter("Date", "com.example.MyDate", "com.example.MyAdapter()") }

And use a literal (%L) codeblock in KotlinPoet/JavaPoet. This will not add the nice imports but maybe that's ok?

I think I agree, the distinction is not that useful. The documentation can explain how it works, and the fact that an instance or an instantiation expression can both be used.

In that case, maybe we can keep the name mapScalar?

/** * Map a GraphQL scalar type to the Java/Kotlin type and adapter configured at runtime. * * For example: * `mapScalar("Date", "com.example.Date")` */ fun mapScalar(graphQLName: String, targetName: String) /** * Map a GraphQL scalar type to the Java/Kotlin type and provided adapter expression. * * For example: * `mapScalar("Date", "com.example.Date", "com.example.DateAdapter")` (an instance property or object) * `mapScalar("Date", "com.example.Date", "com.example.DateAdapter()")` (instantiate the class on the fly) */ fun mapScalar(graphQLName: String, targetName: String, adatpterExpression: String)

mapScalarToKotlinMap / mapScalarToJavaMap should be mapScalarToKotlinAny / mapScalarToJavaObject or am I missing something?

Yes, indeed. Until we have #3243, we can't use Map or other parametrized classes.

design-docs/Scalar adapter configuration.md

BoD · 2022-01-14T13:10:53Z

I've pushed the Kotlin implementation for review

Remaining:

Java
Documentation updates

martinbonnin · 2022-01-16T22:01:01Z

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

@@ -170,6 +170,11 @@ object ApolloCompiler {
      options.operationOutputFile.writeText(operationOutput.toJson("  "))
    }

+    // Merge scalar mappings from upstream modules and this module's options
+    val allScalarMapping = options.incomingCompilerMetadata.fold(options.scalarMapping.toMutableMap()) { acc, it ->
+      acc.apply { putAll(it.scalarMapping) }


This feels dangerous. What if there's a conflicting mapping in 2 modules? Should we mandate that all scalar mappings are defined in the schema module?

I can't think of any use case where different modules could use different mappings but if something comes up we can always relax the restriction.

I thought it was already the case actually. The multi module doc says "The schema module and only the schema module must define customScalarsMapping".

Should I ensure there are no duplicates and throw if there are any? Or should we not merge them at all (not sure how that would work)?

Better throw fast if we see something like this.

martinbonnin · 2022-01-16T22:03:45Z

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

+ * Controls how scalar adapters are used in the generated code.
+ */
+@JsonClass(generateAdapter = true, generator = "sealed:type")
+sealed interface AdapterInitializer : Serializable


Why does it need to be Serializable ?

Got it. This is need by Gradle to snapshot the input properties.

I always feel weird implementing Serializable since it's already serialized to Json. Also using Kotlin features like sealed interfaces and data classes in Gradle APIs feels dangerous too (think Groovy callers).

I would certainly have used 2 maps:

scalarsMapping: Map<String, String> scalarAdapters: Map<String, String>

But I can't find a strong reason for the change so your choice!

Yes sorry I should have commented :). I agree, I'm not very fond of making them as Serializable either. Using String feels safer, I'll make the change.

martinbonnin · 2022-01-16T22:22:28Z

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

@@ -132,8 +133,8 @@ internal class IrBuilder(
      visitedTypes.add(name)
      val typeDefinition = schema.typeDefinition(name)
      if (typeDefinition.isBuiltIn()) {
-        // We don't generate builtin types
-        continue
+        // We don't generate builtin types, unless they are explicitly mapped


Suggested change

// We don't generate builtin types, unless they are explicitly mapped

// We don't generate builtin types, unless they are explicitly mapped

// Because users need String.type (or others) to register their adapters

Thinking a bit more about this. I think it makes sense to keep the builtin types in apollo-api always and not generate them. This means for builtin types that require a runtime adapter, users need to do something like:

apolloClientBuilder.addScalarAdapter(CompiledIDType, MyIDAdapter) // instead of // apolloClientBuilder.addScalarAdapter(ID.type, MyIDAdapter)

But I think this is such an advanced case that it's ok? It keeps the logic more symmetric. Builting types are in apollo-api, others get generated

👍 I'll have a look!

In retrospect, CompiledIDType should certainly have been BuiltinIDType but that'll be for v4 😅

Hmm after more digging, the CustomScalarAdapters methods take instances of CustomScalarTypes, whereas the compiled types are ScalarType - so apolloClientBuilder.addScalarAdapter(ID.type, MyIDAdapter) for instance doesn't work.

We could make CustomScalarType be a subclass of ScalarType, and change CustomScalarAdapters methods to take ScalarType instead. But that looks like a breaking change 😅

Or we can add ScalarType based methods (responseAdapterFor, add, addCustomScalarAdapter) in addition to the existing CustomScalarType based ones.

WDYT?

Gosh this is hard. I'm not too much a fan of duplicating everything either. I think your initial approach of generating a CustomScalarType for builtin types if they have a mapping was best then. CustomScalarType will not be a super accurate name and the whole thing feels a bit asymmetrical but I don't see a better way.

Or should we always generate all builtin types? And mark CompiledStringType, etc.. as deprecated? I was initially not too fond of this because it puts String, Int, etc.. as top level classes but maybe since they're using a specific package name, that'll be ok...

Or should we always generate all builtin types?

I'll five it a go so we can see what it looks like :)

apollo-gradle-plugin/src/main/kotlin/com/apollographql/apollo3/gradle/api/Service.kt

martinbonnin · 2022-01-16T22:27:35Z

...e-plugin/src/main/kotlin/com/apollographql/apollo3/gradle/internal/DefaultApolloExtension.kt

+                  service.customTypeMapping
+              )
+                  .getOrNull()?.asScalarInfoMapping() ?: emptyMap()
+          )


throw if both customScalarsMapping and scalarMapping are set?

Good point!

tests/custom-scalars/src/test/kotlin/test/CustomScalarTest.kt

martinbonnin · 2022-01-16T22:55:08Z

...compiler/src/main/kotlin/com/apollographql/apollo3/compiler/codegen/kotlin/KotlinResolver.kt

      type is IrModelType -> resolveAndAssert(ResolverKeyKind.Model, type.path)
      type is IrNamedType -> resolveAndAssert(ResolverKeyKind.SchemaType, type.name)
      else -> error("$type is not a schema type")
    }.copy(nullable = true)
  }

+  private fun resolveIrScalarType(type: IrScalarType): ClassName {
+    // Try mapping first, then built-ins, then fallback to Any
+    return resolve(ResolverKeyKind.CustomScalarTarget, type.name) ?: when (type.name) {


Rename CustomScalarTarget to ScalarTarget?

martinbonnin · 2022-01-16T23:13:24Z

...compiler/src/main/kotlin/com/apollographql/apollo3/compiler/codegen/kotlin/KotlinResolver.kt

+          "ID" -> CodeBlock.of("%M", KotlinSymbols.StringAdapter)
+          "String" -> CodeBlock.of("%M", KotlinSymbols.StringAdapter)
+          "Int" -> CodeBlock.of("%M", KotlinSymbols.IntAdapter)
+          "Float" -> CodeBlock.of("%M", KotlinSymbols.DoubleAdapter)


I'd love this to be moved up a few layers so that the resolver can apply the same logic everywhere. Not really sure how to do this yet but I might investigate this in a follow up PR unless you have ideas how to do this already.

…anted adapter in the generated code directly

Co-authored-by: Martin Bonnin <[email protected]>

…alarAdapterInitializer and resolveCompiledType

…tances on the fly

…ti module scenarios

…ding) / not that useful

…Also don't expose it on the Service.

… code (#3793) * Mark compiled types as deprecated and no longer use them in generated code * Always generate types for built-in scalars * Fix MetadataTest * Prefix builtin scalar names with 'GraphQL' to avoid a clash in multiplatform * Schema object: revert using "__Schema" name by default and add a `generatedSchemaName` option. Revert to using compiled types for non scalar built-in types (__Schema, __Field, etc.).

…3795) * Apply same changes to Java codegen * Update Upload's kdoc about how to use it in the plugin conf * Java: mark compiled types as deprecated and no longer use them in generated code * Rebase on feature branch and apply last changes on KotlinResolver to JavaResolver

* rename custom-scalar test to scalar-adapters * fix KDoc for registerOperations * add a section in CONTRIBUTING.md about naming Gradle APIs * Kdoc tweaks * add a test to make sure we don't allow mapScalar + customScalarMapping at the same time * remove ScalarTarget as a resolver entry and instead use scalarMapping * fix Gradle tests

BoD force-pushed the scalar-adapters-in-gradle-plugin-conf branch from 9796670 to ce8bed2 Compare January 12, 2022 08:54

martinbonnin reviewed Jan 13, 2022

View reviewed changes

design-docs/Scalar adapter configuration.md Outdated Show resolved Hide resolved

martinbonnin reviewed Jan 13, 2022

View reviewed changes

design-docs/Scalar adapter configuration.md Outdated Show resolved Hide resolved

martinbonnin reviewed Jan 13, 2022

View reviewed changes

design-docs/Scalar adapter configuration.md Outdated Show resolved Hide resolved

martinbonnin reviewed Jan 13, 2022

View reviewed changes

design-docs/Scalar adapter configuration.md Outdated Show resolved Hide resolved

martinbonnin reviewed Jan 16, 2022

View reviewed changes

apollo-gradle-plugin/src/main/kotlin/com/apollographql/apollo3/gradle/api/Service.kt Outdated Show resolved Hide resolved

martinbonnin reviewed Jan 16, 2022

View reviewed changes

tests/custom-scalars/src/test/kotlin/test/CustomScalarTest.kt Outdated Show resolved Hide resolved

martinbonnin reviewed Jan 16, 2022

View reviewed changes

BoD mentioned this pull request Jan 18, 2022

Mark compiled types as deprecated and no longer use them in generated code #3793

Merged

martinbonnin approved these changes Jan 18, 2022

View reviewed changes

BoD mentioned this pull request Jan 19, 2022

Doc update following changes in scalar mapping configuration #3797

Merged

BoD marked this pull request as ready for review January 24, 2022 08:44

BoD requested review from sav007 and tasomaniac as code owners January 24, 2022 08:44

martinbonnin approved these changes Jan 25, 2022

View reviewed changes

BoD and others added 8 commits January 25, 2022 20:38

Add a design doc

7ccc7a8

Address feedback: avoid using CustomScalarAdapters, instead use the w…

7e6486b

…anted adapter in the generated code directly

Apply suggestions from code review

cb79b03

Co-authored-by: Martin Bonnin <[email protected]>

Introduce ScalarInfo, AdapterInitializer and mapScalar

381e891

Use scalarMapping in KotlinResolver.resolveIrType

6721a8e

Use scalarMapping in KotlinResolver.adapterInitializer, nonNullableSc…

c034d00

…alarAdapterInitializer and resolveCompiledType

Introduce ScalarAdapterInstancesBuilder to avoid creating adapter ins…

026df8b

…tances on the fly

Rename customScalarsMapping -> scalarMapping

65e2639

BoD and others added 16 commits January 25, 2022 20:38

Improve CustomScalarTest

f818fa0

Update CodegenTest with more scalar cases

a7eb38c

Pass / recover scalar mapping in metadata, to work as expected in mul…

e9cf742

…ti module scenarios

Cleanup

3cd05a4

Revert ScalarAdapterInstancesBuilder as it's potentially risky (threa…

6fefb5a

…ding) / not that useful

Simplify the user-facing API following review

3e95b02

Update design doc after review

afb8a82

Add convenience methods for builtin types/adapters

30b96f9

Throw if scalar mapping are defined in several modules

a966675

Keep scalar mapping as 2 maps of String instead of using ScalarInfo. …

29d1329

…Also don't expose it on the Service.

Clarify test by explicitly using Longs

7b0994e

Rename CustomScalarTarget -> ScalarTarget

e62a2b7

Update API dump

822896f

BoD force-pushed the scalar-adapters-in-gradle-plugin-conf branch from 7094dbc to 4c82ca9 Compare January 25, 2022 19:39

BoD merged commit a1536a2 into main Jan 25, 2022

BoD deleted the scalar-adapters-in-gradle-plugin-conf branch January 25, 2022 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scalar mapping and adapter configuration improvements #3779

Scalar mapping and adapter configuration improvements #3779

BoD commented Jan 10, 2022

martinbonnin Jan 13, 2022 •

edited

Loading

BoD Jan 14, 2022

martinbonnin Jan 14, 2022

BoD Jan 14, 2022

martinbonnin Jan 14, 2022

BoD Jan 14, 2022

BoD Jan 14, 2022

martinbonnin Jan 14, 2022

BoD commented Jan 14, 2022

martinbonnin Jan 16, 2022

BoD Jan 17, 2022

martinbonnin Jan 17, 2022

martinbonnin Jan 16, 2022

martinbonnin Jan 16, 2022

BoD Jan 17, 2022

martinbonnin Jan 16, 2022

martinbonnin Jan 16, 2022

BoD Jan 17, 2022

martinbonnin Jan 17, 2022

BoD Jan 17, 2022

martinbonnin Jan 17, 2022

BoD Jan 18, 2022

martinbonnin Jan 16, 2022 •

edited

Loading

BoD Jan 17, 2022

martinbonnin Jan 16, 2022

martinbonnin Jan 16, 2022

	// We don't generate builtin types, unless they are explicitly mapped
	// We don't generate builtin types, unless they are explicitly mapped
	// Because users need String.type (or others) to register their adapters

Scalar mapping and adapter configuration improvements #3779

Scalar mapping and adapter configuration improvements #3779

Conversation

BoD commented Jan 10, 2022

martinbonnin Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BoD commented Jan 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinbonnin Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinbonnin Jan 13, 2022 •

edited

Loading

martinbonnin Jan 16, 2022 •

edited

Loading