add check for hydration arg type against actor field #408
Conversation
engine-nextgen/src/main/kotlin/graphql/nadel/validation/NadelSchemaValidationError.kt
Outdated
Show resolved
Hide resolved
engine-nextgen/src/test/kotlin/graphql/nadel/validation/NadelHydrationValidationTest.kt
Outdated
Show resolved
Hide resolved
engine-nextgen/src/main/kotlin/graphql/nadel/validation/NadelHydrationValidation.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # lib/src/main/java/graphql/nadel/validation/NadelHydrationValidation.kt # lib/src/main/java/graphql/nadel/validation/NadelSchemaValidationError.kt
| @@ -139,82 +150,4 @@ internal class NadelHydrationValidation( | |||
| return typeValidation + outputTypeMustBeNullable | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
moved all this into it's own class
|
|
||
| return duplicatedArgumentsErrors + remoteArgErrors + missingActorArgErrors | ||
| } | ||
|
|
There was a problem hiding this comment.
everything above this moved from NadelHydrationArgumentValidation class, everything below is new
| @@ -0,0 +1,1004 @@ | |||
| package graphql.nadel.validation | |||
There was a problem hiding this comment.
skip to line 390 for the new tests
| val errors = validate(fixture) | ||
| assert(errors.map { it.message }.isEmpty()) | ||
| } | ||
|
|
There was a problem hiding this comment.
everything below is new
faizan-siddiqui
changed the title
| assert(error.overallField.name == "creator") | ||
| assert(error.remoteArg.name == "id") | ||
| assert(GraphQLTypeUtil.simplePrint(error.hydrationType) == "ID!") | ||
| assert(GraphQLTypeUtil.simplePrint(error.actorArgInputType) == "Int!") |
There was a problem hiding this comment.
Hmm technically this is allowed. An input ID can be of Int type. We can circle back if we need.
lib/src/test/kotlin/graphql/nadel/validation/NadelHydrationArgumentValidationTest.kt
Outdated
Show resolved
Hide resolved
lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt
Outdated
Show resolved
Hide resolved
lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt
Outdated
Show resolved
Hide resolved
lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt
Outdated
Show resolved
Hide resolved
| var hydrationSource: GraphQLType = hydrationSourceType | ||
| var actorArgument: GraphQLType = actorArgumentInputType | ||
|
|
||
| while (hydrationSource.isWrapped && actorArgument.isWrapped) { |
There was a problem hiding this comment.
I'm really not sure about this code… Can you explain it more? Perhaps it's okay.
I really think we should have checked whether the hydration was batched or not before taking any action… This code makes too many assumptions on whether something is batched or not and there's a lot of paths to follow.
There was a problem hiding this comment.
yea so I found over here was really the only appropriate place I can check to see if it's a batch hydration, as I need to unravel and check the actor argument input type and the hydration source type, to see if one is a list and the other is a single value, there's no where before this execution that could indicate whether a hydration is batched or not, even the isBatched function checks if the actorFieldDef is a list type
|
@gnawf comments implemented or replied to, plz re-review:) |
No description provided.