Fix Missing Referenced Fragments in Query Document

[tylerbwong]

Fixes #2644.

[apollo-cla]

@tylerbwong: 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/

[martinbonnin]

Nice catch, Thanks a lot!

[tylerbwong]

@martinbonnin We'll have to hold off on merging this. The current patch doesn't walk every child fragment on operation.fragments. Should be working as intended in b7dcb05.

[martinbonnin]

Thanks for digging into this!

Looking at this code made me realise two things:

1. looks like we're going to have the same issue with inline fragments, which are not even in the Operation IR at the moment
2. We should add validation on the root fragments so that querying a fragment on another type than the root (schema.queryType) fails:

query PlanetRootQueryBug {
  #This should fail with 
  # Fragment `planetFragment` can't be spread here as result can never be of type Root
  ...planetFragment
}

fragment planetFragment on Planet {
  name
  diameter
  rotationPeriod
  orbitalPeriod
}

Let me know if you want to look into this as part of this PR or I can look into it in a follow up PR.

[tylerbwong]

I'll look into both issues as part of this PR!

[martinbonnin]

Mostly nitpicking at this point but it feels a bit "off" that we need a special case for the root fragmentsRefs compared to field.fragmentRefs. What about moving the schema.rootTypeForOperationType(operationType) to the call site and re-using List<FragmentRef>.findFragments ?

[tylerbwong]

Ah nice catch. A lot of code here ended up being mostly duplicated from List<FragmentRef>.findFragments and I didn't even realize. Will refactor! I think I'll keep this function here though since we still need to do some extra lookups for all the nested fragments inside the root fragment.

[martinbonnin]

👍

[tylerbwong]

So after looking into supporting root inline fragments, I was able to add inlineFragments to the Operation IR and even get the Kotlin codegen to work properly for that use-case, but I wasn't able to figure out the Java codegen. If it's alright I think maybe we can get these changes merged in for root fragment support and I can open a separate issue/PR for supporting root inline fragments as a follow-up?

[martinbonnin]

If it's alright I think maybe we can get these changes merged in for root fragment support and I can open a separate issue/PR for supporting root inline fragments as a follow-up?

That sounds good 👍 , thanks again !

Re: inline fragment, I didn't realise this would require big changes to the codegen. That's an area that's changing considerably in version 3.0.0 so we might want to implement that in the dev-3.x branch directly. I'll leave it up to you whether to:

1. fix in main first and merge to dev-3.x later (but codegen will be most likely quite different, meaning a bunch of conflicts, I can help with merging if needed)
2. or fix directly in dev-3.x and backport in main if required (maybe it won't be required).

In all cases, feel free to ignore the java codegen for the time being as it's currently removed from 3.0.0 (see #2616)