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

defer classes to talk to gateway #518

Merged
merged 8 commits into from
Mar 5, 2024
Merged

Conversation

sbarker2
Copy link
Collaborator

@sbarker2 sbarker2 commented Mar 3, 2024

No description provided.

@sbarker2 sbarker2 changed the title IN PROGRESS - defer classes to talk to agg defer classes to talk to agg Mar 4, 2024
@sbarker2 sbarker2 changed the title defer classes to talk to agg defer classes to talk to gateway Mar 4, 2024
felipe-gdr
felipe-gdr previously approved these changes Mar 4, 2024
@@ -1,7 +1,16 @@
package graphql.nadel

import graphql.incremental.DelayedIncrementalPartialResult
import org.reactivestreams.Publisher

open class ServiceExecutionResult @JvmOverloads constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make ServiceExecutionResult sealed, and not make NadelncrementalServiceExecutionResult open?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yeah, forgot to change that back

Copy link
Collaborator Author

@sbarker2 sbarker2 Mar 4, 2024

Choose a reason for hiding this comment

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

@felipe-gdr Hmm, I'm not sure if this is what we want actually. Sealed types cannot be instantiated, but it needs to be instantiated here in IntrospectionService.kt

https://github.com/atlassian-labs/nadel/blob/9897a5db6024cd179c8b1840ee9043448618cb7c/lib/src/main/java/graphql/nadel/engine/blueprint/IntrospectionService.kt#L48C1-L51C18

(causing the build error)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right
[A sealed class is abstract by itself, it cannot be instantiated directly](https://kotlinlang.org/docs/sealed-classes.html#location-of-direct-subclasses)

We could create a ServiceExecutionResultImpl class that extends it and instantiate that instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed, thanks

bbakerman
bbakerman previously approved these changes Mar 4, 2024
@sbarker2 sbarker2 merged commit e978849 into master Mar 5, 2024
2 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