Skip to content

Allow use of legacy names for schema validation #1194

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

Merged
merged 1 commit into from
Jan 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/type/__tests__/validation-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,43 @@ describe('Type System: Objects must have fields', () => {
},
]);
});

it('accepts an Object type with explicitly allowed legacy named fields', () => {
const schemaBad = new GraphQLSchema({
query: new GraphQLObjectType({
name: 'Query',
fields: { __badName: { type: GraphQLString } },
}),
});
const schemaOk = new GraphQLSchema({
query: new GraphQLObjectType({
name: 'Query',
fields: { __badName: { type: GraphQLString } },
}),
allowedLegacyNames: ['__badName'],
});
expect(validateSchema(schemaBad)).to.containSubset([
{
message:
'Name "__badName" must not begin with "__", which is reserved by ' +
'GraphQL introspection.',
},
]);
expect(validateSchema(schemaOk)).to.deep.equal([]);
});

it('throws with bad value for explicitly allowed legacy names', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thorough 👌

expect(
() =>
new GraphQLSchema({
query: new GraphQLObjectType({
name: 'Query',
fields: { __badName: { type: GraphQLString } },
}),
allowedLegacyNames: true,
}),
).to.throw('"allowedLegacyNames" must be Array if provided but got: true.');
});
});

describe('Type System: Fields args must be properly named', () => {
Expand Down
17 changes: 17 additions & 0 deletions src/type/schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ export class GraphQLSchema {
_possibleTypeMap: ?ObjMap<ObjMap<boolean>>;
// Used as a cache for validateSchema().
__validationErrors: ?$ReadOnlyArray<GraphQLError>;
// Referenced by validateSchema().
__allowedLegacyNames: ?$ReadOnlyArray<string>;

constructor(config: GraphQLSchemaConfig): void {
// If this schema was built from a source known to be valid, then it may be
Expand All @@ -103,6 +105,12 @@ export class GraphQLSchema {
'"directives" must be Array if provided but got: ' +
`${String(config.directives)}.`,
);
invariant(
!config.allowedLegacyNames || Array.isArray(config.allowedLegacyNames),
'"allowedLegacyNames" must be Array if provided but got: ' +
`${String(config.allowedLegacyNames)}.`,
);
this.__allowedLegacyNames = config.allowedLegacyNames;
}

this._queryType = config.query;
Expand Down Expand Up @@ -228,6 +236,15 @@ type GraphQLSchemaConfig = {
directives?: ?Array<GraphQLDirective>,
astNode?: ?SchemaDefinitionNode,
assumeValid?: boolean,
/**
* If provided, the schema will consider fields or types with names included
* in this list valid, even if they do not adhere to the specification's
* schema validation rules.
*
* This option is provided to ease adoption and may be removed in a future
* major release.
*/
allowedLegacyNames?: ?Array<string>,
};

function typeMapReducer(map: TypeMap, type: ?GraphQLType): TypeMap {
Expand Down
16 changes: 13 additions & 3 deletions src/type/validate.js
Original file line number Diff line number Diff line change
Expand Up @@ -215,9 +215,17 @@ function validateName(
context: SchemaValidationContext,
node: { +name: string, +astNode: ?ASTNode },
): void {
// If a schema explicitly allows some legacy name which is no longer valid,
// allow it to be assumed valid.
if (
context.schema.__allowedLegacyNames &&
context.schema.__allowedLegacyNames.indexOf(node.name) !== -1
) {
return;
}
// Ensure names are valid, however introspection types opt out.
const error = isValidNameError(node.name, node.astNode || undefined);
if (error && !isIntrospectionType((node: any))) {
if (error) {
context.addError(error);
}
}
Expand All @@ -236,8 +244,10 @@ function validateTypes(context: SchemaValidationContext): void {
return;
}

// Ensure they are named correctly.
validateName(context, type);
// Ensure it is named correctly (excluding introspection types).
if (!isIntrospectionType(type)) {
validateName(context, type);
}

if (isObjectType(type)) {
// Ensure fields are valid
Expand Down