safeql icon indicating copy to clipboard operation
safeql copied to clipboard

Boolean created by comparing two not null timestamps is incorrectly nullable.

Open kfajdsl opened this issue 1 year ago • 8 comments

Describe the bug When comparing two not null timestamps, the result will never be null, but SafeQL interprets it at nullable.

To Reproduce Steps to reproduce the behavior: Create a table with a NOT NULL timestampz column:

CREATE TABLE token (
  id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
  expires_at TIMESTAMPZ NOT NULL DEFAULT NOW()
);

Query that table, checking if the timestamp is in the past:

  const tokenExpired = (await sql<{ expired: boolean; }[]>`
    SELECT expires_at < NOW() AS expired
    FROM token
    WHERE id = ${tokenId}
  `)[0]?.expired;

SafeQL now reports an error,

Query has incorrect type annotation.
	Expected: { expired: boolean; }
	Actual: { expired: boolean | null; }[]

Expected behavior SafeQL doesn't report an error.

Desktop (please complete the following information):

  • OS: macOS
  • PostgreSQL version: 16.1

kfajdsl avatar Jan 14 '24 20:01 kfajdsl

I also get this problem with a timestampz that is created as the result of adding an interval to a timestampz.

kfajdsl avatar Jan 14 '24 21:01 kfajdsl

Good catch. As a workaround for now, you could wrap the condition with coalesce. I will push a new version soon that should fix it.

Newbie012 avatar Jan 14 '24 23:01 Newbie012

I can confirm that this happens, also for IS NOT NULL expressions with LEFT JOINed tables:

// 💥 Query has incorrect type annotation.
//	Expected: { ... is_mammal: boolean; }
//	Actual: { ... is_mammal: boolean | null; }[]
await sql<{ id: number, first_name: string, is_mammal: boolean }[]>`
  SELECT
    animals.id,
    animals.first_name,
    mammals.id IS NOT NULL AS is_mammal
  FROM
    animals
    LEFT JOIN mammals ON animals.id = mammals.animal_id
`;

karlhorky avatar Sep 17 '24 16:09 karlhorky

Workaround

The coalesce workaround mentioned above seems to work, although needs also extra ::bool casting on the coalesce():

 await sql<{ id: number, first_name: string, is_mammal: boolean }[]>`
   SELECT
     animals.id,
     animals.first_name,
-    mammals.id IS NOT NULL AS is_mammal
+    -- coalesce() workaround for SafeQL boolean
+    -- https://github.com/ts-safeql/safeql/issues/199#issuecomment-2356407115
+    coalesce(mammals.id IS NOT NULL)::bool AS is_mammal
   FROM
     animals
     LEFT JOIN mammals ON animals.id = mammals.animal_id
 `;

However, the coalesce() workaround fails with a subquery in a FROM clause:

// 💥 Query has incorrect type annotation.
//	Expected: { ... is_mammal: boolean; }
//	Actual: { ... is_mammal: boolean | null; }[]
await sql<{ id: number, first_name: string, is_mammal: boolean }[]>`
  SELECT
    derived_table.*
  FROM
    (
      SELECT
        animals.id,
        animals.first_name,
        coalesce(mammals.id IS NOT NULL)::bool AS is_mammal
      FROM
        animals
        LEFT JOIN mammals ON animals.id = mammals.animal_id
    ) AS derived_table
`;

karlhorky avatar Sep 17 '24 16:09 karlhorky

@Newbie012 would this change be a small special-casing change, similar to this commit here?

  • https://github.com/ts-safeql/safeql/commit/07c746a380e43efd84411a0220c8bdedca1a834b

cc @timvandam in case you'd like to try out a small PR

karlhorky avatar Sep 17 '24 16:09 karlhorky

I can confirm that this happens, also for IS NOT NULL expressions with LEFT JOINed tables:

Ok, it seems this is a very different case when it comes to the code.

I've tried out my hand at a first PR, not sure if this makes sense like this:

  • https://github.com/ts-safeql/safeql/pull/265

karlhorky avatar Sep 18 '24 20:09 karlhorky

I'll try to open another PR today to fix expressions with comparison operators (<, >, =, more?) being recognized as nullable.

karlhorky avatar Sep 22 '24 09:09 karlhorky

@karlhorky The above-mentioned example is a bit more complex since you can't tell whether it's nullable or not just by the AST. If the column reference is nullable, then col > now() could be null. This missing context needs to be passed down to the nullability checker.

Newbie012 avatar Sep 22 '24 10:09 Newbie012