Incorrect type when using jsonb_agg

[timvandam]

Describe the bug
A clear and concise description of what the bug is.

When using jsonb_agg safeql is suggesting an incorrect TS type.

To Reproduce
Steps to reproduce the behavior:

DB setup:

await sql`
    CREATE TABLE users (
        id SERIAL PRIMARY KEY,
        public_id VARCHAR(255) UNIQUE NOT NULL,
        username VARCHAR(255) UNIQUE NOT NULL,
        created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
        updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
    );`;

await sql`
    CREATE TABLE user_authentication_methods (
        id SERIAL PRIMARY KEY,
        user_id INTEGER NOT NULL REFERENCES users (id),
        name VARCHAR(255) NOT NULL,
        -- Data depends on the authentication method
        data JSONB NOT NULL,
        created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
        updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
    );`;

await sql`
    CREATE UNIQUE INDEX user_authentication_methods_user_id_name_index
    ON user_authentication_methods (user_id, name);`;

await sql`
    CREATE FUNCTION updated_at() RETURNS TRIGGER AS $$
    BEGIN
        NEW.updated_at = NOW();
        RETURN NEW;
    END;
    $$ LANGUAGE plpgsql;`;

await sql`
    CREATE TRIGGER users_updated_at_trigger
    BEFORE UPDATE ON users
    FOR EACH ROW
    EXECUTE PROCEDURE updated_at();`;

await sql`
    CREATE TRIGGER user_authentication_methods_updated_at_trigger
    BEFORE UPDATE ON user_authentication_methods
    FOR EACH ROW
    EXECUTE PROCEDURE updated_at();`;

Example of wrong type:

await sql<{
    id: number;
    public_id: string;
    username: string;
    created_at: Date;
    updated_at: Date;
    jsonb_agg:
        | {
              id: number | null;
              user_id: number | null;
              name: string | null;
              data: unknown | null;
              created_at: Date | null;
              updated_at: Date | null;
          }[]
        | null;
}>`
    SELECT users.*, jsonb_agg(user_authentication_methods)
    FROM users
    LEFT JOIN user_authentication_methods ON users.id = user_authentication_methods.user_id
    GROUP BY users.id
`;

// Running this with some test data gives:
// 1,test,test,2024-06-10 12:31:20.240877 +00:00,2024-06-10 12:31:20.240877 +00:00,"[{""id"": 1, ""data"": {""some_data"": 123}, ""name"": ""test1"", ""user_id"": 1, ""created_at"": ""2024-06-10T12:31:38.655757+00:00"", ""updated_at"": ""2024-06-10T12:31:38.655757+00:00""}, {""id"": 2, ""data"": {""some_other_data"": 321}, ""name"": ""test2"", ""user_id"": 1, ""created_at"": ""2024-06-10T12:31:47.674517+00:00"", ""updated_at"": ""2024-06-10T12:31:51.324642+00:00""}]"

// However, when the LEFT JOIN yields no user_authentication_methods rows we get a return value that does not match the TS type for json_agg:
await sql`
    SELECT users.*, jsonb_agg(user_authentication_methods)
    FROM users
    LEFT JOIN user_authentication_methods ON FALSE
    GROUP BY users.id;
`;

// Results in:
// 1,test,test,2024-06-10 12:31:20.240877 +00:00,2024-06-10 12:31:20.240877 +00:00,[null]

// Clearly [null] does not match json_agg's type

Expected behavior
A clear and concise description of what you expected to happen.

I would expect json_agg to have the type

({
    id: number;
    user_id: number;
    name: string;
    data: unknown; // usually any, but I override it in safeql.config.ts
    created_at: Date;
    updated_at: Date;
} | null)[]

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

Desktop (please complete the following information):

• OS: [e.g. iOS] macOS
• PostgreSQL version [e.g. 13, 14] 15.6
• Version [e.g. 22] @ts-safeql/eslint-plugin@3.2.1 @ts-safeql/sql-tag@0.2.0

Additional context
Add any other context about the problem here.

Another bonus issue I found while playing around:

await sql<{
    id: number;
    public_id: string;
    username: string;
    created_at: Date;
    updated_at: Date;
    jsonb_agg: { method: number | null }[] | null;
}>`
    SELECT users.*, jsonb_agg(json_build_object('method', user_authentication_methods))
    FROM users
    LEFT JOIN user_authentication_methods ON users.id = user_authentication_methods.user_id
    GROUP BY users.id;
`;

method is definitely not a number

[Newbie012]

Thanks for the detailed reproduction. I'll look into it.

[timvandam]

Thanks for the detailed reproduction. I'll look into it.

Nice

I forgot to include the data in the issue, here are inserts for the data I used to test this:

insert into users (id, public_id, username, created_at, updated_at)
values  (1, 'test', 'test', '2024-06-10 12:31:20.240877 +00:00', '2024-06-10 12:31:20.240877 +00:00');

insert into user_authentication_methods (id, user_id, name, data, created_at, updated_at)
values  (1, 1, 'test1', '{"some_data": 123}', '2024-06-10 12:31:38.655757 +00:00', '2024-06-10 12:31:38.655757 +00:00'),
        (2, 1, 'test2', '{"some_other_data": 321}', '2024-06-10 12:31:47.674517 +00:00', '2024-06-10 12:31:51.324642 +00:00');

[timvandam]

I've been doing some debugging to see if I can fix this. I've ended up here, and realized that this inner type should be made nullable if the argument given to json_agg may be null (such as in the case of a LEFT JOIN in my case). I am checking this with isDescribedColumnNullable, however this is not correct as I wan't to know whether the argument is nullable, while this applies to a DescribedColumn. Is there any way of doing this? E.g. I want to know whether x may be null in the case of jsonb_agg(x)

I see that context.resolver.sources contains some basic information, but it does not tell me whether tables/columns used within the query can be nullable. I'm not sure whether this is even possible.

Also, detecting whether the value in the final selection can be null (e.g. in the case above where I use GROUP BY) would require a more sophisticated way of constructing context.nonNullableColumns. For instance in my case the GROUP BY pretty much filters out any rows where jsonb_agg would have returned null, so this would need to be detected. Also seems very challenging.

I think it would make most sense to assume that jsonb_agg can contain null values whenever its argument is not the table in the FROM clause. Not 100% accurate but close enough I think

[Newbie012]

This is somewhat related to #210

I have unstaged changes that attempts to solve this issue by introducing a new "group" node which can be "nullable" and contain "children". This is quite a fundamental change from the way I initially wrote the ast-describe part. Another required fundamental change would be to merge the non nullable columns logic with the descriptor, since sometimes it needs additional context. Once I have enough time keep working on it, I'll send some updates. In the meantime, feel free to explore this path.

[timvandam]

That's a nice approach. I think this wouldn't fully solve the GROUP BY causing any potentially null results from being omitted, but it is probably not even needed since you can just COALESCE to get rid of that null type

[karlhorky]

I just upgraded to @ts-safeql/eslint-plugin@4.0.3, which includes PR #412, and these 2 subquery columns (learning_outcomes and videos) now return proper objects instead of unknown 🎉

SELECT

  (
    SELECT
      jsonb_agg(
        jsonb_build_object(
          'slug',
          learning_outcomes.slug
        )
      )
    FROM
      projects_learning_outcomes
      INNER JOIN learning_outcomes ON projects_learning_outcomes.learning_outcome_id = learning_outcomes.id
    WHERE
      projects_learning_outcomes.project_id = projects.id
  ) AS learning_outcomes,

  (
    SELECT
      coalesce(
        jsonb_agg(
          jsonb_build_object(
            'videoSpecTitle',
            video_specs.title,
            'youtubeId',
            videos.youtube_id
          )
        ),
        '[]'::jsonb
      )
    FROM
      lectures_video_specs
      INNER JOIN video_specs ON (
        lectures_video_specs.video_spec_id = video_specs.id
        AND video_specs.curriculum_version_id = cohorts.curriculum_version_id
      )
      INNER JOIN videos ON video_specs.id = videos.video_spec_id
    WHERE
      lectures_video_specs.lecture_id = lectures.id
  ) AS videos

FROM
  -- ...

@timvandam maybe worth trying this version of SafeQL with your use case, in case subqueries could work for you.

[timvandam]

Hey @karlhorky! I just upgraded SafeQL to latest but am running into the following everywhere:

Internal error: parser.parse is not a function  @ts-safeql/check-sql

[karlhorky]

Ah I think that has to do with something unrelated - having an old version of libpg-query IIRC. Try upgrading that?