Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zod schema is not returning proper types #219

Closed
krishna-404 opened this issue Dec 1, 2023 · 19 comments
Closed

zod schema is not returning proper types #219

krishna-404 opened this issue Dec 1, 2023 · 19 comments
Labels
enhancement New feature or request

Comments

@krishna-404
Copy link
Contributor

I totally love what you are doing here. Have been looking for postgres specific ORM & this is kind of ticking all the boxes for me.

Sadly it breaks when working with trpc using zodValidators. Here is the screen-shot of the error.
image

Sadly this is a deal breaker for me. Please let me know if I am doing something wrong, here is the CreateBaseTable function.

export const BaseTable = createBaseTable({
  // Set `snakeCase` to `true` if columns in your database are in snake_case.
  snakeCase: true,

  // Customize column types for all tables.
  columnTypes: (t) => ({
    ...t,
    // Set min and max validations for all text columns,
    // it is only checked when validating with Zod schemas derived from the table.
    text: (min = 0, max = Infinity) => t.text(min, max),
    // parse timestamps to numbers
    timestamp: () => t.timestamp().asNumber(),
    date: () => t.date().asNumber(),
  }),
  schemaProvider: zodSchemaProvider,
});

I love how Drizzle creates different validators for Insert & Select. Having such a functionality here would be so great. I have no experience with transformers but would be happy to contribute if somebody could guide me.

Unrelated:
Even with snakeCase true, the timestamps fields we get are updatedAt & createdAt. Is that the expected behavior?

And yes, Thanks for building this :)

@romeerez
Copy link
Owner

romeerez commented Dec 2, 2023

Thank you for the supportive words!

It was a bug after recent changes, now it's fixed and you can make it work by updating orchid-orm related libraries.

Supporting various schemas for different cases is a good idea, but it will take some time, and I'll publish a breaking change for it.

Going to make it so:

// for create:
UserTable.inputSchema()

// for update:
UserTable.inputSchema().partial()

// to validate selected data:
UserTable.outputSchema()

// for filters:
UserTable.querySchema()

Query schema is for types that where({ column: 'value' }) can accept.

Perhaps worth having a separate schema for update where primary keys are excluded.
And a separate schema for primary keys.

This is a large change, will have to rewrite this part completely.

@romeerez
Copy link
Owner

romeerez commented Dec 2, 2023

Even with snakeCase true, the timestamps fields we get are updatedAt & createdAt. Is that the expected behavior?

Yes, it's intended. snakeCase is only for how it is named in db. If you have updatedAt in the code, and updated_at in the db - that's good. If you have snakeCase: true and on both sides it is updatedAt - that's a bug.

@msfunty
Copy link

msfunty commented Dec 4, 2023

Yes, it's intended. snakeCase is only for how it is named in db. If you have updatedAt in the code, and updated_at in the db - that's good. If you have snakeCase: true and on both sides it is updatedAt - that's a bug.

Just double-checked by freshly scaffolding with "create orchid-orm" and set snakeCase: true in baseTable.ts.
In TS as well as in DB it is camelCase - so it's a bug

DB:
image

@romeerez
Copy link
Owner

romeerez commented Dec 5, 2023

If you set snakeCase: true to dbScript.ts it works. If you set it to baseTable.ts it won't change it for migrations, yes it's not right.

@krishna-404
Copy link
Contributor Author

Thanks for the quick & thorough reply 🙌

Any way I can help by contributing, please guide me...

@krishna-404
Copy link
Contributor Author

Thank you for the supportive words!

It was a bug after recent changes, now it's fixed and you can make it work by updating orchid-orm related libraries.

Im still getting the same error. Deleted my node_modules & did a fresh install. Nothing seems to have changed on my end. One key difference I see between Orchid & Drizzle is that

createInsertSchema in Drizzle returns a type z.zodObject<{...}> whereas Orchids Table.schema() returns a type of InstanceToZod<{shape: columnShapeBase}> so either this InstanceToZod is not returning the correct types or tRPC is not able to infer this type?

@krishna-404
Copy link
Contributor Author

If you set snakeCase: true to dbScript.ts it works. If you set it to baseTable.ts it won't change it for migrations, yes it's not right.

Probably that reflects correctly in the database but typescript still returns camelCase
image

@romeerez
Copy link
Owner

romeerez commented Dec 6, 2023

whereas Orchids Table.schema() returns a type of InstanceToZod<{shape: columnShapeBase}>

It is the same error indeed. If you deleted node_modules and updated all versions it should be solved. I'm trying the fresh ORM with the command pnpm create orchid-orm and the zod schema infers correctly - it has column names inside that InstanceToZod<>. Libraries are connected to each other, so when updating make sure you set the last versions to orchid-orm, orchid-orm-schema-to-zodand other related libs if installed. pqb lib can be removed if you have it in package.json.

But, it's only solving a bug that the schema didn't work at all. I'm sorry, the implementation is not correct, and I realized it only with this issue. It derives zod types based on db types: timestamp column will be a ZodDate. It seems to be ok, but it's not, because when validating JSON input you expect it to validate ISO date strings and probably epoch numbers.

How long to make it like in Drizzle (and hopefully better) - 2-3 weeks I guess. I'm currently working on a tool for optimizing TS, to make sure that new changes won't degrade TS performance, and soon it will be ready and I'll look how to rewrite columns code to support multiple schemas.

Any way I can help by contributing, please guide me...

Thank you for your willingness, I appreciate it a lot! The code is overly complex, messy, and chaotic, I try writing comments but don't think it helps much. This particular schema task is highly coupled with a lot of code. If you're seriously set for contributing, let me know if you'd like to try doing smaller things.

Probably that reflects correctly in the database but typescript still returns camelCase

That's intended, you can have snake case in db and camel case in TS. Or camel case for both. But no functions/methods are named in snake_case because it's unconventional.

@krishna-404
Copy link
Contributor Author

Hey @romeerez thanks for the updates. I'll wait for drizzle type handling of zodSchemas. Thanks!

Yes, I am serious about contributing. Just point me to small issues & we can increase complexity over-time. Looking forward :)

PS: I just started a project in Nestjs & love what it has to offer so far. So maybe will work-on nestjs-dto-creator, similar to https://github.com/risen228/nestjs-zod-prisma sometime 🤞.

@romeerez
Copy link
Owner

@krishna-404 I created two tasks here, they are for rake-db, it is simpler than ORM, and I don't have any ORM-related well-formed tasks in mind.

nestjs-dto-creator

But there is no need for dto creator in Orchid ORM?
Once this issue is resolved, you can use zod schemas of a table, and no need to generate any code like it is with Prisma.

@krishna-404
Copy link
Contributor Author

Thanks! on it

@krishna-404
Copy link
Contributor Author

@romeerez see if this makes sense. I'll need access to create branches, commit & open PR. Thanks!

image

@romeerez
Copy link
Owner

You can fork this repo and make a pull request, it's a normal flow for public repos.
On the screenshot, it looks like a log of db reset command.

@romeerez
Copy link
Owner

Published a big update for the column schemas 🚀

Brief sum up:

import { createBaseTable } from "orchid-orm";
// import zod config in such way: 
import { zodSchemaConfig } from "orchid-orm-schema-to-zod";

export const BaseTable = createBaseTable({
  // set up the config:
  schemaConfig: zodSchemaConfig,
});

export class UserTable extends BaseTable {
  readonly table = "user";
  columns = this.setColumns((t) => ({
    id: t.identity().primaryKey(),
    // min and max are enforced by zod:
    username: t.string().unique().min(3).max(30),
    // if wanted, set custom zod transforms and refines
    custom: t
      .string()
      .input((z) => z.transform((val) => val.split("").reverse().join("")))
      .output((z) => z.refine((val) => val === "value"))
  }));
}

// validating input for creating records:
User.inputSchema().parse(data)

// validating data returned by db:
User.outputSchema().parse(data)

// the same as inputSchema, but you can customize type for input, but not for query, to use in `where` filters:
User.querySchema().parse(data)

// partial inputSchema:
User.updateSchema().parse(data)

// schema to validate primary key in object like { id: 123 }
User.pkeySchema().parse(data)

Breaking change for columns parse and encode, now takes zod schema for the first argument:

export class Table extends BaseTable {
  readonly table = 'table';
  columns = this.setColumns((t) => ({
    column: t
      .string()
      .parse(z.number().int(), (input) => parseInt(input))
      .encode(
        z.boolean().or(z.number()).or(z.string()),
        (input: boolean | number | string) => String(input),
      )
  }));
}

@bingtsingw @IlyaSemenov be careful with updating, breaking changes arrived

@bingtsingw
Copy link
Contributor

@romeerez Thanks for the reminder.
Is zodSchemaConfig a necessary option? Or if we don't configure zodSchemaConfig, we can use parse and encode in the old way

@IlyaSemenov
Copy link
Contributor

I don't mind if the change is breaking (although I'd appreciate proper semvers and Orchid 3.0), what I am not yet following if it's hard-coded with zod specifically or if's using some well-thought generic concept of validation?

I have attitude against Zod. Following are my observations:

  1. Zod is not tree shakeable. It puts all its methods in the base class.
  2. Zod is not consistently extensible. One can't add z.customString() alongside z.string().
  3. Despite its popularity, it's poorly maintained. I opened a few pull requests (e.g. Add zod.select, Make EnumValues generic) for which I never got any reaction from the maintainers in almost a year.
  4. It's poorly architected. Basically, the entire Zod source code is kept in a single behemoth 6000-liner src/types.ts.

I switched to Valibot which works similarly to Zod but is fully tree shakeable and can be extended seamlessly (which I actually do with valibotx, my small collection of Valibot extensions).

@romeerez
Copy link
Owner

@bingtsingw @IlyaSemenov it's an optional setting. here are docs for parse and serve - can be done without it. json column with zod takes a callback for schema, without zod you can specify a type via generic. While it's optional, I generally focused on testing code with validation lib rather than without, so a little bigger chance to found a bug.

I have attitude against Zod. Following are my observations:

Sure, it's not perfect, especially I wish your 2nd point was possible. And there are many alternatives each offering something unique.

I'd like to add support for typebox as it's promising to be the most performant one. In general, validation speed doesn't matter, but in some particular cases where validating huge chunks or thousands rps it can matter.

Zod integration is around 800 LOCs + 800 LOCs of tests, I can handle supporting a second integration later, but for more - hopefully, the community will grow in the future and people could add contribute validation libraries they prefer.

@romeerez
Copy link
Owner

@IlyaSemenov is Orchid more-or-less fine for serverless? I mean the bundle size, so you're saying about valibot and bundle size, so you're probably running Orchid on serverless. I had no chance yet to work on improving bundle size so have no idea how is it after proper minification, and leaving that for the future as it may require large changes, there is Drizzle which focuses on serverless and bundle size from the beginning.

@IlyaSemenov
Copy link
Contributor

@romeerez No, I'm not running it serverless at the moment, but I have a project in development which I am planning to deploy to a serverless environment in a few months.

I use zod/valibot for client-side forms validation, that's why I'm in favor of tree shaking.

@romeerez romeerez added the enhancement New feature or request label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants