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.

Sign up for GitHub

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

More allowed legacy names #1235

Merged
merged 10 commits into from
Feb 14, 2018
Merged

More allowed legacy names #1235

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5cb3dd9
Allow legacy names when building AST schema.
alloy Feb 8, 2018
5f9ff08
Always store allowed legacy names on schema.
alloy Feb 8, 2018
3958eae
Allow additional legacy names when extending a schema.
alloy Feb 9, 2018
8f936da
Standardise schema validation options.
alloy Feb 9, 2018
bb9c68c
Allow legacy names in client schemas.
alloy Feb 9, 2018
3beb355
Add test coverage for schema validation options.
alloy Feb 9, 2018
50be8c6
Simplify value merge
leebyron Feb 14, 2018
e58be0f
Add test
leebyron Feb 14, 2018
ae1ab1d
Trailing space
leebyron Feb 14, 2018
e8b6262
Recycling this value means we need to be read-only strict
leebyron Feb 14, 2018
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
62 changes: 62 additions & 0 deletions src/type/__tests__/schema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,66 @@ describe('Type System: Schema', () => {
expect(Schema.getTypeMap()).to.include.key('WrappedDirInput');
});
});

describe('Validity', () => {
describe('when not assumed valid', () => {
it('configures the schema to still needing validation', () => {
expect(
new GraphQLSchema({
assumeValid: false,
}).__validationErrors,
).to.equal(undefined);
});

it('configures the schema for allowed legacy names', () => {
expect(
new GraphQLSchema({
allowedLegacyNames: ['__badName'],
}).__allowedLegacyNames,
).to.deep.equal(['__badName']);
});

it('checks the configuration for mistakes', () => {
expect(() => new GraphQLSchema(() => null)).to.throw();
expect(() => new GraphQLSchema({ types: {} })).to.throw();
expect(() => new GraphQLSchema({ directives: {} })).to.throw();
expect(() => new GraphQLSchema({ allowedLegacyNames: {} })).to.throw();
});
});

describe('when assumed valid', () => {
it('configures the schema to have no errors', () => {
expect(
new GraphQLSchema({
assumeValid: true,
}).__validationErrors,
).to.deep.equal([]);
});

it('still configures the schema for allowed legacy names', () => {
expect(
new GraphQLSchema({
assumeValid: true,
allowedLegacyNames: ['__badName'],
}).__allowedLegacyNames,
).to.deep.equal(['__badName']);
});

it('does not check the configuration for mistakes', () => {
expect(() => {
const config = () => null;
config.assumeValid = true;
return new GraphQLSchema(config);
}).to.not.throw();
expect(() => {
return new GraphQLSchema({
assumeValid: true,
types: {},
directives: { reduce: () => [] },
allowedLegacyNames: {},
});
}).to.not.throw();
});
});
});
});
28 changes: 20 additions & 8 deletions src/type/schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,9 @@ export class GraphQLSchema {
'"allowedLegacyNames" must be Array if provided but got: ' +
`${String(config.allowedLegacyNames)}.`,
);
this.__allowedLegacyNames = config.allowedLegacyNames;
}

this.__allowedLegacyNames = config.allowedLegacyNames;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assumeValid configuration is about whether or not to check the configuration for common mistakes, so regardless of that the allowed legacy names still need to be stored on the schema instance.

There’s no tests for assumeValid and the below configuration being set or not, so wasn’t sure what you’d like. Should I add a test for any configuration being set, regardless of assumeValid?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a test would be great!

Let me know if I understand this correctly - you could create a GraphQLSchema with both __allowedLegacyNames and assumeValid, then extend that schema with a legacy name (which might no longer be assumed invalid), and then expect validation to pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I now see that validate depends on the private __validationErrors property of a schema instance.

I’m now starting to think that maybe what Relay is doing isn’t entirely correct. First it creates a schema with assumeValid set, then after extending that schema a few times it expects to still be able to validate that schema, which appears to only really be possible because extendSchema does not copy over the assumeValid configuration.

Maybe extendSchema should also copy over the assumeValid setting, but validate could accept an optional parameter to force performing the validations regardless of __validationErrors already being set?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite. extendSchema produces a new schema - while the provided schema may have been "valid" (either assumed or validated), the returned schema is different and may be invalid, so it must not copy over assumeValid or __validationErrors from the input. We would expect the extended schema to be validated by validateSchema().

Now, we may want to provide a similar assumeValid option for extendSchema, but that would be a separate thing.

validate checks __validationErrors first as a form of memoization. Since schema are considered immutable, we do not want to wastefully validate the same schema multiple times.

Copy link
Contributor

Choose a reason for hiding this comment

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

Relay first builds a schema with the assumeValid set, because it expects that schema has been provided from a valid GraphQL server - whereas any client side extensions are within Relay's responsibility to validate

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the GraphQLValidator second link is validating the graphql operations and fragments found within relay product code, not the schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I see, that makes sense. So to answer your initial question, when an original schema has __allowedLegacyNames then extending that schema should still allow those original legacy names too, regardless from whether or not they were validated on the original schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That validate call in GraphQLValidator is definitely still leading to an error about the legacy name in the schema . Maybe it’s just a side-effect, though, I’ll double-check that in the morrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a side effect - the document validator first asserts that a valid schema if being used to validate a document. If the schema wasn't validated before that point, it will throw

this._queryType = config.query;
this._mutationType = config.mutation;
this._subscriptionType = config.subscription;
Expand Down Expand Up @@ -229,14 +229,16 @@ export class GraphQLSchema {

type TypeMap = ObjMap<GraphQLNamedType>;

export type GraphQLSchemaConfig = {
query?: ?GraphQLObjectType,
mutation?: ?GraphQLObjectType,
subscription?: ?GraphQLObjectType,
types?: ?Array<GraphQLNamedType>,
directives?: ?Array<GraphQLDirective>,
astNode?: ?SchemaDefinitionNode,
export type GraphQLSchemaValidationOptions = {|
/**
* When building a schema from a GraphQL service's introspection result, it
* might be safe to assume the schema is valid. Set to true to assume the
* produced schema is valid.
*
* Default: false
*/
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
Expand All @@ -246,6 +248,16 @@ export type GraphQLSchemaConfig = {
* major release.
*/
allowedLegacyNames?: ?Array<string>,
|};

export type GraphQLSchemaConfig = {
query?: ?GraphQLObjectType,
mutation?: ?GraphQLObjectType,
subscription?: ?GraphQLObjectType,
types?: ?Array<GraphQLNamedType>,
directives?: ?Array<GraphQLDirective>,
astNode?: ?SchemaDefinitionNode,
...GraphQLSchemaValidationOptions,
};

function typeMapReducer(map: TypeMap, type: ?GraphQLType): TypeMap {
Expand Down
11 changes: 11 additions & 0 deletions src/utilities/__tests__/buildASTSchema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,17 @@ describe('Schema Builder', () => {
const errors = validateSchema(schema);
expect(errors.length).to.be.above(0);
});

it('Accepts legacy names', () => {
const doc = parse(dedent`
type Query {
__badName: String
}
`);
const schema = buildASTSchema(doc, { allowedLegacyNames: ['__badName'] });
const errors = validateSchema(schema);
expect(errors.length).to.equal(0);
});
});

describe('Failures', () => {
Expand Down
28 changes: 28 additions & 0 deletions src/utilities/__tests__/buildClientSchema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,34 @@ describe('Type System: build schema from introspection', () => {
expect(secondIntrospection.data).to.containSubset(newIntrospection);
});

it('builds a schema with legacy names', () => {
const introspection = {
__schema: {
queryType: {
name: 'Query',
},
types: [
{
name: 'Query',
kind: 'OBJECT',
fields: [
{
name: '__badName',
args: [],
type: { name: 'String' },
},
],
interfaces: [],
},
],
},
};
const schema = buildClientSchema(introspection, {
allowedLegacyNames: ['__badName'],
});
expect(schema.__allowedLegacyNames).to.deep.equal(['__badName']);
});

it('builds a schema aware of deprecation', async () => {
const schema = new GraphQLSchema({
query: new GraphQLObjectType({
Expand Down
13 changes: 9 additions & 4 deletions src/utilities/__tests__/extendSchema-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1225,18 +1225,23 @@ describe('extendSchema', () => {
query: new GraphQLObjectType({
name: 'Query',
fields: () => ({
id: { type: GraphQLID },
__badName: { type: GraphQLString },
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a new test rather than changing this existing one?

}),
}),
allowedLegacyNames: ['__badName'],
});
const ast = parse(`
extend type Query {
__badName: String
__anotherBadName: String
}
`);
const schema = extendSchema(testSchemaWithLegacyNames, ast);
expect(schema.__allowedLegacyNames).to.deep.equal(['__badName']);
const schema = extendSchema(testSchemaWithLegacyNames, ast, {
allowedLegacyNames: ['__anotherBadName'],
});
expect(schema.__allowedLegacyNames).to.deep.equal([
'__badName',
'__anotherBadName',
]);
});

describe('does not allow extending a non-object type', () => {
Expand Down
11 changes: 3 additions & 8 deletions src/utilities/buildASTSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ import { introspectionTypes } from '../type/introspection';
import { specifiedScalarTypes } from '../type/scalars';

import { GraphQLSchema } from '../type/schema';
import type { GraphQLSchemaValidationOptions } from '../type/schema';

import type {
GraphQLType,
Expand All @@ -72,14 +73,7 @@ import type {
} from '../type/definition';

type Options = {|
/**
* When building a schema from a GraphQL service's introspection result, it
* might be safe to assume the schema is valid. Set to true to assume the
* produced schema is valid.
*
* Default: false
*/
assumeValid?: boolean,
...GraphQLSchemaValidationOptions,

/**
* Descriptions are defined as preceding string literals, however an older
Expand Down Expand Up @@ -227,6 +221,7 @@ export function buildASTSchema(
directives,
astNode: schemaDef,
assumeValid: options && options.assumeValid,
allowedLegacyNames: options && options.allowedLegacyNames,
});

function getOperationTypes(schema: SchemaDefinitionNode) {
Expand Down
12 changes: 4 additions & 8 deletions src/utilities/buildClientSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,10 @@ import type {
IntrospectionNamedTypeRef,
} from './introspectionQuery';

import type { GraphQLSchemaValidationOptions } from '../type/schema';

type Options = {|
/**
* When building a schema from a GraphQL service's introspection result, it
* might be safe to assume the schema is valid. Set to true to assume the
* produced schema is valid.
*
* Default: false
*/
assumeValid?: boolean,
...GraphQLSchemaValidationOptions,
|};
Copy link
Member

Choose a reason for hiding this comment

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

I still think it's better to have type Options = {| ...GraphQLSchemaValidationOptions |} even though there are no other options at the moment. It shows that options arg could be used for some other options beyond validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, makes sense 👍


/**
Expand Down Expand Up @@ -414,5 +409,6 @@ export function buildClientSchema(
types,
directives,
assumeValid: options && options.assumeValid,
allowedLegacyNames: options && options.allowedLegacyNames,
});
}
22 changes: 12 additions & 10 deletions src/utilities/extendSchema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { ASTDefinitionBuilder } from './buildASTSchema';
import { GraphQLError } from '../error/GraphQLError';
import { isSchema, GraphQLSchema } from '../type/schema';

import type { GraphQLSchemaValidationOptions } from '../type/schema';

import {
isObjectType,
isInterfaceType,
Expand All @@ -38,14 +40,7 @@ import type {
} from '../language/ast';

type Options = {|
/**
* When extending a schema with a known valid extension, it might be safe to
* assume the schema is valid. Set to true to assume the produced schema
* is valid.
*
* Default: false
*/
assumeValid?: boolean,
...GraphQLSchemaValidationOptions,

/**
* Descriptions are defined as preceding string literals, however an older
Expand Down Expand Up @@ -245,6 +240,14 @@ export function extendSchema(
types.push(definitionBuilder.buildType(typeName));
});

// Support both original legacy names and extended legacy names.
const schemaAllowedLegacyNames = schema.__allowedLegacyNames;
const extendAllowedLegacyNames = options && options.allowedLegacyNames;
const allowedLegacyNames =
schemaAllowedLegacyNames && extendAllowedLegacyNames
? schemaAllowedLegacyNames.concat(extendAllowedLegacyNames)
: schemaAllowedLegacyNames || extendAllowedLegacyNames;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice 👍


// Then produce and return a Schema with these types.
return new GraphQLSchema({
query: queryType,
Expand All @@ -253,8 +256,7 @@ export function extendSchema(
types,
directives: getMergedDirectives(),
astNode: schema.astNode,
allowedLegacyNames:
schema.__allowedLegacyNames && schema.__allowedLegacyNames.slice(),
allowedLegacyNames,
});

function appendExtensionToTypeExtensions(
Expand Down