feat: a simple LRUCache in frontend

[zhaoyongjie]

SUMMARY

the Drill to detail modal needs a LRUCache to handle response data in the client, so making a simple LRUCache in superset-ui/core.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

passes all CI

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

Merging #20842 (3d78b94) into master (9bf7ed5) will decrease coverage by 0.05%.
The diff coverage is 47.23%.

❗ Current head 3d78b94 differs from pull request most recent head 02fbbdd. Consider uploading reports for the commit 02fbbdd to get more accurate results

@@            Coverage Diff             @@
##           master   #20842      +/-   ##
==========================================
- Coverage   66.33%   66.27%   -0.06%     
==========================================
  Files        1756     1758       +2     
  Lines       66769    66957     +188     
  Branches     7059     7106      +47     
==========================================
+ Hits        44288    44375      +87     
- Misses      20684    20767      +83     
- Partials     1797     1815      +18     

Flag	Coverage Δ
javascript	51.93% <47.70%> (-0.05%)	⬇️

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

Impacted Files	Coverage Δ
...ages/superset-ui-core/src/query/types/Dashboard.ts	100.00% <ø> (ø)
...t-frontend/src/dashboard/actions/dashboardState.js	37.05% <ø> (ø)
superset-frontend/src/dashboard/actions/hydrate.js	1.96% <0.00%> (-0.11%)	⬇️
...src/dashboard/components/FiltersBadge/selectors.ts	70.37% <0.00%> (+1.90%)	⬆️
...dashboard/components/SliceHeaderControls/index.tsx	65.88% <0.00%> (+2.35%)	⬆️
...board/components/nativeFilters/FilterBar/index.tsx	60.14% <0.00%> (-0.44%)	⬇️
...nd/src/dashboard/components/nativeFilters/utils.ts	46.66% <ø> (ø)
...perset-frontend/src/dashboard/containers/Chart.jsx	75.00% <ø> (ø)
...set-frontend/src/dashboard/containers/Dashboard.ts	0.00% <ø> (ø)
...shboard/util/charts/getFormDataWithExtraFilters.ts	88.88% <ø> (-5.56%)	⬇️
... and 31 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

[michael-s-molina]

Can we break this into multiple tests? We could have one to test eviction, one to test invalid key types, and one to test cache clearing.

[zhaoyongjie]

Thanks for the review. This test case is for entire LRU operations. the L43 depends on the cache instance. The purpose of this test case is to prevent incorrect keys from being inserted at runtime, rather than compile time. the clear() method also depends on the cache instance.

[michael-s-molina]

Here's an example:

const newCache = () => {
  const cache = lruCache<string>(3);
  cache.set('1', 'a');
  cache.set('2', 'b');
  cache.set('3', 'c');
  return cache;
};

test('initial LRU', () => {
  expect(lruCache().capacity).toBe(100);
  expect(lruCache(10).capacity).toBe(10);
  expect(lruCache(10).size).toBe(0);
  expect(() => lruCache(0)).toThrow(Error);
});

test('correctly evicts LRU value', () => {
  const cache = newCache();
  expect(cache.size).toBe(3);
  cache.set('4', 'd');
  expect(cache.has('1')).toBeFalsy();
  expect(cache.get('1')).toBeUndefined();
});

test('correctly updates LRU value', () => {
  const cache = newCache();
  cache.get('1');
  cache.set('5', 'e');
  expect(cache.has('1')).toBeTruthy();
  expect(cache.has('2')).toBeFalsy();
});

test('throws exception when key is invalid', () => {
  const cache = lruCache<string>(3);
  // @ts-ignore
  expect(() => cache.set(0)).toThrow(TypeError);
});

test('clears the cache', () => {
  const cache = newCache();
  cache.clear();
  expect(cache.size).toBe(0);
  expect(cache.capacity).toBe(3);
});

This way, when a test fails, it's easier to identify what parts of the code are broken. It also avoids state dependencies between different test cases. You can reuse an initial state or optimize for each test.

[zhaoyongjie]

This is just a test case, do we really need "modularity"? the original test case has 19 lines and the suggestion from you has 32 lines(removed empty line between test case).

the newCache is also not a setup fixture, we can't use it in initial LRU and LRU handle null and undefined. This is a completely additional abstraction.

[michael-s-molina]

I think readability and separation of concerns are more important than the number of lines. It's not a single test case because they are different requirements. You could have a bug related to value eviction but have the requirements for clearing the cache and type checking the keys still working.

[zhaoyongjie]

a modularity test case and all-in-one test case are not equivalent, because the each get or set method will affect the entire LRU.
For example:

test('LRU operation', () => {
  const cache = lruCache<string>(3);
  cache.set('1', 'a');
  cache.set('2', 'b');
  cache.set('3', 'c');
  cache.set('4', 'd');
  expect(cache.size).toBe(3);
....

not equal to

const newCache = () => {
  const cache = lruCache<string>(3);
  cache.set('1', 'a');
  cache.set('2', 'b');
  cache.set('3', 'c');
  return cache;
};
test('correctly evicts LRU value', () => {
  const cache = newCache();
  expect(cache.size).toBe(3);
  cache.set('4', 'd');
....

[michael-s-molina]

If you move the expect after the set then they will be.

[michael-s-molina]

The code that I sent you is just an example. Feel free to improve it. For me, the important part is that the each requirement is in its own test case.

[zhaoyongjie]

@michael-s-molina Sorry, my point was that each call to get and set has an effect on the entire LRU, for example, the correctly evicts LRU value and the correctly updates LRU value should operate on a same LRU instance rather than separately. For this argument, we should probably get someone else to take a look at it as well, and I'd be happy to revise it, thank you very much for the review.

[michael-s-molina]

I see no problem in merging these two tests together since they are highly related 😉

[stephenLYZ]

Looks good to me.

[ktmud]

Given the simplicity of this cache class, I think it's okay to structure basic operations in one test case. However, can we add a test for setting non-string keys? Cache should be expected to return value when trying to retrieve a value for non-string keys, too. (Or throw an error if keys are required to be strings.)

We can always add more tests once there is need to expand the functionalities. Or better yet, just use an established open source solutions (e.g. node-lru-cache) if more requirements arise.

[zhaoyongjie]

@michael-s-molina @ktmud @stephenLYZ Thanks for the review, I will put your consideration into future iterations, if more requirements arise, use the open source solution.