Ready - Improved hydration validation #463

sbarker2 · 2023-10-09T03:20:48Z

Merged: https://github.com/atlassian-labs/nadel/pull/461/files/e3ff70e3eb4a60f03f1fed2a6e014dc2775de069

This PR is Ready For Review and not awaiting any other PRs 👍

Tested in central schema ✅
Passed Engine Tests ✅
Added unit tests covering all cases ✅

lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt

test/src/test/kotlin/graphql/nadel/tests/EngineTests.kt

…entValidation.kt Co-authored-by: Franklin Wang <[email protected]>

Co-authored-by: Franklin Wang <[email protected]>

test/src/test/kotlin/graphql/nadel/tests/CentralSchemaTesting.kt

lib/src/main/java/graphql/nadel/validation/NadelHydrationValidation.kt

sbarker2 · 2023-10-12T01:27:20Z

lib/src/main/java/graphql/nadel/validation/NadelHydrationValidation.kt

-                null
+            StaticArgument -> {
+                val staticArg = remoteArgSource.staticValue
+                if (!validationUtil.isValidLiteralValue(


Just a note / context:

isValidLiteralValue is a public function from this class in graphql-java:
https://github.com/graphql-java/graphql-java/blob/dd50648b5e178f965918d0c352787bd81654c7fe/src/main/java/graphql/validation/ValidationUtil.java

This is an internal class, but I have checked over the code and confirmed it is what we want, including recursively checking inner types of objects/functions and everything.
Have also added extensive tests for static args to check this in NadelHydrationArgumentValidationTest.kt
It is an internal class in

sbarker2 · 2023-10-12T02:11:41Z

lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt

-            if (actorFieldArgType.isNonNull && !hydrationSourceFieldType.isNonNull) {
+            //need to check null compatibility
+            val sourceType = remoteArg.remoteArgumentSource.sourceType
+            if (sourceType != ObjectField && actorFieldArgType.isNonNull && !hydrationSourceFieldType.isNonNull) {


added sourceType != ObjectField so that it ignores null compatibility for $source arguments as that is handled differently by nadel.

(As discussed in the live PR review)

sbarker2 · 2023-10-12T22:48:51Z

lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt

+            //could have ID feed into [ID] (as a possible batch hydration case).
+            // in this case we need to unwrap the list and check types
+            if (isBatchHydration && (!unwrappedHydrationSourceFieldType.isList && unwrappedActorFieldArgType.isList)) {
+                val error = getHydrationInputErrors(


Note:

a recurring theme here is:
val error = getErrors()
if (error != null) { return Error(blah)}

the reason we cant just simplify this to return getErrors() is because we are recursively unwrapping the supplied & actor arg types, but for the error, it is generally more helpful for the message to display the original type, not some inner type that we have unwrapped

lib/src/main/java/graphql/nadel/validation/NadelHydrationValidation.kt

sbarker2 · 2023-10-12T22:52:16Z

lib/src/main/java/graphql/nadel/validation/NadelHydrationValidation.kt

@@ -142,50 +139,51 @@ internal class NadelHydrationValidation(
    }

    private fun getArgumentErrors(


actual added logic for this file is in this function

sbarker2 · 2023-10-16T03:42:06Z

lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt

+                    (typeToAssign.name == Scalars.GraphQLString.name || typeToAssign.name == Scalars.GraphQLInt.name)) {
+                return true
+            }
+            if (targetType.name == Scalars.GraphQLString.name && typeToAssign.name == Scalars.GraphQLID.name) {


Allowing ID -> String. see comment here: https://hello.atlassian.net/wiki/spaces/PSRV/pages/2987269760/Analysis+of++Current+Hydration+Arg+Validation+Errors+in+Central+Schema?focusedCommentId=2990608894

Not sure I agree with this.

Sure, we'll have a discussion on the page

lib/src/main/java/graphql/nadel/validation/NadelHydrationValidation.kt

lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt

gnawf · 2023-10-16T19:10:06Z

lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt

+                    (typeToAssign.name == Scalars.GraphQLString.name || typeToAssign.name == Scalars.GraphQLInt.name)) {
+                return true
+            }
+            if (targetType.name == Scalars.GraphQLString.name && typeToAssign.name == Scalars.GraphQLID.name) {


Not sure I agree with this.

gnawf · 2023-10-16T19:10:50Z

lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt

+
+            // scalar feed into scalar
+            if (unwrappedHydrationSourceFieldType is GraphQLScalarType && unwrappedActorFieldArgType is GraphQLScalarType) {
+                return validateScalarArg(hydrationSourceFieldType, actorFieldArgType, parent, overallField, remoteArg, actorFieldName)


Same comment as before, can you pass in unwrappedHydrationSourceFieldType here instead of passing in the raw GraphQLType and then unwrapping it inside the function?

passing in wrapped type because we want to preserve the wrapped type to print in the error message to avoid any confusion from the user

the only reason we could pass in the unwrapped type previously (in the case that its an object) is because we do not show the type in that one error message

lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt

Co-authored-by: Franklin Wang <[email protected]>

…ation.kt Co-authored-by: Franklin Wang <[email protected]>

…ssian-labs/nadel into improved-hydration-validation

…Type

sbarker2 added 2 commits October 8, 2023 14:40

add static & array hydration validation (objects to come)

d94ab04

add hydration arg validation

f9bb817

sbarker2 requested a review from gnawf October 9, 2023 03:20

gnawf reviewed Oct 9, 2023

View reviewed changes

lib/src/main/java/graphql/nadel/validation/NadelHydrationArgumentValidation.kt Outdated Show resolved Hide resolved

update / add tests

ef9f72d

gnawf reviewed Oct 9, 2023

View reviewed changes

add edge cases for batch hydration and manyToOne hydration

7e89235

sbarker2 requested a review from bbakerman October 9, 2023 23:55

bbakerman reviewed Oct 10, 2023

View reviewed changes

test/src/test/kotlin/graphql/nadel/tests/EngineTests.kt Outdated Show resolved Hide resolved

sbarker2 and others added 2 commits October 10, 2023 15:04

add tests for batch hydration and non-batch ManyToOne edge cases

660ca73

Update lib/src/main/java/graphql/nadel/validation/NadelHydrationArgum…

6d79700

…entValidation.kt Co-authored-by: Franklin Wang <[email protected]>

sbarker2 self-assigned this Oct 11, 2023

sbarker2 and others added 5 commits October 11, 2023 16:21

Apply suggestions from code review

5e46d0c

Co-authored-by: Franklin Wang <[email protected]>

default to running all tests in EngineTests

3fbc9ce

unwrap and use smart casting

650a2d2

skip outer null check as it is already being done elsewhere

1b5caed

Merge branch 'master' into improved-hydration-validation

0bebf4a

sbarker2 commented Oct 11, 2023

View reviewed changes

test/src/test/kotlin/graphql/nadel/tests/CentralSchemaTesting.kt Outdated Show resolved Hide resolved

sbarker2 added 2 commits October 12, 2023 11:39

remove firstPass logic

9aea487

add static arg logic

d350b46

sbarker2 commented Oct 12, 2023

View reviewed changes

lib/src/main/java/graphql/nadel/validation/NadelHydrationValidation.kt Show resolved Hide resolved

sbarker2 commented Oct 12, 2023

View reviewed changes

ignore null check for $source arguments

2d3a6cc

sbarker2 commented Oct 12, 2023

View reviewed changes

add hydration arg validation case for $argument object

ee43711

sbarker2 requested a review from felipe-gdr October 12, 2023 03:20

sbarker2 added 2 commits October 12, 2023 16:30

tidying up logic & tests

043e128

add more tests for static arg hydration validation

598e3bd

sbarker2 requested review from dondonz and russellyou October 12, 2023 22:14

sbarker2 requested a review from temaEmelyan October 12, 2023 22:14

format code

ee3929b

sbarker2 changed the title ~~WIP - Improved hydration validation~~ Ready - Improved hydration validation Oct 12, 2023

sbarker2 requested review from gnawf and bbakerman October 12, 2023 22:42

sbarker2 commented Oct 12, 2023

View reviewed changes

lib/src/main/java/graphql/nadel/validation/NadelHydrationValidation.kt Show resolved Hide resolved

sbarker2 commented Oct 12, 2023

View reviewed changes

sbarker2 added 6 commits October 13, 2023 11:56

tidy up error message

b99db73

change schema for static arg check

9854491

add actor field name to error message

c31aa9e

add enum logic and tests

b6467b5

allow ID -> String in validation

75f7c48

change tests to allow ID -> String

c9298a0

sbarker2 commented Oct 16, 2023

View reviewed changes

bbakerman previously approved these changes Oct 16, 2023

View reviewed changes

gnawf requested changes Oct 16, 2023

View reviewed changes

Apply suggestions from code review

05f3001

Co-authored-by: Franklin Wang <[email protected]>

sbarker2 dismissed bbakerman’s stale review via 05f3001 October 16, 2023 19:36

sbarker2 and others added 9 commits October 17, 2023 08:44

Update lib/src/main/java/graphql/nadel/validation/NadelHydrationValid…

0b310a8

…ation.kt Co-authored-by: Franklin Wang <[email protected]>

change type of hydrationSourceFieldTYpe

02ee361

Merge branch 'improved-hydration-validation' of ssh://github.com/atla…

84ade39

…ssian-labs/nadel into improved-hydration-validation

change argument validation class pattern

10a97b0

remove unused variables

bd3ca33

tidy up unwrapping logic

426d481

rename hydrationSourceFieldType to more accurate name hydrationSource…

0f09c4c

…Type

reformat code

366fd3f

simplify code logic

3d7f219

gnawf approved these changes Oct 17, 2023

View reviewed changes

sbarker2 merged commit 8844fb9 into master Oct 17, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ready - Improved hydration validation #463

Ready - Improved hydration validation #463

sbarker2 commented Oct 9, 2023 •

edited

Loading

sbarker2 Oct 12, 2023 •

edited

Loading

sbarker2 Oct 12, 2023 •

edited

Loading

sbarker2 Oct 12, 2023 •

edited

Loading

sbarker2 Oct 12, 2023

sbarker2 Oct 16, 2023

gnawf Oct 16, 2023

sbarker2 Oct 16, 2023

gnawf Oct 16, 2023

gnawf Oct 16, 2023

sbarker2 Oct 16, 2023 •

edited

Loading

sbarker2 Oct 16, 2023

		@@ -142,50 +139,51 @@ internal class NadelHydrationValidation(
		}

		private fun getArgumentErrors(

Ready - Improved hydration validation #463

Ready - Improved hydration validation #463

Conversation

sbarker2 commented Oct 9, 2023 • edited Loading

sbarker2 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

sbarker2 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

sbarker2 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

sbarker2 Oct 12, 2023

Choose a reason for hiding this comment

sbarker2 Oct 16, 2023

Choose a reason for hiding this comment

gnawf Oct 16, 2023

Choose a reason for hiding this comment

sbarker2 Oct 16, 2023

Choose a reason for hiding this comment

gnawf Oct 16, 2023

Choose a reason for hiding this comment

gnawf Oct 16, 2023

Choose a reason for hiding this comment

sbarker2 Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

sbarker2 Oct 16, 2023

Choose a reason for hiding this comment

sbarker2 commented Oct 9, 2023 •

edited

Loading

sbarker2 Oct 12, 2023 •

edited

Loading

sbarker2 Oct 12, 2023 •

edited

Loading

sbarker2 Oct 12, 2023 •

edited

Loading

sbarker2 Oct 16, 2023 •

edited

Loading