do not perform nested fetches if the parent one returned null

[Ty3uK]

Related to https://github.com/apollographql/federation/blob/main/gateway-js/src/executeQueryPlan.ts#L179

If I have two subgraphs (simplified):

type Mutation {
	mutationA: Mutation
}

type Mutation {
	mutationB: Boolean
}

And run mutation like this:

mutation {
	mutationA {
		mutationB
	}
}

Then, mutationB will be called even if mutationA returns null

[apollo-cla]

@Ty3uK: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[netlify]

✅ Deploy Preview for apollo-router-docs canceled.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	c228ce9
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-router-docs/deploys/62be2495d1cc85000841f13d

[Geal]

it's not the right place to do that. The check should be performed at variables aggregation in https://github.com/apollographql/router/blob/main/apollo-router/src/query_planner/mod.rs#L492-L508
as you can see there's already a check to avoid a subquery if the list of variables is empty, this needs an additional check that the variables are not null

[Ty3uK]

Thanks for feedback!
Do I need to perform this check inside match?

[Ty3uK]

Also, the problem is that with my example, variables are empty object ({}) in both cases

[Ty3uK]

@Geal, fixed, hope I understood you right. 🙂

[Geal]

sorry 😓 I was not very clear, I think it has to be done inside Variables::new. If you want I can take a closer look at it and add some tests

[Ty3uK]

@Geal,

I have free time, so I can do it by myself 🙂

One question: do I need to add this check inside this else branch?

And another question - do I need to extend existing tests or create a separate one?

[Ty3uK]

@Geal, check latest commit please 🙂

[Geal]

thanks, looking into it

[Geal]

so, we need a bit more context to evaluate this. The code looks ok but I don't understand the use case enough to decide. Correct me if I'm wrong: this is used to "batch" mutations and do them in a specific order, right? Could you tell more about the kind of use case that warrants this design?

Could you add a test to validate this code? There are examples to create a router and subgraph services in https://github.com/apollographql/router/blob/main/apollo-router/src/query_planner/mod.rs#L687

Thank you for taking the time to patch this :)

[Ty3uK]

@Geal, let's look into some production code from my work 🙂

Schema

type Captcha_Captcha {
    token: String!
    image: String!
}

input Captcha_CaptchaCreateInput {
    width: Int!
    height: Int!
}

type Captcha_CaptchaCreateError implements Error @shareable {
    message: String!
}

type Captcha_CaptchaCreateLimitError implements Error @shareable {
    message: String!
}

input Captcha_CaptchaVerifyInput {
    token: String!
    text: String!
}

type Captcha_CaptchaVerificationInputError implements Error @shareable {
    message: String!
}

type Captcha_CaptchaVerificationError implements Error @shareable {
    message: String!
}

type Captcha_CaptchaVerificationLimitError implements Error @shareable {
    message: String!
}

union Captcha_CaptchaCreateResult =
    | Captcha_Captcha
    | Captcha_CaptchaCreateError
    | Captcha_CaptchaCreateLimitError

type Captcha_CaptchaVerifyResult {
    error: Error
    mutation: Mutation
}

type Captcha_CaptchaMutations {
    create(input: Captcha_CaptchaCreateInput): Captcha_CaptchaCreateResult!
    verify(input: Captcha_CaptchaVerifyInput!): Captcha_CaptchaVerifyResult!
}

extend type Mutation {
    captcha: Captcha_CaptchaMutations
}

With this schema, I can verify captcha and do next mutation in one request. As I know, there is no limitation for doing such thing 🙂 And, as I say in first message, @apollo/graphql doesn't have this problem.

Question about testing: do I need to change existing schema.graphql or create a separate one?

[Geal]

if you can make the test schema smaller it would be nice, but otherwise that could work

[Ty3uK]

@Geal, I'm so sorry, I don't have enough experience in Rust to write tests 😕
I pushed schema and query plan, can I ask you to write the test?

[Geal]

just FYI, I have not forgotten about your PR, I had to handle other features this week, I'll get back to this one next week to write the test

[o0Ignition0o]

This looks great!

[Ty3uK]

@Geal, @BrynCooke, @o0Ignition0o
Thank you so much!
Can you tell me what I need to do next? 🙂

[o0Ignition0o]

@Ty3uK There seems to be a merge conflict, i thinks you want to update the main branch and solve conflicts, do you know how to do this or would you like us to pair on that ? :)

[Ty3uK]

@o0Ignition0o, thanks, I'll do it 🙂

[Ty3uK]

@o0Ignition0o, I merged main into my branch without conflicts (as I can see) 🙂

[o0Ignition0o]

Awesome!

Let's run the Continuous Integration checks, it looks like you are getting super close to landing your first contribution to the router! 🏅

[o0Ignition0o]

Tests are passing on my machine, let's land this!