Skip to content

[validator] Add suggested types to incorrect field message #282

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

Merged
merged 1 commit into from
Feb 2, 2016
Merged

[validator] Add suggested types to incorrect field message #282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 49 additions & 15 deletions src/validation/__tests__/FieldsOnCorrectType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* of patent rights can be found in the PATENTS file in the same directory.
*/

import { expect } from 'chai';
import { describe, it } from 'mocha';
import { expectPassesRule, expectFailsRule } from './harness';
import {
Expand All @@ -15,9 +16,9 @@ import {
} from '../rules/FieldsOnCorrectType';


function undefinedField(field, type, line, column) {
function undefinedField(field, type, suggestions, line, column) {
return {
message: undefinedFieldMessage(field, type),
message: undefinedFieldMessage(field, type, suggestions),
locations: [ { line, column } ],
};
}
Expand Down Expand Up @@ -84,8 +85,8 @@ describe('Validate: Fields on correct type', () => {
}
}
}`,
[ undefinedField('unknown_pet_field', 'Pet', 3, 9),
undefinedField('unknown_cat_field', 'Cat', 5, 13) ]
[ undefinedField('unknown_pet_field', 'Pet', [], 3, 9),
undefinedField('unknown_cat_field', 'Cat', [], 5, 13) ]
);
});

Expand All @@ -94,7 +95,7 @@ describe('Validate: Fields on correct type', () => {
fragment fieldNotDefined on Dog {
meowVolume
}`,
[ undefinedField('meowVolume', 'Dog', 3, 9) ]
[ undefinedField('meowVolume', 'Dog', [], 3, 9) ]
);
});

Expand All @@ -105,7 +106,7 @@ describe('Validate: Fields on correct type', () => {
deeper_unknown_field
}
}`,
[ undefinedField('unknown_field', 'Dog', 3, 9) ]
[ undefinedField('unknown_field', 'Dog', [], 3, 9) ]
);
});

Expand All @@ -116,7 +117,7 @@ describe('Validate: Fields on correct type', () => {
unknown_field
}
}`,
[ undefinedField('unknown_field', 'Pet', 4, 11) ]
[ undefinedField('unknown_field', 'Pet', [], 4, 11) ]
);
});

Expand All @@ -127,7 +128,7 @@ describe('Validate: Fields on correct type', () => {
meowVolume
}
}`,
[ undefinedField('meowVolume', 'Dog', 4, 11) ]
[ undefinedField('meowVolume', 'Dog', [], 4, 11) ]
);
});

Expand All @@ -136,7 +137,7 @@ describe('Validate: Fields on correct type', () => {
fragment aliasedFieldTargetNotDefined on Dog {
volume : mooVolume
}`,
[ undefinedField('mooVolume', 'Dog', 3, 9) ]
[ undefinedField('mooVolume', 'Dog', [], 3, 9) ]
);
});

Expand All @@ -145,7 +146,7 @@ describe('Validate: Fields on correct type', () => {
fragment aliasedLyingFieldTargetNotDefined on Dog {
barkVolume : kawVolume
}`,
[ undefinedField('kawVolume', 'Dog', 3, 9) ]
[ undefinedField('kawVolume', 'Dog', [], 3, 9) ]
);
});

Expand All @@ -154,16 +155,16 @@ describe('Validate: Fields on correct type', () => {
fragment notDefinedOnInterface on Pet {
tailLength
}`,
[ undefinedField('tailLength', 'Pet', 3, 9) ]
[ undefinedField('tailLength', 'Pet', [], 3, 9) ]
);
});

it('Defined on implmentors but not on interface', () => {
it('Defined on implementors but not on interface', () => {
expectFailsRule(FieldsOnCorrectType, `
fragment definedOnImplementorsButNotInterface on Pet {
nickname
}`,
[ undefinedField('nickname', 'Pet', 3, 9) ]
[ undefinedField('nickname', 'Pet', [ 'Cat', 'Dog' ], 3, 9) ]
);
});

Expand All @@ -180,7 +181,7 @@ describe('Validate: Fields on correct type', () => {
fragment directFieldSelectionOnUnion on CatOrDog {
directField
}`,
[ undefinedField('directField', 'CatOrDog', 3, 9) ]
[ undefinedField('directField', 'CatOrDog', [], 3, 9) ]
);
});

Expand All @@ -189,7 +190,15 @@ describe('Validate: Fields on correct type', () => {
fragment definedOnImplementorsQueriedOnUnion on CatOrDog {
name
}`,
[ undefinedField('name', 'CatOrDog', 3, 9) ]
[
undefinedField(
'name',
'CatOrDog',
[ 'Being', 'Pet', 'Canine', 'Cat', 'Dog' ],
3,
9
)
]
);
});

Expand All @@ -206,4 +215,29 @@ describe('Validate: Fields on correct type', () => {
`);
});

describe('Fields on correct type error message', () => {
it('Works with no suggestions', () => {
expect(
undefinedFieldMessage('T', 'f', [])
).to.equal('Cannot query field "T" on type "f".');
});

it('Works with no small numbers of suggestions', () => {
expect(
undefinedFieldMessage('T', 'f', [ 'A', 'B' ])
).to.equal('Cannot query field "T" on type "f". ' +
'However, this field exists on "A", "B". ' +
'Perhaps you meant to use an inline fragment?');
});

it('Works with lots of suggestions', () => {
expect(
undefinedFieldMessage('T', 'f', [ 'A', 'B', 'C', 'D', 'E', 'F' ])
).to.equal('Cannot query field "T" on type "f". ' +
'However, this field exists on "A", "B", "C", "D", "E", ' +
'and 1 other types. ' +
'Perhaps you meant to use an inline fragment?');
});
});
});

12 changes: 11 additions & 1 deletion src/validation/__tests__/harness.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,16 @@ const Pet = new GraphQLInterfaceType({
}),
});

const Canine = new GraphQLInterfaceType({
name: 'Canine',
fields: () => ({
name: {
type: GraphQLString,
args: { surname: { type: GraphQLBoolean } },
}
}),
});

const DogCommand = new GraphQLEnumType({
name: 'DogCommand',
values: {
Expand Down Expand Up @@ -93,7 +103,7 @@ const Dog = new GraphQLObjectType({
args: { x: { type: GraphQLInt }, y: { type: GraphQLInt } },
},
}),
interfaces: [ Being, Pet ],
interfaces: [ Being, Pet, Canine ],
});

const Cat = new GraphQLObjectType({
Expand Down
6 changes: 3 additions & 3 deletions src/validation/__tests__/validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ describe('Validate: Supports full validation', () => {
);

expect(errors).to.deep.equal([
{ message: 'Cannot query field "catOrDog" on "QueryRoot".' },
{ message: 'Cannot query field "furColor" on "Cat".' },
{ message: 'Cannot query field "isHousetrained" on "Dog".' }
{ message: 'Cannot query field "catOrDog" on type "QueryRoot".' },
{ message: 'Cannot query field "furColor" on type "Cat".' },
{ message: 'Cannot query field "isHousetrained" on type "Dog".' }
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe these 3 should actually be calls to undefinedField to match the other tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably; if we do so, I definitely need to add a dedicated test for undefinedField (I'll do that anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'll keep this as a true "integration" test.

]);
});

Expand Down
79 changes: 74 additions & 5 deletions src/validation/rules/FieldsOnCorrectType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,31 @@
import type { ValidationContext } from '../index';
import { GraphQLError } from '../../error';
import type { Field } from '../../language/ast';
import type { GraphQLType } from '../../type/definition';

import {
isAbstractType,
GraphQLAbstractType,
GraphQLObjectType,
} from '../../type/definition';

export function undefinedFieldMessage(
fieldName: string,
type: GraphQLType
type: string,
suggestedTypes: Array<string>
): string {
return `Cannot query field "${fieldName}" on "${type}".`;
let message = `Cannot query field "${fieldName}" on type "${type}".`;
const MAX_LENGTH = 5;
if (suggestedTypes.length !== 0) {
let suggestions = suggestedTypes
.slice(0, MAX_LENGTH)
.map(t => `"${t}"`)
.join(', ');
if (suggestedTypes.length > MAX_LENGTH) {
suggestions += `, and ${suggestedTypes.length - MAX_LENGTH} other types`;
}
message += ` However, this field exists on ${suggestions}.`;
message += ` Perhaps you meant to use an inline fragment?`;
}
return message;
}

/**
Expand All @@ -34,12 +51,64 @@ export function FieldsOnCorrectType(context: ValidationContext): any {
if (type) {
const fieldDef = context.getFieldDef();
if (!fieldDef) {
// This isn't valid. Let's find suggestions, if any.
let suggestedTypes = [];
if (isAbstractType(type)) {
suggestedTypes =
getSiblingInterfacesIncludingField(type, node.name.value);
suggestedTypes = suggestedTypes.concat(
getImplementationsIncludingField(type, node.name.value)
);
}
context.reportError(new GraphQLError(
undefinedFieldMessage(node.name.value, type.name),
undefinedFieldMessage(node.name.value, type.name, suggestedTypes),
[ node ]
));
}
}
}
};
}

/**
* Return implementations of `type` that include `fieldName` as a valid field.
*/
function getImplementationsIncludingField(
type: GraphQLAbstractType,
fieldName: string
): Array<string> {
return type.getPossibleTypes()
.filter(t => t.getFields()[fieldName] !== undefined)
.map(t => t.name)
.sort();
}

/**
* Go through all of the implementations of type, and find other interaces
* that they implement. If those interfaces include `field` as a valid field,
* return them, sorted by how often the implementations include the other
* interface.
*/
function getSiblingInterfacesIncludingField(
type: GraphQLAbstractType,
fieldName: string
): Array<string> {
const implementingObjects = type.getPossibleTypes()
.filter(t => t instanceof GraphQLObjectType);
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit is the same as lines 46/47 above. Perhaps there's a way to only do this once?

Maybe you could set this up in three steps:

  1. Collect possible types that define this field.
  2. Collect overlapping interfaces (that types from (1) implement) that also define this field.
  3. Combine to create list of names: (2).map(type => type.name).concat((1).map(type => type.name).sort())


const suggestedInterfaces = implementingObjects.reduce((acc, t) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if you wanted this to operate on objects instead of strings, you could do:

const implCount = new Map();

implementingObjectTypes.forEach(objType => {
  objType.getInterfaces().forEach(interfaceType => {
    const count = implCount.get(interfaceType);
    if (count) {
      implCount.set(interfaceType, count + 1);
    } else if (interfaceType.getFields()[fieldName]) {
      implCount.set(interfaceType, 1);
    }
  })
})

return [ ...implCount.values() ].sort(
  (a, b) => implCount.get(b) - implCount.get(a)
);

t.getInterfaces().forEach(i => {
if (i.getFields()[fieldName] === undefined) {
return;
}
if (acc[i.name] === undefined) {
acc[i.name] = 0;
}
acc[i.name] += 1;
});
return acc;
}, {});
return Object.keys(suggestedInterfaces)
.sort((a,b) => suggestedInterfaces[b] - suggestedInterfaces[a]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, I think it's preferable to put functions which are only used internally below any that are exported, so it's easier to see what functions are exported on first read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

}