Add Runtime Validation Spike by w1am · Pull Request #349 · kurrent-io/KurrentDB-Client-NodeJS

w1am · 2023-11-15T05:49:33Z

Changed: Integrate Zod for Stronger Type Checking

This PR aims to enhance type safety and data integrity, ensuring our application handles data more robustly and reduces the likelihood of runtime errors due to type mismatches. Additional validations include checking UUIDs and ensuring certain numbers fall within specified limits.

- Update node version to 16 in build and lint workflow - Ensure deletion result BigInt values are type-checked

linear · 2023-11-15T05:49:36Z

DEV-197 Add runtime validation

- Upgrade typescript to support satisfies operator

yordis · 2024-01-04T01:58:23Z

+      // we don't want to spam the user with all the errors. Take the first one.
+      const message = error.issues[0].message;


Suggested change

// we don't want to spam the user with all the errors. Take the first one.

const message = error.issues[0].message;

// we don't want to spam the user with all the errors. Take the first one.

const message = error.issues[0].message;

Why not? I prefer to see the bag of errors than filtering and rerunning the code to find the next problem

It's something I was planning to change. If the user enters 10k events with a bad ID format, it will print 10k repeated errors.

I would expect a single error ZodError and if that prints all the values under issues when logging, so be it!

In other word, don't hide ZodError and let them use error.issues however they want. At this layer of problem, it is almost a system error.

yordis · 2024-01-04T02:05:37Z

+  validateField(schemas.streamName, streamName);
+  validateField(
+    schemas.listPersistentSubscriptionsToStreamOptions.optional(),
+    options
+  );


This pattern feels really C# style, specially if you are gonna be using Zod, instead I would expect to validate a DTO

You raise a good point. I will take that into consideration.

yordis · 2024-01-04T02:10:31Z

I am curious to learn more about who is asking for this and what problems they are running into and why. I would appreciate context since Non-ESDB peeps do not get to have access to Linear to understand more about the linked ticket.

I prefer lean clients that are extremely simple, and definitely would appreciate if EventStoreDB doesn't rely on zod or external packages for something trivial like the validations being done. Especially if they are gonna be performance in a C# guard styles and filtering error list to a single value in the try-catch.

Maybe the solution is better documentation or typespec or even better, introduce Value Objects to enforce correctness.

w1am · 2024-01-04T05:52:05Z

I am curious to learn more about who is asking for this and what problems they are running into and why. I would appreciate context since Non-ESDB peeps do not get to have access to Linear to understand more about the linked ticket.

I prefer lean clients that are extremely simple, and definitely would appreciate if EventStoreDB doesn't rely on zod or external packages for something trivial like the validations being done. Especially if they are gonna be performance in a C# guard styles and filtering error list to a single value in the try-catch.

Maybe the solution is better documentation or typespec or even better, introduce Value Objects to enforce correctness.

Thanks for the input. Some people were experiencing runtime errors, and we were thinking zod would be a great plug-and-play solution to that. However, I definitely need to conduct some benchmarking before applying this change.

yordis · 2024-01-04T05:59:54Z

Thanks for the input. Some people were experiencing runtime errors

Is there any context about their environments and what exactly was the problem? Are they doing type checking of any kind? How many times a given team made the mistake vs. newbies that weren't familiar with the stack? Are they copy+pasting code or do they have a slim wrapper around ESDB client? Could they have their own validation layer in their end if they really want "Safety"?

I would love to understand and have more public information about what drove the decision here before we all pay the cost because of "some" users. I do not want to dismiss their experience, while also make sure a layer such as ESDB client is lean, simple and performance.

w1am · 2024-01-04T06:39:47Z

Is there any context about their environments and what exactly was the problem?

We had 2 issues in the past where someone tried to append to a stream with an invalid UUID, and another case when someone tried to append to a stream with an invalid 'expected revision'.

How many times a given team made the mistake vs. newbies that weren't familiar with the stack

I am not sure about their skill level or their familiarity with the stack. This small PR is more of a developer experience improvement, and I left it in draft for experimenting. Also, if we were to proceed with this solution, users could continue to use the functions as before without many changes.

yordis · 2024-01-04T07:02:02Z

We had 2 issues in the past where someone tried to append to a stream with an invalid UUID, and another case when someone tried to append to a stream with an invalid 'expected revision'.

That is a really low instances. What would the server do in those cases?

I am not sure about their skill level or their familiarity with the stack.

With low instances and without much understand of the situation, I would prefer if ESDB isn't reactionary in such situation and seek to honestly comprehend what was the root caused of it.

users could continue to use the functions as before without many changes.

Well, sort of, I wish it was that easy.

It will introduce a new external dependency, which in the worst case, tools like package manager will try to dedupe the dependency and may cause potential issues in production.

Or taking into consideration Zod accidental/unintentional breaking changes, security problems and so on, the external dependency problems becomes our problems now.

I would expect ESDB to be conservative with the external package dependencies, especially in the JavaScript ecosystem and npm.

yordis · 2024-01-04T07:06:43Z

+  id: z.string().uuid(),
+  contentType: z.literal("application/json"),
+  type: z.string(),
+  data: jsonType,


Suggested change

data: jsonType,

data: jsonType,

I am confused about this type 🤔 What is the motivation on enforcing a given data payload structure from the perspective of the ESDB Client?

I would expect the data to be any JSON payload

It should be any.

yordis · 2024-01-04T07:07:48Z

+]) satisfies z.ZodType<JSONType>;
+
+export const metadataType = z.union([
+  jsonType,


Same here 🤔 I would expect this to be inlined with other SDKs where the metadata is a byte[] of anything

yordis · 2024-01-04T07:13:13Z

+import * as common from "./common";
+import * as stream from "./stream";
+import * as persistentSubscription from "./persistentSubscription";
+import * as projection from "./projection";
+import { z } from "zod";
+
+z.setErrorMap((issue, ctx) => {
+  const fieldName = issue.path.join(".");
+  let message = ctx.defaultError;
+
+  // When the path is empty, it means that the error is on the root.
+  if (issue.path.length !== 0) {
+    switch (issue.code) {
+      case "too_small":
+        message = `the field '${fieldName}' must be greater than or equal to ${issue.minimum}. Received ${ctx.data}`;
+        break;
+      case "too_big":
+        message = `the field '${fieldName}' must be less than or equal to ${issue.maximum}. Received ${ctx.data}`;
+        break;
+    }
+  }
+
+  return { message };
+});
+
+const schemas = {
+  ...common,
+  ...stream,
+  ...persistentSubscription,
+  ...projection,
+};
+
+export default schemas;


Suggested change

import * as common from "./common";

import * as stream from "./stream";

import * as persistentSubscription from "./persistentSubscription";

import * as projection from "./projection";

import { z } from "zod";

z.setErrorMap((issue, ctx) => {

const fieldName = issue.path.join(".");

let message = ctx.defaultError;

// When the path is empty, it means that the error is on the root.

if (issue.path.length !== 0) {

switch (issue.code) {

case "too_small":

message = `the field '${fieldName}' must be greater than or equal to ${issue.minimum}. Received ${ctx.data}`;

break;

case "too_big":

message = `the field '${fieldName}' must be less than or equal to ${issue.maximum}. Received ${ctx.data}`;

break;

}

}

return { message };

});

const schemas = {

...common,

...stream,

...persistentSubscription,

...projection,

};

export default schemas;

export * from "./common";

export * from "./stream";

export * from "./persistentSubscription";

export * from "./projection";

import { z } from "zod";

z.setErrorMap((issue, ctx) => {

const fieldName = issue.path.join(".");

let message = ctx.defaultError;

// When the path is empty, it means that the error is on the root.

if (issue.path.length !== 0) {

switch (issue.code) {

case "too_small":

message = `the field '${fieldName}' must be greater than or equal to ${issue.minimum}. Received ${ctx.data}`;

break;

case "too_big":

message = `the field '${fieldName}' must be less than or equal to ${issue.maximum}. Received ${ctx.data}`;

break;

}

}

return { message };

});

That style of export is an anti-pattern, you can do export * from and if people want everything under a given object as a module scope they can do import * as schemas from

yordis · 2024-01-04T07:14:37Z

+z.setErrorMap((issue, ctx) => {
+  const fieldName = issue.path.join(".");
+  let message = ctx.defaultError;
+
+  // When the path is empty, it means that the error is on the root.
+  if (issue.path.length !== 0) {
+    switch (issue.code) {
+      case "too_small":
+        message = `the field '${fieldName}' must be greater than or equal to ${issue.minimum}. Received ${ctx.data}`;
+        break;
+      case "too_big":
+        message = `the field '${fieldName}' must be less than or equal to ${issue.maximum}. Received ${ctx.data}`;
+        break;
+    }
+  }
+
+  return { message };
+});


Suggested change

z.setErrorMap((issue, ctx) => {

const fieldName = issue.path.join(".");

let message = ctx.defaultError;

// When the path is empty, it means that the error is on the root.

if (issue.path.length !== 0) {

switch (issue.code) {

case "too_small":

message = `the field '${fieldName}' must be greater than or equal to ${issue.minimum}. Received ${ctx.data}`;

break;

case "too_big":

message = `the field '${fieldName}' must be less than or equal to ${issue.maximum}. Received ${ctx.data}`;

break;

}

}

return { message };

});

z.setErrorMap((issue, ctx) => {

const fieldName = issue.path.join(".");

let message = ctx.defaultError;

// When the path is empty, it means that the error is on the root.

if (issue.path.length !== 0) {

switch (issue.code) {

case "too_small":

message = `the field '${fieldName}' must be greater than or equal to ${issue.minimum}. Received ${ctx.data}`;

break;

case "too_big":

message = `the field '${fieldName}' must be less than or equal to ${issue.maximum}. Received ${ctx.data}`;

break;

}

}

return { message };

});

Would this code change Zod as a singleton? If so, how will this work with project using Zod for other things and potentially introducing unexpected behavior from ESDB client due to the singleton issue?

I could encapsulate the error mapping in a separate function.

yordis · 2024-01-04T07:23:27Z

Maybe this SDK could follow the lead of other strong type languages and introduce classes/JS objects/Value Objects where you get to validate the integrity of the data in the constructor instead of relying on types from TypeScript.

yordis · 2024-01-04T18:11:53Z

@w1am sorry I comment again, just that yesterday while I was trying to fall sleep, I remember another way.

Instead of using class and new keyword, you could make factory functions, for example, makeReadStreamOptions({}), where people opt-in to leverage those functions if they want to enforce correctness in the client side.

This way, the type system could stay the same and does not have to rely on "classes" but just types.

w1am · 2024-01-05T08:38:29Z

you could make factory functions, for example, makeReadStreamOptions({}), where people opt-in to leverage those functions if they want to enforce correctness in the client side.

Yes, that's a possibility. I'll keep that in mind when making the decision.

Upgrade dependencies

c37bc29

- Update node version to 16 in build and lint workflow - Ensure deletion result BigInt values are type-checked

w1am mentioned this pull request Nov 15, 2023

appendToStream with expectedRevision -1 causes cryptic error #348

Open

w1am force-pushed the w1am/dev-197-add-runtime-validation branch from 192404b to ad8c051 Compare November 17, 2023 15:32

w1am marked this pull request as draft November 17, 2023 16:46

w1am force-pushed the w1am/dev-197-add-runtime-validation branch 3 times, most recently from 0688727 to 2d92e62 Compare November 20, 2023 09:55

Add runtime validation

2342396

- Upgrade typescript to support satisfies operator

w1am force-pushed the w1am/dev-197-add-runtime-validation branch from 2d92e62 to 2342396 Compare November 20, 2023 13:23

w1am added 6 commits November 21, 2023 12:50

Throw InvalidArgumentError for runtime validation errors

af09b66

Try with updated esdb versions

c2822cf

Increase timeout in subscribeToAll test

fcd7ea4

Test mid stream disconnect with latest grpc

705f4de

Rollback grpc to 1.9.9

fd5682e

Try with 1.9.7

a3f0ea2

yordis reviewed Jan 4, 2024

View reviewed changes

w1am changed the title ~~Add Runtime Validation~~ Add Runtime Validation Spike Apr 30, 2024

w1am closed this Apr 30, 2024

w1am deleted the w1am/dev-197-add-runtime-validation branch April 30, 2024 08:39

		// we don't want to spam the user with all the errors. Take the first one.
		const message = error.issues[0].message;

Conversation

w1am commented Nov 15, 2023

Uh oh!

linear Bot commented Nov 15, 2023

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!

yordis commented Jan 4, 2024

Uh oh!

w1am commented Jan 4, 2024

Uh oh!

yordis commented Jan 4, 2024

Uh oh!

w1am commented Jan 4, 2024

Uh oh!

yordis commented Jan 4, 2024

Uh oh!

yordis Jan 4, 2024 • 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!

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!

yordis commented Jan 4, 2024

Uh oh!

yordis commented Jan 4, 2024

Uh oh!

w1am commented Jan 5, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yordis Jan 4, 2024 •

edited

Loading