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(federation): Require user-defined @tag directive definition when @tag is used #882

Merged
merged 11 commits into from
Jul 18, 2021

Conversation

trevor-scheer
Copy link
Member

@trevor-scheer trevor-scheer commented Jul 16, 2021

  • Document new error codes

@trevor-scheer trevor-scheer self-assigned this Jul 16, 2021
@trevor-scheer trevor-scheer added this to the MM-2021-07 milestone Jul 16, 2021
@trevor-scheer trevor-scheer changed the base branch from main to release-gateway-0.34 July 16, 2021 18:30
@glasser
Copy link
Member

glasser commented Jul 16, 2021

Could this instead be structured as "require all used directives to be defined, with exceptions made for historical reasons for the Federation Spec directives"?

@trevor-scheer
Copy link
Member Author

Some good discussion spun out of @glasser's comment that resulted in us realizing a great way to do typical SDL validation of the subgraphs. We can perform most of the validations and just omit the rules that make a subgraph invalid + filter against our special federation directives.

A snippet, which I'll capture in a separate issue as well.

import { federationDirectives } from '../../../directives';
import { specifiedSDLRules } from 'graphql/validation/specifiedRules';
import { validateSDL } from 'graphql/validation/validate';
import { ServiceDefinition } from '../../types';
// import { errorWithCode, logDirective } from '../../utils';

const rulesToExclude = ['KnownTypeNamesRule', 'PossibleTypeExtensionsRule'];
const errorsToFilter = federationDirectives.map(directive => directive.name);
/**
 * If there are tag usages in the service definition, check that the tag directive
 * definition is included and correct.
 */
export const graphqlValidations = ({
  name: _serviceName,
  typeDefs,
}: ServiceDefinition) => {
  const filteredRules = specifiedSDLRules.filter((rule) =>
    !rulesToExclude.includes(rule.name),
  );
  const errors = validateSDL(typeDefs, undefined, filteredRules);
  return errors.filter(({ message }) => {
    return !errorsToFilter.some(keyWord => message.includes(keyWord));
  });
};

Going to apply a similar approach (using graphql-js and specific rules) to accomplish some of this instead.

Copy link
Member

@glasser glasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't necessarily think we need to add ad hoc "defined correctly" checks before we have a solid pattern but it doesn't hurt. I have a few suggestions below but otherwise looks good.

@trevor-scheer trevor-scheer force-pushed the trevor/require-tag-definition branch from 5609271 to 7596b53 Compare July 18, 2021 20:55
@trevor-scheer trevor-scheer merged commit c3bd6dd into release-gateway-0.34 Jul 18, 2021
@trevor-scheer trevor-scheer deleted the trevor/require-tag-definition branch July 18, 2021 21:02
trevor-scheer added a commit that referenced this pull request Jul 21, 2021
Release
* @apollo/gateway@0.34.0
* @apollo/federation@0.27.0
* @apollo/harmonizer@0.27
* @apollo/query-planner@0.3.0

PRs:
* feat(gateway): Default to Uplink for composed supergraph managed federation (#881)
* fix(federation): Require user-defined @tag directive definition (#882)
* Remove @inaccessible elements when converting to API schema (#807)
* Move toAPISchema call into try/catch block (#894)
* fix(gateway): Prevent inaccessible type names from being leaked in error messages (#893)
* docs: rm instruction to set APOLLO_SCHEMA_CONFIG_DELIVERY_ENDPOINT for Uplink (#899)
* fix(gateway): Remove path, query and variables field from subgraph error responses (#900)
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.

3 participants