mercurius-js / mercurius-gateway

Mercurius federation support plugin
MIT License
17 stars 11 forks source link

federation mandatory field is not working #24

Open nabsofken opened 3 years ago

nabsofken commented 3 years ago

When starting a federated service with one non mandatory federated service down, the entire federated service doesn't start. This is the error I see:


    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1146:16)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:21376) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:21376) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.```
mcollina commented 3 years ago

Can you provide steps to reproduce?

aldis-ameriks commented 3 years ago

I believe the mandatory flag is used only when the schema is being refreshed. Meaning that if any of the (even non-mandatory) services are not running when starting the gateway, there will be a promise rejection during the init phase. I also just stumbled on this and was wondering if it would make sense that the non-mandatory services do not throw an error during init if the schema refresh (via pollingInterval) is also enabled.

If schema refresh is not enabled, then the non-mandatory service would never actually join the federation if the service was not running during init. In which case it might be better for the user if gateway exits if any of the services are unavailable during init.

mcollina commented 3 years ago

Non-mandatory should not throw if missing during init.

nabsofken commented 3 years ago

import fastify from 'fastify';
import mercurius from 'mercurius';
import fastifyCors from 'fastify-cors';

const init = (opts = {logger:{level:"error"}}) => {
  const gateway = fastify(opts);

  gateway.register(fastifyCors);

  gateway.register(mercurius, {
    graphiql: "playground",
    gateway: {
      services: [
        {
          name: 'appointments',
          url: 'http://localhost:4002/graphql',
          mandatory: true
        },
        {
          name: 'invoice',
          url: 'http://localhost:4001/graphql',
          mandatory: false
        },
        {
          name: 'profile',
          url: 'http://localhost:4004/graphql',
          mandatory: false
        },
        {
          name: 'search',
          url: 'http://localhost:4003/graphql',
          mandatory: false
        },
      ],
    },
  });
  return gateway;
}

if (require.main === module) {
  init().listen(4000);
}

export default init;```
nabsofken commented 3 years ago

And the federated service is as follows:


import fastify from 'fastify';
import mercurius from 'mercurius';
import fastifyCors from 'fastify-cors';
import { typeDefs } from './schema';
import { resolvers } from './resolvers';

const init = (opts = {logger:{level:"error"}}) => {
  const app = fastify(opts);

  app.register(fastifyCors);

  app.register(mercurius, {
    graphiql: "playground",
    schema: typeDefs,
    resolvers,
    federationMetadata: true
  });

  return app;
}

if (require.main === module) {
  init().listen(4001);
}

export default init;```
nabsofken commented 3 years ago

@mcollina the docs for the mandatory flag mentioned that one service is necessary does that mean I need at least 1 mandatory service?

mcollina commented 3 years ago

Maybe @asciidisco or @pscarey might help you more.

asciidisco commented 3 years ago

@nabsofken

It's not really possible to further debug your problem with the code samples you uploaded, not for a problem which contains that much variables like the one you're describing... So, if you could create a minimal reproducible testcase & upload that one to a public place, it will be much easier to help you.

To elaborate a it further on the mandatory flag, I asked for it during the PR that added the schema refresh functionality (you can read a bit more about the reasoning here: https://github.com/mercurius-js/mercurius/pull/206#issuecomment-660644278).

To answer your specific question, yes, at least one service needs to be mandatory, otherwise no federated schema could be produced on startup. In a GraphQL federation, schemas can explicitely depend on each other by extending types from that other services schema; so those services should be considered mandatory at startup in order to resolve those dependencies. If a service has no dependents (and the thing you're building does work without it from a user experience perspective), then it's considered a best practice in a GraphQL Federation to mark that service/schema as optional (not mandatory) as you're leveraging one of the strengths of GraphQL Federation here: A system that keeps working even if parts of it fail & are unavailable for some reason.

I hope that clarifies things a bit. As said, your specific problem can't be solved without a minimal test-case I'm afraid. If you can provide one, I'm happy to have a look.

nabsofken commented 3 years ago

@asciidisco here is the sample code https://github.com/pashamo/federation-test To reproduce the issue run npm run serviceA only then run the npm run gateway. The gateway throws an error even though serviceB is set as not mandatory.

nabsofken commented 3 years ago

@asciidisco did you get a chance to look into the test case?

asciidisco commented 3 years ago

@nabsofken Not until now; I scheduled some time within the next 2 days to do so; will get back to you with more details once I have them. At a first glance, your code looks fine.

asciidisco commented 3 years ago

@nabsofken So, I was able to check your problem. The mandatory flag/feature seems to be not working as advertised; meaning it only does 50% of the job it is supposed to do. It works when the schema needs to be refreshed, meaning a non-mandatory service (your ServiceB) is allowed to not respond when an already running federation does request/refresh its schema, then there'll be no error. Which is what this flag is supposed to accomplish.

What doesn't work, if the service isn't online & responding at the federation startup, the whole thing bails out. This seems to be due to the fact, that the defaultErrorHandler isn't called (that's the one who checks if an error should be thrown according to the mandators flag or not) if creating the federated schema at startup fails. It gets called, if a refresh operation fails, allowing the system to work as configured.

I've checked your code against the fastify-gql version 5.3.0 which introduced the feature, and it did never work as intended.

So it should be classified as a bug, reason is that the defaultErrorHandler is not called on the initial schema request, at least not for ECONNREFUSED errors.

@nabsofken you think you could provide a PR with a fix?

valdestron commented 3 years ago

@nabsofken So, I was able to check your problem. The mandatory flag/feature seems to be not working as advertised; meaning it only does 50% of the job it is supposed to do. It works when the schema needs to be refreshed, meaning a non-mandatory service (your ServiceB) is allowed to not respond when an already running federation does request/refresh its schema, then there'll be no error. Which is what this flag is supposed to accomplish.

What doesn't work, if the service isn't online & responding at the federation startup, the whole thing bails out. This seems to be due to the fact, that the defaultErrorHandler isn't called (that's the one who checks if an error should be thrown according to the mandators flag or not) if creating the federated schema at startup fails. It gets called, if a refresh operation fails, allowing the system to work as configured.

I've checked your code against the fastify-gql version 5.3.0 which introduced the feature, and it did never work as intended.

So it should be classified as a bug, reason is that the defaultErrorHandler is not called on the initial schema request, at least not for ECONNREFUSED errors.

@nabsofken you think you could provide a PR with a fix?

This mandatory flag would be awesome feature, if only gateway could start without mandatory services, otherwise it is absolutely not usable because we can not trust the flag and have to implement schema querying for each service before starting up the gateway. All services becomes hard dependencies. E.g. non-mandatory service is down and gateway pod restarting at that time.