safeql icon indicating copy to clipboard operation
safeql copied to clipboard

Unexpected error on sql helper function

Open Eprince-hub opened this issue 3 years ago • 17 comments

Describe the bug SafeQL throws an error when using the sql() helper in Postgres. This is a feature that is supported in postgres.js but it seems like SafeQL doesn't allow using this helper in writing queries. The queries below would throw this error Invalid Query: the type "Helper<number[], []>" is not supported".

export async function query(a: number[]) {
  return await sql`
    SELECT
      *
    FROM
      try_safe_ql
    WHERE
      id IN ${sql(a)}
  `;
}

To Reproduce Steps to reproduce the behavior:

  1. Setup SafeQL
  2. Use this code in .ts file
export async function query(a: number[]) {
  return await sql`
    SELECT
      *
    FROM
      try_safe_ql
    WHERE
      id IN ${sql(a)}
  `;
}

Expected behavior Usage of sql() helper should not throw errors

Screenshots Screenshot 2022-11-10 at 12 27 34

Screenshot 2022-11-10 at 12 28 16

Desktop (please complete the following information):

  • OS: MAC OS
  • PostgreSQL version 14
  • Version [e.g. 22]

Additional context Add any other context about the problem here.

Eprince-hub avatar Nov 10 '22 15:11 Eprince-hub

Thanks for the detailed report.

This issue is similar to the other issue you've opened (https://github.com/ts-safeql/safeql/issues/99) so I'll answer for both of them:

SafeQL tries to get the type for each "expression" in the SQL tag. It doesn't know that sql should be dealt with differently.

for example:

function sum(a, b) {
  return a + b;
}

const q = (x, y) => sql`select from tbl where col = ${sum(x, y)}`;

SafeQL takes the expression sum(x, y), get the return type (which is number) and converts it to pg type (int):

select from tbl where col = $1::int

SafeQL should start supporting query builders, but it shouldn't be specific to Postgres.js. Libraries such as Slonik should also be taken into account.

Newbie012 avatar Nov 11 '22 00:11 Newbie012

I think that the main advantage of supporting this is to be able to DRY up some code around column selection.

const cols = ['id', 'name', ...]; // e.g. users table
sql `select ${sql(cols)} from users`;

This is mentioned in the docs for Postgres.js, dynamic columns

esdee avatar Nov 30 '22 01:11 esdee

Correct. Ideally, I'm not against using sql fragments in code.

I think I have no other option than implementing library-specific behavior { sqlFragmentSyntax: "postgres" | "slonik" | "..." }` to prevent incompatibilities between libraries.

Newbie012 avatar Nov 30 '22 09:11 Newbie012

I tweeted about my progress on this matter. Hopefully I'll push a V1 for this.

Newbie012 avatar Dec 29 '22 20:12 Newbie012

@Newbie012 Thanks a lot for creating such a useful plugin. I have found it to be very helpful in my work, and I appreciate the effort you put into developing and maintaining it. I see that you started the implementation of the SQL fragment support here https://github.com/ts-safeql/safeql/pull/113 I just wanted to know how it is progressing and when we are expected to have this feature working in SafeQL

Thanks 👍

Eprince-hub avatar Mar 17 '23 11:03 Eprince-hub

Hi @Eprince-hub

Thanks for the support 🙂 I think I need to rewrite my implementation for SQL fragments so it can be more robust and easier to maintain. SafeQL doesn't have a community so it's a bit hard for me to prioritize features. I hope I'll continue working on fragment support in the next week. Although, fragments have their own limitations, so be advised.

Newbie012 avatar Mar 17 '23 12:03 Newbie012

Hi @Newbie012, I hope you're doing well. I wanted to show you an "upsert" query (INSERT...ON CONFLICT UPDATE) like the example I have below. I know you are working on this issue, but I just want to show you more examples of the SQL fragments usage

export async function upsertQuery(inputs: Input[]) {
  return await sql<Input[]>`
    INSERT INTO try_safe_ql
      ${sql(inputs)}
    ON CONFLICT
      (id)
    DO UPDATE SET
      ${Object.keys(inputs[0]!).map(
        (field, count) =>
          sql`${count > 0 ? sql`,` : sql``} ${sql(field)} = excluded.${sql(
            field,
          )}`,
      )}
    RETURNING
      *
  `;
}

Eprince-hub avatar Jun 29 '23 09:06 Eprince-hub

Workaround 1

You can disable the ESLint check for SafeQL for the lines showing this error.

export async function query(a: number[]) {
  return await sql`
    SELECT
      *
    FROM
      try_safe_ql
    WHERE
      id IN ${
        // eslint-disable-next-line @ts-safeql/check-sql -- Postgres.js sql() helper function currently not supported by SafeQL https://github.com/ts-safeql/safeql/issues/101#issuecomment-1623687201
        sql(a)
      }
  `;
}

Eprince-hub avatar Jul 06 '23 13:07 Eprince-hub

Workaround 2

If disabling the ESLint checking is not what you want to do, then you can do something like this.

// Workaround for the Postgres.js sql() helper function that is causing a SafeQL linting error
// https://github.com/ts-safeql/safeql/issues/101#issuecomment-1623699171
export async function query(a: number[]) {
  return await sql`
    SELECT
      *
    FROM
      try_safe_ql
    WHERE
      id IN (SELECT unnest(${a}::int[]));
 `;
}

Eprince-hub avatar Jul 06 '23 13:07 Eprince-hub

Unfortunately, the workaround 1 suppress the error for the whole sql statement and not only the fragment when using a conditional:

export function query(id: number) {
  return sql`
    SELECT *
    FROM try_safe_ql
    WHERE
      test = 'test'
      ${id ? sql`AND id = ${id}` : sql``}
  `;
}

louneskmt avatar Aug 08 '23 16:08 louneskmt

@Eprince-hub another workaround would be to simply use = ANY instead of IN:

export async function query(a: number[]) {
  return await sql`
    SELECT
      *
    FROM
      try_safe_ql
    WHERE
      id = ANY(${a})
  `;
}

@louneskmt While SafeQL won't support conditional queries due to performance reasons, You could write your query with CASE WHEN expression instead (which personally I think it looks nicer):

export function query(id: number) {
  return sql`
    SELECT *
    FROM try_safe_ql
    WHERE
      test = 'test'
      AND CASE WHEN ${id !== ...} THEN id = ${id} END
  `;
}

Newbie012 avatar Aug 08 '23 22:08 Newbie012

Nice, I'll try that. Thanks 👍

louneskmt avatar Aug 08 '23 22:08 louneskmt

Joining the party late. Does this currently work with Slonik helpers?

gajus avatar Sep 13 '23 23:09 gajus

I have a private branch that allows 3rd party providers (such as DrizzleORM, kysely, postgres-js and slonik) to "translate" their expressions to sql syntax. I started with postgres-js, but it's way too dynamic.

I need to rewrite the branch so it could be easily maintained by others, but that will require some time that I don't have these days. I will get back to it eventually.

Newbie012 avatar Sep 13 '23 23:09 Newbie012

Looking forward to contributing Slonik adapter.

gajus avatar Sep 13 '23 23:09 gajus

It's really important feacher. I hope that you will give an update soon. Thank you.

mustakimkr avatar Oct 03 '23 11:10 mustakimkr