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

Add max query depth #458

Merged
merged 3 commits into from
Jul 13, 2023
Merged

Add max query depth #458

merged 3 commits into from
Jul 13, 2023

Conversation

gnawf
Copy link
Collaborator

@gnawf gnawf commented Jul 13, 2023

Ok I also removed the engine abstraction stuff and added transforms and introspectionRunnerFactory to Nadel.Builder instead of passing those directly to NextgenEngine.

Tested this new build with the Gateway and it works fine.

@@ -7,7 +7,7 @@ plugins {
id("com.bnorm.power.kotlin-power-assert")
}

val graphqlJavaVersion = "0.0.0-2023-06-07T00-38-50-5b19375"
val graphqlJavaVersion = "0.0.0-2023-07-12T23-55-39-fa8cf1b"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Brings in the max ENF depth change.

@@ -38,15 +41,12 @@ import java.util.concurrent.atomic.AtomicReference
import java.util.function.Function

class Nadel private constructor(
private val engine: NadelExecutionEngine,
private val engine: NextgenEngine,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the engine abstraction. Nadel itself creates the engine now. All constructor params that were tied to NextgenEngine are now in Nadel.Builder.

internal val instrumentation: NadelInstrumentation,
internal val serviceExecutionHooks: ServiceExecutionHooks,
internal val preparsedDocumentProvider: PreparsedDocumentProvider,
internal val executionIdProvider: ExecutionIdProvider,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed some things that are no longer needed in this class. Most of the things are now directly passed down to NextgenEngine

private var instrumentation: NadelInstrumentation = object : NadelInstrumentation {}
private var serviceExecutionHooks: ServiceExecutionHooks = object : ServiceExecutionHooks {}
private var preparsedDocumentProvider: PreparsedDocumentProvider = NoOpPreparsedDocumentProvider.INSTANCE
private var executionIdProvider = ExecutionIdProvider.DEFAULT_EXECUTION_ID_PROVIDER
private var engineFactory: NadelExecutionEngineFactory = NadelExecutionEngineFactory(::NextgenEngine)
private var transforms = emptyList<NadelTransform<out Any>>()
private var introspectionRunnerFactory = NadelIntrospectionRunnerFactory(::NadelDefaultIntrospectionRunner)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These used to be constructor parameters to NextgenEngine but now are here


private var schemaBuilder = NadelSchemas.Builder()

private var maxQueryDepth = Integer.MAX_VALUE
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New option to set max ENF depth


class NextgenEngine @JvmOverloads constructor(
nadel: Nadel,
internal class NextgenEngine(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of this is just changes to the constructor now that the engine abstraction is gone.

The Nadel.Builder now just passes everything required directly in here, so we don't need to go nadel. to get everything out.


override fun execute(
private val operationParseOptions = executableNormalizedOperationFactoryOptions()
.maxChildrenDepth(maxQueryDepth)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Base options with the query depth set.

@gnawf gnawf requested review from bbakerman, felipe-gdr and temaEmelyan and removed request for bbakerman July 13, 2023 00:57
Copy link
Member

@temaEmelyan temaEmelyan left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

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

Nice one

@gnawf gnawf merged commit 6e10a07 into master Jul 13, 2023
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