From 47d7c615a5d795c167f064b28648dfe68868ad6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Sch=C3=A4fer?= Date: Fri, 10 May 2019 17:13:54 +0200 Subject: [PATCH 1/3] Refactoring: Put all data validations in one place @ulfgebhardt @mattwr18 @tirokk Here's how I imagined the data validation middleware. If we roll our own input validations I would suggest to put them all in one place. @ulfgebhardt this commit is a great example of how tests can speed you up: Since I can rely on existing tests, I don't have to check the validations manually. With tests you can refactor with confidence! :+1: --- backend/src/middleware/index.js | 4 +-- backend/src/middleware/userMiddleware.js | 7 +---- backend/src/middleware/userMiddleware.spec.js | 3 +- backend/src/middleware/validUrlMiddleware.js | 18 ----------- backend/src/middleware/validation/index.js | 30 +++++++++++++++++++ 5 files changed, 35 insertions(+), 27 deletions(-) delete mode 100644 backend/src/middleware/validUrlMiddleware.js create mode 100644 backend/src/middleware/validation/index.js diff --git a/backend/src/middleware/index.js b/backend/src/middleware/index.js index 4f725ef72..17b9d63fb 100644 --- a/backend/src/middleware/index.js +++ b/backend/src/middleware/index.js @@ -10,14 +10,14 @@ import permissionsMiddleware from './permissionsMiddleware' import userMiddleware from './userMiddleware' import includedFieldsMiddleware from './includedFieldsMiddleware' import orderByMiddleware from './orderByMiddleware' -import validUrlMiddleware from './validUrlMiddleware' +import validationMiddleware from './validation' import notificationsMiddleware from './notifications' export default schema => { let middleware = [ passwordMiddleware, dateTimeMiddleware, - validUrlMiddleware, + validationMiddleware, sluggifyMiddleware, excerptMiddleware, notificationsMiddleware, diff --git a/backend/src/middleware/userMiddleware.js b/backend/src/middleware/userMiddleware.js index 4789b4cbd..78f04638d 100644 --- a/backend/src/middleware/userMiddleware.js +++ b/backend/src/middleware/userMiddleware.js @@ -1,5 +1,4 @@ import dotenv from 'dotenv' -import { UserInputError } from 'apollo-server' import createOrUpdateLocations from './nodes/locations' @@ -13,13 +12,9 @@ export default { return result }, UpdateUser: async (resolve, root, args, context, info) => { - const USERNAME_MIN_LENGTH = 3 // TODO move to the correct place - if (!args.name || args.name.length < USERNAME_MIN_LENGTH) { - throw new UserInputError(`Username must be at least ${USERNAME_MIN_LENGTH} characters long!`) - } const result = await resolve(root, args, context, info) await createOrUpdateLocations(args.id, args.locationName, context.driver) return result - } + }, } } diff --git a/backend/src/middleware/userMiddleware.spec.js b/backend/src/middleware/userMiddleware.spec.js index 9aa8f96c1..ef752c729 100644 --- a/backend/src/middleware/userMiddleware.spec.js +++ b/backend/src/middleware/userMiddleware.spec.js @@ -71,7 +71,8 @@ describe('userMiddleware', () => { it('with too short name', async () => { const variables = { - id: 'u1' + id: 'u1', + name: ' ' } const expected = 'Username must be at least 3 characters long!' await expect(client.request(mutation, variables)) diff --git a/backend/src/middleware/validUrlMiddleware.js b/backend/src/middleware/validUrlMiddleware.js deleted file mode 100644 index 37f06199c..000000000 --- a/backend/src/middleware/validUrlMiddleware.js +++ /dev/null @@ -1,18 +0,0 @@ -const validURL = str => { - const isValid = str.match(/^(?:https?:\/\/)(?:[^@\n])?(?:www\.)?([^:/\n?]+)/g) - return !!isValid -} - -export default { - Mutation: { - CreateSocialMedia: async (resolve, root, args, context, info) => { - let socialMedia - if (validURL(args.url)) { - socialMedia = await resolve(root, args, context, info) - } else { - throw Error('Input is not a URL') - } - return socialMedia - } - } -} diff --git a/backend/src/middleware/validation/index.js b/backend/src/middleware/validation/index.js new file mode 100644 index 000000000..c84e111ee --- /dev/null +++ b/backend/src/middleware/validation/index.js @@ -0,0 +1,30 @@ +import { UserInputError } from 'apollo-server' + +const USERNAME_MIN_LENGTH = 3 + +const validateUsername = async (resolve, root, args, context, info) => { + if (args.name && args.name.length >= USERNAME_MIN_LENGTH) { + return await resolve(root, args, context, info) + } else { + throw new UserInputError(`Username must be at least ${USERNAME_MIN_LENGTH} characters long!`) + } +} + +const validateUrl = async (resolve, root, args, context, info) => { + const { url } = args + const isValid = url.match(/^(?:https?:\/\/)(?:[^@\n])?(?:www\.)?([^:/\n?]+)/g) + if (isValid) { + return await resolve(root, args, context, info) + } else { + throw new UserInputError('Input is not a URL') + } +} + +export default { + Mutation: { + // CreateUser: validateUsername, + UpdateUser: validateUsername, + CreateSocialMedia: validateUrl + } +} + From 81a26e14ffbbc680f2e49c46903dd799025539e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Sch=C3=A4fer?= Date: Fri, 10 May 2019 17:34:05 +0200 Subject: [PATCH 2/3] Fix test case, move spec to the right location @ulfgebhardt: The reason why the test case was failing is pretty obvious. You forgot to create a user that you want to update. If there is no user to update, then you get an empty response. @ulfgebhardt: I moved the spec also in the right directory. You're testing resolvers, so that's where I moved the `.spec` file. --- backend/src/middleware/userMiddleware.js | 2 +- backend/src/middleware/validation/index.js | 5 +-- .../users.spec.js} | 34 +++++++++++-------- 3 files changed, 24 insertions(+), 17 deletions(-) rename backend/src/{middleware/userMiddleware.spec.js => resolvers/users.spec.js} (71%) diff --git a/backend/src/middleware/userMiddleware.js b/backend/src/middleware/userMiddleware.js index 78f04638d..b3fc1bf2c 100644 --- a/backend/src/middleware/userMiddleware.js +++ b/backend/src/middleware/userMiddleware.js @@ -15,6 +15,6 @@ export default { const result = await resolve(root, args, context, info) await createOrUpdateLocations(args.id, args.locationName, context.driver) return result - }, + } } } diff --git a/backend/src/middleware/validation/index.js b/backend/src/middleware/validation/index.js index c84e111ee..de9be72e9 100644 --- a/backend/src/middleware/validation/index.js +++ b/backend/src/middleware/validation/index.js @@ -4,6 +4,7 @@ const USERNAME_MIN_LENGTH = 3 const validateUsername = async (resolve, root, args, context, info) => { if (args.name && args.name.length >= USERNAME_MIN_LENGTH) { + /* eslint-disable-next-line no-return-await */ return await resolve(root, args, context, info) } else { throw new UserInputError(`Username must be at least ${USERNAME_MIN_LENGTH} characters long!`) @@ -14,6 +15,7 @@ const validateUrl = async (resolve, root, args, context, info) => { const { url } = args const isValid = url.match(/^(?:https?:\/\/)(?:[^@\n])?(?:www\.)?([^:/\n?]+)/g) if (isValid) { + /* eslint-disable-next-line no-return-await */ return await resolve(root, args, context, info) } else { throw new UserInputError('Input is not a URL') @@ -22,9 +24,8 @@ const validateUrl = async (resolve, root, args, context, info) => { export default { Mutation: { - // CreateUser: validateUsername, + CreateUser: validateUsername, UpdateUser: validateUsername, CreateSocialMedia: validateUrl } } - diff --git a/backend/src/middleware/userMiddleware.spec.js b/backend/src/resolvers/users.spec.js similarity index 71% rename from backend/src/middleware/userMiddleware.spec.js rename to backend/src/resolvers/users.spec.js index ef752c729..6d2b791e3 100644 --- a/backend/src/middleware/userMiddleware.spec.js +++ b/backend/src/resolvers/users.spec.js @@ -5,15 +5,15 @@ import Factory from '../seed/factories' const factory = Factory() let client -afterAll(async () => { +afterEach(async () => { await factory.cleanDatabase() }) -describe('userMiddleware', () => { - describe('create User', () => { +describe('users', () => { + describe('CreateUser', () => { const mutation = ` - mutation($id: ID, $password: String!, $email: String!) { - CreateUser(id: $id, password: $password, email: $email) { + mutation($id: ID, $name: String, $password: String!, $email: String!) { + CreateUser(id: $id, name: $name, password: $password, email: $email) { id } } @@ -22,6 +22,7 @@ describe('userMiddleware', () => { it('with password and email', async () => { const variables = { + name: 'John Doe', password: '123', email: '123@123.de' } @@ -35,34 +36,39 @@ describe('userMiddleware', () => { }) }) - describe('update User', () => { + describe('UpdateUser', () => { + beforeEach(async () => { + await factory.create('User', { id: 'u47', name: 'John Doe' }) + }) + const mutation = ` mutation($id: ID!, $name: String) { UpdateUser(id: $id, name: $name) { + id name } } ` client = new GraphQLClient(host) - // TODO why is this failing - it returns { UpdateUser: null } - that should not be - /* it('name within specifications', async () => { + it('name within specifications', async () => { const variables = { - id: 'u1', - name: 'Peter Lustig' + id: 'u47', + name: 'James Doe' } const expected = { UpdateUser: { - name: 'Peter Lustig' + id: 'u47', + name: 'James Doe' } } await expect(client.request(mutation, variables)) .resolves.toEqual(expected) - }) */ + }) it('with no name', async () => { const variables = { - id: 'u1' + id: 'u47' } const expected = 'Username must be at least 3 characters long!' await expect(client.request(mutation, variables)) @@ -71,7 +77,7 @@ describe('userMiddleware', () => { it('with too short name', async () => { const variables = { - id: 'u1', + id: 'u47', name: ' ' } const expected = 'Username must be at least 3 characters long!' From 2d1daee23aceec496a2a91d3193af64b34bad3d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Sch=C3=A4fer?= Date: Mon, 13 May 2019 12:44:47 +0200 Subject: [PATCH 3/3] Don't document that we can set IDs in mutations --- backend/src/resolvers/users.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/src/resolvers/users.spec.js b/backend/src/resolvers/users.spec.js index 6d2b791e3..48e4741d7 100644 --- a/backend/src/resolvers/users.spec.js +++ b/backend/src/resolvers/users.spec.js @@ -12,8 +12,8 @@ afterEach(async () => { describe('users', () => { describe('CreateUser', () => { const mutation = ` - mutation($id: ID, $name: String, $password: String!, $email: String!) { - CreateUser(id: $id, name: $name, password: $password, email: $email) { + mutation($name: String, $password: String!, $email: String!) { + CreateUser(name: $name, password: $password, email: $email) { id } }