Skip to content

Commit

Permalink
fix(graphql-key-transformer): prevent non-scalar key fields (#5319)
Browse files Browse the repository at this point in the history 
* fix(graphql-key-transformer): prevent non-scalar key fields

resolves #5300

Logic for 'isScalar' treated lists as scalar types. This allowed indexes to be created with invalid key types.

This modifies the isScalar function to return false for list types, and updates other areas as needed with a new 'isScalarOrScalarList' function

Includes new test which fails without this fix.
Fixes adjacent tests failing because of missing model transforms, and not for the stated reason.

* fix: schema generation for list input types
  • Loading branch information
Ross Williams committed Dec 11, 2020
1 parent ebde537 commit 4a5b305
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 32 deletions.
20 changes: 13 additions & 7 deletions packages/graphql-connection-transformer/src/ModelConnectionTransformer.ts
View file
Expand Up @@ -281,13 +281,19 @@ export class ModelConnectionTransformer extends Transformer {
// We use this to configure key condition arguments for the resolver on the many side of the @connection.
const foreignAssociatedSortField =
associatedSortFieldName && relatedType.fields.find((f: FieldDefinitionNode) => f.name.value === associatedSortFieldName);
const sortKeyInfo = foreignAssociatedSortField
? {
fieldName: foreignAssociatedSortField.name.value,
attributeType: attributeTypeFromScalar(foreignAssociatedSortField.type),
typeName: getBaseType(foreignAssociatedSortField.type),
}
: undefined;

let sortKeyInfo = undefined;

if (foreignAssociatedSortField) {
if (isListType(foreignAssociatedSortField.type)) {
throw new InvalidDirectiveError(`sortField "${foreignAssociatedSortField}" is a list. It should be a scalar.`);
}
sortKeyInfo = {
fieldName: foreignAssociatedSortField.name.value,
attributeType: attributeTypeFromScalar(foreignAssociatedSortField.type),
typeName: getBaseType(foreignAssociatedSortField.type),
}
}

// Relationship Cardinalities:
// 1. [] to []
Expand Down
6 changes: 6 additions & 0 deletions packages/graphql-connection-transformer/src/__tests__/NewConnectionTransformer.test.ts
View file
Expand Up @@ -554,6 +554,7 @@ test('Many-to-many without conflict resolution generates correct schema', () =>
type Post @model {
id: ID!
title: String!
frontMatter: [FrontMatter]
editors: [PostEditor] @connection(keyName: "byPost", fields: ["id"])
}
Expand All @@ -575,6 +576,11 @@ test('Many-to-many without conflict resolution generates correct schema', () =>
username: String!
posts: [PostEditor] @connection(keyName: "byEditor", fields: ["id"])
}
type FrontMatter {
name: String
value: String
}
`;

const transformer = new GraphQLTransform({
Expand Down
13 changes: 13 additions & 0 deletions ...-connection-transformer/src/__tests__/__snapshots__/NewConnectionTransformer.test.ts.snap
View file
Expand Up @@ -295,6 +295,7 @@ exports[`Many-to-many without conflict resolution generates correct schema 1`] =
"type Post {
id: ID!
title: String!
frontMatter: [FrontMatter]
editors(editorID: ModelIDKeyConditionInput, filter: ModelPostEditorFilterInput, sortDirection: ModelSortDirection, limit: Int, nextToken: String): ModelPostEditorConnection
createdAt: AWSDateTime!
updatedAt: AWSDateTime!
Expand All @@ -318,6 +319,16 @@ type User {
updatedAt: AWSDateTime!
}
type FrontMatter {
name: String
value: String
}
input FrontMatterInput {
name: String
value: String
}
enum ModelSortDirection {
ASC
DESC
Expand Down Expand Up @@ -432,11 +443,13 @@ type Query {
input CreatePostInput {
id: ID
title: String!
frontMatter: [FrontMatterInput]
}
input UpdatePostInput {
id: ID!
title: String
frontMatter: [FrontMatterInput]
}
input DeletePostInput {
Expand Down
4 changes: 4 additions & 0 deletions packages/graphql-connection-transformer/src/resources.ts
View file
Expand Up @@ -28,6 +28,7 @@ import {
NONE_INT_VALUE,
applyKeyConditionExpression,
attributeTypeFromScalar,
isListType,
toCamelCase,
applyCompositeKeyConditionExpression,
} from 'graphql-transformer-common';
Expand Down Expand Up @@ -323,6 +324,9 @@ export class ResourceFactory {
const sortKeyField = relatedType.fields.find(f => f.name.value === keySchema[1].AttributeName);

if (sortKeyField) {
if (isListType(sortKeyField.type)) {
throw new InvalidDirectiveError(`sortField "${sortKeyField.name}" is a list. It should be a scalar.`);
}
setup.push(applyKeyConditionExpression(String(keySchema[1].AttributeName), attributeTypeFromScalar(sortKeyField.type), 'query'));
} else {
setup.push(
Expand Down
9 changes: 5 additions & 4 deletions packages/graphql-dynamodb-transformer/src/definitions.ts
View file
Expand Up @@ -31,6 +31,7 @@ import {
makeValueNode,
withNamedNodeNamed,
isListType,
isScalarOrScalarList,
} from 'graphql-transformer-common';
import { TransformerContext } from 'graphql-transformer-core';
import { getCreatedAtFieldName, getUpdatedAtFieldName } from './ModelDirectiveArgs';
Expand All @@ -57,7 +58,7 @@ export function getNonModelObjectArray(
): ObjectTypeDefinitionNode[] {
// loop over all fields in the object, picking out all nonscalars that are not @model types
for (const field of obj.fields) {
if (!isScalar(field.type)) {
if (!isScalarOrScalarList(field.type)) {
const def = ctx.getType(getBaseType(field.type));

if (
Expand Down Expand Up @@ -147,7 +148,7 @@ export function makeCreateInputObject(
.filter((field: FieldDefinitionNode) => {
const fieldType = ctx.getType(getBaseType(field.type));
if (
isScalar(field.type) ||
isScalarOrScalarList(field.type) ||
nonModelTypes.find(e => e.name.value === getBaseType(field.type)) ||
(fieldType && fieldType.kind === Kind.ENUM_TYPE_DEFINITION)
) {
Expand Down Expand Up @@ -214,7 +215,7 @@ export function makeUpdateInputObject(
.filter(f => {
const fieldType = ctx.getType(getBaseType(f.type));
if (
isScalar(f.type) ||
isScalarOrScalarList(f.type) ||
nonModelTypes.find(e => e.name.value === getBaseType(f.type)) ||
(fieldType && fieldType.kind === Kind.ENUM_TYPE_DEFINITION)
) {
Expand Down Expand Up @@ -303,7 +304,7 @@ export function makeModelXFilterInputObject(
const fields: InputValueDefinitionNode[] = obj.fields
.filter((field: FieldDefinitionNode) => {
const fieldType = ctx.getType(getBaseType(field.type));
if (isScalar(field.type) || (fieldType && fieldType.kind === Kind.ENUM_TYPE_DEFINITION)) {
if (isScalarOrScalarList(field.type) || (fieldType && fieldType.kind === Kind.ENUM_TYPE_DEFINITION)) {
return true;
}
})
Expand Down
14 changes: 12 additions & 2 deletions packages/graphql-key-transformer/src/KeyTransformer.ts
View file
Expand Up @@ -43,6 +43,7 @@ import {
graphqlName,
toUpper,
getDirectiveArgument,
isListType,
} from 'graphql-transformer-common';
import { makeModelConnectionType } from 'graphql-dynamodb-transformer';
import {
Expand Down Expand Up @@ -538,7 +539,7 @@ export class KeyTransformer extends Transformer {
const ddbKeyType = attributeTypeFromType(existingField.type, ctx);
if (this.isPrimaryKey(directive) && !isNonNullType(existingField.type)) {
throw new InvalidDirectiveError(`The primary @key on type '${definition.name.value}' must reference non-null fields.`);
} else if (ddbKeyType !== 'S' && ddbKeyType !== 'N' && ddbKeyType !== 'B') {
} else if (ddbKeyType !== 'S' && ddbKeyType !== 'N') {
throw new InvalidDirectiveError(`A @key on type '${definition.name.value}' cannot reference non-scalar field ${fieldName}.`);
}
}
Expand Down Expand Up @@ -710,6 +711,9 @@ function keySchema(args: KeyArguments) {
}

function attributeTypeFromType(type: TypeNode, ctx: TransformerContext) {
if (isListType(type)) {
return 'L';
}
const baseTypeName = getBaseType(type);
const ofType = ctx.getType(baseTypeName);
if (ofType && ofType.kind === Kind.ENUM_TYPE_DEFINITION) {
Expand Down Expand Up @@ -934,7 +938,13 @@ function setQuerySnippet(definition: ObjectTypeDefinitionNode, directive: Direct
const keys = args.fields;
const keyTypes = keys.map(k => {
const field = definition.fields.find(f => f.name.value === k);
return attributeTypeFromType(field.type, ctx);
const type = attributeTypeFromType(field.type, ctx);

if (type === 'L') {
throw new InvalidDirectiveError(`A @key on type '${definition.name.value}' cannot reference non-scalar field ${field.name}.`);
}

return type;
});

const expressions: Expression[] = [];
Expand Down
39 changes: 27 additions & 12 deletions packages/graphql-key-transformer/src/__tests__/KeyTransformer.test.ts
View file
Expand Up @@ -27,88 +27,103 @@ test('Check KeyTransformer Resolver Code', () => {
});
test('KeyTransformer should fail if more than 1 @key is provided without a name.', () => {
const invalidSchema = `
type Test @key(fields: ["id"]) @key(fields: ["email"]) {
type Test @model @key(fields: ["id"]) @key(fields: ["email"]) {
id: ID!
email: String
}
`;

const transformer = new GraphQLTransform({
transformers: [new KeyTransformer()],
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
});

expect(() => transformer.transform(invalidSchema)).toThrowError(InvalidDirectiveError);
});

test('KeyTransformer should fail if more than 1 @key is provided with the same name.', () => {
const invalidSchema = `
type Test @key(name: "Test", fields: ["id"]) @key(name: "Test", fields: ["email"]) {
type Test @model @key(name: "Test", fields: ["id"]) @key(name: "Test", fields: ["email"]) {
id: ID!
email: String
}
`;

const transformer = new GraphQLTransform({
transformers: [new KeyTransformer()],
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
});

expect(() => transformer.transform(invalidSchema)).toThrowError(InvalidDirectiveError);
});

test('KeyTransformer should fail if referencing a field that does not exist.', () => {
const invalidSchema = `
type Test @key(fields: ["someWeirdId"]) {
type Test @model @key(fields: ["someWeirdId"]) {
id: ID!
email: String
}
`;

const transformer = new GraphQLTransform({
transformers: [new KeyTransformer()],
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
});

expect(() => transformer.transform(invalidSchema)).toThrowError(InvalidDirectiveError);
});

test('Test that a primary @key fails if pointing to nullable fields.', () => {
const invalidSchema = `
type Test @key(fields: ["email"]) {
type Test @model @key(fields: ["email"]) {
id: ID!
email: String
}
`;

const transformer = new GraphQLTransform({
transformers: [new KeyTransformer()],
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
});

expect(() => transformer.transform(invalidSchema)).toThrowError(InvalidDirectiveError);
});

test('Test that model with an LSI but no primary sort key will fail.', () => {
const invalidSchema = `
type Test @key(fields: ["id"]) @key(name: "SomeLSI", fields: ["id", "email"]) {
type Test @model @key(fields: ["id"]) @key(name: "SomeLSI", fields: ["id", "email"]) {
id: ID!
email: String!
}
`;

const transformer = new GraphQLTransform({
transformers: [new KeyTransformer()],
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
});
expect(() => transformer.transform(invalidSchema)).toThrowError(InvalidDirectiveError);
});

test('KeyTransformer should fail if a non-existing type field is defined as key field.', () => {
const invalidSchema = `
type Test @key(name: "Test", fields: ["one"]) {
type Test @model @key(name: "Test", fields: ["one"]) {
id: ID!
email: String
}
`;

const transformer = new GraphQLTransform({
transformers: [new KeyTransformer()],
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
});

expect(() => transformer.transform(invalidSchema)).toThrowError(InvalidDirectiveError);
});

test('KeyTransformer should fail if a list type field is defined as key field.', () => {
const invalidSchema = `
type Test @model @key(name: "Test", fields: ["emails"]) {
id: ID!
emails: [String!]!
}
`;

const transformer = new GraphQLTransform({
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
});

expect(() => transformer.transform(invalidSchema)).toThrowError(InvalidDirectiveError);
Expand Down
24 changes: 17 additions & 7 deletions packages/graphql-transformer-common/src/definition.ts
View file
Expand Up @@ -67,7 +67,7 @@ export const MAP_SCALARS: { [k: string]: boolean } = {
AWSJSON: true,
};

export function attributeTypeFromScalar(scalar: TypeNode): 'S' | 'N' {
export function attributeTypeFromScalar(scalar: NamedTypeNode | NonNullTypeNode): 'S' | 'N' {
const baseType = getBaseType(scalar);
const baseScalar = DEFAULT_SCALARS[baseType];
if (!baseScalar) {
Expand All @@ -87,21 +87,21 @@ export function attributeTypeFromScalar(scalar: TypeNode): 'S' | 'N' {
}
}

export function isScalar(type: TypeNode) {
export function isScalar(type: TypeNode): type is NamedTypeNode | NonNullTypeNode {
if (type.kind === Kind.NON_NULL_TYPE) {
return isScalar(type.type);
} else if (type.kind === Kind.LIST_TYPE) {
return isScalar(type.type);
return false;
} else {
return Boolean(DEFAULT_SCALARS[type.name.value]);
}
}

export function isScalarOrEnum(type: TypeNode, enums: EnumTypeDefinitionNode[]) {
export function isScalarOrEnum(type: TypeNode, enums: EnumTypeDefinitionNode[]): type is NamedTypeNode | NonNullTypeNode {
if (type.kind === Kind.NON_NULL_TYPE) {
return isScalarOrEnum(type.type, enums);
} else if (type.kind === Kind.LIST_TYPE) {
return isScalarOrEnum(type.type, enums);
return false;
} else {
for (const e of enums) {
if (e.name.value === type.name.value) {
Expand All @@ -112,6 +112,16 @@ export function isScalarOrEnum(type: TypeNode, enums: EnumTypeDefinitionNode[])
}
}

export function isScalarOrScalarList(type: TypeNode): boolean {
if (type.kind === Kind.NON_NULL_TYPE) {
return isScalar(type.type);
} else if (type.kind === Kind.LIST_TYPE) {
return isScalar(type.type);
} else {
return Boolean(DEFAULT_SCALARS[type.name.value]);
}
}

export function getBaseType(type: TypeNode): string {
if (type.kind === Kind.NON_NULL_TYPE) {
return getBaseType(type.type);
Expand All @@ -122,7 +132,7 @@ export function getBaseType(type: TypeNode): string {
}
}

export function isListType(type: TypeNode): boolean {
export function isListType(type: TypeNode): type is ListTypeNode {
if (type.kind === Kind.NON_NULL_TYPE) {
return isListType(type.type);
} else if (type.kind === Kind.LIST_TYPE) {
Expand All @@ -132,7 +142,7 @@ export function isListType(type: TypeNode): boolean {
}
}

export function isNonNullType(type: TypeNode): boolean {
export function isNonNullType(type: TypeNode): type is NonNullTypeNode {
return type.kind === Kind.NON_NULL_TYPE;
}

Expand Down

0 comments on commit 4a5b305

Please sign in to comment.