refactor: shared internal postgres package that wraps slonik#7887
refactor: shared internal postgres package that wraps slonik#7887
Conversation
… I/O decoding Co-authored-by: n1ru4l <14338007+n1ru4l@users.noreply.github.com>
unknown + Zod parsing in storage
unknown + Zod parsing in storage
This comment was marked as resolved.
This comment was marked as resolved.
…llable().parse
- Replace `r => r ? Model.parse(r) : null` with `Model.nullable().parse` (point-free)
- Replace `v => Model.nullable().parse(v)` with `Model.nullable().parse`
- Replace `.query<unknown>(...).then(r => ({ ...r, rows: r.rows.map(...) }))`
with `.any<unknown>(...).then(z.array(Model).parse)`
- Update all downstream `.rows` array accesses after the above changes
- Replace `.then(rows => rows.map(row => Model.parse(row)))`
with `.then(z.array(Model).parse)`
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… query to any Co-authored-by: n1ru4l <14338007+n1ru4l@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
… parsing Co-authored-by: n1ru4l <14338007+n1ru4l@users.noreply.github.com>
|
@copilot There is more functions like |
6e75a5a to
7bff3e4
Compare
3320f21 to
21ec7e2
Compare
6c4aa68 to
ab14763
Compare
95e4f72 to
212ac08
Compare
…-connection-usage
8e5a011 to
a24dba9
Compare
a24dba9 to
4a99486
Compare
| private taskScheduler: TaskScheduler, | ||
| private rateLimiter: InMemoryRateLimiter, | ||
| @Inject(WEB_APP_URL) private appBaseUrl: string, | ||
| @Inject(PG_POOL_CONFIG) private pool: DatabasePool, |
There was a problem hiding this comment.
I like the small side-effect that we now have a class and this @Inject(PG_POOL_CONFIG) thingy is no longer needed.
| delete process.env.CLICKHOUSE_TTL_HOURLY_MV_TABLES; | ||
| delete process.env.CLICKHOUSE_TTL_MINUTELY_MV_TABLES; | ||
|
|
||
| vi.resetModules(); |
There was a problem hiding this comment.
This is already being imported on line 25. The reset looks to be necessary to reset the environment variables on import
There was a problem hiding this comment.
It does not seem to be necessary. Everyyhing works without it. 😓 Keeping it raised an exception.
There was a problem hiding this comment.
That's odd. What was the error? I don't see any big changes with the code.
| RETURNING ${schemaPolicyFields(psql``)}; | ||
| `, | ||
| ) | ||
| .then(SchemaPolicyModel.parse); |
There was a problem hiding this comment.
Do we use the returned fields from the update calls?
I'd expect we already have that information and by returning everything we're wasting bandwidth. Prefer returning void where it makes sense.
| RETURNING | ||
| "organization_id" AS "organizationId", | ||
| "external_billing_reference_id" AS "externalBillingReference", | ||
| "billing_email_address" AS "billingEmailAddress" |
There was a problem hiding this comment.
Should this also be abstracted to a const to be used in other calls?
| return results.rows; | ||
| return pool | ||
| .any( | ||
| psql`/* getGetOrganizationsAndTargetsWithLimitInfo */ |
There was a problem hiding this comment.
minor: prefer moving to snake case to be consistent
Background
See #7811
Description
Note: this started as a copilot thing, but I (@n1ru4l), quickly took over and did a lot of manual work.
This PR updates all usages of
slonikto go through our internal package@hive/postgres.Because
slonikfrequently introduces breaking changes, this creates a single abstraction layer around it. This allows us to centralize how the library is used and makes it easier to adopt future major versions in a backward-compatible way.This PR exploded a bit, since I initially only started with introducing
zodmodel parsing to existing database calls that still used unsafeas typecastsvia the generic parameter.While working on that I realized that we could benefit from having a single slonik entrypoint shared in the whole repository.
Summary:
@hive/postgrescreateDatabaseQueryString, traced transactions)sqltopsql, to avoid confusion and conflicts with our in-house clickhousesqltagPostgresDatabasePool, that has the query methods withunknownhard-coded so you are forced to usezodto decode the resultpsqlinstead ofsqlPostgresDatabasePoolWhether we keep or remove
slonikas a dependency (still uncovering things bit by bit that we need to address in case we update in #7897), I see benefit of having a single entrypoint nevertheless.