feat: slonik major upgrade#7897
feat: slonik major upgrade#7897n1ru4l wants to merge 7 commits intocopilot/update-slonik-connection-usagefrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant upgrade of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
|
||
| Object.assign(psqlFn, createSqlTag()); | ||
|
|
||
| export const psql = psqlFn as any as SqlTag<any> & TaggedTemplateLiteralInvocation; |
There was a problem hiding this comment.
By keeping the tag callable, we avoid introducing psql.unknown and transforming all existing use to use psql.unknown instead of psql.
packages/services/api/src/modules/auth/providers/supertokens-store.ts
Outdated
Show resolved
Hide resolved
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
| SELECT id, user_id | ||
| FROM organizations | ||
| WHERE id IN (${psql.join(organizations, psql`, `)})`, | ||
| WHERE id IN (${psql.join(organizations, psql.fragment`, `)})`, |
There was a problem hiding this comment.
This changed and needed a few updates.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request updates Slonik dependencies and refactors database interaction logic. Key changes include removing the slonik@30.4.4 patch, upgrading slonik and slonik-interceptor-query-logging to 48.13.2, and introducing @standard-schema/spec. The PostgresDatabasePool class is updated to use QuerySqlToken for improved type safety and getRawPgPool is removed. The TaskScheduler is refactored to directly use PostgresDatabasePool methods instead of graphile-worker's WorkerUtils, which was removed. Several bigint to Number transformations in Zod schemas (e.g., limitRetentionDays, limitOperationsMonthly, membersCount, total) are identified as potential sources of precision loss, and the use of psql.fragment for SQL join separators is noted, requiring verification of generated SQL. Additionally, type assertions (as any) in psql.ts and postgres-database-pool.ts are highlighted as areas that might bypass type checking.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/services/workflows/src/kit.ts (135-151)
The code is using tools.withPgClient which is no longer available since tools has been removed. The code needs to be refactored to use the pgPool directly.
packages/services/workflows/src/kit.ts (166-169)
The code is using tools.addJob which is no longer available since tools has been removed. The code needs to be refactored to use the pgPool directly.
packages/services/workflows/src/kit.ts (79-85)
The tools property is removed and the pgPool is now of type PostgresDatabasePool. This changes the way tasks are scheduled. Ensure that the new implementation is correct and that all task scheduling logic is updated accordingly.
packages/services/storage/src/index.ts (5506-5507)
The limitRetentionDays and limitOperationsMonthly fields are transformed from bigint to Number. This could lead to precision loss if the bigint values are larger than the maximum safe integer in JavaScript. Consider using a library like bignumber.js to handle large integer values safely, or ensure that the values are within the safe integer range.
packages/internal/postgres/src/postgres-database-pool.ts (51-53)
The method getRawPgPool has been removed. Ensure that all usages of this method have been updated to use the getSlonikPool method or another appropriate alternative. If it's no longer needed, remove it from the codebase.
packages/services/storage/src/index.ts (5760)
The total field is transformed from bigint to Number. This could lead to precision loss if the bigint values are larger than the maximum safe integer in JavaScript. Consider using a library like bignumber.js to handle large integer values safely, or ensure that the values are within the safe integer range.
packages/services/storage/src/index.ts (1828)
The total field is transformed from bigint to Number. This could lead to precision loss if the bigint values are larger than the maximum safe integer in JavaScript. Consider using a library like bignumber.js to handle large integer values safely, or ensure that the values are within the safe integer range.
packages/services/commerce/src/index.ts (62)
The TaskScheduler is now initialized with storage.pool instead of storage.pool.getRawPgPool(). Ensure that the TaskScheduler is compatible with the PostgresDatabasePool interface and that all required methods are available.
packages/services/server/src/index.ts (172)
The TaskScheduler is now initialized with storage.pool instead of storage.pool.getRawPgPool(). Ensure that the TaskScheduler is compatible with the PostgresDatabasePool interface and that all required methods are available.
packages/services/storage/src/index.ts (1866)
The total field is transformed from bigint to Number. This could lead to precision loss if the bigint values are larger than the maximum safe integer in JavaScript. Consider using a library like bignumber.js to handle large integer values safely, or ensure that the values are within the safe integer range.
packages/services/storage/src/index.ts (673)
The total field is transformed from bigint to Number. This could lead to precision loss if the bigint values are larger than the maximum safe integer in JavaScript. Consider using a library like bignumber.js to handle large integer values safely, or ensure that the values are within the safe integer range.
packages/services/storage/src/index.ts (1856)
The total field is transformed from bigint to Number. This could lead to precision loss if the bigint values are larger than the maximum safe integer in JavaScript. Consider using a library like bignumber.js to handle large integer values safely, or ensure that the values are within the safe integer range.
packages/services/storage/src/index.ts (1840)
The total field is transformed from bigint to Number. This could lead to precision loss if the bigint values are larger than the maximum safe integer in JavaScript. Consider using a library like bignumber.js to handle large integer values safely, or ensure that the values are within the safe integer range.
packages/internal/postgres/src/psql.ts (21)
The type assertion psqlFn as any as SqlTag<any> & TaggedTemplateLiteralInvocation is used to combine the properties of psqlFn and createSqlTag. While this may be necessary to satisfy the TypeScript compiler, it's important to ensure that this doesn't bypass any type checking or introduce runtime errors. Consider adding a comment explaining why this assertion is needed.
packages/services/storage/src/index.ts (1720)
The separator in psql.join has been changed from , ( to fragment), (`. Ensure that this change does not affect the generated SQL query and that the values are correctly joined.
targets.map(dest => psql.join([target, dest], psql.fragment`, `)),
psql.fragment`), (`,
)}packages/services/storage/src/index.ts (1502)
The separator in psql.join has been changed from , ( to fragment), (`. Ensure that this change does not affect the generated SQL query and that the values are correctly joined.
uniqueSelectors.map(s => psql`${s.targetId}, ${s.projectId}`),
psql.fragment`), (`,
)})packages/services/storage/src/index.ts (839)
The separator in psql.join has been changed from , ( to fragment), (`. Ensure that this change does not affect the generated SQL query and that the values are correctly joined.
pairs.map(p => psql`${p.organizationId}, ${p.userId}`),
psql.fragment`), (`,
)}))packages/services/storage/src/index.ts (697)
The separator in psql.join has been changed from , ( to fragment), (`. Ensure that this change does not affect the generated SQL query and that the values are correctly joined.
selectors.map(s => psql`${s.organizationId}, ${s.userId}`),
psql.fragment`), (`,
)}))packages/services/storage/src/index.ts (620)
The separator in psql.join has been changed from , to fragment, `. Ensure that this change does not affect the generated SQL query and that the values are correctly joined.
WHERE id IN (${psql.join(organizations, psql.fragment`, `)})packages/services/storage/src/index.ts (2419)
The separator in psql.join has been changed from , ( to fragment), (`. Ensure that this change does not affect the generated SQL query and that the values are correctly joined.
selectors.map(s => psql`${s.commit}, ${s.targetId}`),
psql.fragment`), (`,
)}))packages/internal/postgres/package.json (10-12)
The versions of slonik and slonik-interceptor-query-logging should be kept in sync to avoid compatibility issues. Ensure that both packages are updated to the same major and minor version.
packages/internal/postgres/src/postgres-database-pool.ts (202)
The type assertion interceptor as any is used to assign the interceptor function to the pool method. While this may be necessary to satisfy the TypeScript compiler, it's important to ensure that this doesn't bypass any type checking or introduce runtime errors. Consider adding a comment explaining why this assertion is needed.
packages/internal/postgres/src/postgres-database-pool.ts (189-193)
The type definition for the interceptor function has been simplified. Verify that this change does not introduce any type-related issues or break existing functionality. Ensure that the sql parameter is correctly typed as QuerySqlToken<any>.
packages/internal/postgres/src/postgres-database-pool.ts (125-137)
The transaction handler's methods are being redefined using the methods available on the methods argument. This is unnecessary as the methods can be directly assigned. This simplifies the code and reduces redundancy.
return handler({
exists: methods.exists,
any: methods.any,
maybeOne: methods.maybeOne,
async query(
sql: QuerySqlToken<any>,
values?: PrimitiveValueExpression[],
): Promise<void> {
await methods.query(sql, values);
},
oneFirst: methods.oneFirst,
maybeOneFirst: methods.maybeOneFirst,
anyFirst: methods.anyFirst,
one: methods.one,
});packages/internal/postgres/src/postgres-database-pool.ts (17-45)
The type definitions for the query methods have been updated to use generics with StandardSchemaV1. This is good for type safety, but ensure that StandardSchemaV1 is correctly used throughout the codebase and that the inferred types are as expected. Also, confirm that the return types are accurate for all possible query results.
There was a problem hiding this comment.
This is one of the most annoying changes. Slonik introduced their own connection pool abstraction, which does not make it easy and straightforward to re-use the pg connection pool with graphile-workers. I will investigate ways to bridge the gap if possible.
n1ru4l
force-pushed
the
feat-upgrade-slonik
branch
2 times, most recently
from
1b080b8 to
db3e117
Compare
n1ru4l
force-pushed
the
copilot/update-slonik-connection-usage
branch
2 times, most recently
from
a24dba9 to
4a99486
Compare
n1ru4l
force-pushed
the
feat-upgrade-slonik
branch
3 times, most recently
from
34b92eb to
b798336
Compare
n1ru4l
force-pushed
the
feat-upgrade-slonik
branch
from
a570a2e to
b90983f
Compare
| const typeParsers = [ | ||
| ...createTypeParserPreset().filter(parser => parser.name !== 'int8'), | ||
| { | ||
| name: 'int8', | ||
| parse: (value: string) => parseInt(value, 10), | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
In slonik the default type parser for int was changed to retrieve bigint. Introducing the old behaviour reduces the need to change all queries or zod model parsings. I don't see a benefit or issues for using bigint
n1ru4l
force-pushed
the
feat-upgrade-slonik
branch
from
b90983f to
6d64dd5
Compare
n1ru4l
force-pushed
the
feat-upgrade-slonik
branch
from
6d64dd5 to
f047644
Compare
n1ru4l
force-pushed
the
feat-upgrade-slonik
branch
4 times, most recently
from
59c52d1 to
a62d40f
Compare
n1ru4l
force-pushed
the
feat-upgrade-slonik
branch
from
a62d40f to
73dcef5
Compare
1 participant
Background
Major slonik upgrade on top of #7887
Description
Checklist