Skip to content

Commit

Permalink
fix(graphql-key-transformer): prevent non-scalar key fields
Browse files Browse the repository at this point in the history 
resolves aws-amplify#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.
  • Loading branch information
Ross Williams committed Sep 12, 2020
1 parent 42810c9 commit dcd68ef
Show file tree
Hide file tree
Showing 6 changed files with 76 additions and 30 deletions.
20 changes: 13 additions & 7 deletions packages/graphql-connection-transformer/src/ModelConnectionTransformer.ts
View file
Original file line number Diff line number Diff line change
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
4 changes: 4 additions & 0 deletions packages/graphql-connection-transformer/src/resources.ts
View file
Original file line number Diff line number Diff line change
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
5 changes: 3 additions & 2 deletions packages/graphql-dynamodb-transformer/src/definitions.ts
View file
Original file line number Diff line number Diff line change
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 @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
graphqlName,
toUpper,
getDirectiveArgument,
isListType,
} from 'graphql-transformer-common';
import { makeModelConnectionType } from 'graphql-dynamodb-transformer';
import {
Expand Down Expand Up @@ -495,7 +496,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 @@ -667,6 +668,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 @@ -891,7 +895,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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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 dcd68ef

Please sign in to comment.