feat(api-service,framework): validate liquidjs filter args for the variables #8178

LetItRock · 2025-04-22T16:15:06Z

What changed? Why was the change needed?

Implemented the validation logic for the liquid variable filters, which is built on top of the step issues.
The filter args validation function is defined close to the LiquidJS filter, meaning that in the Framework. We can later decide if we want to do similar validation in the Framework.

Screenshots

Screen.Recording.2025-04-22.at.18.01.22.mov

linear · 2025-04-22T16:15:09Z

NV-5705 The toSentence filter with the `steps.x.events` variable and no key should result in validation error

netlify · 2025-04-22T16:15:30Z

✅ Deploy Preview for dashboard-v2-novu-staging ready!

Name	Link
🔨 Latest commit	`b411e67`
🔍 Latest deploy log	https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/6808e3b904ebe600081c4b9a
😎 Deploy Preview	https://deploy-preview-8178.dashboard-v2.novu-staging.co
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

pkg-pr-new · 2025-04-22T16:28:44Z

Open in StackBlitz

npm i https://pkg.pr.new/novuhq/novu/@novu/framework@8178

npm i https://pkg.pr.new/novuhq/novu@8178

npm i https://pkg.pr.new/novuhq/novu/@novu/providers@8178

npm i https://pkg.pr.new/novuhq/novu/@novu/shared@8178

commit: b411e67

LetItRock · 2025-04-22T16:18:51Z

apps/api/src/app/workflows-v2/usecases/build-step-issues/build-step-issues.usecase.ts

+          if ('filterMessage' in error) {
+            return {
+              message: `Variable ${error.output} filter ${error.filterMessage}`,
+              issueType: StepContentIssueEnum.INVALID_FILTER_ARG_IN_VARIABLE,
+              variableName: error.output,
+            };
+          }


The LiquidJS filter errors have the filterMessage field so this way we differentiate it from the variable errors.
In the future, there will be more fields like filterBegin: number; filterEnd: number, which could tell the exact place in the template where the error is.

LetItRock · 2025-04-22T16:23:11Z

apps/api/src/app/workflows-v2/util/build-variables.ts

+  const { validVariables, invalidVariables } = extractLiquidTemplateVariables({
+    template: JSON.stringify(variableControlValue),
+    variableSchema,
+  });


We now have only one place where we create valid/invalid variables.
This change also simplifies the future extension for the additional invalid variables information, like a start, end positions in the template, which the FE could then use to inform the user.

LetItRock · 2025-04-22T16:23:29Z

apps/api/src/app/workflows-v2/util/build-variables.ts

-  };
-}
-
-function isPropertyAllowed(schema: Record<string, unknown>, propertyPath: string) {


this function is used in the another file

LetItRock · 2025-04-22T16:25:02Z

apps/api/src/app/workflows-v2/util/template-parser/liquid-parser.ts

-export function extractLiquidTemplateVariables(template: string): TemplateVariables {
+export function extractLiquidTemplateVariables({
+  template,
+  variableSchema,


pass the variableSchema to be able to validate the variables against it

LetItRock · 2025-04-22T16:26:37Z

apps/api/src/app/workflows-v2/util/template-parser/liquid-parser.ts

-  const validVariables: Variable[] = [];
-  const invalidVariables: Variable[] = [];
-  const parsed = parserEngine.parse(rawOutput) as unknown as Template[];
+function isPropertyAllowed(schema: Record<string, unknown> | undefined, propertyPath: string) {


LetItRock · 2025-04-22T16:29:17Z

apps/api/src/app/workflows-v2/util/template-parser/liquid-parser.ts

+      const filters = extractFilters(template);
+      const filterIssues = validateFilters(filters, isDigestEventsVariable);


Extract filters from the parsed liquid template, and then validate the filters.
The single filter has the information about the filter name and args, also includes the position in the template.

apps/api/src/app/workflows-v2/util/template-parser/liquid-parser.ts

LetItRock · 2025-04-22T16:33:55Z

packages/framework/src/filters/to-sentence.ts

+  const argsSchema = z.object({
+    keyPath: requireKeyPath ? z.string().min(1, 'must be non-empty') : z.string().optional().default(DEFAULT_KEY_PATH),
+    limit: z
+      .number()
+      .optional()
+      .default(DEFAULT_LIMIT)
+      .refine((val) => {
+        return val >= 0;
+      }, 'must be greater than or equal to 0'),
+    overflowSuffix: z.string().optional().default(DEFAULT_OVERFLOW_SUFFIX),
+    wordsConnector: z.string().optional().default(DEFAULT_WORDS_CONNECTOR),
+    twoWordsConnector: z.string().optional().default(DEFAULT_TWO_WORDS_CONNECTOR),
+    lastWordConnector: z.string().optional().default(DEFAULT_LAST_WORD_CONNECTOR),
+  });


Zod schema for the filter args

LetItRock · 2025-04-22T16:34:14Z

packages/framework/src/filters/to-sentence.ts

+  args.forEach((arg, index) => {
+    if (!Array.isArray(arg)) {
+      let value: string | number = arg.getText();
+      if (arg.kind === TokenKind.Quoted) {
+        value = (arg as QuotedToken).content;
+      } else if (arg.kind === TokenKind.Number) {
+        value = (arg as NumberToken).content;
+      }
+      const argName = ARG_INDEX_TO_ARG_NAME[index];
+      argsObject[argName] = value;
+    }
+  });


convert args array to object that will be validated

LetItRock · 2025-04-22T16:34:28Z

packages/framework/src/filters/to-sentence.ts

+    for (const error of result.error.issues) {
+      let type = 'string';
+      if ('type' in error) {
+        type = error.type;
+      }
+
+      const path = error.path[0];
+      const argIndexToArgName = Object.entries(ARG_INDEX_TO_ARG_NAME).find(([_, argName]) => argName === path);
+      const argIndex = argIndexToArgName ? parseInt(argIndexToArgName[0], 10) : null;
+      const token = typeof argIndex === 'number' ? args[argIndex] : null;
+
+      if (token && !Array.isArray(token)) {
+        issues.push({
+          message: `"toSentence" expects a ${type}${error.message ? ` that ${error.message}` : ''} for argument "${path}"`,
+          begin: token.begin,
+          end: token.end,
+          value: token.getText(),
+        });
+      }
+    }


build the validation errors

apps/api/src/app/workflows-v2/util/template-parser/liquid-parser.ts

apps/api/src/app/workflows-v2/usecases/build-step-issues/build-step-issues.usecase.ts

SokratisVidros · 2025-04-22T17:04:05Z

apps/api/src/app/workflows-v2/util/template-parser/liquid-parser.ts

+  output: string;
+};
+
+type InvalidContentVariable = Variable & {


I am not sure I prefer having two separate types for this. It introduces too many types and I think it makes the code more cumbersome that it should be. Variables are still variables, regardless if they appear in filters or in content, right?

Alternatively, I believe extending the Variable object with extra optional attributes is a better idea. For now, the filterMessage is only used here to define the error message.

My suggestion is just populate the existing variable.message field for now with the following copywriting:

Filter ${error.filterMessage} in ${variable.name}.

Notice that I used the name not the full output of the variable for a shorter and friendlier message.

I've separated the types because in the future there could be details about the filter start and end positions. Also we don't have a way to differentiate these validation errors to build the different messages, that's why I introduced the filterMessage field.

So I'll revert to one type but with two fields for message, unless we want the type.

apps/api/src/app/workflows-v2/util/template-parser/liquid-parser.ts

SokratisVidros · 2025-04-22T17:07:03Z

packages/framework/src/filters/to-sentence.ts

+): LiquidFilterIssue[] {
+  const { requireKeyPath = false } = options;
+  const issues: LiquidFilterIssue[] = [];
+  if (args.length < 1) {


@LetItRock I am not sure I follow the logic here. Can't we verify args only using zod by converting the filter arguyments arguments to an object without having to do all this pre and post processing?

I'm not sure what you mean by pre/post processing. Can you clarify or pseudo code?

converting the filter arguyments arguments to an object

This is what I do at line 121

But if it's about the args, there are a few reasons it takes an args array:

the LiquidJS engine produces the output tokens for variables where the filters have args as an array

we don't need to have the conversion logic on the API side for every filter, so the code is generic there

it's aligned with the order of defined args in the filter function

SokratisVidros

My bad, I wanted to request changes :)

… errors

…riables (novuhq#8178)

LetItRock requested review from BiswaViraj, ChmaraX, SokratisVidros, desiprisg, djabarovgeorge and tatarco April 22, 2025 16:15

LetItRock self-assigned this Apr 22, 2025

LetItRock temporarily deployed to Linting April 22, 2025 16:15 — with GitHub Actions Inactive

github-actions bot added @novu/shared @novu/api-service labels Apr 22, 2025

feat(api-service,framework): validate liquidjs filter args

8273085

LetItRock force-pushed the nv-5705-validate-liquidjs-filter-args branch from ef89a0b to 8273085 Compare April 22, 2025 16:26

LetItRock commented Apr 22, 2025

View reviewed changes

SokratisVidros approved these changes Apr 22, 2025

View reviewed changes

SokratisVidros suggested changes Apr 22, 2025

View reviewed changes

chore(api-service): suggestions from the pr

293545d

LetItRock temporarily deployed to Linting April 22, 2025 21:44 — with GitHub Actions Inactive

LetItRock requested a review from SokratisVidros April 22, 2025 21:44

SokratisVidros approved these changes Apr 23, 2025

View reviewed changes

chore(api-service): fixed failing unit tests

1e5a6da

LetItRock temporarily deployed to Linting April 23, 2025 08:20 — with GitHub Actions Inactive

chore(dashboard): disable the step preview tab when it has validation…

160a471

… errors

github-actions bot added the @novu/dashboard label Apr 23, 2025

LetItRock added 2 commits April 23, 2025 14:56

Merge branch 'next' into nv-5705-validate-liquidjs-filter-args

0fc7c1e

chore(dashboard): fix prop types

b411e67

LetItRock temporarily deployed to Linting April 23, 2025 12:57 — with GitHub Actions Inactive

SokratisVidros approved these changes Apr 23, 2025

View reviewed changes

LetItRock merged commit aa3a725 into next Apr 23, 2025
32 checks passed

LetItRock deleted the nv-5705-validate-liquidjs-filter-args branch April 23, 2025 15:34

bricehemery pushed a commit to jack-agency/novu that referenced this pull request Jul 7, 2025

feat(api-service,framework): validate liquidjs filter args for the va…

2f63c55

…riables (novuhq#8178)

		const filters = extractFilters(template);
		const filterIssues = validateFilters(filters, isDigestEventsVariable);

feat(api-service,framework): validate liquidjs filter args for the variables #8178

feat(api-service,framework): validate liquidjs filter args for the variables #8178

Uh oh!

Conversation

LetItRock commented Apr 22, 2025

What changed? Why was the change needed?

Screenshots

Uh oh!

linear bot commented Apr 22, 2025

Uh oh!

netlify bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for dashboard-v2-novu-staging ready!

Uh oh!

pkg-pr-new bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LetItRock Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SokratisVidros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

netlify bot commented Apr 22, 2025 •

edited

Loading

pkg-pr-new bot commented Apr 22, 2025 •

edited

Loading

LetItRock Apr 22, 2025 •

edited

Loading