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

fix: use column name instead of field name for redis cache key #124

Merged
merged 1 commit into from
May 12, 2021

Conversation

wschurman
Copy link
Member

@wschurman wschurman commented May 12, 2021

Why

Having the cache key on field name means that field renames need to bump the cache key for the entity itself, which is non-obvious (we've been bit by it twice at Expo). Instead, it makes more sense to cache keyed by column name which is much clearer that a cache key bump is needed when changed.

Closes #107.
Closes ENG-291.

How

Change to use column name in key generation function.

Note that upgrading to a version that includes this change will cause a full entity cache invalidation of all entities. This should be fine for most applications but was why we were hesitant to make this change previously. Now the benefits outweigh the costs though.

Test Plan

Run all tests.

@wschurman wschurman requested review from ide and jkhales May 12, 2021 18:12
@linear
Copy link

linear bot commented May 12, 2021

@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #124 (76a3256) into master (eaab0b1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #124   +/-   ##
=======================================
  Coverage   95.10%   95.10%           
=======================================
  Files          67       67           
  Lines        1694     1697    +3     
  Branches      203      183   -20     
=======================================
+ Hits         1611     1614    +3     
  Misses         81       81           
  Partials        2        2           
Flag Coverage Δ
integration 95.10% <100.00%> (+<0.01%) ⬆️
unittest 95.10% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ntity-cache-adapter-redis/src/RedisCacheAdapter.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaab0b1...76a3256. Read the comment docs.

@wschurman wschurman merged commit d78f452 into master May 12, 2021
@wschurman wschurman deleted the @wschurman/cache-key-thing branch May 12, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redis cache key should use column name instead of field name
2 participants