[OTE-753] add new persistent cache table #2175
Conversation
WalkthroughThe changes introduce a persistent caching system within a PostgreSQL database context. New types and interfaces are defined for managing cache entries, including a migration for creating a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CacheTable
participant Database
User->>CacheTable: create(key, value)
CacheTable->>Database: INSERT INTO persistent_cache (key, value)
Database-->>CacheTable: Confirmation
User->>CacheTable: upsert(key, value)
CacheTable->>Database: UPSERT INTO persistent_cache (key, value)
Database-->>CacheTable: Confirmation
User->>CacheTable: findAll()
CacheTable->>Database: SELECT * FROM persistent_cache
Database-->>CacheTable: Return all entries
CacheTable-->>User: List of cache entries
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (5)
indexer/packages/postgres/src/types/persistent-cache-types.ts (2)
1-4: Add comments for better readability.Consider adding comments to the
PersistentCacheCreateObjectinterface to describe the purpose of each property.export interface PersistentCacheCreateObject { + // The key for the cache entry key: string, + // The value for the cache entry value: string, }
6-9: Add comments for better readability.Consider adding comments to the
PersistentCacheColumnsenum to describe the purpose of each column.export enum PersistentCacheColumns { + // The key column for the cache table key = 'key', + // The value column for the cache table value = 'value', }indexer/packages/postgres/src/db/migrations/migration_files/20240828130730_create_persistent_cache_table.ts (2)
3-8: Add comments for better readability.Consider adding comments to the
upfunction to describe the purpose of each column and the table.export async function up(knex: Knex): Promise<void> { + // Create the persistent_cache table return knex.schema.createTable('persistent_cache', (table) => { + // The primary key for the cache entry table.string('key').primary().notNullable(); + // The value for the cache entry table.text('value').notNullable(); }); }
10-12: Add comments for better readability.Consider adding comments to the
downfunction to describe the purpose of dropping the table.export async function down(knex: Knex): Promise<void> { + // Drop the persistent_cache table return knex.schema.dropTable('persistent_cache'); }indexer/packages/postgres/src/models/persistent-cache-model.ts (1)
15-23: Add comments for better readability.Consider adding comments to the
jsonSchemagetter to describe the purpose of each property.static get jsonSchema() { return { type: 'object', required: ['key', 'value'], properties: { + // The key for the cache entry key: { type: 'string' }, + // The value for the cache entry value: { type: 'string' }, }, }; }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- indexer/packages/postgres/tests/helpers/constants.ts (2 hunks)
- indexer/packages/postgres/tests/stores/persistent-cache-table.test.ts (1 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20240828130730_create_persistent_cache_table.ts (1 hunks)
- indexer/packages/postgres/src/helpers/db-helpers.ts (1 hunks)
- indexer/packages/postgres/src/index.ts (2 hunks)
- indexer/packages/postgres/src/models/persistent-cache-model.ts (1 hunks)
- indexer/packages/postgres/src/stores/persistent-cache-table.ts (1 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (1 hunks)
- indexer/packages/postgres/src/types/index.ts (1 hunks)
- indexer/packages/postgres/src/types/persistent-cache-types.ts (1 hunks)
- indexer/packages/postgres/src/types/query-types.ts (2 hunks)
Additional comments not posted (24)
indexer/packages/postgres/src/models/persistent-cache-model.ts (4)
1-4: LGTM!The code changes are approved.
5-7: LGTM!The code changes are approved.
9-11: LGTM!The code changes are approved.
26-31: LGTM!The code changes are approved.
indexer/packages/postgres/src/types/index.ts (1)
31-31: LGTM!The export statement correctly integrates the new types into the existing module.
indexer/packages/postgres/__tests__/stores/persistent-cache-table.test.ts (7)
7-9: LGTM!The
beforeAllfunction correctly sets up the database migration.
11-13: LGTM!The
afterEachfunction correctly clears the database data.
15-17: LGTM!The
afterAllfunction correctly tears down the database setup.
19-21: LGTM!The test correctly verifies the creation of a key-value pair.
23-42: LGTM!The test correctly verifies the upsertion of a key-value pair multiple times.
44-61: LGTM!The test correctly verifies finding all key-value pairs.
63-71: LGTM!The test correctly verifies finding a key-value pair.
indexer/packages/postgres/src/stores/persistent-cache-table.ts (4)
18-62: LGTM!The
findAllfunction correctly retrieves all key-value pairs based on the provided query configuration.
64-71: LGTM!The
createfunction correctly inserts a new key-value pair into the database.
73-82: LGTM!The
upsertfunction correctly inserts or updates a key-value pair in the database.
83-94: LGTM!The
findByIdfunction correctly retrieves a key-value pair by its key.indexer/packages/postgres/src/index.ts (2)
21-21: LGTM!The addition of
PersistentCacheModelexport is straightforward and aligns with the PR objectives.
47-47: LGTM!The addition of
PersistentCacheTableexport is straightforward and aligns with the PR objectives.indexer/packages/postgres/src/helpers/db-helpers.ts (1)
31-31: LGTM!The addition of
persistent_cacheto thelayer1Tablesarray is straightforward and aligns with the PR objectives.indexer/packages/postgres/src/types/db-model-types.ts (1)
276-279: LGTM!The addition of
PersistentCacheFromDatabaseinterface is straightforward and aligns with the PR objectives.indexer/packages/postgres/src/types/query-types.ts (2)
91-91: LGTM!The addition of the
KEYfield to theQueryableFieldenum is straightforward and enhances the flexibility of the enum for various query configurations.
328-330: LGTM!The introduction of the
PersistentCacheQueryConfiginterface is well-structured and aligns with the existing pattern of query configuration interfaces.indexer/packages/postgres/__tests__/helpers/constants.ts (2)
947-950: LGTM!The addition of the
defaultKVconstant is straightforward and provides a default value for persistent cache entries.
952-955: LGTM!The addition of the
defaultKV2constant is straightforward and provides a default value for persistent cache entries.
jerryfan01234
force-pushed
the
affiliates-new-persistent-cache-table
branch
from
bc2edc6 to
ef21ed4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- indexer/packages/postgres/tests/stores/persistent-cache-table.test.ts (1 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20240829171730_create_persistent_cache_table.ts (1 hunks)
- indexer/packages/postgres/src/models/persistent-cache-model.ts (1 hunks)
- indexer/packages/postgres/src/stores/persistent-cache-table.ts (1 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (1 hunks)
- indexer/packages/postgres/src/types/persistent-cache-types.ts (1 hunks)
- indexer/packages/postgres/src/types/query-types.ts (2 hunks)
Files skipped from review as they are similar to previous changes (6)
- indexer/packages/postgres/tests/stores/persistent-cache-table.test.ts
- indexer/packages/postgres/src/models/persistent-cache-model.ts
- indexer/packages/postgres/src/stores/persistent-cache-table.ts
- indexer/packages/postgres/src/types/db-model-types.ts
- indexer/packages/postgres/src/types/persistent-cache-types.ts
- indexer/packages/postgres/src/types/query-types.ts
Additional comments not posted (2)
indexer/packages/postgres/src/db/migrations/migration_files/20240829171730_create_persistent_cache_table.ts (2)
1-1: LGTM!The import statement is correct and necessary for the migration functions.
10-12: LGTM!The function correctly defines the rollback operation for the migration.
| export async function up(knex: Knex): Promise<void> { | ||
| return knex.schema.createTable('persistent_cache', (table) => { | ||
| table.string('key').primary().notNullable(); | ||
| table.string('value').notNullable(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
LGTM! Consider adding a timestamp column.
The function correctly defines the schema for the persistent_cache table. However, it might be beneficial to add a timestamp column to track when entries are created or updated.
Consider adding the following lines to the table definition:
table.timestamps(true, true);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function up(knex: Knex): Promise<void> { | |
| return knex.schema.createTable('persistent_cache', (table) => { | |
| table.string('key').primary().notNullable(); | |
| table.string('value').notNullable(); | |
| }); | |
| } | |
| export async function up(knex: Knex): Promise<void> { | |
| return knex.schema.createTable('persistent_cache', (table) => { | |
| table.string('key').primary().notNullable(); | |
| table.string('value').notNullable(); | |
| table.timestamps(true, true); | |
| }); | |
| } |
| ).upsert(kvToUpsert).returning('*'); | ||
| // should only ever be one key value pair | ||
| return kvs[0]; | ||
| } |
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
persistent_cachetable.Bug Fixes
Documentation