Skip to content

Commit

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

This reverts commit 4a5b305.
  • Loading branch information
@SwaySway
SwaySway committed Dec 14, 2020
1 parent 05b88ad commit c61268d
Show file tree
Hide file tree
Showing 8 changed files with 32 additions and 97 deletions.
20 changes: 7 additions & 13 deletions packages/graphql-connection-transformer/src/ModelConnectionTransformer.ts
View file
Expand Up @@ -281,19 +281,13 @@ 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);

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),
}
}
const sortKeyInfo = foreignAssociatedSortField
? {
fieldName: foreignAssociatedSortField.name.value,
attributeType: attributeTypeFromScalar(foreignAssociatedSortField.type),
typeName: getBaseType(foreignAssociatedSortField.type),
}
: undefined;

// Relationship Cardinalities:
// 1. [] to []
Expand Down
6 changes: 0 additions & 6 deletions packages/graphql-connection-transformer/src/__tests__/NewConnectionTransformer.test.ts
View file
Expand Up @@ -554,7 +554,6 @@ 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 @@ -576,11 +575,6 @@ 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: 0 additions & 13 deletions ...-connection-transformer/src/__tests__/__snapshots__/NewConnectionTransformer.test.ts.snap
View file
Expand Up @@ -302,7 +302,6 @@ 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 @@ -326,16 +325,6 @@ type User {
updatedAt: AWSDateTime!
}
type FrontMatter {
name: String
value: String
}
input FrontMatterInput {
name: String
value: String
}
enum ModelSortDirection {
ASC
DESC
Expand Down Expand Up @@ -457,13 +446,11 @@ type Query {
input CreatePostInput {
id: ID
title: String!
frontMatter: [FrontMatterInput]
}
input UpdatePostInput {
id: ID!
title: String
frontMatter: [FrontMatterInput]
}
input DeletePostInput {
Expand Down
4 changes: 0 additions & 4 deletions packages/graphql-connection-transformer/src/resources.ts
View file
Expand Up @@ -28,7 +28,6 @@ import {
NONE_INT_VALUE,
applyKeyConditionExpression,
attributeTypeFromScalar,
isListType,
toCamelCase,
applyCompositeKeyConditionExpression,
} from 'graphql-transformer-common';
Expand Down Expand Up @@ -324,9 +323,6 @@ 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: 4 additions & 5 deletions packages/graphql-dynamodb-transformer/src/definitions.ts
View file
Expand Up @@ -31,7 +31,6 @@ import {
makeValueNode,
withNamedNodeNamed,
isListType,
isScalarOrScalarList,
} from 'graphql-transformer-common';
import { TransformerContext } from 'graphql-transformer-core';
import { getCreatedAtFieldName, getUpdatedAtFieldName } from './ModelDirectiveArgs';
Expand Down Expand Up @@ -60,7 +59,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 (!isScalarOrScalarList(field.type)) {
if (!isScalar(field.type)) {
const def = ctx.getType(getBaseType(field.type));

if (
Expand Down Expand Up @@ -150,7 +149,7 @@ export function makeCreateInputObject(
.filter((field: FieldDefinitionNode) => {
const fieldType = ctx.getType(getBaseType(field.type));
if (
isScalarOrScalarList(field.type) ||
isScalar(field.type) ||
nonModelTypes.find(e => e.name.value === getBaseType(field.type)) ||
(fieldType && fieldType.kind === Kind.ENUM_TYPE_DEFINITION)
) {
Expand Down Expand Up @@ -217,7 +216,7 @@ export function makeUpdateInputObject(
.filter(f => {
const fieldType = ctx.getType(getBaseType(f.type));
if (
isScalarOrScalarList(f.type) ||
isScalar(f.type) ||
nonModelTypes.find(e => e.name.value === getBaseType(f.type)) ||
(fieldType && fieldType.kind === Kind.ENUM_TYPE_DEFINITION)
) {
Expand Down Expand Up @@ -306,7 +305,7 @@ export function makeModelXFilterInputObject(
const fields: InputValueDefinitionNode[] = obj.fields
.filter((field: FieldDefinitionNode) => {
const fieldType = ctx.getType(getBaseType(field.type));
if (isScalarOrScalarList(field.type) || (fieldType && fieldType.kind === Kind.ENUM_TYPE_DEFINITION)) {
if (isScalar(field.type) || (fieldType && fieldType.kind === Kind.ENUM_TYPE_DEFINITION)) {
return true;
}
})
Expand Down
14 changes: 2 additions & 12 deletions packages/graphql-key-transformer/src/KeyTransformer.ts
View file
Expand Up @@ -43,7 +43,6 @@ import {
graphqlName,
toUpper,
getDirectiveArgument,
isListType,
} from 'graphql-transformer-common';
import {
makeModelConnectionType,
Expand Down Expand Up @@ -582,7 +581,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') {
} else if (ddbKeyType !== 'S' && ddbKeyType !== 'N' && ddbKeyType !== 'B') {
throw new InvalidDirectiveError(`A @key on type '${definition.name.value}' cannot reference non-scalar field ${fieldName}.`);
}
}
Expand Down Expand Up @@ -757,9 +756,6 @@ 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 @@ -984,13 +980,7 @@ 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);
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;
return attributeTypeFromType(field.type, ctx);
});

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

const transformer = new GraphQLTransform({
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
transformers: [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 @model @key(name: "Test", fields: ["id"]) @key(name: "Test", fields: ["email"]) {
type Test @key(name: "Test", fields: ["id"]) @key(name: "Test", fields: ["email"]) {
id: ID!
email: String
}
`;

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

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

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

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

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

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

const transformer = new GraphQLTransform({
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
transformers: [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 @model @key(fields: ["id"]) @key(name: "SomeLSI", fields: ["id", "email"]) {
type Test @key(fields: ["id"]) @key(name: "SomeLSI", fields: ["id", "email"]) {
id: ID!
email: String!
}
`;

const transformer = new GraphQLTransform({
transformers: [new DynamoDBModelTransformer(), new KeyTransformer()],
transformers: [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 @model @key(name: "Test", fields: ["one"]) {
type Test @key(name: "Test", fields: ["one"]) {
id: ID!
email: String
}
`;

const transformer = new GraphQLTransform({
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()],
transformers: [new KeyTransformer()],
});

expect(() => transformer.transform(invalidSchema)).toThrowError(InvalidDirectiveError);
Expand Down
24 changes: 7 additions & 17 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: NamedTypeNode | NonNullTypeNode): 'S' | 'N' {
export function attributeTypeFromScalar(scalar: TypeNode): 'S' | 'N' {
const baseType = getBaseType(scalar);
const baseScalar = DEFAULT_SCALARS[baseType];
if (!baseScalar) {
Expand All @@ -87,21 +87,21 @@ export function attributeTypeFromScalar(scalar: NamedTypeNode | NonNullTypeNode)
}
}

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

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

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

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

Expand Down

0 comments on commit c61268d

Please sign in to comment.