Skip to content
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

feat: refactor cache functions #3079

Merged
merged 9 commits into from
Mar 30, 2023

Conversation

djabarovgeorge
Copy link
Contributor

@djabarovgeorge djabarovgeorge commented Mar 26, 2023

What change does this PR introduce?

General refactor i will add comments on the code in order to explain what and why.

Why was this change needed?

Other information (Screenshots)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed this decorator so no one will use the evil scan on the cache by mistake.

Comment on lines -10 to -40
public async clearCache({
storeKeyPrefix,
credentials,
}: {
storeKeyPrefix: CacheKeyPrefixEnum | CacheKeyPrefixEnum[];
credentials: { _id: string; _environmentId: string } | Record<string, unknown>;
}) {
Logger.log('Clearing the cache of keys with the specified prefixes');
Logger.debug('StoreKeyPrefix(s) are: ' + storeKeyPrefix);
Logger.debug('Credentials are: ' + (credentials._id as string));
if (!this.cacheService?.cacheEnabled()) {
Logger.verbose('Cashing service is not enabled to clear Cache.');

return;
}

if (Array.isArray(storeKeyPrefix)) {
Logger.verbose('Mapping all keys to flush');
const invalidatePromises = storeKeyPrefix.map((prefix) => {
return this.clearByPattern(prefix, credentials);
});

Logger.debug('invalidate promises are: ' + invalidatePromises);
Logger.verbose('Removing all keys with prefix');
await Promise.all(invalidatePromises);
} else {
Logger.warn('StoreKeyPrefix is not in array format');
await this.clearByPattern(storeKeyPrefix, credentials);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here removed this function so no one will use the evil scan on the cache by mistake.
I still left the 'clearByPattern' function because it is private, I may delete it in near future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split the entities keys builder functions to separate files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split the quieries keys builder functions to separate files.

@djabarovgeorge
Copy link
Contributor Author

Still did not have the time to check if we can optimize the notification templates invalidation. hopefully will look into it this week.

Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great refactor. Even if it is quite subtle.
👏🏻

Comment on lines +294 to +295
buildUserKey({
_id: command._id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one.

@@ -1,3 +1,2 @@
export * from './invalidate-cache.service';
export * from './cache.service';
export { CacheKeyPrefixEnum } from './keys';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would export the key builders so the imports would be simplified in the rest of the codebase.

type: CacheKeyTypeEnum.ENTITY,
keyEntity: CacheKeyPrefixEnum.SUBSCRIBER,
environmentId: _environmentId,
identifierPrefix: 's',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would create constants for the different identifier prefixes. Or an enum.
And export them as we are reusing them in different places and can led to misalignment.

@scopsy scopsy merged commit c8e217a into cache-notification-template-by-trigger Mar 30, 2023
@scopsy scopsy deleted the cache-refactoring branch March 30, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants