Handle external types that do not exist gracefully

[mduesterhoeft]

Fixes https://github.com/apollographql/apollo-server/issues/3859

Currently, the gateway fails when an entity referenced by an external type does not exist. Thus even entities that exist and could be processed further are not considered.

I added a book product to the test data set with upc = 9999999999 which is not know by the product service but not by the book service.

When the resolvable and the non-resolvable book are queried, the response is the following:

[Object: null prototype] {
      products: [
        [Object: null prototype] {
          upc: '9999999999',
          name: null,
          price: '31'
        },
        [Object: null prototype] {
          upc: '0987654321',
          name: null,
          price: '29'
        }
      ]
    }

Notice how the name is not resolved also for 0987654321 which would be possible to resolve. Also If I query these two books in a different order, the result becomes different. The gateway would resolve everything until it processes the book unknown to the book service. Every following product would fail to resolve further.

With this PR such errors are collected but do not interrupt processing entities further that could be resolved. So the result for the query above is:

[Object: null prototype] {
      products: [
        [Object: null prototype] {
          upc: '9999999999',
          name: 'undefined (undefined)',
          price: '31'
        },
        [Object: null prototype] {
          upc: '0987654321',
          name: 'No Books Like This Book! (2019)',
          price: '29'
        }
      ]
    }

[apollo-cla]

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

[mduesterhoeft]

Any thoughts on this one? Any chance for this suggestion to be merged? (cc @abernix)

[trevor-scheer]

Overall I'm very happy with the simplicity of the implementation here. This all makes sense. One comment within this review that I'd love to hear your thoughts on.

[trevor-scheer]

This part gives me the most pause.

The string with undefined bits is a result of the name resolver receiving unexpectedly missing @requires data, but carrying on anyway. If the requires fields were nested, this would throw.

name(object, { delimeter }) {
  return `${object.title}${delimeter}(${object.year})`;
},

Does this mean our resolvers needs to treat all @requires fields as nullable? I think so! Is that unreasonable or terrible, maybe not - maybe it's the best we can do? In any case, I think we need to document this since it's a potential pitfall of any @requires fields that our resolvers expect to have. And with a wider lens, I think we could set the same expectations for any requests that cross service boundaries like this.

I don't think I have any real ask with this comment, but I do want to raise concerns and instigate conversation about the implications here.

[mduesterhoeft]

Thanks for your review @trevor-scheer . I also was wondering about this case. In this case here both fields are nullable. And I think external fields should always be. Maybe we should document this as a kind of best-practice.

[nihalgonsalves]

Agreed - this isn't something that's documented much in the Federation docs, but extending with non-nullable fields makes your aggregated service not fault-tolerant at all.

[trevor-scheer]

Also, please add a changelog entry (the gateway has a specific changelog separate from the monorepo's)

[mduesterhoeft]

@trevor-scheer thanks for your review. I added the changelog entry.

[mduesterhoeft]

@trevor-scheer do you see a chance to merge this soon? With the changelog entry change, we get conflicts here a lot and it becomes harder for me to keep the PR up-to-date. Thanks!

[mduesterhoeft]

I rebased the PR again - any chance for this to be merged @trevor-scheer

(Just trying a last time and will give up afterwards)

[abernix]

Hi @mduesterhoeft!,

This is a semi-automated message. Apologies for the lack of personal detail. I’d encourage reading the rest of this message but, if you're busy, hopefully this is as easy for you — or someone who is willing to own the contribution — as pushing the button later in this message to re-create this PR, from an auto-migrated branch, on the new Federation repository. If you have time, read on!

We’re in the process of moving federation-related concerns out of this repository (where it has historically lived) into a new home specifically for Apollo Federation. This PR is affected by the transition since it touches code which has been moved.

I’ve done some preparations to make this as easy as possible, but we’ll need to close this PR, and I could use your help in re-opening it on the new Apollo Federation repository.

In apollographql/federation#134 (which has a lot more details, if you’re interested), we introduced the same code that was in this repository to the new Federation repository (maintaining its history) and removed the code from this repository in #4561.

While this PR still needs to get reviewed and approved to land, it should no longer live in this repository in its current state since it cannot merge in anymore. To facilitate the movement of this PR to the new home, I’ve automatically generated branches on the new repository using the same commit authorship and messages that you originally included on this PR.

Pull-requests can’t be moved on GitHub in the same way Issues can be. While I could just create the PRs from these new branches too, the contribution belongs to you!

---

Original PR author only: Click this button to open a new PR from the auto-created apollo-server-import/pr-3914 branch on the new Apollo Federation repository

(The code and your commits should be the same and you will have an opportunity to confirm, but this way you can continue to be the author and track its progress on the new Federation repository!)

---

There’s no easy way to bring over any existing comments on this PR, so I encourage you to copy and paste those onto the new issue if possible.

Overall, I hope this process is relatively easy for you while maintaining your commit contribution authorship. I apologize for any inconvenience caused by this shuffle, but please ping me if I can help in any way!

🚀