chore: cache projects response #10927
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| this.timer = (action) => | ||
| metricsHelper.wrapTimer(eventBus, DB_TIME, { | ||
| store: 'project', | ||
| store: 'project-read-model', |
There was a problem hiding this comment.
Just to differentiate this from the project store that also has a similar query
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const cacheTtl = secondsToMilliseconds(20); | ||
| this.getProjectsForAdminUiCached = memoizee( | ||
| ( | ||
| projectsQuery?: IProjectQuery & IProjectsQuery, | ||
| projectsUserId?: number, | ||
| ) => | ||
| this.projectReadModel.getProjectsForAdminUi( | ||
| projectsQuery, | ||
| projectsUserId, | ||
| ), | ||
| { | ||
| promise: true, | ||
| maxAge: cacheTtl, | ||
| normalizer: ([projectsQuery, projectsUserId]) => | ||
| this.buildProjectsCacheKey( | ||
| projectsQuery as | ||
| | (IProjectQuery & IProjectsQuery) | ||
| | undefined, | ||
| projectsUserId as number | undefined, | ||
| ), | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| async getProjects( | ||
| query?: IProjectQuery & IProjectsQuery, | ||
| userId?: number, | ||
| ): Promise<ProjectForUi[]> { | ||
| const projects = await this.projectReadModel.getProjectsForAdminUi( | ||
| query, | ||
| userId, | ||
| ); | ||
| const projects = await this.getProjectsForAdminUiCached(query, userId); | ||
|
|
There was a problem hiding this comment.
The memoized getProjectsForAdminUiCached now caches the project list per user for 20 s but the cache is never cleared when the underlying data changes. After a client loads the list once, creating or archiving a project or toggling a favorite within the same process will still return the cached result at the next getProjects() call, so the new project or updated favorite flag will not appear until the TTL expires. This is user-visible for the admin UI and automation that expects immediate consistency (the commit message already hints at Terraform usage). Consider clearing the memoization in the project mutation flows or otherwise invalidating the cache when projects change.
Useful? React with 👍 / 👎.
kwasniew
left a comment
kwasniew
left a comment
There was a problem hiding this comment.
shouldn't we have CachedProjectReadModel that is a drop-in replacement for ProjectReadModel? IMO it would be much more clear than another method in the project service that is already overloaded with capabilities
| return projects; | ||
| } | ||
|
|
||
| private buildProjectsCacheKey( |
There was a problem hiding this comment.
Pretty sure this will work but it feels a bit dirty to be using big ass JSON blobs as hash keys, we can't just do this?
private buildProjectsCacheKey(
query?: IProjectQuery & IProjectsQuery,
userId?: number,
): string {
const hash = createHash('sha256');
if (query?.ids) {
const sorted = [...query.ids].sort();
hash.update(sorted.join(','));
}
hash.update(String(userId ?? ''));
hash.update(String(query?.id ?? ''));
hash.update(String(query?.archived ?? ''));
return hash.digest('base64url');
}
There was a problem hiding this comment.
Yeah, we could just concatenate strings... hsould be better, will do!
There was a problem hiding this comment.
But I like your approach
I think the idea is to always return the cached version. It should be smart enough to know when to query the cache or not, but it's still a workaround, to me the problem is having the API overloaded with features that can be different resources. So I prefer to keep it yanky for now. I'm also tempted to just enable this to the customer having issues |
About the changes
This could backfire if our terraform integration might be expecting to see new data after creating a project... we'd need cache invalidation and that starts to feel like a lot of complexity...