Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: Limit built-in scalars to the ones defined by the spec #158

Merged
merged 1 commit into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/content/Reference/Type System/scalars.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,6 @@ stringScalar<UUID> {
}
}
```

In addition to the built-in types, KGraphQL provides support for `Long` and `Short` which can be added to a schema
using `extendedScalars()`.
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.apurebase.kgraphql.schema

import com.apurebase.kgraphql.request.isIntrospectionType
import com.apurebase.kgraphql.schema.builtin.BuiltInScalars
import com.apurebase.kgraphql.schema.directive.Directive
import com.apurebase.kgraphql.schema.introspection.TypeKind
import com.apurebase.kgraphql.schema.introspection.__Described
Expand Down Expand Up @@ -30,6 +29,8 @@ data class SchemaPrinterConfig(
)

class SchemaPrinter(private val config: SchemaPrinterConfig = SchemaPrinterConfig()) {
private val builtInScalarNames = setOf("Int", "Float", "String", "Boolean", "ID")

/**
* Returns the given [schema] in schema definition language (SDL). Types and fields are sorted
* ascending by their name and appear in order of their corresponding spec section, i.e.
Expand Down Expand Up @@ -229,7 +230,7 @@ class SchemaPrinter(private val config: SchemaPrinterConfig = SchemaPrinterConfi
private fun __Directive.isBuiltIn(): Boolean =
name in setOf(Directive.DEPRECATED.name, Directive.INCLUDE.name, Directive.SKIP.name)

private fun __Type.isBuiltInScalar(): Boolean = name in BuiltInScalars.entries.map { it.typeDef.name }
private fun __Type.isBuiltInScalar(): Boolean = name in builtInScalarNames

private fun __Type.implements(): String =
interfaces
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,20 @@ private const val BOOLEAN_DESCRIPTION =
"The Boolean scalar type represents true or false"

/**
* These scalars are created only for sake of documentation in introspection, not during execution
*
* https://spec.graphql.org/October2021/#sec-Scalars.Built-in-Scalars
*/
enum class BuiltInScalars(val typeDef: TypeDef.Scalar<*>) {
STRING(TypeDef.Scalar(String::class.defaultKQLTypeName(), String::class, STRING_COERCION, STRING_DESCRIPTION)),
SHORT(TypeDef.Scalar(Short::class.defaultKQLTypeName(), Short::class, SHORT_COERCION, SHORT_DESCRIPTION)),
INT(TypeDef.Scalar(Int::class.defaultKQLTypeName(), Int::class, INT_COERCION, INT_DESCRIPTION)),

// GraphQL does not differentiate between float and double, treat double like float
DOUBLE(TypeDef.Scalar(Float::class.defaultKQLTypeName(), Double::class, DOUBLE_COERCION, FLOAT_DESCRIPTION)),
FLOAT(TypeDef.Scalar(Float::class.defaultKQLTypeName(), Float::class, FLOAT_COERCION, FLOAT_DESCRIPTION)),
BOOLEAN(TypeDef.Scalar(Boolean::class.defaultKQLTypeName(), Boolean::class, BOOLEAN_COERCION, BOOLEAN_DESCRIPTION)),
}

enum class ExtendedBuiltInScalars(val typeDef: TypeDef.Scalar<*>) {
SHORT(TypeDef.Scalar(Short::class.defaultKQLTypeName(), Short::class, SHORT_COERCION, SHORT_DESCRIPTION)),
LONG(TypeDef.Scalar(Long::class.defaultKQLTypeName(), Long::class, LONG_COERCION, LONG_DESCRIPTION))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.apurebase.kgraphql.schema.dsl
import com.apurebase.kgraphql.schema.Publisher
import com.apurebase.kgraphql.schema.Schema
import com.apurebase.kgraphql.schema.SchemaException
import com.apurebase.kgraphql.schema.builtin.ExtendedBuiltInScalars
import com.apurebase.kgraphql.schema.dsl.operations.MutationDSL
import com.apurebase.kgraphql.schema.dsl.operations.QueryDSL
import com.apurebase.kgraphql.schema.dsl.operations.SubscriptionDSL
Expand Down Expand Up @@ -140,6 +141,12 @@ class SchemaBuilder internal constructor() {
booleanScalar(T::class, block)
}

fun extendedScalars() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there tests specifying that with not invoking this, the additional scalars are not present?

Copy link
Owner Author

@stuebingerb stuebingerb Jan 13, 2025

Choose a reason for hiding this comment

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

I've seen all tests fail that I adjusted but it would make sense to have it validated explicitly. Will update but I have found some strange behavior with the implemented coercions that I want to analyze first.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This looks more complicated: #160

But I have at least added some validation

ExtendedBuiltInScalars.values().forEach {
model.addScalar(it.typeDef)
}
}

//================================================================================
// TYPE
//================================================================================
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ class DataLoaderPreparedRequestExecutor(val schema: DefaultSchema) : RequestExec
value is Double -> node.aliasOrKey toValue JsonPrimitive(value)
value is Boolean -> node.aliasOrKey toValue JsonPrimitive(value)
value is Long -> node.aliasOrKey toValue JsonPrimitive(value)
value is Short -> node.aliasOrKey toValue JsonPrimitive(value)
value is Deferred<*> -> {
deferredLaunch {
applyKeyToElement(ctx, value.await(), node, returnType, parentCount)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ class ParallelRequestExecutor(val schema: DefaultSchema) : RequestExecutor {
value is Double -> jsonNodeFactory.numberNode(value)
value is Boolean -> jsonNodeFactory.booleanNode(value)
value is Long -> jsonNodeFactory.numberNode(value)
//big decimal etc?
value is Short -> jsonNodeFactory.numberNode(value)

node.children.isNotEmpty() -> {
createObjectNode(ctx, value, node, returnType)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,35 @@ class LongScalarTest {
@Test
fun testLongField() {
val schema = defaultSchema {
extendedScalars()
query("long") {
resolver { -> Long.MAX_VALUE }
}
}

val response = schema.executeBlocking("{long}")
val response = schema.executeBlocking("{ long }")
val long = deserialize(response).extract<Long>("data/long")
assertThat(long, equalTo(Long.MAX_VALUE))
}

@Test
fun testLongArgument() {
val schema = defaultSchema {
extendedScalars()
query("isLong") {
resolver { long: Long -> if (long > Int.MAX_VALUE) "YES" else "NO" }
resolver { long: Long ->
if (long > Int.MAX_VALUE) {
"YES"
} else {
"NO"
}
}
}
}

val isLong =
deserialize(schema.executeBlocking("{isLong(long: ${Int.MAX_VALUE.toLong() + 1})}")).extract<String>("data/isLong")
val isLong = deserialize(
schema.executeBlocking("{ isLong(long: ${Int.MAX_VALUE.toLong() + 1}) }")
).extract<String>("data/isLong")
assertThat(isLong, equalTo("YES"))
}

Expand All @@ -52,7 +61,7 @@ class LongScalarTest {
}

val value = Int.MAX_VALUE.toLong() + 2
val response = deserialize(schema.executeBlocking("{number(number: $value)}"))
val response = deserialize(schema.executeBlocking("{ number(number: $value) }"))
assertThat(response.extract<Long>("data/number"), equalTo(value))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ class GitHubIssue75 {
useDefaultPrettyPrinter = true
}

extendedScalars()

query("findTrace") {
resolver { traceID: String ->
Trace(
Expand Down Expand Up @@ -132,6 +134,6 @@ class GitHubIssue75 {
}
}
""", "{\"traceID\": \"646851f15cb2dad1\"}"
).let(::println)
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -750,12 +750,12 @@ class SchemaBuilderTest {
@Test
fun `Short int types are mapped to Short Scalar`() {
val schema = defaultSchema {
extendedScalars()
query("shortQuery") {
resolver { -> 1.toShort() }
}
}


val typesIntrospection = deserialize(schema.executeBlocking("{__schema{types{name}}}"))
val types = typesIntrospection.extract<List<Map<String, String>>>("data/__schema/types")
val names = types.map { it["name"] }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,7 @@ class SchemaPrinterTest {
@Test
fun `schema with descriptions should be printed as expected if descriptions are included`() {
val schema = KGraphQL.schema {
extendedScalars()
type<TestObject> {
property(TestObject::name) {
description = "This is the name"
Expand Down Expand Up @@ -582,6 +583,12 @@ class SchemaPrinterTest {
subscription: Subscription
}

"The Long scalar type represents a signed 64-bit numeric non-fractional value"
scalar Long

"The Short scalar type represents a signed 16-bit numeric non-fractional value"
scalar Short

"Mutation object"
type Mutation {
"Add a test object"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import com.apurebase.kgraphql.KGraphQL
import com.apurebase.kgraphql.Specification
import com.apurebase.kgraphql.deserialize
import com.apurebase.kgraphql.extract
import com.apurebase.kgraphql.schema.SchemaException
import com.apurebase.kgraphql.schema.model.ast.ValueNode
import com.apurebase.kgraphql.schema.scalar.StringScalarCoercion
import org.amshove.kluent.invoking
Expand All @@ -21,6 +22,71 @@ import java.util.UUID
@Specification("3.1.1 Scalars")
class ScalarsSpecificationTest {

@Test
fun `built-in scalars should be available by default`() {
val schema = KGraphQL.schema {
query("int") {
resolver<Int> { 1 }
}
query("float") {
resolver<Float> { 2.0f }
}
query("double") {
resolver<Double> { 3.0 }
}
query("string") {
resolver<String> { "foo" }
}
query("boolean") {
resolver<Boolean> { true }
}
// TODO: ID, cf. https://github.com/stuebingerb/KGraphQL/issues/83
}

val response = deserialize(schema.executeBlocking("{ int float double string boolean }"))
assertThat(response.extract<Int>("data/int"), equalTo(1))
assertThat(response.extract<Float>("data/float"), equalTo(2.0))
Copy link
Owner Author

Choose a reason for hiding this comment

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

IMHO this should be equalTo(2.0f) but haven't found out why that doesn't work.

assertThat(response.extract<Double>("data/double"), equalTo(3.0))
assertThat(response.extract<String>("data/string"), equalTo("foo"))
assertThat(response.extract<Boolean>("data/boolean"), equalTo(true))
}

@Test
fun `extended scalars should not be available by default`() {
invoking {
KGraphQL.schema {
query("long") {
resolver<Long> { 1L }
}
}
} shouldThrow SchemaException::class withMessage "An Object type must define one or more fields. Found none on type Long"

invoking {
KGraphQL.schema {
query("short") {
resolver<Short> { 2.toShort() }
}
}
} shouldThrow SchemaException::class withMessage "An Object type must define one or more fields. Found none on type Short"
}

@Test
fun `extended scalars should be available if included`() {
val schema = KGraphQL.schema {
extendedScalars()
query("long") {
resolver<Long> { Long.MAX_VALUE }
}
query("short") {
resolver<Short> { 2.toShort() }
}
}

val response = deserialize(schema.executeBlocking("{ long short }"))
assertThat(response.extract<Long>("data/long"), equalTo(9223372036854775807L))
assertThat(response.extract<Int>("data/short"), equalTo(2))
Copy link
Owner Author

Choose a reason for hiding this comment

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

IMHO this should be extract<Short> and equalTo(2.toShort()) but haven't found out why that doesn't work. There is also a cast in TestUtils.extract that apparently successfully casts an Int to a Short, which shouldn't be possible. Hopefully that will all become clearer during work on #160.

}

data class Person(val uuid: UUID, val name: String)

@Test
Expand All @@ -44,12 +110,12 @@ class ScalarsSpecificationTest {
}
}

val queryResponse = deserialize(testedSchema.executeBlocking("{person{uuid}}"))
val queryResponse = deserialize(testedSchema.executeBlocking("{ person{ uuid } }"))
assertThat(queryResponse.extract<String>("data/person/uuid"), equalTo(uuid.toString()))

val mutationResponse = deserialize(
testedSchema.executeBlocking(
"mutation{createPerson(uuid: \"$uuid\", name: \"John\"){uuid name}}"
"mutation { createPerson(uuid: \"$uuid\", name: \"John\"){ uuid name } }"
)
)
assertThat(mutationResponse.extract<String>("data/createPerson/uuid"), equalTo(uuid.toString()))
Expand All @@ -68,7 +134,7 @@ class ScalarsSpecificationTest {
}

invoking {
schema.executeBlocking("mutation{Int(int: ${Integer.MAX_VALUE.toLong() + 2L})}")
schema.executeBlocking("mutation { Int(int: ${Integer.MAX_VALUE.toLong() + 2L}) }")
} shouldThrow GraphQLError::class with {
message shouldBeEqualTo "Cannot coerce to type of Int as '${Integer.MAX_VALUE.toLong() + 2L}' is greater than (2^-31)-1"
}
Expand All @@ -84,7 +150,7 @@ class ScalarsSpecificationTest {
resolver { float: Float -> float }
}
}
val map = deserialize(schema.executeBlocking("mutation{float(float: 1)}"))
val map = deserialize(schema.executeBlocking("mutation { float(float: 1) }"))
assertThat(map.extract<Double>("data/float"), equalTo(1.0))
}

Expand All @@ -104,7 +170,7 @@ class ScalarsSpecificationTest {

val randomUUID = UUID.randomUUID()
val map =
deserialize(testedSchema.executeBlocking("query(\$id: ID = \"$randomUUID\"){personById(id: \$id){uuid, name}}"))
deserialize(testedSchema.executeBlocking("query(\$id: ID = \"$randomUUID\"){ personById(id: \$id) { uuid, name } }"))
assertThat(map.extract<String>("data/personById/uuid"), equalTo(randomUUID.toString()))
}

Expand All @@ -121,15 +187,14 @@ class ScalarsSpecificationTest {
}

invoking {
schema.executeBlocking("mutation{Int(int: \"223\")}")
schema.executeBlocking("mutation { Int(int: \"223\") }")
} shouldThrow GraphQLError::class withMessage "Cannot coerce \"223\" to numeric constant"
}

data class Number(val int: Int)

@Test
fun `Schema may declare custom int scalar type`() {

val schema = KGraphQL.schema {
intScalar<Number> {
deserialize = ::Number
Expand All @@ -142,15 +207,14 @@ class ScalarsSpecificationTest {
}

val value = 3434
val response = deserialize(schema.executeBlocking("{number(number: $value)}"))
val response = deserialize(schema.executeBlocking("{ number(number: $value) }"))
assertThat(response.extract<Int>("data/number"), equalTo(value))
}

data class Bool(val boolean: Boolean)

@Test
fun `Schema may declare custom boolean scalar type`() {

val schema = KGraphQL.schema {
booleanScalar<Bool> {
deserialize = ::Bool
Expand All @@ -163,7 +227,7 @@ class ScalarsSpecificationTest {
}

val value = true
val response = deserialize(schema.executeBlocking("{boolean(boolean: $value)}"))
val response = deserialize(schema.executeBlocking("{ boolean(boolean: $value) }"))
assertThat(response.extract<Boolean>("data/boolean"), equalTo(value))
}

Expand All @@ -177,7 +241,6 @@ class ScalarsSpecificationTest {

@Test
fun `Schema may declare custom double scalar type`() {

val schema = KGraphQL.schema {
floatScalar<Dob> {
deserialize = ::Dob
Expand All @@ -190,7 +253,7 @@ class ScalarsSpecificationTest {
}

val value = 232.33
val response = deserialize(schema.executeBlocking("{double(double: $value)}"))
val response = deserialize(schema.executeBlocking("{ double(double: $value) }"))
assertThat(response.extract<Double>("data/double"), equalTo(value))
}

Expand Down Expand Up @@ -240,7 +303,7 @@ class ScalarsSpecificationTest {
val d = '$'

val req = """
query Query(${d}boo: Boo!, ${d}sho: Sho!, ${d}lon: Lon!, ${d}dob: Dob!, ${d}num: Num!, ${d}str: Str!){
query Query(${d}boo: Boo!, ${d}sho: Sho!, ${d}lon: Lon!, ${d}dob: Dob!, ${d}num: Num!, ${d}str: Str!) {
boo(boo: ${d}boo)
sho(sho: ${d}sho)
lon(lon: ${d}lon)
Expand Down
Loading