feat: add RedisHashKeyValueDB

[mrnagydavid]

In this PR, I implemented the RedisHashKeyValueDB which is a special class providing the usual CommonKeyValueDB functions on a hash field in Redis.

The underlying use-case is to

1. keep certain data in a hash together for easier fetching (similar to having all fields top-level with a table: prefix)
2. be able to increment values of certain fields

Compared to using the more idiomatic table: prefix approach, the hash field approach has the benefit of fetching the entire hash with an O(n) operation n being the size of the hash - meanwhile with a table: prefix, the scan operation would be O(m) where m is the number of keys in redis. (At least that is my understanding.)

const db = new RedisHashKeyValueDB({ 
  client: new RedisClient()
  hashKey: 'pageStats' // the name of the field in Redis which will store the hash
})

const dao = new CommonKeyValueDao({
  db,
  table: 'table' // this would become the prefix of the property names in the hash
})

const result = await dao.increment('viewIndexPage')
// --> HINCRBY pageStats table:viewIndexPage 1
expect(result).toBe(1)

This PR depends on NaturalCycles/db-lib#18

[mrnagydavid]

Looks like yarn test-manual is flaky.
At least 1 out of 4 runs it fails a few tests in runCommonKeyValueDBTest with the same error:
ReplyError: ERR wrong number of arguments for 'mget' command

I think it is may be related to the fact that the tests are not isolated.

[kirillgroshkov]

LGTM with minor comments.

Great job with tests! 🙏

[kirillgroshkov]

should it be in lowercase? (consistent)

Suggested change

	async hScanCount(key: string, opt: ScanStreamOptions): Promise<number> {
	async hscanCount(key: string, opt: ScanStreamOptions): Promise<number> {

[mrnagydavid]

My impression was that you tried to follow the same naming pattern whenever we are just providing a wrapper:
for example async mget() wraps this.redis().mget() (instead of async mGet()).

And you followed camelCase whenever this lib provides extra funtionality, e.g.: mgetBuffer or scanStreamFlat.

So my logic was:

• hscanStream() wraps this.redis().hscanStream() call, hence identically named
• hScanCount() provides custom functionality (i.e. there is no redis().hscanCount call), hence camelCase

TBH I don't know and I don't feel strongly about it, so I trust your judgement in using lowercase

[kirillgroshkov]

No strong opinion either, I just thought it's a typo. You can pick how you prefer.

My gut feeling is with lowercase hscan though

[kirillgroshkov]

p3: I'd use native map here (instead of _mapObject), like:

const map = Object.fromEntries(entries.map(...))

why?
Since you're already using native Object.fromEntries.

[kirillgroshkov]

Q: why is it async (and has concurrency), while I see no await inside?

[mrnagydavid]

Copy-pasted from redisKeyValueDB where it is correctly async.
As I re-wrote the function, I didn't notice that I removed the awaited calls in it.

[kirillgroshkov]

Q why async?

[mrnagydavid]

Also confirmed that the flakiness is due to parallel running the test suits on the same REDIS with the same table name.
yarn test-manual --runInBand removes flakiness in this repo.
Other fix should probably go to db-lib where the TEST_TABLE is defined.

[kirillgroshkov]

Also confirmed that the flakiness is due to parallel running the test suits on the same REDIS with the same table name. yarn test-manual --runInBand removes flakiness in this repo. Other fix should probably go to db-lib where the TEST_TABLE is defined.

I never ran manual tests in parallel, so, never encountered this.

"Manual tests" are not very well defined, I must admit. Maybe we can define them as "only to be run manually one by one, and never in bulk".