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

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

Open
kfajdsl opened this issue Jan 14, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@kfajdsl
Copy link

kfajdsl commented Jan 14, 2024

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
Copy link
Author

kfajdsl commented Jan 14, 2024

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

@Newbie012 Newbie012 added the bug Something isn't working label Jan 14, 2024
@Newbie012
Copy link
Collaborator

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.

@karlhorky
Copy link
Collaborator

karlhorky commented Sep 17, 2024

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
Copy link
Collaborator

karlhorky commented Sep 17, 2024

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
Copy link
Collaborator

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

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

@karlhorky
Copy link
Collaborator

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:

@karlhorky
Copy link
Collaborator

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

@Newbie012
Copy link
Collaborator

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants