Use Kotlin's Inline value classes for custom scalars

[x-sheep]

Use case

Inline value classes are a thin wrapper over other types, which can potentially be erased entirely by the compiler.

value class ID(val value: String)

The above is safer to use in Client code, since it's not possible to assign a String to an ID or vice versa. It also discourages (but doesn't prevent) string manipulation by the client, which is useful for scalars that are documented to be opaque.

Describe the solution you'd like

The various shortcuts like mapScalarToKotlinLong, mapScalarToKotlinString etc. could be expanded to take an additional argument asValueClass: Boolean which will generate a value class instead of a typealias.

I'm not sure if it's preferable to generate another Adapter for this new type, or if its usage should be applied inside every existing object adapter:

val id = ID(value = StringAdapter.fromJson(_, _)

val json = StringAdapter.toJson(_, _, id.value)

I think avoiding a new adapter would allow the compiler to inline the type in more cases and increase performance.

[BoD]

Thanks for the excellent suggestion!

The way it works currently doesn't involve typealiases, when using e.g. mapScalarToKotlinLong, the generated model will use a Long directly.

Currently, you can uses value classes for scalars with adapters looking like this:

val IDAdapter = object : Adapter<ID> {
  override fun fromJson(reader: JsonReader, customScalarAdapters: CustomScalarAdapters): ID {
    return ID(reader.nextString()!!)
  }

  override fun toJson(writer: JsonWriter, customScalarAdapters: CustomScalarAdapters, value: ID) {
    writer.value(value.value)
  }
}

and configuring it like:

mapScalar(graphQLName = "ID", targetName = "com.example.ID", expression = "com.example.IDAdapter")

To make things less manual, the library could do 2 things:

• A/ generate the scalar value class and make the generated code wrap/unwrap it before using the existing String/Long/etc. adapter
• B/ let the user define their scalar value class (like now) and make the generated code wrap/unwrap it before using the existing String/Long/etc. adapter

A/ Generate scalar

Configuration:

mapScalarToKotlinLong("Length", asValueClass = true)

Would generate your scalar:

package package com.example

@JvmInline
value class Length(val value: Long)

Generated model adapter:

// fromJson
_length = com.example.Length(
  LongAdapter.fromJson(reader, customScalarAdapters)
)

// toJson
LongAdapter.toJson(writer, customScalarAdapters, value.length.value)

B/ Bring your scalar

Configuration:

mapScalarToKotlinLong("Length", valueClass = "com.example.MyLength", valueClassProperty = "length")

Generated model adapter:

// fromJson
_length = com.example.Length(
  LongAdapter.fromJson(reader, customScalarAdapters)
)

// toJson
LongAdapter.toJson(writer, customScalarAdapters, value.length.length)

A does more but B is more flexible as you may want your own functionality in the value class, rather than being just a wrapper.

We could also start with B first and add the option to generate the scalar later.

[x-sheep]

Looking into the Kotlin documentation, defining the value class yourself does have some benefits you can't use extension properties/methods for:

• Throw an exception on initialization, which could be surfaced as a response validation error (same as the JSON adapters are doing now)
• Additional convenience constructors
• Inherit from interfaces

The only type that would benefit from being generated by codegen (or just included in the library) would be the built-in ID, as it can be defined with a one-liner.

[BoD]

Some additional thoughts:

• wrapping/unwrapping directly in the generated adapter avoids boxing/unboxing in the simple case, but not for lists and nullable cases, which delegate to NullableAdapter and ListAdapter
• this is already the case today with Int, Long etc.

[x-sheep]

Here's the current design notes on how the Kotlin developers want to expand the usage of value classes: https://github.com/Kotlin/KEEP/blob/master/notes/value-classes.md#value-classes-and-arrays. They're also looking at Project Valhalla which has a similar goal for the JVM specifically.

One of the aims is to replace specialialized types like IntArray and FloatArray with a generic Array<int> and Array<float> that can avoid boxing at runtime.

So the performance benefit of Value types isn't great at this point, but will increase in the future as Kotlin develops.

[x-sheep]

Counter-example: The GitLab GraphQL API defines a new scalar type for every single type of ID. Having to manually write a value class for all of these would be a lot of work, and even writing a mapScalarTo... line for all of them isn't ideal. It could be a good idea to add an option to generate a new value type for every scalar not explicitly mapped (off by default).

https://docs.gitlab.com/ee/api/graphql/reference/#scalar-types

[martinbonnin]

Having to manually write a value class for all of these would be a lot of work, and even writing a mapScalarTo... line for all of them isn't ideal. It could be a good idea to add an option to generate a new value type for every scalar not explicitly mapped (off by default).

My favorite solution would be to define that in the schema. Something like:

extend scalar AbuseReportID @kotlinValueClass(coerceAs: String)

This makes it work with other build systems than Gradle out of the box and if you have a lot of such ids, you could always write a task that generates the schema extensions:

scalarTypes.forEach {
  if (it.name.endsWith("ID")) {
    append("extend scalar ${it.name} @kotlinValueClass(coerceAs: String)
  }
}

Edit: add coerceAs

[x-sheep]

This makes it work with other build systems than Gradle out of the box and if you have a lot of such ids, you could always write a task that generates the schema extensions:

Is there currently a mechanism in place that parses/fetches a schema, appends extensions, then parses it again?

[martinbonnin]

Is there currently a mechanism in place that parses/fetches a schema, appends extensions, then parses it again?

The Gradle plugin takes a FileCollection so you can pass the server schema + extensions and the compiler merges them (kdoc):

apollo {
  service("service") {
    schemaFiles.from("src/main/graphql/schema.graphqls", "src/main/graphql/extra.graphqls")
    // ...
  }
}

How extra.graphqls is generated is left to the user but if you have a task with a single output file, you should be able to pass it as well thanks to Gradle magic:

apollo {
  service("service") {
    schemaFiles.from("src/main/graphql/schema.graphqls", generateSchemaExtensionsTaskProvider)
    // ...
  }
}

[x-sheep]

I don't think adding another directive to change the behavior is a good idea. In my opinion, using a separate class should actually be the default behavior for all custom scalars, but that's obviously not possible since it's a major breaking change for users.

[martinbonnin]

using a separate class should actually be the default behavior for all custom scalars

How would you specify the coercing of the wrapped value?

GraphQL:

scalar AbuseReportID

Kotlin:

@JvmInline
value class AbuseReportID(
   // Is that OK?
   val value: Any
)

[x-sheep]

Apollo Kotlin's current behavior is replacing every scalar without a mapping to use String, so that seems like a reasonable default type to use for value classes.

[martinbonnin]

Apollo Kotlin's current behavior is replacing every scalar without a mapping to use String

Mmm that shouldn't be the case. They should be of kotlin.Any type by default (because of things like Json and more generally the coercing can be anything).