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

Union hydration validation #663

Merged
merged 3 commits into from
Jan 7, 2025
Merged

Union hydration validation #663

merged 3 commits into from
Jan 7, 2025

Conversation

gnawf
Copy link
Collaborator

@gnawf gnawf commented Jan 6, 2025

Adds validation to

  • Ensure all union members have a corresponding @hydrated that returns the union member type.
  • Ensure this also works with ID hydration.

Copy link

github-actions bot commented Jan 6, 2025

Test Results

  138 files  ±0  138 suites  ±0   58s ⏱️ +2s
1 031 tests +3  964 ✅ +3  67 💤 ±0  0 ❌ ±0 
1 039 runs  +3  972 ✅ +3  67 💤 ±0  0 ❌ ±0 

Results for commit 7d91dd7. ± Comparison against base commit 38ac346.

This pull request removes 6 and adds 9 tests. Note that renamed tests count towards both.
graphql.nadel.validation.NadelHydrationValidationTest ‑ checks the output type of the actor field against the output type of the hydrated field()
graphql.nadel.validation.NadelHydrationValidationTest ‑ fails if actor output type does not implement interface()
graphql.nadel.validation.NadelHydrationValidationTest ‑ fails if hydration actor field does not exist()
graphql.nadel.validation.NadelHydrationValidationTest ‑ fails if hydration actor field exists only in the underlying and not in the overall()
graphql.nadel.validation.NadelHydrationValidationTest ‑ passes if actor output type belongs in union()
graphql.nadel.validation.NadelHydrationValidationTest ‑ passes if actor output type implements the interface()
graphql.nadel.validation.NadelHydrationValidationTest ‑ checks the output type of the backing field against the output type of the hydrated field()
graphql.nadel.validation.NadelHydrationValidationTest ‑ fails if backing output type does not implement interface()
graphql.nadel.validation.NadelHydrationValidationTest ‑ fails if hydration backing field does not exist()
graphql.nadel.validation.NadelHydrationValidationTest ‑ fails if hydration backing field exists only in the underlying and not in the overall()
graphql.nadel.validation.NadelHydrationValidationTest ‑ passes if backing output type belongs in union()
graphql.nadel.validation.NadelHydrationValidationTest ‑ passes if backing output type implements the interface()
graphql.nadel.validation.NadelHydrationValidationTest2 ‑ union hydration can use a mix of @idHydrated and @hydrated()
graphql.nadel.validation.NadelHydrationValidationTest2 ‑ union hydration is missing default hydrations for some members()
graphql.nadel.validation.NadelHydrationValidationTest2 ‑ union hydration uses @idHydrated for all member types()

♻️ This comment has been updated with latest results.

@@ -56,13 +52,6 @@ class NadelDefaultHydrationDefinition(
val batchSize: Int
get() = appliedDirective.getArgument(Keyword.batchSize).getValue()

val arguments: List<NadelHydrationArgumentDefinition>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not used

@@ -34,11 +32,9 @@ class NadelDefaultHydrationDefinition(
"How to identify matching results"
identifiedBy: String! = "id"
"The batch size"
batchSize: Int
batchSize: Int! = 200
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to match normal @hydrated definition.

Not sure why it was different to begin with.

unionMemberType as GraphQLObjectType,
).onErrorCast { return it }
.mapNotNull { unionMemberType ->
if ((unionMemberType as? GraphQLNamedType)?.getDefaultHydrationOrNull() == null) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only resolve hydration for those types wiith default hydration defined.

@@ -79,36 +84,44 @@ internal class NadelIdHydrationDefinitionParser {
val defaultHydration = (type as? GraphQLNamedType)?.getDefaultHydrationOrNull()
?: return NadelValidationInterimResult.Error.of(NadelMissingDefaultHydrationError(parent, virtualField))

val hydration = object : NadelHydrationDefinition {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made this a named class

@gnawf gnawf merged commit d981abb into master Jan 7, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants