safeql: Wrong type on json

Describe the bug A clear and concise description of what the bug is. SafeQL doesn’t accept types for json, other than any. Using the json_agg aggregate function makes SafeQL think that the returning column must be any type and throws an error otherwise. The query below throws an incorrect type annotation if any other type is used for the returningJson other than any

type JsonAgg = {
  a: string;
  b: string;
  c: string;
};

type SafeqlRecordWithJson = {
  id: number;
  firstName: string | null;
  lastName: string | null;
  returningJson: JsonAgg;
};

export async function getAllTestSafeqlRecordWithJson() {
  return await sql<SafeqlRecordWithJson[]>`
      SELECT
        *,
      (
        SELECT json_agg(ingredients)::json
        FROM (
        SELECT
          *
        FROM
          try_safe_ql
        )ingredients
      ) AS returning_json
      FROM
        test_safeql;
          `;
}

To Reproduce Steps to reproduce the behavior:

Setup SafeQL
Use the code above in .ts file

Expected behavior A clear and concise description of what you expected to happen. I expected that using these kinds of aggregate functions shouldn’t cause an error, when i am using the type the query should return or that SafeQL would suggest types other than any

Screenshots If applicable, add screenshots to help explain your problem.

Screenshot 2022-11-16 at 08 19 08

Screenshot 2022-11-16 at 08 18 01

Screenshot 2022-11-16 at 08 22 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.

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 22

Most upvoted comments

For what it’s worth, I wrote a parser with Peggy. While it’s not a standard, the same syntax is also used by PostGraphile. (Note: that’s tagging in general, not the @type tag specifically).

I think tagging could become a semi-standard if more libraries adopt it so I would be very happy to contribute in that regard. As for types, it’s a bit more difficult because even if we agree that this is Postgres-only, other people might want types for other languages where the syntax would inevitably have to be different.

kristiandupont on Nov 30, 2023

Thanks for the feedback! you can change that using overrides.types:

{
  "connections": {
    // ...
    "overrides": {
      "types": {
        "json": "JsonAgg"
      }
    }
  }
}

Newbie012 on Nov 17, 2022

It seems like pg comments aren’t parsed at the moment.

About your suggestion - While I understand how it somewhat solves your issue, It doesn’t feel like a future-proof code. Queries are volatile, and comments may get disconnected from the logic. I think gradually adding support for json type inference would be more promising.

Newbie012 on Dec 3, 2023

There are some shortcomings for sure, but not necessarily intentional. I am happy to expand on it, I’ve only been using it for personal stuff so far. The only criterium I have is that tags should be able to live side by side with a “regular” comment.

kristiandupont on Nov 30, 2023

Thanks for sharing that @kristiandupont.

It seems like the parser is having hard time catching more complex terms:

import { parse } from "tagged-comment-parser";

const result = parse("@type:number @literal:'a'|'b' @object:{foo:number}");

But when I think about it, it doesn’t have to. It could return a simple string and then SafeQL could treat it as a “pg” type, which could be transformed to any TypeScript type via overrides.types:

COMMENT ON COLUMN "user".address IS '@type:geo ...'

{
  "connections": {
    "overrides": {
      "types": {
        "geo": "Geo"
      }
    }
  }
}

Since SafeQL should be used only with TypeScript & PostgreSQL, I’m less worried about conflicts with other languages. My issue is there can be potentially too much clutter when multiple clients access a single database.

Newbie012 on Nov 30, 2023

I understand. My only concern here is that there’s no “standard” of defining types in the database comments, which may lead to:

One tool could use the syntax ... IS '@type:string while a different tool would use ... IS '@type(string)'.
One database, multiple server packages - each one can define the type differently.

I could allow the option to find the type in the comments by regex while using @type(...) as the default.

Newbie012 on Nov 30, 2023