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

Fix bug with null result for federated objects #101

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

jbreeden-sfx
Copy link
Contributor

@jbreeden-sfx jbreeden-sfx commented Mar 27, 2020

Problem

I'm experiencing an issue with federation. I have a schema like the following:

# Service 1
type Parent {
  child: Child
}

type Child {
    id: ID!
}
type Child {
  id: ID!
  name: String!
}

When I make a request like this:

{
  parent("someid") {
    child {
      id
      name
    }
  }
}

It works if all parents have a child. However, if any one child field is null, then the result of the query is basically empty. No data. No error.

Solution

The MR detects the null root object and returns a nil insertionPoint list, so that we don't execute the next step for a null child.

It also validates that the field is nullable. If it's not, and the root object is null, then it triggers an error. In this case, the result is still empty. Unfortunately I haven't figured out how to solve that part yet, but I've at least added a warning log for it. It should be uncommon, as it means there is a bug in the queryer (it's returning null for non-nullable fields), but still it would be nice to return something like "cannot return null for non-nullable field to the caller.

@AlecAivazis
Copy link
Member

Ah good catch!

I suspect that most responses that return null when they shouldn't would contain an error from the backing server's runtime that gets bubbled up. But if not, we might be able to address it as part of the work going into #100.

@jbreeden-sfx
Copy link
Contributor Author

Sorry, @AlecAivazis, my description was wrong. The child field of Parentis nullable in my case. So no error from the backing service is expected (indeed, it's not even called), but the gateway still returned empty data.

I've updated my description to reflect the correct schema.

@jbreeden-splunk jbreeden-splunk force-pushed the task/handle-nil-federated-object branch from f23e3c8 to 7b23fa2 Compare April 3, 2020 14:02
@jbreeden-sfx
Copy link
Contributor Author

I've gone ahead and resolved the merge conflict. The test still needs to be filled in. In fact I think you have a stub test for this case already 😆

func TestFindInsertionPoint_handlesNullObjects(t *testing.T) {
	t.Skip("Not yet implemented")
}

I'll take a whack at that when I get a chance.

@AlecAivazis AlecAivazis merged commit d95dab6 into master Apr 21, 2020
@AlecAivazis AlecAivazis deleted the task/handle-nil-federated-object branch April 21, 2020 21:14
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