From b8f0cc6fa1a3041bcfb185843e35d39e5454a0fe Mon Sep 17 00:00:00 2001 From: Hariom Balhara Date: Tue, 15 Oct 2024 20:37:40 +0530 Subject: [PATCH] fix: Add back missing Any in and Not in operators for Single Select (#17102) * fix: Missing any_in and not_anhy_in opperators * Add tests --- .../routing-forms/__tests__/config.test.ts | 104 +++++++++ .../routing-forms/__tests__/zod.test.ts | 212 ++++++++++++++++++ .../config/BasicConfig.ts | 12 +- .../config/config.tsx | 5 +- .../trpc/formMutation.handler.ts | 47 +++- packages/app-store/routing-forms/zod.ts | 58 +++++ 6 files changed, 426 insertions(+), 12 deletions(-) create mode 100644 packages/app-store/routing-forms/__tests__/config.test.ts create mode 100644 packages/app-store/routing-forms/__tests__/zod.test.ts diff --git a/packages/app-store/routing-forms/__tests__/config.test.ts b/packages/app-store/routing-forms/__tests__/config.test.ts new file mode 100644 index 0000000000..db9c427c3e --- /dev/null +++ b/packages/app-store/routing-forms/__tests__/config.test.ts @@ -0,0 +1,104 @@ +import { describe, it, vi, expect } from "vitest"; + +import { + FormFieldsBaseConfig, + AttributesBaseConfig, +} from "../components/react-awesome-query-builder/config/config"; + +vi.mock("../components/react-awesome-query-builder/widgets", () => ({ + default: {}, +})); +vi.mock("@calcom/ui", () => ({})); + +const assertCommonStructure = (config: any) => { + expect(config).toHaveProperty("conjunctions"); + expect(config).toHaveProperty("operators"); + expect(config).toHaveProperty("types"); + expect(config).toHaveProperty("widgets"); + expect(config).toHaveProperty("settings"); +}; + +const assertCommonWidgetTypes = (config: any) => { + expect(config.widgets).toHaveProperty("text"); + expect(config.widgets).toHaveProperty("textarea"); + expect(config.widgets).toHaveProperty("number"); + expect(config.widgets).toHaveProperty("multiselect"); + expect(config.widgets).toHaveProperty("select"); + expect(config.widgets).toHaveProperty("phone"); + expect(config.widgets).toHaveProperty("email"); +}; + +const assertSelectOperators = (config: any) => { + expect(config.operators).toHaveProperty("select_any_in"); + expect(config.operators).toHaveProperty("select_not_any_in"); + expect(config.operators).toHaveProperty("select_equals"); + expect(config.operators).toHaveProperty("select_not_equals"); + + // Verify corresponding widgets for select operators + expect(config.types.select.widgets.multiselect.operators).toContain("select_any_in"); + expect(config.types.select.widgets.multiselect.operators).toContain("select_not_any_in"); + + // Important to verify that select_equals and select_not_equals are not present in multiselect as that might cause multiselect widget operand to show up for these operators + expect(config.types.select.widgets.multiselect.operators).not.toContain("select_equals"); + expect(config.types.select.widgets.multiselect.operators).not.toContain("select_not_equals"); + + + expect(config.types.select.widgets.select.operators).toContain("select_equals"); + expect(config.types.select.widgets.select.operators).toContain("select_not_equals"); + + // Important to verify that select_any_in and select_not_any_in are not present in select as that might cause select widget operand to show up for these operators + expect(config.types.select.widgets.select.operators).not.toContain("select_any_in"); + expect(config.types.select.widgets.select.operators).not.toContain("select_not_any_in"); +}; + +const assertMaxNesting = (config: any, value: number) => { + expect(config.settings.maxNesting).toBe(value); +}; + +describe("Query Builder Config", () => { + describe("FormFieldsBaseConfig", () => { + it("should have the correct structure", () => { + assertCommonStructure(FormFieldsBaseConfig); + }); + + it("should not support multiselect_contains and multiselect_not_contains - because they are not supported in Prisma for reporting(probably)", () => { + expect(FormFieldsBaseConfig.operators).not.toHaveProperty("multiselect_contains"); + expect(FormFieldsBaseConfig.operators).not.toHaveProperty("multiselect_not_contains"); + }); + + it("should support select_any_in, select_not_any_in, select_equals, select_not_equals - Verify both types.widgets and operators", () => { + assertSelectOperators(FormFieldsBaseConfig); + }); + + it("should have specific widget types", () => { + assertCommonWidgetTypes(FormFieldsBaseConfig); + }); + + it("should have maxNesting set to 1 in settings", () => { + assertMaxNesting(FormFieldsBaseConfig, 1); + }); + }); + + describe("AttributesBaseConfig", () => { + it("should have the correct structure", () => { + assertCommonStructure(AttributesBaseConfig); + }); + + it("should support multiselect_contains and multiselect_not_contains operators", () => { + expect(AttributesBaseConfig.operators).toHaveProperty("multiselect_contains"); + expect(AttributesBaseConfig.operators).toHaveProperty("multiselect_not_contains"); + }); + + it("should support select_any_in, select_not_any_in, select_equals, select_not_equals - Verify both types.widgets and operators", () => { + assertSelectOperators(AttributesBaseConfig); + }); + + it("should have specific widget types", () => { + assertCommonWidgetTypes(AttributesBaseConfig); + }); + + it("should have maxNesting set to 1 in settings", () => { + assertMaxNesting(AttributesBaseConfig, 1); + }); + }); +}); diff --git a/packages/app-store/routing-forms/__tests__/zod.test.ts b/packages/app-store/routing-forms/__tests__/zod.test.ts new file mode 100644 index 0000000000..54f581d695 --- /dev/null +++ b/packages/app-store/routing-forms/__tests__/zod.test.ts @@ -0,0 +1,212 @@ +import { describe, it, expect } from "vitest"; + +import { queryValueSaveValidationSchema } from "../zod"; + +describe("queryValueValidationSchema", () => { + it("should allow a rule with value", () => { + const validQueryValue = { + id: "1", + type: "group", + properties: {}, + children1: { + rule1: { + type: "rule", + properties: { + field: "name", + operator: "equal", + value: ["John"], + }, + }, + }, + }; + + const result = queryValueSaveValidationSchema.safeParse(validQueryValue); + expect(result.success).toBe(true); + }); + + it("should allow a query value with switch_group type for queryValue", () => { + const switchGroupQueryValue = { + id: "2", + type: "switch_group", + properties: {}, + children1: {}, + }; + + const result = queryValueSaveValidationSchema.safeParse(switchGroupQueryValue); + expect(result.success).toBe(true); + }); + + it('should allow a possibly invalid query value if the rule type is not "rule" - Goal is to ensure that rule type children1 is correct', () => { + const switchGroupQueryValue = { + id: "2", + type: "switch_group", + properties: {}, + children1: { + rule1: { + type: "abc", + properties: { + field: "name", + operator: "equal", + value: [], + }, + }, + }, + }; + + const result = queryValueSaveValidationSchema.safeParse(switchGroupQueryValue); + expect(result.success).toBe(true); + }); + + it("should reject an invalid type for queryValue", () => { + const invalidTypeQueryValue = { + id: "3", + type: "invalid_type", + properties: {}, + children1: {}, + }; + + const result = queryValueSaveValidationSchema.safeParse(invalidTypeQueryValue); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].path).toEqual(["type"]); + } + }); + + it("should reject a rule with an empty value array", () => { + const emptyValueQueryValue = { + id: "4", + type: "group", + properties: {}, + children1: { + rule1: { + type: "rule", + properties: { + field: "name", + operator: "equal", + value: [], + }, + }, + }, + }; + + const result = queryValueSaveValidationSchema.safeParse(emptyValueQueryValue); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toEqual( + "Looks like you are trying to create a rule with no value" + ); + } + }); + + it("should reject a rule with 2D empty value array", () => { + const emptyValueQueryValue = { + id: "4", + type: "group", + properties: {}, + children1: { + rule1: { + type: "rule", + properties: { + field: "name", + operator: "equal", + value: [[]], + }, + }, + }, + }; + + const result = queryValueSaveValidationSchema.safeParse(emptyValueQueryValue); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toEqual( + "Looks like you are trying to create a rule with no value" + ); + } + }); + + it("should reject a rule with just undefined values", () => { + const emptyValueQueryValue = { + id: "4", + type: "group", + properties: {}, + children1: { + rule1: { + type: "rule", + properties: { + field: "name", + operator: "equal", + value: [undefined, undefined], + }, + }, + }, + }; + + const result = queryValueSaveValidationSchema.safeParse(emptyValueQueryValue); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0].message).toEqual( + "Looks like you are trying to create a rule with no value" + ); + } + }); + + it("should allow a rule with null values", () => { + const nullValueQueryValue = { + id: "5", + type: "group", + properties: {}, + children1: { + rule1: { + type: "rule", + properties: { + field: "name", + operator: "equal", + value: [null], + }, + }, + }, + }; + + const result = queryValueSaveValidationSchema.safeParse(nullValueQueryValue); + expect(result.success).toBe(true); + }); + + it("should allow a rule with empty string values", () => { + const nullValueQueryValue = { + id: "5", + type: "group", + properties: {}, + children1: { + rule1: { + type: "rule", + properties: { + field: "name", + operator: "equal", + value: [""], + }, + }, + }, + }; + + const result = queryValueSaveValidationSchema.safeParse(nullValueQueryValue); + expect(result.success).toBe(true); + }); + + it("should allow omitting the children1 and properties field - e.g. fallback route doesn't have it", () => { + const queryValueWithoutChildren = { + id: "6", + type: "group", + }; + + const result = queryValueSaveValidationSchema.safeParse(queryValueWithoutChildren); + expect(result.success).toBe(true); + }); + + it("we are fine with no queryValue even", () => { + const result = queryValueSaveValidationSchema.safeParse(null); + expect(result.success).toBe(true); + + const result2 = queryValueSaveValidationSchema.safeParse(undefined); + expect(result2.success).toBe(true); + }); +}); diff --git a/packages/app-store/routing-forms/components/react-awesome-query-builder/config/BasicConfig.ts b/packages/app-store/routing-forms/components/react-awesome-query-builder/config/BasicConfig.ts index 7465193343..6cfabbea3c 100644 --- a/packages/app-store/routing-forms/components/react-awesome-query-builder/config/BasicConfig.ts +++ b/packages/app-store/routing-forms/components/react-awesome-query-builder/config/BasicConfig.ts @@ -343,10 +343,6 @@ const types: Types = { operators: [ "select_equals", "select_not_equals", - // "is_empty", - // "is_not_empty", - "is_null", - "is_not_null", ], widgetProps: { customProps: { @@ -356,12 +352,8 @@ const types: Types = { }, multiselect: { operators: [ - // "select_any_in", - // "select_not_any_in", - // "is_empty", - // "is_not_empty", - "is_null", - "is_not_null", + "select_any_in", + "select_not_any_in", ], }, }, diff --git a/packages/app-store/routing-forms/components/react-awesome-query-builder/config/config.tsx b/packages/app-store/routing-forms/components/react-awesome-query-builder/config/config.tsx index 769aacc086..f1c66fe765 100644 --- a/packages/app-store/routing-forms/components/react-awesome-query-builder/config/config.tsx +++ b/packages/app-store/routing-forms/components/react-awesome-query-builder/config/config.tsx @@ -52,7 +52,9 @@ function getSettings(_configFor: ConfigFor) { renderProvider: (props) => renderComponent(props, Provider), groupActionsPosition: "bottomCenter", - + // TODO: Test it and then enable it. It might allow us to show better error messages. + // But it doesn't detect every kind of error like an operator gone missing e.g. what happened in https://github.com/calcom/cal.com/pull/17102 + // showErrorMessage: true, // Disable groups maxNesting: 1, }; @@ -166,6 +168,7 @@ function getOperators(configFor: ConfigFor) { const operators: Operators = { ...BasicConfig.operators, // Attributes don't need reporting at the moment. So, we can support contains and not contains operators for attributes. + // We could probably use them in FormFields if they are supported through Prisma query as well. ...(configFor === ConfigFor.Attributes ? { multiselect_contains: { diff --git a/packages/app-store/routing-forms/trpc/formMutation.handler.ts b/packages/app-store/routing-forms/trpc/formMutation.handler.ts index 1ae0b6a7d2..1f6ae9c57b 100644 --- a/packages/app-store/routing-forms/trpc/formMutation.handler.ts +++ b/packages/app-store/routing-forms/trpc/formMutation.handler.ts @@ -13,7 +13,7 @@ import { isFormCreateEditAllowed } from "../lib/isFormCreateEditAllowed"; import isRouter from "../lib/isRouter"; import isRouterLinkedField from "../lib/isRouterLinkedField"; import type { SerializableForm } from "../types/types"; -import { zodFields, zodRouterRoute, zodRoutes } from "../zod"; +import { zodFields, zodRouterRoute, zodRoutes, queryValueSaveValidationSchema } from "../zod"; import type { TFormMutationInputSchema } from "./formMutation.schema"; interface FormMutationHandlerOptions { @@ -23,6 +23,43 @@ interface FormMutationHandlerOptions { }; input: TFormMutationInputSchema; } + +function throwIfInvalidQueryValueToBeSaved({ + routes, +}: { + routes: FormMutationHandlerOptions["input"]["routes"]; +}) { + if (!routes) { + return; + } + routes.forEach((route, routeIndex) => { + if (isRouter(route)) { + return; + } + // We use separate schema for queryValye here which is much more strict + // It allows that we are still lenient with schema while reading the queryValue but while saving it we are strict + const parsedFormFieldsQueryValue = queryValueSaveValidationSchema.safeParse(route.queryValue); + if (!parsedFormFieldsQueryValue.success) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: `Route ${routeIndex + 1} form fields: ${parsedFormFieldsQueryValue.error.errors + .map((err) => err.message) + .join(", ")}`, + }); + } + + const parsedAttributesQueryValue = queryValueSaveValidationSchema.safeParse(route.attributesQueryValue); + if (!parsedAttributesQueryValue.success) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: `Route ${routeIndex + 1} attributes: ${parsedAttributesQueryValue.error.errors + .map((err) => err.message) + .join(", ")}`, + }); + } + }); +} + export const formMutationHandler = async ({ ctx, input }: FormMutationHandlerOptions) => { const { user, prisma } = ctx; const { name, id, description, disabled, addFallback, duplicateFrom, shouldConnect } = input; @@ -34,6 +71,14 @@ export const formMutationHandler = async ({ ctx, input }: FormMutationHandlerOpt }); } let { routes: inputRoutes } = input; + + // Ensures that wrong queryValue is not saved + // This is super useful when we make some wrong change in RAQB config accidentally and end up + // - Removing an operator support from a rule + // - Populating things wrong for any reason in RAQB + // It would just ensure that the wrong queryValue is not saved. Because it is impossible to fix it once saved. User would have to manually fix it then. + throwIfInvalidQueryValueToBeSaved({ routes: inputRoutes }); + let { fields: inputFields } = input; inputFields = inputFields || []; inputRoutes = inputRoutes || []; diff --git a/packages/app-store/routing-forms/zod.ts b/packages/app-store/routing-forms/zod.ts index 02da376a4e..faf97a0b8b 100644 --- a/packages/app-store/routing-forms/zod.ts +++ b/packages/app-store/routing-forms/zod.ts @@ -55,6 +55,64 @@ const queryValueSchema = z.object({ properties: z.any(), }); +/** + * Stricter schema for validating before saving to DB + */ +export const queryValueSaveValidationSchema = queryValueSchema + .omit({ children1: true }) + .merge( + z.object({ + children1: z + .record( + z.object({ + type: z.string().optional(), + properties: z + .object({ + field: z.any().optional(), + operator: z.any().optional(), + value: z.any().optional(), + }) + .optional(), + }) + ) + .optional() + // Be very careful and lenient here. Just ensure that the rule isn't invalid without breaking anything + .superRefine((children1, ctx) => { + if (!children1) return; + const isObject = (value: unknown): value is Record => + typeof value === "object" && value !== null; + Object.entries(children1).forEach(([, _rule]) => { + const rule = _rule as unknown; + if (!isObject(rule) || rule.type !== "rule") return; + if (!isObject(rule.properties)) return; + + const value = rule.properties.value || []; + if (!(value instanceof Array)) { + return; + } + + // MultiSelect array can be 2D array + const flattenedValues = value.flat(); + + const validValues = flattenedValues.filter((value: unknown) => { + // Might want to restrict it to filter out null and empty string as well. But for now we know that Prisma errors only for undefined values when saving it in JSON field + // Also, it is possible that RAQB has some requirements to support null or empty string values. + if (value === undefined) return false; + return true; + }); + + if (!validValues.length) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "Looks like you are trying to create a rule with no value", + }); + } + }); + }), + }) + ) + .nullish(); + export const zodNonRouterRoute = z.object({ id: z.string(), attributeRoutingConfig: z