drizzle-orm icon indicating copy to clipboard operation
drizzle-orm copied to clipboard

[BUG]: isNotNull does not impact type of returned column

Open sp88011 opened this issue 1 year ago • 2 comments

What version of drizzle-orm are you using?

0.33.0

What version of drizzle-kit are you using?

0.24.2

Describe the Bug

Say you have this schema

export const user = pgTable("user", {
   name: varchar("name").notNull(),
 age: integer("age").notNull(),
})

export const pet = pgTable("pet", {
   name: varchar("name").notNull(),
   age: integer("age")
})

And you want to combine two queries like this:

const humanAge=db.select({age:user.age}).from(user)
const animalAge = db.select({age:pet.age}).from(pet).where(isNotNull(pet.age))

const age = union(humanAge,animalAge)

Even though we ensured that age can never be null (isNotNull...), Typescript complains: Type 'number | null' is not assignable to type 'number'

image

Expected behavior

{ age: number }

Environment & setup

No response

sp88011 avatar Sep 11 '24 20:09 sp88011

I know I can cast the type with ...{age: sql<number> ${pet.age} but ideally I don't have to do this.

sp88011 avatar Sep 11 '24 20:09 sp88011

I think this is a duplicate of #2045

acontreras89 avatar Sep 30 '24 10:09 acontreras89

@L-Mario564 I'd be happy to try tackle this if you can point me in the right direction

acontreras89 avatar Oct 19 '24 20:10 acontreras89

@acontreras89 Read the CONTRIBUTING.md file. If you get stuck at any point, you can join the official Drizzle Discord server and ask questions there.

L-Mario564 avatar Oct 20 '24 01:10 L-Mario564

loop me in @acontreras89 if ever you try and tackle this, would want to try and help if i can to get this resolved since i use is not null a lot and have resulted to using sql instead

Vasallius avatar Oct 20 '24 05:10 Vasallius

Another case, where drizzle might infer a return value is not null:

const profile = await db.select({
  userId: Users.id,
  username: Users.username,
  image: Users.image,
})
.from(Users)
.where(eq(Users.username, "mario"))
.get();

// `profile.username: string | null` but should be `profile.username: string`

abegehr avatar Jan 16 '25 06:01 abegehr

loop me in @acontreras89 if ever you try and tackle this, would want to try and help if i can to get this resolved since i use is not null a lot and have resulted to using sql instead

I actually gave up on this because the change wasn't actually simple at all 😅 I opened 2 different PRs to get a grasp of the codebase and the contributing procedures, but they are still waiting for someone to look at them. My idea was to eventually address this if I get comfy enough with the source code.

acontreras89 avatar Jan 16 '25 22:01 acontreras89

I am not sure if I am trying to solve the same issue, but in project I noticed that insert types don't includes optional parameters, the issue was within the tsconfig.json where I had to use strict: true

epsecially with the https://github.com/nitrojs/nitro framework, the auto-generated tsconfig has set to false in that case we can just update nitro config like this

typescript: {
    strict: true,
  },

kleinpetr avatar Feb 04 '25 10:02 kleinpetr

In addition of the infer problem, I also noticed that if I want to use the magic sql for casting a timestampz the result will produce always a string not Date:

.select({
      id: TestTable.id,
      description:TestTable.description,
      scheduledTime: sql<Date>`${TestTable.scheduledTime}`,//Will be always a string in the returned object
    })
    .from(TestTable)
    .where(and(eq(TestTable.userId, userId), isNotNull(TestTable.scheduledTime)));//The isNotNull should not force me to use sql magic

I tried too:

sql<Date>`${TestTable.scheduledTime}::timestamp with time zone`

sql<Date>`cast(${TestTable.scheduledTime} as timestamp with time zone)`

Any clue why?

SennaSanzo avatar Feb 13 '25 08:02 SennaSanzo

@SennaSanzo this has nothing to do with this issue.


That said, the problem with your code is that you need to map the value. This is done automatically for some types of columns (timestamps, dates) when you don't provide the { mode: 'string' } option explicitly.

However, since you are using an arbitrary expression, drizzle cannot know if the result of that expression needs mapping or what mapping it needs. You need to be explicit about it:

scheduledTime: sql`${TestTable.scheduledTime}`.mapWith(value => new Date(value))
// or alternatively
scheduledTime: sql`${TestTable.scheduledTime}`.mapWith(TestTable.scheduledTime)

:warning: you need to manually pass the value to the date constructor, you cannot do .mapWith(Date) because calling Date without the new keyword will generate a string, not a date. Also, it will ignore the value completely.

Note that you can pass a column to the mapWith method, in which case the mapper for that specific column will be used to map the result of the expression. This comes very handy when using things like min or max:

date: sql`min(${table.someDateColumn})`.mapWith(table.someDateColumn)
// map with the same column from which we will be retrieving the value ↑

acontreras89 avatar Feb 13 '25 09:02 acontreras89

@acontreras89, my issue with date mapping wasn’t indeed related to that problem; however, the OP’s issue led me to a situation where I wanted to cast my returned value because isNotNull wasn’t inferring the type correctly and returned a type of Date | null.

Thank you so much for your clear explanations—I’ll give it a try this afternoon. To be honest, I’m just starting to use Drizzle and I have a ton to learn quickly, which explains my limited knowledge in this area.

One of my intuitions was that the generic part of the sql<T> syntax would be sufficient to cast to the correct type when retrieving the value (since it works with numbers, for example). But I suppose I don’t know enough about the underlying levels of abstraction. Anyway, thank you so much.

SennaSanzo avatar Feb 13 '25 10:02 SennaSanzo

Hey everyone!

I've created this message to send in a batch to all opened issues we have, just because there are a lot of them and I want to update all of you with our current work, why issues are not responded to, and the amount of work that has been done by our team over ~8 months.

I saw a lot of issues with suggestions on how to fix something while we were not responding – so thanks everyone. Also, thanks to everyone patiently waiting for a response from us and continuing to use Drizzle!

We currently have 4 major branches with a lot of work done. Each branch was handled by different devs and teams to make sure we could make all the changes in parallel.


First branch is drizzle-kit rewrite

All of the work can be found on the alternation-engine branch. Here is a PR with the work done: https://github.com/drizzle-team/drizzle-orm/pull/4439

As you can see, it has 167k added lines of code and 67k removed, which means we've completely rewritten the drizzle-kit alternation engine, the way we handle diffs for each dialect, together with expanding our test suite from 600 tests to ~9k test units for all different types of actions you can do with kit. More importantly, we changed the migration folder structure and made commutative migrations, so you won't face complex conflicts on migrations when working in a team.

What's left here:

  • We are finishing handling defaults for Postgres, the last being geometry (yes, we fixed the srid issue here as well).
  • We are finishing commutative migrations for all dialects.
  • We are finishing up the command, so the migration flow will be as simple as drizzle-kit up for you.

Where it brings us:

  • We are getting drizzle-kit into a new good shape where we can call it [email protected]!

Timeline:

  • We need ~2 weeks to finish all of the above and send this branch to beta for testing.

Second big branch is a complex one with several HUGE updates

  • Bringing Relational Queries v2 finally live. We've done a lot of work here to actually make it faster than RQBv1 and much better from a DX point of view. But in implementing it, we had to make another big rewrite, so we completely rewrote the drizzle-orm type system, which made it much simpler and improved type performance by ~21.4x:
(types instantiations for 3300 lines production drizzle schema + 990 lines relations)

TS v5.8.3: 728.8k -> 34.1k
TS v5.9.2: 553.7k -> 25.4k

You can read more about it here.

What's left here:

Where it brings us:

  • We are getting drizzle-orm into a new good shape where we can call it [email protected]!

Breaking changes:

  • We will have them, but we will have open channels for everyone building on top of drizzle types, so we can guide you through all the changes.

Third branch is adding support for CockroachDB and MSSQL dialects

Support for them is already in the alternation-engine branch and will be available together with the drizzle-kit rewrite.

Summary

All of the work we are doing is crucial and should be done sooner rather than later. We've received a lot of feedback and worked really hard to find the best strategies and decisions for API, DX, architecture, etc., so we can confidently mark it as v1 and be sure we can improve it and remain flexible for all the features you are asking for, while becoming even better for everyone building on top of the drizzle API as well.

We didn't want to stay with some legacy decisions and solutions we had, and instead wanted to shape Drizzle in a way that will be best looking ahead to 2025–2026 trends (v1 will get proper effect support, etc.).

We believe that all of the effort we've put in will boost Drizzle and benefit everyone using it.

Thanks everyone, as we said, we are here to stay for a long time to build a great tool together!

Timelines

We are hoping to get v1 for drizzle in beta this fall and same timeline for latest. Right after that we can go through all of the issues and PRs and resond everyone. v1 for drizzle should close ~70% of all the bug tickets we have, so on beta release we will start marking them as closed!

AndriiSherman avatar Aug 30 '25 18:08 AndriiSherman