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

return empty response when subscription are running on the internal introspection service #649

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

temaEmelyan
Copy link
Member

@temaEmelyan temaEmelyan commented Dec 5, 2024

Please make sure you consider the following:

  • Add tests that use __typename in queries
  • Does this change work with all nadel transformations (rename, type rename, hydration, etc)? Add tests for this.
  • Is it worth using hints for this change in order to be able to enable a percentage rollout?
  • Do we need to add integration tests for this change in the graphql gateway?
  • Do we need a pollinator check for this?

Copy link

github-actions bot commented Dec 5, 2024

Test Results

  138 files  ±0  138 suites  ±0   56s ⏱️ -2s
1 023 tests +3  956 ✅ +3  67 💤 ±0  0 ❌ ±0 
1 031 runs  +3  964 ✅ +3  67 💤 ±0  0 ❌ ±0 

Results for commit 9ec7555. ± Comparison against base commit a6ccb09.

♻️ This comment has been updated with latest results.

"locations": [],
"message": "null",
"extensions": {
"classification": "ValidationError"
Copy link
Member Author

Choose a reason for hiding this comment

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

this error is coming from a "remove field" transform that we use for tests. I activated this transform with onCommentUpdated(id: ID): Comment @toBeDeleted directive in the overall schema

Copy link
Member Author

Choose a reason for hiding this comment

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

previously we would blow up with your data fetcher must return a publisher ... kind of an error

@@ -40,6 +41,9 @@ open class NadelDefaultIntrospectionRunner(schema: GraphQLSchema) : ServiceExecu
.build()

override fun execute(serviceExecutionParameters: ServiceExecutionParameters): CompletableFuture<ServiceExecutionResult> {
if (serviceExecutionParameters.operationDefinition.operation == OperationDefinition.Operation.SUBSCRIPTION) {
return CompletableFuture.completedFuture(NadelServiceExecutionResultImpl())
Copy link
Contributor

Choose a reason for hiding this comment

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

So just empty result?

russellyou
russellyou previously approved these changes Dec 5, 2024
bbakerman
bbakerman previously approved these changes Dec 9, 2024
"locations": [],
"message": "null",
"extensions": {
"classification": "ValidationError"
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is an error from the introspection service? Why is the error message null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I clarified the error message on the RemoveFieldTransform to make it less confusing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see

@temaEmelyan temaEmelyan dismissed stale reviews from bbakerman and russellyou via 1213053 December 12, 2024 05:36
@temaEmelyan temaEmelyan self-assigned this Dec 12, 2024
@temaEmelyan temaEmelyan merged commit 8a5e1b4 into master Dec 17, 2024
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.

4 participants