Skip to content
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
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
<dependency>
<groupId>com.graphql-java</groupId>
<artifactId>graphql-java</artifactId>
<version>12.0</version>
<version>15.0</version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jexp Do you think we need to add a compatibility layer to support older versions of graphql-java. Since they are not very compatible code my break. E.g. I'm using graphql-spqr-spring-boot-starter with this library, which has not yet been updated to use version 15.0 of graphql-java. There is an open issue for this leangen/graphql-spqr#356 . So also other users my have this compatibility issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was similar to the position I was in and why I tried to help update the library here. I wanted to use a library similar to spqr (graphql-kotlin) which uses a later graphql-java. Rather than a compatibility layer, I opted to help move this project forward with the thinking that a rising tide lifts all boats. I could help update other projects too. This is a common problem with libraries that use common dependencies. It is often an issue with Guava or something similar. The two strategies I've seen most often used successfully are to severely limit dependencies used in a library and/or to be aggressive about getting everyone to update the common deps.

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion is:

  1. we release a version of 1.0.1 with the nested sorting PR merged and graphql-java 12 (and create a branch/tag for that)
  2. then we release 1.1.0 with the upgrade of graphql-java to 15 (master)

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
Expand Down
22 changes: 12 additions & 10 deletions src/main/kotlin/org/neo4j/graphql/BuildingEnv.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ package org.neo4j.graphql
import graphql.Scalars
import graphql.schema.*

class BuildingEnv(val types: MutableMap<String, GraphQLType>) {
import graphql.schema.GraphQLTypeUtil.simplePrint

class BuildingEnv(val types: MutableMap<String, GraphQLNamedType>) {

private val typesForRelation = types.values
.filterIsInstance<GraphQLObjectType>()
Expand All @@ -23,7 +25,7 @@ class BuildingEnv(val types: MutableMap<String, GraphQLType>) {
type = GraphQLNonNull(type)
}
return GraphQLFieldDefinition.newFieldDefinition()
.name("$prefix${resultType.name}")
.name("$prefix${simplePrint(resultType)}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

use resultType.name()

.arguments(getInputValueDefinitions(scalarFields, forceOptionalProvider))
.type(type.ref() as GraphQLOutputType)
}
Expand Down Expand Up @@ -71,7 +73,7 @@ class BuildingEnv(val types: MutableMap<String, GraphQLType>) {
}
val existingFilterType = types[filterName]
if (existingFilterType != null) {
return (existingFilterType as? GraphQLInputType)?.name
return simplePrint(existingFilterType as? GraphQLInputType)
Copy link
Collaborator

Choose a reason for hiding this comment

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

use (existingFilterType as? GraphQLInputType)?.name()

?: throw IllegalStateException("Filter type $filterName is already defined but not an input type")
}
createdTypes.add(filterName)
Expand All @@ -87,7 +89,7 @@ class BuildingEnv(val types: MutableMap<String, GraphQLType>) {
.forEach { field ->
val typeDefinition = field.type.inner()
val filterType = when {
typeDefinition.isNeo4jType() -> getInputType(typeDefinition).name
typeDefinition.isNeo4jType() -> getInputType(typeDefinition).requiredName()
typeDefinition.isScalar() -> typeDefinition.innerName()
typeDefinition is GraphQLEnumType -> typeDefinition.innerName()
else -> addFilterType(getInnerFieldsContainer(typeDefinition), createdTypes)
Expand Down Expand Up @@ -122,7 +124,7 @@ class BuildingEnv(val types: MutableMap<String, GraphQLType>) {
val orderingName = "_${type.name}Ordering"
var existingOrderingType = types[orderingName]
if (existingOrderingType != null) {
return (existingOrderingType as? GraphQLInputType)?.name
return (existingOrderingType as? GraphQLInputType)?.requiredName()
?: throw IllegalStateException("Ordering type $type.name is already defined but not an input type")
}
val sortingFields = type.fieldDefinitions
Expand Down Expand Up @@ -190,7 +192,7 @@ class BuildingEnv(val types: MutableMap<String, GraphQLType>) {
?: throw IllegalArgumentException("${innerType.name} is unknown")
}
return innerType as? GraphQLFieldsContainer
?: throw IllegalArgumentException("${innerType.name} is neither an object nor an interface")
?: throw IllegalArgumentException("${innerType.name()} is neither an object nor an interface")
}

private fun getInputType(type: GraphQLType): GraphQLInputType {
Expand All @@ -200,12 +202,12 @@ class BuildingEnv(val types: MutableMap<String, GraphQLType>) {
}
if (inner.isNeo4jType()) {
return neo4jTypeDefinitions
.find { it.typeDefinition == inner.name }
.find { it.typeDefinition == inner.name() }
?.let { types[it.inputDefinition] } as? GraphQLInputType
?: throw IllegalArgumentException("Cannot find input type for ${inner.name}")
?: throw IllegalArgumentException("Cannot find input type for ${inner.name()}")
}
return type as? GraphQLInputType
?: throw IllegalArgumentException("${type.name} is not allowed for input")
?: throw IllegalArgumentException("${type.name()} is not allowed for input")
}

}
}
10 changes: 7 additions & 3 deletions src/main/kotlin/org/neo4j/graphql/GraphQLExtensions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,11 @@ fun GraphQLType.inner(): GraphQLType = when (this) {
else -> this
}

fun GraphQLType.name(): String? = (this as? GraphQLNamedType)?.name
fun GraphQLType.requiredName(): String = requireNotNull(name()) { -> "name is required but cannot be determined for " + this.javaClass }

fun GraphQLType.isList() = this is GraphQLList || (this is GraphQLNonNull && this.wrappedType is GraphQLList)
fun GraphQLType.isScalar() = this.inner().let { it is GraphQLScalarType || it.name.startsWith("_Neo4j") }
fun GraphQLType.isScalar() = this.inner().let { it is GraphQLScalarType || it.innerName().startsWith("_Neo4j") }
fun GraphQLType.isNeo4jType() = this.innerName().startsWith("_Neo4j")
fun GraphQLType.isNeo4jSpatialType() = this.innerName().startsWith("_Neo4jPoint")
fun GraphQLFieldDefinition.isNeo4jType(): Boolean = this.type.isNeo4jType()
Expand Down Expand Up @@ -119,7 +122,7 @@ fun GraphQLType.ref(): GraphQLType = when (this) {
is GraphQLScalarType -> this
is GraphQLEnumType -> this
is GraphQLTypeReference -> this
else -> GraphQLTypeReference(name)
else -> GraphQLTypeReference(name())
}

fun relDetails(type: GraphQLFieldsContainer, relDirective: GraphQLDirective): RelationshipInfo {
Expand Down Expand Up @@ -183,7 +186,8 @@ data class RelationshipInfo(
}

fun Field.aliasOrName() = (this.alias ?: this.name).quote()
fun GraphQLType.innerName(): String = inner().name
fun GraphQLType.innerName(): String = inner().name()
?: throw IllegalStateException("inner name cannot be retrieved for " + this.javaClass)

fun GraphQLFieldDefinition.propertyName() = getDirectiveArgument(PROPERTY, PROPERTY_NAME, this.name)!!

Expand Down
4 changes: 2 additions & 2 deletions src/main/kotlin/org/neo4j/graphql/Neo4jTypes.kt
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ data class Neo4jQueryConversion(val name: String, val propertyName: String, val
if (!isNeo4jType) {
Neo4jQueryConversion(name, name)
}
val converter = getNeo4jTypeConverter(fieldDefinition.type.inner().name)
val converter = getNeo4jTypeConverter(fieldDefinition.type.innerName())
val objectValue = (value as? ObjectValue)
?.objectFields
?.map { it.name to it.value }
Expand All @@ -80,4 +80,4 @@ val neo4jTypeDefinitions = listOf(
TypeDefinition("Time", "_Neo4jLocalTime"),
TypeDefinition("LocalDateTime", "_Neo4jLocalDateTime"),
TypeDefinition("Point", "_Neo4jPoint")
)
)
9 changes: 4 additions & 5 deletions src/main/kotlin/org/neo4j/graphql/Predicates.kt
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ data class ExpressionPredicate(
) : Predicate {
val not = if (op.not) "NOT " else ""
override fun toExpression(variable: String): Cypher {
val paramName: String = ProjectionBase.FILTER + paramName(variable, name, value).capitalize() + "_" + op.name + nestedField.replace('.','_')
val paramName: String = ProjectionBase.FILTER + paramName(variable, name, value).capitalize() + "_" + op.name + nestedField.replace('.', '_')
val query = if (fieldDefinition.isNativeId()) {
if (op.list) {
"${not}ID($variable) ${op.op} [id IN \$$paramName | toInteger(id)]"
Expand Down Expand Up @@ -151,7 +151,7 @@ data class RelationPredicate(val fieldName: String, val op: RelationOperator, va
RelationOperator.NOT -> "ALL" // bc of not
else -> op.op
}
if (type.getFieldDefinition(fieldName).isList()) {
if (type.getFieldDefinition(fieldName).type.isList()) {
if (op == RelationOperator.EQ_OR_NOT_EXISTS) {
LOGGER.info("$fieldName on type ${type.name} was used for filtering, consider using ${fieldName}${RelationOperator.EVERY.suffix} instead")
}
Expand Down Expand Up @@ -254,7 +254,7 @@ enum class FieldOperator(
fun resolve(queriedField: String, field: GraphQLFieldDefinition, value: Any?): FieldOperator? {
val fieldName = field.name
if (value == null) {
return listOf(IS_NULL, IS_NOT_NULL).find { queriedField == fieldName + it.suffix } ?: return null
return listOf(IS_NULL, IS_NOT_NULL).find { queriedField == fieldName + it.suffix }
}
val ops = enumValues<FieldOperator>().filterNot { it == IS_NULL || it == IS_NOT_NULL }
return ops.find { queriedField == fieldName + it.suffix }
Expand All @@ -263,7 +263,6 @@ enum class FieldOperator(
} else {
null
}
?: return null
}

fun forType(type: GraphQLType): List<FieldOperator> =
Expand All @@ -277,7 +276,7 @@ enum class FieldOperator(
// todo list types
!type.isScalar() -> listOf(EQ, NEQ, IN, NIN)
else -> listOf(EQ, NEQ, IN, NIN, LT, LTE, GT, GTE) +
if (type.name == "String" || type.name == "ID") listOf(C, NC, SW, NSW, EW, NEW) else emptyList()
if (type.name() == "String" || type.name() == "ID") listOf(C, NC, SW, NSW, EW, NEW) else emptyList()
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/main/kotlin/org/neo4j/graphql/SchemaBuilder.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import graphql.Scalars
import graphql.language.*
import graphql.schema.*
import graphql.schema.idl.RuntimeWiring
import graphql.schema.idl.ScalarInfo.STANDARD_SCALAR_DEFINITIONS
import graphql.schema.idl.ScalarInfo.GRAPHQL_SPECIFICATION_SCALARS_DEFINITIONS
import graphql.schema.idl.SchemaGenerator
import graphql.schema.idl.SchemaParser
import graphql.schema.idl.TypeDefinitionRegistry
Expand Down Expand Up @@ -43,7 +43,7 @@ object SchemaBuilder {

val builder = RuntimeWiring.newRuntimeWiring()
typeDefinitionRegistry.scalars()
.filterNot { entry -> STANDARD_SCALAR_DEFINITIONS.containsKey(entry.key) }
.filterNot { entry -> GRAPHQL_SPECIFICATION_SCALARS_DEFINITIONS.containsKey(entry.key) }
.forEach { (name, definition) ->
val scalar = when (name) {
"DynamicProperties" -> DynamicProperties.INSTANCE
Expand Down
16 changes: 13 additions & 3 deletions src/test/kotlin/org/neo4j/graphql/TranslatorExceptionTests.kt
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,26 @@ class TranslatorExceptionTests : AsciiDocTestSuite("translator-tests1.adoc") {
}

override fun schemaTestFactory(schema: String): List<DynamicNode> {
val translator = Translator(SchemaBuilder.buildSchema(schema));
val translator = Translator(SchemaBuilder.buildSchema(schema))
return listOf(
DynamicTest.dynamicTest("unknownType") {
Assertions.assertThrows(IllegalArgumentException::class.java) {
translator.translate(" { company { name } } ")
translator.translate("""
{
company {
name
}
}
""")
}
},
DynamicTest.dynamicTest("mutation") {
Assertions.assertThrows(InvalidSyntaxException::class.java) {
translator.translate(" { createPerson() } ")
translator.translate("""
{
createPerson()
}
""".trimIndent())
}
}

Expand Down
38 changes: 8 additions & 30 deletions src/test/kotlin/org/neo4j/graphql/utils/GraphQLSchemaTestSuite.kt
Original file line number Diff line number Diff line change
@@ -1,21 +1,23 @@
package org.neo4j.graphql.utils

import graphql.language.InterfaceTypeDefinition
import graphql.schema.GraphQLFieldDefinition
import graphql.schema.GraphQLObjectType
import graphql.schema.GraphQLSchema
import graphql.schema.GraphQLType
import graphql.schema.diff.DiffSet
import graphql.schema.diff.SchemaDiff
import graphql.schema.diff.reporting.CapturingReporter
import graphql.schema.idl.*
import graphql.schema.idl.RuntimeWiring
import graphql.schema.idl.SchemaGenerator
import graphql.schema.idl.SchemaParser
import graphql.schema.idl.SchemaPrinter
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Assumptions
import org.junit.jupiter.api.DynamicNode
import org.junit.jupiter.api.DynamicTest
import org.neo4j.graphql.DynamicProperties
import org.neo4j.graphql.SchemaBuilder
import org.neo4j.graphql.SchemaConfig
import org.neo4j.graphql.requiredName
import org.opentest4j.AssertionFailedError
import java.util.*
import java.util.regex.Pattern
Expand Down Expand Up @@ -76,40 +78,16 @@ class GraphQLSchemaTestSuite(fileName: String) : AsciiDocTestSuite(fileName) {
private val SCHEMA_PRINTER = SchemaPrinter(SchemaPrinter.Options.defaultOptions()
.includeDirectives(true)
.includeScalarTypes(true)
.includeExtendedScalarTypes(true)
.includeSchemaDefintion(true)
.includeSchemaDefinition(true)
.includeIntrospectionTypes(false)
.setComparators(DefaultSchemaPrinterComparatorRegistry.newComparators()
.addComparator({ env ->
env.parentType(GraphQLObjectType::class.java)
env.elementType(GraphQLFieldDefinition::class.java)
}, GraphQLFieldDefinition::class.java, fun(o1: GraphQLFieldDefinition, o2: GraphQLFieldDefinition): Int {
val (op1, name1) = o1.splitName()
val (op2, name2) = o2.splitName()
if (op1 == null && op2 == null) {
return name1.compareTo(name2)
}
if (op1 == null) {
return -1
}
if (op2 == null) {
return 1
}
val prio1 = name1.compareTo(name2)
if (prio1 == 0) {
return op1.compareTo(op2)
}
return prio1
})
.build())
)

fun GraphQLType.splitName(): Pair<String?, String> {
val m = METHOD_PATTERN.matcher(this.name)
val m = METHOD_PATTERN.matcher(this.requiredName())
return if (m.find()) {
m.group(1) to m.group(2).toLowerCase()
} else {
null to this.name.toLowerCase()
null to this.requiredName().toLowerCase()
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/test/resources/translator-tests1.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -499,12 +499,12 @@ query($_param:String) { p:values(_param:$_param) { age } }
----
MATCH (p:Person)
WHERE p._param = $_param
AND p._string = $p_string
AND p._int = $p_int
AND p._float = $p_float
AND p._array = $p_array
AND p._boolean = $p_boolean
AND p._enum = $p_enum
AND p._float = $p_float
AND p._int = $p_int
AND p._string = $p_string
AND p._boolean = $p_boolean
RETURN p { .age } AS p
----

Expand Down