Skip to content

ParallelRequestExecutor: fields with @include and @skip aren't excluded, but return null #211

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ParallelRequestExecutor: fields with @include and @skip aren't excluded, but return null #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions kgraphql/src/main/kotlin/com/apurebase/kgraphql/schema/execution/ParallelRequestExecutor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,15 +182,15 @@ class ParallelRequestExecutor(val schema: DefaultSchema) : RequestExecutor {
when (child) {
is Execution.Fragment -> objectNode.setAll<JsonNode>(handleFragment(ctx, value, child))
else -> {
val (key, jsonNode) = handleProperty(ctx, value, child, type, node.children.size)
val (key, jsonNode) = handleProperty(ctx, value, child, type, node.children.size) ?: continue
objectNode.merge(key, jsonNode)
}
}
}
return objectNode
}

private suspend fun <T> handleProperty(ctx: ExecutionContext, value: T, child: Execution, type: Type, childrenSize: Int): Pair<String, JsonNode?> {
private suspend fun <T> handleProperty(ctx: ExecutionContext, value: T, child: Execution, type: Type, childrenSize: Int): Pair<String, JsonNode?>? {
when (child) {
//Union is subclass of Node so check it first
is Execution.Union -> {
Expand All @@ -205,7 +205,7 @@ class ParallelRequestExecutor(val schema: DefaultSchema) : RequestExecutor {
is Execution.Node -> {
val field = type.unwrapped()[child.key]
?: throw IllegalStateException("Execution unit ${child.key} is not contained by operation return type")
return child.aliasOrKey to createPropertyNode(ctx, value, child, field, childrenSize)
return child.aliasOrKey to (createPropertyNode(ctx, value, child, field, childrenSize) ?: return null)
}
else -> {
throw UnsupportedOperationException("Handling containers is not implemented yet")
Expand All @@ -224,7 +224,7 @@ class ParallelRequestExecutor(val schema: DefaultSchema) : RequestExecutor {
when (child) {
is Execution.Fragment -> handleFragment(ctx, value, child).toList()
// TODO: Should not be 1
else -> listOf(handleProperty(ctx, value, child, expectedType, 1))
else -> listOfNotNull(handleProperty(ctx, value, child, expectedType, 1))
}
}.fold(mutableMapOf()) { map, entry -> map.merge(entry.first, entry.second) }
}
Expand Down
2 changes: 1 addition & 1 deletion kgraphql/src/test/kotlin/com/apurebase/kgraphql/DataLoaderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ class DataLoaderTest {
val result = schema.executeBlocking(query).also(::println).deserialize()

result.extract<String>("data/abc[0]/person/fullName") shouldBeEqualTo "${jogvan.firstName} ${jogvan.lastName}"
extractOrNull<String>(result, "data/abc[1]/person") shouldBeEqualTo null
result.extract<String?>("data/abc[1]/person") shouldBeEqualTo null
result.extract<String>("data/abc[2]/person/fullName") shouldBeEqualTo "${juul.firstName} ${juul.lastName}"
}
}
Expand Down
17 changes: 5 additions & 12 deletions kgraphql/src/test/kotlin/com/apurebase/kgraphql/TestUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import com.apurebase.kgraphql.schema.Schema
import com.apurebase.kgraphql.schema.dsl.SchemaBuilder
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
import org.amshove.kluent.shouldBeEqualTo
import org.amshove.kluent.shouldEqual
import org.hamcrest.CoreMatchers
import org.hamcrest.FeatureMatcher
import org.hamcrest.MatcherAssert.assertThat
Expand All @@ -31,23 +30,17 @@ fun <T> Map<*, *>.extract(path: String) : T {
try {
return tokens.fold(this as Any?) { workingMap, token ->
if(token.contains('[')){
val list = (workingMap as Map<*,*>)[token.substringBefore('[')]
if (!(workingMap as Map<*, *>).containsKey(token.substringBefore('['))) throw IllegalArgumentException()
val list = workingMap[token.substringBefore('[')]
val index = token.substring(token.indexOf('[')+1, token.length -1).toInt()
(list as List<*>)[index]
} else {
(workingMap as Map<*,*>)[token]
if (!(workingMap as Map<*, *>).containsKey(token)) throw IllegalArgumentException()
workingMap[token]
}
} as T
} catch (e : Exception){
throw IllegalArgumentException("Path: $path does not exist in map: ${this}", e)
}
}

fun <T>extractOrNull(map: Map<*,*>, path : String) : T? {
try {
return map.extract(path)
} catch (e: IllegalArgumentException){
return null
Comment on lines -46 to -50
Copy link
Author

@tiagonuneslx tiagonuneslx Feb 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I removed extractOrNull function because tests using this function were passing regardless of wether the value in the path was null or the path didn't exist.

throw IllegalArgumentException("Path: $path does not exist in map: $this", e)
}
}

Expand Down
12 changes: 9 additions & 3 deletions kgraphql/src/test/kotlin/com/apurebase/kgraphql/integration/BaseSchemaTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package com.apurebase.kgraphql.integration

import com.apurebase.kgraphql.*
import com.apurebase.kgraphql.schema.execution.ExecutionOptions
import org.junit.jupiter.api.AfterEach
import java.io.ByteArrayInputStream


abstract class BaseSchemaTest {
Expand Down Expand Up @@ -317,8 +317,14 @@ abstract class BaseSchemaTest {
@AfterEach
fun cleanup() = createdActors.clear()

fun execute(query: String, variables : String? = null) = testedSchema
.executeBlocking(query, variables)
fun execute(
query: String,
variables: String? = null,
context: Context = Context(emptyMap()),
options: ExecutionOptions = ExecutionOptions(),
operationName: String? = null,
) = testedSchema
.executeBlocking(query, variables, context, options, operationName)
.also(::println)
.deserialize()

Expand Down
14 changes: 8 additions & 6 deletions ...c/test/kotlin/com/apurebase/kgraphql/specification/language/FragmentsSpecificationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ import com.apurebase.kgraphql.integration.BaseSchemaTest
import com.apurebase.kgraphql.integration.BaseSchemaTest.Companion.INTROSPECTION_QUERY
import com.apurebase.kgraphql.GraphQLError
import org.amshove.kluent.invoking
import org.amshove.kluent.shouldBeEqualTo
import org.amshove.kluent.shouldThrow
import org.amshove.kluent.withMessage
import org.hamcrest.CoreMatchers
import org.hamcrest.CoreMatchers.*
import org.hamcrest.MatcherAssert
import org.hamcrest.MatcherAssert.assertThat
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.assertThrows

@Specification("2.8 Fragments")
class FragmentsSpecificationTest {
Expand Down Expand Up @@ -70,8 +72,8 @@ class FragmentsSpecificationTest {
"{\"expandedInfo\":false}"
))
assertNoErrors(response)
assertThat(extractOrNull(response, "data/actor/actualActor/name"), equalTo("Boguś Linda"))
assertThat(extractOrNull(response, "data/actor/actualActor/age"), nullValue())
assertThat(response.extract("data/actor/actualActor/name"), equalTo("Boguś Linda"))
assertThrows<IllegalArgumentException> { response.extract("data/actor/actualActor/age") }
}

@Test
Expand All @@ -83,11 +85,11 @@ class FragmentsSpecificationTest {
when (name) {
"David Fincher" /* director */ -> {
MatcherAssert.assertThat(map.extract<List<*>>("data/people[$i]/favActors"), CoreMatchers.notNullValue())
MatcherAssert.assertThat(extractOrNull<Boolean>(map, "data/people[$i]/isOld"), CoreMatchers.nullValue())
assertThrows<IllegalArgumentException> { map.extract("data/people[$i]/isOld") }
}
"Brad Pitt" /* actor */ -> {
MatcherAssert.assertThat(map.extract<Boolean>("data/people[$i]/isOld"), CoreMatchers.notNullValue())
MatcherAssert.assertThat(extractOrNull<List<*>>(map, "data/people[$i]/favActors"), CoreMatchers.nullValue())
assertThrows<IllegalArgumentException> { map.extract("data/people[$i]/favActors") }
}
}
}
Expand All @@ -102,11 +104,11 @@ class FragmentsSpecificationTest {
when (name) {
"David Fincher" /* director */ -> {
MatcherAssert.assertThat(map.extract<List<*>>("data/people[$i]/favActors"), CoreMatchers.notNullValue())
MatcherAssert.assertThat(extractOrNull<Boolean>(map, "data/people[$i]/isOld"), CoreMatchers.nullValue())
assertThrows<IllegalArgumentException> { map.extract("data/people[$i]/isOld") }
}
"Brad Pitt" /* actor */ -> {
MatcherAssert.assertThat(map.extract<Boolean>("data/people[$i]/isOld"), CoreMatchers.notNullValue())
MatcherAssert.assertThat(extractOrNull<List<*>>(map, "data/people[$i]/favActors"), CoreMatchers.nullValue())
assertThrows<IllegalArgumentException> { map.extract("data/people[$i]/favActors") }
}
}
}
Expand Down
109 changes: 95 additions & 14 deletions ...est/kotlin/com/apurebase/kgraphql/specification/typesystem/DirectivesSpecificationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,56 +2,55 @@ package com.apurebase.kgraphql.specification.typesystem

import com.apurebase.kgraphql.*
import com.apurebase.kgraphql.integration.BaseSchemaTest
import com.apurebase.kgraphql.schema.execution.ExecutionOptions
import com.apurebase.kgraphql.schema.execution.Executor
import org.amshove.kluent.shouldBeEqualTo
import org.amshove.kluent.shouldEqual
import org.hamcrest.CoreMatchers.notNullValue
import org.hamcrest.CoreMatchers.nullValue
import org.hamcrest.MatcherAssert.assertThat
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.assertThrows

@Specification("3.2 Directives")
class DirectivesSpecificationTest : BaseSchemaTest() {

@Test
fun `query with @include directive on field`(){
val map = execute("{film{title, year @include(if: false)}}")
assertThat(extractOrNull(map, "data/film/year"), nullValue())
assertThrows<IllegalArgumentException> { map.extract("data/film/year") }
}

@Test
fun `query with @skip directive on field`(){
val map = execute("{film{title, year @skip(if: true)}}")
assertThat(extractOrNull(map, "data/film/year"), nullValue())
assertThrows<IllegalArgumentException> { map.extract("data/film/year") }
}

@Test
fun `query with @include and @skip directive on field`(){
val mapBothSkip = execute("{film{title, year @include(if: false) @skip(if: true)}}")
assertThat(extractOrNull(mapBothSkip, "data/film/year"), nullValue())
assertThrows<IllegalArgumentException> { mapBothSkip.extract("data/film/year") }

val mapOnlySkip = execute("{film{title, year @include(if: true) @skip(if: true)}}")
assertThat(extractOrNull(mapOnlySkip, "data/film/year"), nullValue())
assertThrows<IllegalArgumentException> { mapOnlySkip.extract("data/film/year") }

val mapOnlyInclude = execute("{film{title, year @include(if: false) @skip(if: false)}}")
assertThat(extractOrNull(mapOnlyInclude, "data/film/year"), nullValue())
assertThrows<IllegalArgumentException> { mapOnlyInclude.extract("data/film/year") }

val mapNeither = execute("{film{title, year @include(if: true) @skip(if: false)}}")
assertThat(extractOrNull(mapNeither, "data/film/year"), notNullValue())
mapNeither.extract<Int>("data/film/year") shouldBeEqualTo 2006
}

@Test
fun `query with @include and @skip directive on field object`() {
val mapWithSkip = execute("{ number(big: true), film @skip(if: true) { title } }")
mapWithSkip.extract<String?>("data/film") shouldBeEqualTo null
assertThrows<IllegalArgumentException> { mapWithSkip.extract("data/film") }

val mapWithoutSkip = execute("{ number(big: true), film @skip(if: false) { title } }")
mapWithoutSkip.extract<String>("data/film/title") shouldBeEqualTo "Prestige"

val mapWithInclude = execute("{ number(big: true), film @include(if: true) { title } }")
mapWithInclude.extract<String?>("data/film/title") shouldBeEqualTo "Prestige"
mapWithInclude.extract<String>("data/film/title") shouldBeEqualTo "Prestige"

val mapWithoutInclude = execute("{ number(big: true), film @include(if: false) { title } }")
mapWithoutInclude.extract<String>("data/film") shouldBeEqualTo null
assertThrows<IllegalArgumentException> { mapWithoutInclude.extract("data/film") }
}

@Test
Expand All @@ -60,6 +59,88 @@ class DirectivesSpecificationTest : BaseSchemaTest() {
"query film (\$include: Boolean!) {film{title, year @include(if: \$include)}}",
"{\"include\":\"false\"}"
)
assertThat(extractOrNull(map, "data/film/year"), nullValue())
assertThrows<IllegalArgumentException> { map.extract("data/film/year") }
}

@Test
fun `query with @include directive on field (using DataLoaderPrepared executor)`() {
val map = execute(
"{film{title, year @include(if: false)}}",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
assertThrows<IllegalArgumentException> { map.extract("data/film/year") }
}

@Test
fun `query with @skip directive on field (using DataLoaderPrepared executor)`() {
val map = execute(
"{film{title, year @skip(if: true)}}",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
assertThrows<IllegalArgumentException> { map.extract("data/film/year") }
}

@Test
fun `query with @include and @skip directive on field (using DataLoaderPrepared executor)`() {
val mapBothSkip = execute(
"{film{title, year @include(if: false) @skip(if: true)}}",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
assertThrows<IllegalArgumentException> { mapBothSkip.extract("data/film/year") }

val mapOnlySkip = execute(
"{film{title, year @include(if: true) @skip(if: true)}}",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
assertThrows<IllegalArgumentException> { mapOnlySkip.extract("data/film/year") }

val mapOnlyInclude = execute(
"{film{title, year @include(if: false) @skip(if: false)}}",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
assertThrows<IllegalArgumentException> { mapOnlyInclude.extract("data/film/year") }

val mapNeither = execute(
"{film{title, year @include(if: true) @skip(if: false)}}",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
mapNeither.extract<Int>("data/film/year") shouldBeEqualTo 2006
}

@Test
fun `query with @include and @skip directive on field object (using DataLoaderPrepared executor)`() {
val mapWithSkip = execute(
"{ number(big: true), film @skip(if: true) { title } }",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
assertThrows<IllegalArgumentException> { mapWithSkip.extract("data/film") }

val mapWithoutSkip = execute(
"{ number(big: true), film @skip(if: false) { title } }",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
mapWithoutSkip.extract<String>("data/film/title") shouldBeEqualTo "Prestige"

val mapWithInclude = execute(
"{ number(big: true), film @include(if: true) { title } }",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
mapWithInclude.extract<String>("data/film/title") shouldBeEqualTo "Prestige"

val mapWithoutInclude = execute(
"{ number(big: true), film @include(if: false) { title } }",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
assertThrows<IllegalArgumentException> { mapWithoutInclude.extract("data/film") }
}

@Test
fun `query with @include directive on field with variable (using DataLoaderPrepared executor)`() {
val map = execute(
"query film (\$include: Boolean!) {film{title, year @include(if: \$include)}}",
"{\"include\":\"false\"}",
options = ExecutionOptions(executor = Executor.DataLoaderPrepared)
)
assertThrows<IllegalArgumentException> { map.extract("data/film/year") }
}
}
6 changes: 3 additions & 3 deletions ...rc/test/kotlin/com/apurebase/kgraphql/specification/typesystem/UnionsSpecificationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
package com.apurebase.kgraphql.specification.typesystem

import com.apurebase.kgraphql.*
import com.apurebase.kgraphql.helpers.getFields
import com.apurebase.kgraphql.integration.BaseSchemaTest
import com.apurebase.kgraphql.schema.SchemaException
import com.apurebase.kgraphql.GraphQLError
import com.apurebase.kgraphql.helpers.getFields
import com.apurebase.kgraphql.schema.execution.Execution
import org.amshove.kluent.*
import org.hamcrest.CoreMatchers
import org.hamcrest.MatcherAssert
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.assertThrows


@Specification("3.1.4 Unions")
Expand Down Expand Up @@ -236,7 +236,7 @@ class UnionsSpecificationTest : BaseSchemaTest() {
}
""".trimIndent()).also(::println).deserialize().run {
extract<Int>("data/returnUnion/i") shouldBeEqualTo 1
extract<String?>("data/returnUnion/s") shouldBeEqualTo null
assertThrows<IllegalArgumentException> { extract("data/returnUnion/s") }
extract<List<String>>("data/returnUnion/fields") shouldBeEqualTo listOf("i", "fields", "s")
}
}
Expand Down