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

Infer TypeScript literals when possible #269

Closed
karlhorky opened this issue Sep 21, 2024 · 10 comments
Closed

Infer TypeScript literals when possible #269

karlhorky opened this issue Sep 21, 2024 · 10 comments

Comments

@karlhorky
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

Previously, there were requests for support for TypeScript literal types in #120 and #119, which were not accepted, as they did not follow the SafeQL philosophy.

It would be nice for there to be partial support for literals (eg. string literals, number literals), to enable stricter types in applications.

An example:

// 💥 Query has incorrect type annotation.
//	Expected: { video_type: 'personal_intro' | 'tech_achievements'; }
//	Actual: { video_type: string; }[]
await sql<{ video_type: 'personal_intro' | 'tech_achievements' }[]>`
  SELECT
    CASE
      WHEN personal_intro_vimeo_video_id = ${vimeoVideoId} THEN 'personal_intro'
      WHEN tech_achievements_vimeo_video_id = ${vimeoVideoId} THEN 'tech_achievements'
    END AS video_type
  FROM
    users
`;

Describe the solution you'd like

If there are inferable literal values in the query, infer literal values instead of string or number

Describe alternatives you've considered

--

Additional context

--

@Newbie012
Copy link
Collaborator

As stated in previous issues, doing something like this is equivalent to do (string) as literal which is unsafe and WILL cause unexpected behaviors. SafeQL's approach is to get the types from the schema as it's the only source of truth of your data.

Currently, you have 2 options:

  1. Alter the column type to use ENUM, so SafeQL could safely infer it as literals. This should probably be documented.
  2. Override the column's type as documented here.

Another approach which is currently not supported is to infer from table constraints (`col text CONSTRAINT union CHECK (col IN ('a', 'b', 'c'))

@karlhorky
Copy link
Collaborator Author

karlhorky commented Sep 22, 2024

doing something like this is equivalent to do (string) as literal which is unsafe

SafeQL's approach is to get the types from the schema as it's the only source of truth of your data

Oh, is this still true with my example above (generated column / field data)? I thought that this would be a different case, because of the "generated" part... 🤔

To be clear, users.video_type is not a column / field definition in the schema.

@Newbie012
Copy link
Collaborator

Oh, I'm sorry. I missed that. I think this could be achievable, but it would require some adjustments when comparing actual with expected.

@gajus
Copy link
Contributor

gajus commented Dec 6, 2024

I believe I am running into the same limitation.

await pool.query(sql.typeAlias('void')`
  UPDATE recommendation_request
  SET
    created_at = (now() - ${`${daysOld} days`}::interval),
    status = ${status}
  WHERE id = ${recommendationRequestDatabaseId}
`);

Table column status is of type text.

status variable is a union of literals ('APPROVED' | 'DECLINED' | 'EXPIRED' | 'PENDING' | 'SUBMITTED').

This is currently producing a syntax error:

/Users/gajus/Developer/contra/gaia/apps/contra-api/testV2/workflows/expireRecommendationRequestsV1.test.ts
  48:16  error  Invalid Query: Error: syntax error at or near "IN"  @ts-safeql/check-sql

Posting here to calibrate on whether new issue or same, since the original issue does not mention syntax error.

Casting ${status as string} makes the error go away.

@Newbie012
Copy link
Collaborator

Newbie012 commented Dec 6, 2024

Different issue that was introduced in 3.5.0 due to the way safeql handles union of literals. I'll push a fix in the next few days.

Edit - fixed in 3.5.1

@Newbie012
Copy link
Collaborator

@gajus
Copy link
Contributor

gajus commented Dec 27, 2024

Amazing. Thank you!

@karlhorky
Copy link
Collaborator Author

karlhorky commented Dec 29, 2024

@Newbie012 thanks for this! Can confirm that this works for my case above! (needed to add null to the union)

// ✅
await sql<{ video_type: 'personal_intro' | 'tech_achievements' | null }[]>`
  SELECT
    CASE
      WHEN personal_intro_vimeo_video_id = ${vimeoVideoId} THEN 'personal_intro'
      WHEN tech_achievements_vimeo_video_id = ${vimeoVideoId} THEN 'tech_achievements'
    END AS video_type
  FROM
    users
`;

However, it seems to have introduced some new regressions:

A) Missing type on boolean inside CASE

The type for the has_url is actually completely empty (empty string) 🤔 Haven't seen this one before.

// 💥 Query has incorrect type annotation.
//	Expected: { has_url: boolean; }
//	Actual: { has_url: ; }[]
await sql<{ has_url: boolean }[]>`
  SELECT
    CASE
      WHEN TRUE THEN personal_intro_url_private IS NOT NULL
      ELSE tech_achievements_url_private IS NOT NULL
    END AS has_url
  FROM
    users
`;

Also reported in this issue:

@karlhorky
Copy link
Collaborator Author

karlhorky commented Dec 29, 2024

B) Some JsonbAgg types are now returning unknown

With the overrides.types.jsonb configuration:

overrides: {
  types: {
    jsonb: 'JsonbAgg',
  },
},

One of my queries which was previously yielding the type JsonbAgg now yields unknown | null:

type JsonbAgg = { is_guest: boolean; };

type User = { lecturer: JsonbAgg | null };

// 💥 Query has incorrect type annotation.
//	Expected: { lecturer: JsonbAgg | null; }
//	Actual: { lecturer: unknown | null; }[]
await sql<User[]>`
  SELECT
    CASE
      WHEN lecturers.id IS NOT NULL THEN (
        jsonb_build_object(
          'is_guest',
          lecturer_users.email NOT LIKE '%@upleveled.io'
        )
      )
      ELSE NULL
    END AS lecturer
  FROM
    users
`;

Unfortunately, unknown | null cannot be used in a type alias (possibly because TypeScript evaluates that to be unknown):

type User = { lecturer: unknown | null };

// 💥 Query has incorrect type annotation.
//	Expected: { lecturer: unknown; }
//	Actual: { lecturer: unknown | null; }[]
await sql<User[]>`
  SELECT
    CASE
      WHEN lecturers.id IS NOT NULL THEN (
        jsonb_build_object(
          'is_guest',
          lecturer_users.email NOT LIKE '%@upleveled.io'
        )
      )
      ELSE NULL
    END AS lecturer
  FROM
    users
`;

Workaround:

Use the unknown | null in an inline generic type argument:

// ✅
await sql<{ lecturer: unknown | null }[]>`
  SELECT
    CASE
      WHEN lecturers.id IS NOT NULL THEN (
        jsonb_build_object(
          'is_guest',
          lecturer_users.email NOT LIKE '%@upleveled.io'
        )
      )
      ELSE NULL
    END AS lecturer
  FROM
    users
`;

So 2 problems here:

  1. The JsonbAgg type is no longer returned here
  2. unknown | null probably should never be inferred by SafeQL, because it's not compatible with TypeScript unknown | null inside of a type alias

Also reported in this issue:

@Newbie012
Copy link
Collaborator

Addressing both the of issues in #354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants