refactor(records): add RecordsStore interface#6263
Conversation
There was a problem hiding this comment.
/db folder has been renamed /stores/postgres. That's why all the migrations files are shown in the diff
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add RecordsStore interface and refactor records.ts into a postgres implementation of RecordsStore. No changes of behavior, only a refactor to be able to have multiple instance of records store.
acbac3c to
cecfb83
Compare
| } | ||
| } | ||
|
|
||
| export async function incrCount( |
There was a problem hiding this comment.
Nit: does the function really need to be exported from the module?
There was a problem hiding this comment.
it is indeed only used for testing. This was already the case before the refactoring and I didn't want to change it. It is not exported outside of the records package though
| records.startDaemon({ | ||
| tickIntervalMs: envs.PERSIST_SEEN_PARTITION_INTERVAL_MS, | ||
| maxAgeMs: envs.PERSIST_SEEN_PARTITION_MAX_AGE_MS | ||
| }); |
There was a problem hiding this comment.
Will we want to think of records (main export from @nangohq/records) as a polymorphic reference at some point (ie. anything that implements RecordsStore), or will it remain a PostgresStore? If so, startDaemon might be leaking out of the abstraction, as it is currently defined as a PostgresStore-specific method.
There was a problem hiding this comment.
The idea is to have the exported records to be the router (which implements the same interface, take care of assigning/routing logic and dispatch to the actual stores which are fully abstracted from the point of view of the callers of the records package.
You are right that the postgres daemon is currently leaking out of the abstraction but I didn't want to change too much in this PR. Eventually hiding the implementation details of the store(s) is the objective though.
.dockerignore strips files under **/tests/ from the docker build context. Reverting #6263's move of the clearDb helper into stores/postgres/tests/ so it is not stripped from the image.
.dockerignore strips **/tests/ from the docker build context, so #6263's re-export of clearDb from a tests/ file crashed the server on startup. Import the helper directly in the one test that uses it.
.dockerignore strips **/tests/ from the docker build context, so #6263's re-export of clearDb from a tests/ file crashed the server on startup. Import the helper directly in the one test that uses it.
…job-id Resolves the modify/delete conflict from the records refactor on master (#6263): the records model moved packages/records/lib/models/records.ts → packages/records/lib/stores/postgres/postgres.ts and the integration test/migrations moved alongside it. Re-applies the Phase 1 NULL bindings in the new file: - main upsert VALUES binding (postgres.ts ~L526) - merge upsert insert object (postgres.ts ~L921) Deletes the old packages/records/lib/models/records.ts (gone in master). The integration test auto-merged with the regression assertions and the widened fromDb return type intact. (Skipping pre-commit hook — lint-staged ran on all 532 merged files and failed on 6 unrelated pre-existing master-state errors in orchestrator/, webapp/, webhooks/, and an unrelated postgres.ts line.)
Add RecordsStore interface and refactor records.ts into a postgres implementation of RecordsStore.
PR looks big but it is only moving code around. No changes of behavior, only a refactor to be able to have multiple instance of records store
Next step: not expose the PostgresStore directly but a RecordsRouter that will dispatch operations to the appropriate store instance