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

Never wiring factory tweaks #642

Merged
merged 6 commits into from
Jan 8, 2025
Merged

Conversation

bbakerman
Copy link
Collaborator

This bumps the graphql-java version which has the singleton DF code

And it returns a single DF as a default

Copy link

github-actions bot commented Nov 28, 2024

Test Results

  138 files  ±0  138 suites  ±0   58s ⏱️ -2s
1 032 tests ±0  965 ✅ ±0  67 💤 ±0  0 ❌ ±0 
1 040 runs  ±0  973 ✅ ±0  67 💤 ±0  0 ❌ ±0 

Results for commit f1b136d. ± Comparison against base commit 232aab9.

♻️ This comment has been updated with latest results.

@@ -39,7 +39,7 @@ def releaseVersion = System.env.RELEASE_VERSION
version = releaseVersion ? releaseVersion : getDevelopmentVersion()
group = "com.atlassian"
allprojects {
description = "Nadel is a Java library that combines multiple GrahpQL services together into one API."
description = "Nadel is a Kotlin library that combines multiple GraphQL services together into one GraphQL API."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bugged me

@@ -100,18 +100,31 @@ open class NeverWiringFactory : WiringFactory {
}

override fun providesDataFetcher(environment: FieldWiringEnvironment): Boolean {
return true
return false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we dont create a DF per field or make a CodeRegistry entry per field as well but rather use a default

}

override fun getDefaultDataFetcher(environment: FieldWiringEnvironment): DataFetcher<*>? {
return 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.

no default makes it use CodeRegistry default

val schema = overallSchemaGenerator.buildOverallSchema(
serviceRegistries,
builder.overallWiringFactory,
NeverWiringFactory.NEVER_CODE_REGISTRY
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this passed in? Seems like it should be baked into OverallSchemaGenerator

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -84,9 +86,7 @@ open class NeverWiringFactory : WiringFactory {
}

override fun getTypeResolver(environment: InterfaceWiringEnvironment): TypeResolver {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make all of these resolves like functions final e.g.

final override fun getTypeResolver(environment: InterfaceWiringEnvironment): TypeResolver {
  return NEVER_TR
}

Feels like they should never be overriden because we should always delegate to the hardcoded NEVER_CODE_REGISTRY

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

kinda done - we do make a specific version of it hence its an open class

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for tests

@@ -24,10 +25,12 @@ internal class OverallSchemaGenerator {
fun buildOverallSchema(
serviceRegistries: List<NadelDefinitionRegistry>,
wiringFactory: WiringFactory,
codeRegistry: GraphQLCodeRegistry,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think this should be hardcoded if it's not an option in the NadelSchemas.Builder

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we apply the same thing to UnderlyingSchemaGenerator ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@bbakerman bbakerman merged commit 4a63d8f into master Jan 8, 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.

2 participants