From 4d675bb808aa4f56fa9bf27af464ad7b7af39512 Mon Sep 17 00:00:00 2001 From: Timi Duban Date: Sat, 10 Jul 2021 12:26:05 +0200 Subject: [PATCH] regroup data validations functions in one --- .../resolvers/defaultMutationResolvers.ts | 1 - packages/graphql/server/resolvers/mutators.ts | 20 +-- .../graphql/server/resolvers/validation.ts | 152 +++++++----------- .../server/tests/documentValidation.test.ts | 121 +++++++------- 4 files changed, 124 insertions(+), 170 deletions(-) diff --git a/packages/graphql/server/resolvers/defaultMutationResolvers.ts b/packages/graphql/server/resolvers/defaultMutationResolvers.ts index 5c541854..155ef784 100644 --- a/packages/graphql/server/resolvers/defaultMutationResolvers.ts +++ b/packages/graphql/server/resolvers/defaultMutationResolvers.ts @@ -38,7 +38,6 @@ interface BuildDefaultMutationResolversInput { } interface GetDocumentSelectorInput { - // TODO: put in common with the single resolver variables type, that have the same fields variables: { _id?: string; input: any; // SingleInput diff --git a/packages/graphql/server/resolvers/mutators.ts b/packages/graphql/server/resolvers/mutators.ts index 88dd8bbb..3bf17085 100644 --- a/packages/graphql/server/resolvers/mutators.ts +++ b/packages/graphql/server/resolvers/mutators.ts @@ -33,10 +33,9 @@ to the client. */ import { - validateDocument, - validateData, + validateDatas, modifierToData, - dataToModifier, + dataToModifier } from "./validation"; import { runCallbacks } from "@vulcanjs/core"; @@ -71,7 +70,6 @@ const validateMutationData = async ({ mutatorName, context, properties, - validationFunction, }: { model: VulcanGraphqlModel; // data model mutatorName: DefaultMutatorName; @@ -81,12 +79,8 @@ const validateMutationData = async ({ validationFunction?: Function; }): Promise => { const { typeName } = model.graphql; - // basic simple schema validatio - const simpleSchemaValidationErrors = data - ? validationFunction - ? validationFunction(data, model, context) // for update, we use validateData instead of validateDocument - : validateDocument(data, model, context) - : []; // delete mutator has no data, so we skip the simple schema validation + // basic simple schema validation + const simpleSchemaValidationErrors = validateDatas({document: data, model, context, mutatorName}); // custom validation const customValidationErrors = await runCallbacks({ hookName: `${typeName}.${mutatorName}.validate`, @@ -227,12 +221,9 @@ export const createMutator = async ({ }); } - /* If user is logged in, check if userId field is in the schema and add it to document if needed */ if (currentUser) { - // TODO: clean this using "has" - const userIdInSchema = "userId" in schema; - if (!!userIdInSchema && !data.userId) data.userId = currentUser._id; + if (schema.hasOwnProperty('userId') && !data.userId) data.userId = currentUser._id; } /* @@ -388,7 +379,6 @@ export const updateMutator = async ({ mutatorName, context, properties, - validationFunction: validateData, // TODO: update uses validateData instead of validateDocument => why? }); } diff --git a/packages/graphql/server/resolvers/validation.ts b/packages/graphql/server/resolvers/validation.ts index 9e5e58af..6a79061d 100644 --- a/packages/graphql/server/resolvers/validation.ts +++ b/packages/graphql/server/resolvers/validation.ts @@ -8,6 +8,7 @@ import { import _forEach from "lodash/forEach"; import { VulcanModel } from "@vulcanjs/model"; import { ContextWithUser } from "./typings"; +import { DefaultMutatorName } from "../../typings"; import { canCreateField, canUpdateField } from "@vulcanjs/permissions"; import { toSimpleSchema, ValidationError } from "@vulcanjs/schema"; @@ -74,125 +75,80 @@ const validateDocumentPermissions = ( ); return validationErrors; }; + +interface ValidateDatasInput { + document: VulcanDocument; + model: VulcanModel; + context: any; + mutatorName: DefaultMutatorName; + validationContextName?: string; +} /* If document is not trusted, run validation steps: - 1. Check that the current user has permission to edit each field + 1. Check that the current user has permission to insert each field 2. Run SimpleSchema validation step */ -export const validateDocument = ( - document: VulcanDocument, - model: VulcanModel, - context: any, +export const validateDatas = ({ + document, + model, + context, + mutatorName, validationContextName = "defaultContext" // TODO: what is this? -): Array => { +}: ValidateDatasInput): Array => { const { schema } = model; let validationErrors: Array = []; - // validate creation permissions (and other Vulcan-specific constraints) + // delete mutator has no data, so we skip the simple schema validation + if (mutatorName === 'delete') { + return validationErrors; + } + // validate operation permissions on each field (and other Vulcan-specific constraints) validationErrors = validationErrors.concat( - validateDocumentPermissions(document, document, schema, context, "create") + validateDocumentPermissions(document, document, schema, context, mutatorName) ); // build the schema on the fly // TODO: does it work with nested schema??? const simpleSchema = toSimpleSchema(schema); // run simple schema validation (will check the actual types, required fields, etc....) const validationContext = simpleSchema.namedContext(validationContextName); - validationContext.validate(document); - - if (!validationContext.isValid()) { - const errors = (validationContext as any).validationErrors(); - errors.forEach((error) => { - // eslint-disable-next-line no-console - // console.log(error); - if (error.type.includes("intlError")) { - const intlError = JSON.parse(error.type.replace("intlError|", "")); - validationErrors = validationErrors.concat(intlError); - } else { - validationErrors.push({ - id: `errors.${error.type}`, - path: error.name, - properties: { - modelName: model.name, - // typeName: collection.options.typeName, - ...error, - }, - }); - } - }); + // validate the schema, depends on which operation you want to do. + if (mutatorName === 'create') { + validationContext.validate(document); + } + if (mutatorName === 'update') { + const modifier: Modifier = dataToModifier(document); + const set = modifier.$set; + const unset = modifier.$unset + validationContext.validate( + { $set: set, $unset: unset }, + { modifier: true, extendedCustomContext: { documentId: document._id } } + ); } - - return validationErrors; -}; - -/* - - If document is not trusted, run validation steps: - - 1. Check that the current user has permission to insert each field - 2. Run SimpleSchema validation step - -*/ -export const validateModifier = ( - modifier: Modifier, - document: VulcanDocument, - model: VulcanModel, - context, - validationContextName = "defaultContext" -) => { - const { schema } = model; - const set = modifier.$set; - const unset = modifier.$unset; - - let validationErrors: Array = []; - - // 1. check that the current user has permission to edit each field - validationErrors = validationErrors.concat( - validateDocumentPermissions(document, document, schema, context, "update") - ); - - // 2. run SS validation - const validationContext = toSimpleSchema(schema).namedContext( - validationContextName - ); - validationContext.validate( - { $set: set, $unset: unset }, - { modifier: true, extendedCustomContext: { documentId: document._id } } - ); - if (!validationContext.isValid()) { - const errors = validationContext.validationErrors(); - errors.forEach((error) => { - // eslint-disable-next-line no-console - // console.log(error); - if (error?.type?.includes("intlError")) { - validationErrors = validationErrors.concat( - JSON.parse(error.type.replace("intlError|", "")) - ); - } else { - validationErrors.push({ - id: `errors.${error.type}`, - path: error.name, - properties: { - modelName: model.name, - // typeName: collection.options.typeName, - ...error, - }, - }); - } - }); + const errors = (validationContext as any).validationErrors(); + errors.forEach((error) => { + // eslint-disable-next-line no-console + // console.log(error); + if (error.type.includes("intlError")) { + const intlError = JSON.parse(error.type.replace("intlError|", "")); + validationErrors = validationErrors.concat(intlError); + } else { + validationErrors.push({ + id: `errors.${error.type}`, + path: error.name, + properties: { + modelName: model.name, + // typeName: collection.options.typeName, + ...error, + }, + }); + } + }); } return validationErrors; -}; - -export const validateData = ( - data: VulcanDocument, - model: VulcanModel, - context -) => { - return validateModifier(dataToModifier(data), data, model, context); -}; +}; \ No newline at end of file diff --git a/packages/graphql/server/tests/documentValidation.test.ts b/packages/graphql/server/tests/documentValidation.test.ts index ebe81d12..27b9fc41 100644 --- a/packages/graphql/server/tests/documentValidation.test.ts +++ b/packages/graphql/server/tests/documentValidation.test.ts @@ -1,4 +1,4 @@ -import { validateDocument, validateData } from "../resolvers/validation"; +import { validateDatas } from "../resolvers/validation"; import { createModel } from "@vulcanjs/model"; // import Users from "meteor/vulcan:users" @@ -19,9 +19,9 @@ describe("vulcan:lib/validation", () => { }, }); // create - const errors = validateDocument({ foo: "bar" }, model, defaultContext); + const errors = validateDatas({ document: { foo: "bar" }, model, context: defaultContext, mutatorName: 'create' }); expect(errors).toHaveLength(0); - const updateErrors = validateData({ foo: "bar" }, model, defaultContext); + const updateErrors = validateDatas({ document: { foo: "bar" }, model, context: defaultContext, mutatorName: 'update' }); expect(updateErrors).toHaveLength(0); }); test("create error for non creatable field", () => { @@ -40,21 +40,23 @@ describe("vulcan:lib/validation", () => { }, }, }); - const errors = validateDocument( - { foo: "bar", bar: "foo" }, + const errors = validateDatas({ + document: { foo: "bar", bar: "foo" }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'create' + }); expect(errors).toHaveLength(1); expect(errors[0]).toMatchObject({ id: "errors.disallowed_property_detected", properties: { name: "foo" }, }); - const updateErrors = validateData( - { foo: "bar", bar: "foo" }, + const updateErrors = validateDatas({ + document: { foo: "bar", bar: "foo" }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'update' + }); expect(updateErrors).toHaveLength(1); expect(updateErrors[0]).toMatchObject({ id: "errors.disallowed_property_detected", @@ -91,23 +93,25 @@ describe("vulcan:lib/validation", () => { }, }); // create - const errors = validateDocument( - { nested: { foo: "bar", bar: "foo" } }, + const errors = validateDatas({ + document: { nested: { foo: "bar", bar: "foo" } }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'create' + }); expect(errors).toHaveLength(1); expect(errors[0]).toMatchObject({ id: "errors.disallowed_property_detected", properties: { name: "nested.foo" }, }); // update with set and unset - const updateErrors = validateData( - { nested: { foo: "bar", bar: "foo", zed: null } }, - // { nested: { foo: "bar", bar: "foo", zed: "hello" } }, + const updateErrors = validateDatas({ + document: { nested: { foo: "bar", bar: "foo", zed: null } }, + // document: { nested: { foo: "bar", bar: "foo", zed: 'hello' } }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'update' + }); expect(updateErrors).toHaveLength(2); expect(updateErrors[0]).toMatchObject( { @@ -145,23 +149,24 @@ describe("vulcan:lib/validation", () => { }, }, }); - const errors = validateDocument( - { nested: [{ foo: "bar", bar: "foo" }] }, + const errors = validateDatas({ + document: { nested: [{ foo: "bar", bar: "foo" }] }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'create' + }); expect(errors).toHaveLength(1); expect(errors[0]).toMatchObject({ id: "errors.disallowed_property_detected", properties: { name: "nested[0].foo" }, }); - const updateErrors = validateData( - { nested: [{ foo: "bar", bar: "foo" }] }, - // { nested: [{ foo: "bar", bar: "foo" }] }, + const updateErrors = validateDatas({ + document: { nested: [{ foo: "bar", bar: "foo" }] }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'update' + }); expect(updateErrors).toHaveLength(1); expect(updateErrors[0]).toMatchObject({ id: "errors.disallowed_property_detected", @@ -196,23 +201,25 @@ describe("vulcan:lib/validation", () => { }, }); // create - const errors = validateDocument( - { nested: { nok: "bar", ok: "foo" } }, + const errors = validateDatas({ + document: { nested: { nok: "bar", ok: "foo" } }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'create' + }); expect(errors).toHaveLength(1); expect(errors[0]).toMatchObject({ id: "errors.disallowed_property_detected", properties: { name: "nested.nok" }, }); // update with set and unset - const updateErrors = validateData( - { nested: { nok: "bar", ok: "foo", zed: null } }, - // { nested: { nok: "bar", ok: "foo", zed: "hello" } }, + const updateErrors = validateDatas({ + document: { nested: { nok: "bar", ok: "foo", zed: null } }, + // document: { nested: { nok: "bar", ok: "foo", zed: "hello" } }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'update' + }); expect(updateErrors).toHaveLength(2); expect(updateErrors[0]).toMatchObject( { @@ -244,19 +251,20 @@ describe("vulcan:lib/validation", () => { }, }, }); - const errors = validateDocument( - { nested: { foo: "bar" } }, + const errors = validateDatas({ + document: { nested: { foo: "bar" } }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'create' + }); expect(errors).toHaveLength(0); - const updateErrors = validateData( - { nested: { foo: "bar" } }, - // { nested: { foo: "bar" } }, + const updateErrors = validateDatas({ + document: { nested: { foo: "bar" } }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'update' + }); expect(updateErrors).toHaveLength(0); }); test("do not check native arrays", () => { @@ -273,19 +281,20 @@ describe("vulcan:lib/validation", () => { }, }, }); - const errors = validateDocument( - { array: [1, 2, 3] }, + const errors = validateDatas({ + document: { array: [1, 2, 3] }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'create' + }); expect(errors).toHaveLength(0); - const updateErrors = validateData( - { array: [1, 2, 3] }, - // { array: [1, 2, 3] }, + const updateErrors = validateDatas({ + document: { array: [1, 2, 3] }, model, - defaultContext - ); + context: defaultContext, + mutatorName: 'update' + }); expect(updateErrors).toHaveLength(0); }); });