-
Notifications
You must be signed in to change notification settings - Fork 6.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(core): Change VariablesService to DI and use caching #6827
Merged
flipswitchingmonkey
merged 36 commits into
master
from
pay-644-variables-loading-might-be-slow
Aug 2, 2023
Merged
Changes from 34 commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
765ebd2
support redis cluster
flipswitchingmonkey 492e8a6
cleanup, fix config schema
flipswitchingmonkey 4d6b64a
set default prefix to bull
flipswitchingmonkey 76e1e6b
Merge branch 'master' into pay-639-make-redis-client-into-a-service-f…
flipswitchingmonkey 8b924b4
initial commit
flipswitchingmonkey 9686c8d
improve logging
flipswitchingmonkey ffc70dd
improve types and refactor
flipswitchingmonkey 15bb8ca
list support and refactor
flipswitchingmonkey ec08e9a
Merge branch 'master' into pay-639-make-redis-client-into-a-service-f…
flipswitchingmonkey 0e584eb
fix redis service and tests
flipswitchingmonkey 1fa934d
add comment
flipswitchingmonkey 16e2b8a
add redis and cache prefix
flipswitchingmonkey 364e0ec
use injection
flipswitchingmonkey 6bff179
lint fix
flipswitchingmonkey 5bfabdd
Merge branch 'master' into pay-639-make-redis-client-into-a-service-f…
flipswitchingmonkey 331d433
clean schema comments
flipswitchingmonkey f7f83fd
improve naming, tests, cluster client
flipswitchingmonkey d56317c
Merge branch 'master' into pay-639-make-redis-client-into-a-service-f…
flipswitchingmonkey edec7c6
merge master
flipswitchingmonkey 0ced012
cache returns unknown instead of T
flipswitchingmonkey 1a5b761
update cache service, tests and doc
flipswitchingmonkey 86f7eaa
Merge branch 'master' into pay-639-make-redis-client-into-a-service-f…
flipswitchingmonkey ef7a266
remove console.log
flipswitchingmonkey 5b85ec0
VariablesService as DI, add caching, fix tests
flipswitchingmonkey 69dbe17
do not cache null or undefined values
flipswitchingmonkey abbb6f4
import fix
flipswitchingmonkey d66162c
more DI and remove collections
flipswitchingmonkey acbbfdb
Merge branch 'master' into pay-639-make-redis-client-into-a-service-f…
flipswitchingmonkey 659bbe7
fix merge
flipswitchingmonkey 2298c3c
Merge branch 'master' into pay-639-make-redis-client-into-a-service-f…
flipswitchingmonkey 056de7d
lint fix
flipswitchingmonkey dc30d03
Merge branch 'pay-639-make-redis-client-into-a-service-for-injection'…
flipswitchingmonkey 3212d8a
Merge branch 'master' into pay-644-variables-loading-might-be-slow
flipswitchingmonkey 8d3b2cb
rename to ~Cached
flipswitchingmonkey b15630e
fix test for CI
flipswitchingmonkey ae8fa2b
fix ActiveWorkflowRunner test
flipswitchingmonkey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
51 changes: 42 additions & 9 deletions
51
packages/cli/src/environments/variables/variables.service.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,53 @@ | ||
import type { Variables } from '@db/entities/Variables'; | ||
import { collections } from '@/Db'; | ||
import { CacheService } from '@/services/cache.service'; | ||
import Container, { Service } from 'typedi'; | ||
import { VariablesRepository } from '@/databases/repositories'; | ||
import type { DeepPartial } from 'typeorm'; | ||
|
||
@Service() | ||
export class VariablesService { | ||
static async getAll(): Promise<Variables[]> { | ||
return collections.Variables.find(); | ||
constructor( | ||
protected cacheService: CacheService, | ||
protected variablesRepository: VariablesRepository, | ||
) {} | ||
|
||
async getAllCached(): Promise<Variables[]> { | ||
const variables = await this.cacheService.get('variables', { | ||
async refreshFunction() { | ||
// TODO: log refresh cache metric | ||
return Container.get(VariablesService).findAll(); | ||
}, | ||
}); | ||
return (variables as Array<DeepPartial<Variables>>).map((v) => | ||
this.variablesRepository.create(v), | ||
); | ||
} | ||
|
||
async getCount(): Promise<number> { | ||
return (await this.getAllCached()).length; | ||
} | ||
|
||
async getCached(id: string): Promise<Variables | null> { | ||
const variables = await this.getAllCached(); | ||
const foundVariable = variables.find((variable) => variable.id === id); | ||
if (!foundVariable) { | ||
return null; | ||
} | ||
return this.variablesRepository.create(foundVariable as DeepPartial<Variables>); | ||
} | ||
|
||
static async getCount(): Promise<number> { | ||
return collections.Variables.count(); | ||
async delete(id: string): Promise<void> { | ||
await this.variablesRepository.delete(id); | ||
await this.updateCache(); | ||
ivov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
static async get(id: string): Promise<Variables | null> { | ||
return collections.Variables.findOne({ where: { id } }); | ||
async updateCache(): Promise<void> { | ||
// TODO: log update cache metric | ||
const variables = await this.findAll(); | ||
await this.cacheService.set('variables', variables); | ||
} | ||
|
||
static async delete(id: string): Promise<void> { | ||
await collections.Variables.delete(id); | ||
async findAll(): Promise<Variables[]> { | ||
return this.variablesRepository.find(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ import type { | |
} from './types'; | ||
import type { ExecutionData } from '@db/entities/ExecutionData'; | ||
import { generateNanoId } from '@db/utils/generators'; | ||
import { VariablesService } from '@/environments/variables/variables.service'; | ||
|
||
export type TestDBType = 'postgres' | 'mysql'; | ||
|
||
|
@@ -514,11 +515,13 @@ export async function getWorkflowSharing(workflow: WorkflowEntity) { | |
// ---------------------------------- | ||
|
||
export async function createVariable(key: string, value: string) { | ||
return Db.collections.Variables.save({ | ||
const result = await Db.collections.Variables.save({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd expect |
||
id: generateNanoId(), | ||
key, | ||
value, | ||
}); | ||
await Container.get(VariablesService).updateCache(); | ||
return result; | ||
} | ||
|
||
export async function getVariableByKey(key: string) { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to see us move logic away from
*Helpers.ts
and into services. Any reason why we keepgetVariables
out here?