module commands to be marked as unstable and require clients to opt in #2766

sjpotter · 2024-05-26T11:09:29Z

infrastrucutre code to enable search module commands to be marked unstable in RESP3 client usage.

needs, finish conversion of commands to v5, where I'm stuck at the moment

leibale · 2024-05-28T11:26:54Z

packages/client/lib/client/index.ts

    return new ((this as any).Multi as Multi)(
      this._executeMulti.bind(this),
-      this._executePipeline.bind(this)
+      this._executePipeline.bind(this),
+      this._self.#options


multi should throw an error on attempted executoin of an unstableResp3 command. this enables that.

ok, with what we discussed yesterday, putting the handler on object via attachCommand, this is no longer neccessary, so reverted.

leibale · 2024-05-28T11:27:11Z

packages/client/lib/client/index.ts

@@ -163,6 +167,9 @@ export default class RedisClient<
  static #createModuleCommand(command: Command, resp: RespVersions) {
    const transformReply = getTransformReply(command, resp);
    return async function (this: NamespaceProxyClient, ...args: Array<unknown>) {
+      if (command.unstableResp3Module && resp == 3 && !this._self._self.#options?.unstableResp3Modules) {
+        throw new Error("unstable resp3 module, client not configured with proper flag");


need to update the message + include a link

agreed, but unsure what it should be at the moment.

added a FIXME to it, to know we have to work on that string

leibale · 2024-05-28T11:28:28Z

packages/client/lib/client/index.ts

@@ -163,6 +167,9 @@ export default class RedisClient<
  static #createModuleCommand(command: Command, resp: RespVersions) {
    const transformReply = getTransformReply(command, resp);
    return async function (this: NamespaceProxyClient, ...args: Array<unknown>) {
+      if (command.unstableResp3Module && resp == 3 && !this._self._self.#options?.unstableResp3Modules) {


move the if + throw to a function and reuse it everywhere?

ok. makes sense. would be a function.

something like

export function resp3UnstableCheck(resp: RespVersions, unstableResp3Command?: boolean, unstableResp3Client?: boolean) { if (resp == 3 && unstableResp3Command && !unstableResp3Client) { throw new Error("unstable resp3 module, client not configured with proper flag"); } }``` and usage ala

resp3UnstableCheck(resp, command.unstableResp3Module, this._self._self.#options?.unstableResp3Modules);

as discussed on zoom, moved this to how the command is actually attached to object in attachCommand

leibale · 2024-05-28T14:23:01Z

packages/client/lib/client/multi-command.ts

@@ -2,6 +2,7 @@ import COMMANDS from '../commands';
 import RedisMultiCommand, { MULTI_REPLY, MultiReply, MultiReplyType, RedisMultiQueuedCommand } from '../multi-command';
 import { ReplyWithTypeMapping, CommandReply, Command, CommandArguments, CommanderConfig, RedisFunctions, RedisModules, RedisScripts, RespVersions, TransformReply, RedisScript, RedisFunction, TypeMapping } from '../RESP/types';
 import { attachConfig, functionArgumentsPrefix, getTransformReply } from '../commander';
+import { RedisClientOptions } from '.';

 type CommandSignature<


should be "never" when using RESP3 without enabling unstable commands..

I dont know how to do that into the type (I wasn't eager to extend the typing of the client even further)

…o opt in to that.

…y attachCommand

update unstableResp3Module flag to only commands that have unstable api add ability to tag commands to ignore user type mapping (as transformReply will convert it to a non typed map type) update bloom info commands to work with new non typed map ability fill in search/time-series commands and tag with unstableResp3Modules flag as appropriate

adjust all usages of it to SimpleStringReply

packages/bloom/lib/commands/top-k/INFO.ts

packages/client/lib/RESP/types.ts

leibale · 2024-06-25T18:28:11Z

packages/json/lib/commands/TYPE.ts

-    // TODO: ?!??!
-    3: undefined as unknown as () => any
-  }
+    3: undefined as unknown as () => ArrayReply<NullReply | BlobStringReply | ArrayReply<BlobStringReply>>


I'm not too sure if this is right..

in resp3, the same reply that is possible in resp2 is returned in resp3, just wrapped in an array.

pub fn json_type<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>) -> RedisResult { let mut args = args.into_iter().skip(1); let key = args.next_arg()?; let path = Path::new(args.next_str().unwrap_or(default_path())); let key = manager.open_key_read(ctx, &key)?; let value = if path.is_legacy() { json_type_legacy::<M>(&key, path.get_path())? } else { json_type_impl::<M>(&key, path.get_path())? }; // Check context flags to see if RESP3 is enabled and return the appropriate result if is_resp3(ctx) { Ok(vec![value].into()) } else { Ok(value) } }

so if one is ok with the resp2, response, the resp3 response looks right to me.

the problem is really in me messing up the conversion in v4, we have

export declare function transformReply(): string | null | Array<string | null>;

so what we need is the ArrayReply<BlobStringReply | NullReply>

made the change I mentioned now

leibale · 2024-06-25T18:30:25Z

packages/search/lib/commands/CURSOR_DEL.ts

@@ -3,8 +3,8 @@ import { SimpleStringReply, Command, RedisArgument } from '@redis/client/dist/li
 export default {
  FIRST_KEY_INDEX: undefined,
  IS_READ_ONLY: true,
-  transformArguments(index: RedisArgument, cursorId: RedisArgument) {
-    return ['FT.CURSOR', 'DEL', index, cursorId];
+  transformArguments(index: RedisArgument, cursorId: number) {


cursorId can go outside the float64 range.. string has to be supported, number is nice to have

ok, was a copy form v4 which used number. trying to remember why I changed it from what you did originally, I suspect number might be used elsewhere and needs to be fixed, will look into that.

so I'm also unconvinced that "number" is nice to have, its a "number", but in practice, cursors should be opaque, and we shouldn't encourage users to think of it as a number, so thinking that RedisArgument as is, should be enough (as you had originally)

so the issue here is really due to AGGREGATE_WITHCURSOR (where cursor id is returned to user). Changed it to be a NumberReply there. I think before I ran into some problems with tests, will have to redo and see what the problem was.

i.e. the problem is that AGGREGATE_WITHCURSOR when cursor is a NumberReply will default to returning as a number.

instead returning NumberReply (from WITHCURSOR) for cursor and taking an UnwrapReply for cursor in cursor read/del and its up to the user to therefore have a proper number conversion setup.

Honestly, not happy with it. I'd want to force the NumberReply to be treated as a string and not as a number in WITHCURSOR (and then can force a string in cursor functions)

"I'd want to force the NumberReply to be treated as a string and not as a number in WITHCURSOR (and then can force a string in cursor functions)" - I'd want the server to reply with a string and not a number..
The server not gonna change, we should probably support both.

so we have the NumberReply that can be both. With that said, I'd think we should document this (i.e. we strongly reccomend one overrides the type mapping for NumberReplies when using the WITHCURSOR function).

A better solution (which I dont know how to implement) would be to be able to tag commands with a default type mapping that can only override default (blank) client type mapping, but not type mapping that the user specifies.

i.e. by default we might say a NumberReply maps to a a js number, but for some commands we can say it maps to string, and the client would then just do the right thing by default always without user intervention required.

leibale · 2024-06-25T18:30:36Z

packages/search/lib/commands/CURSOR_READ.ts

@@ -8,14 +8,15 @@ export interface FtCursorReadOptions {
 export default {
  FIRST_KEY_INDEX: undefined,
  IS_READ_ONLY: true,
-  transformArguments(index: RedisArgument, cursor: RedisArgument, options?: FtCursorReadOptions) {
-    const args = ['FT.CURSOR', 'READ', index, cursor];
+  transformArguments(index: RedisArgument, cursor: number, options?: FtCursorReadOptions) {


same cursor comment

leibale · 2024-06-25T18:31:24Z

packages/search/lib/commands/EXPLAIN.ts

@@ -1,28 +1,28 @@
-// import { RedisArgument, SimpleStringReply, Command } from '@redis/client/dist/lib/RESP/types';
-// import { Params, pushParamsArgs } from ".";
+import { RedisArgument, SimpleStringReply, Command } from '@redis/client/dist/lib/RESP/types';


Missing tests

I'm confused. I see a spec.ts

unit tests only..

I am trying to add an execution test, but not sure why I'm seeing output that I am via a decoder

i.e. via telnet

FT.EXPLAIN index '*' $11 <WILDCARD>

via cli

127.0.0.1:6379> FT.EXPLAIN index '*' "<WILDCARD>\n" 127.0.0.1:6379>

but via node-redis have to do this to make it pass

assert.equal('<WILDCARD>}\n', reply);

@leibale thoughts?

leibale · 2024-06-25T18:33:45Z

packages/time-series/lib/commands/index.ts

+export interface SampleRawReply2 {
+  timestamp: NumberReply;
+  value: BlobStringReply;
 };


an object/map in resp2 from the server? no way.. TuplesReply<[timestamp: NumberReply, value: BlobStringReply]>; is right i think

copied from v4 badly I think. will do what you said.

leibale · 2024-06-25T18:34:18Z

packages/time-series/lib/commands/MGET.ts

@@ -18,14 +19,47 @@ export function pushFilterArgument(args: CommandArguments, filter: RedisVariadic
  return pushVariadicArguments(args, filter);
 }

+export type MGetRawReply2 = Array<[


use ArrayReply instead of Array

done and BlobStringReply instead of string and UnwrapReply in transformReply

though had some issues with typing in the transformReply, needs to be validated.

and same in RANGE/MRANGE/_WITHLABELS/

leibale · 2024-06-25T18:35:37Z

packages/time-series/lib/commands/MGET.ts

@@ -18,14 +19,47 @@ export function pushFilterArgument(args: CommandArguments, filter: RedisVariadic
  return pushVariadicArguments(args, filter);
 }

+export type MGetRawReply2 = Array<[
+  key: string,


BlobStringReply?

yes, it should be BlobStringReply, this was just a port over from v2, but it's a RedisModule_ReplyWithStringBuffer() so BlobString. Will fix.

addressed above

leibale · 2024-06-26T16:30:46Z

packages/time-series/lib/commands/index.ts

    };
  },
-  3(reply: SampleRawReply[3]) {
-    const [timestamp, value] = reply as unknown as UnwrapReply<typeof reply>;
+  3(reply: SampleRawReply3): SampleReply3 {


In RESP3 the reply raw reply type should be

type SampleRawReply3 = ArrayReply<TuplesReply<[timestamp: NumberReply, value: DoubleReply]>>;

(I checked that in TS.RANGE, I guess its true for the rest of them?)

so I'm confused about how tuplereply and array reply work. do they have to be used together?

i.e. in looking at it now i see for range, we basically just have a single element but the function is a an arrayreply wrapping the tuple reply. I'm confused overall.

in looking at it more, I dont see why you would make SampleRawReply3 an ArrayReply, I'd think both resp2/resp3 should just be TupleReply and then RANGE et al would have it wrapped in an ArrayReply as get an array of the tuples. In practice then, resp2/resp3 would be the exact same then (at least for range) and therefore the code could be simplified.

The type in my comments is for samples, but named sample, my bad..

This should work

type SampleRawReply = TuplesReply<[timestamp: NumberReply, value: DoubleReply]>; const transformSampleReply = { 2(reply: Resp2Reply<SampleRawReply>) { const [ timestamp, value ] = reply as unknown as UnwrapReply<typeof reply>; return { timestamp, value: Number(value) }; }, 3(reply: SampleRawReply) { const [ timestamp, value ] = reply as unknown as UnwrapReply<typeof reply>; return { timestamp, value }; } }; type SamplesRawReply = ArrayReply<SampleRawReply>; const transformSamplesReply = { 2(reply: Resp2Reply<SamplesRawReply>) { return (reply as unknown as UnwrapReply<typeof reply>) .map(sample => transformSampleReply[2](sample)); }, 3(reply: SamplesRawReply) { return (reply as unknown as UnwrapReply<typeof reply>) .map(sample => transformSampleReply[3](sample)); } };

tried implementing this, but cant do it right.

maybe got it now

didn't update tests, and got types wrong

didn't like the null prototype for deep equal

Change testing

1 - use redis-stack only 2 - protect hscan-novalues for older versions 3 - disable graph tests

…BELS|_SELECTED_LABELS]`

sjpotter requested a review from leibale May 26, 2024 11:09

leibale reviewed May 28, 2024

View reviewed changes

sjpotter force-pushed the v5-search-broken branch from 36ad751 to 2b6e99a Compare June 2, 2024 13:21

sjpotter added 7 commits June 3, 2024 17:39

HSCAN VALUES support (v5)

3892f92

enable module commands to be marked as unstable and require clients t…

54d2c0d

…o opt in to that.

update all search commands to v5 / unstable interface

5806f68

update architecture to how we throw an error.

0f2e8d5

revert changes needed for resp3 erroring with multi, as now handled b…

f8a300f

…y attachCommand

leibele's type changing and cleaning up disabling of type mapping

f13f47a

sjpotter force-pushed the v5-search-broken branch from 4fa8734 to f13f47a Compare June 3, 2024 14:39

sjpotter added 5 commits June 4, 2024 00:22

move tuples to map to requiring simple string for map key

5daf11b

adjust all usages of it to SimpleStringReply

missed on removal of unstable flag

a91dded

reove ignoreTypeMapping per offline discussion

9baec20

revert 2 buffer/simple string changes from experimentation

33adc6f

make resp2/resp3 have same return value

8e7f11c

leibale reviewed Jun 6, 2024

View reviewed changes

packages/bloom/lib/commands/top-k/INFO.ts Outdated Show resolved Hide resolved

cleanup

465911e

leibale reviewed Jun 25, 2024

View reviewed changes

packages/client/lib/RESP/types.ts Outdated Show resolved Hide resolved

leibale reviewed Jun 25, 2024

View reviewed changes

leibale and others added 2 commits June 25, 2024 14:37

nit

f08e94f

address leibele's comments + more time series stuff I found in process

6e60233

leibale reviewed Jun 26, 2024

View reviewed changes

sjpotter added 4 commits September 25, 2024 19:57

fix bloom info command update

52bd5c0

didn't update tests, and got types wrong

fix cms.info test

3d0485b

didn't like the null prototype for deep equal

rename unstableResp3SearchModule cofnig + xread/group support

9adac9d

revert to be v4 compat for xread/xreadgroup

ea51f39

sjpotter force-pushed the v5-search-broken branch from 96dcdba to ea51f39 Compare October 6, 2024 10:19

sjpotter and others added 12 commits October 7, 2024 17:44

make versions only use latest release

5202955

Merge branch 'v5-search-broken' into change-testing

b4b67f6

Merge pull request #1 from sjpotter/change-testing

58f88b5

Change testing

run tests against PRs against v5 branch

8ad4171

fixups for testing

d0396e7

1 - use redis-stack only 2 - protect hscan-novalues for older versions 3 - disable graph tests

fix running docker with redis-stack

faa67f3

attempt at fixing 1 github test issue

830d5db

fix hello test

fcdb1c0

fix TS.M[REV]RANGE[_WITHLABELS|_SELECTED_LABELS] & `TS.MGET[_WITHLA…

7ead8e4

…BELS|_SELECTED_LABELS]`

fix time series

68ca83b

fix non profile search commands

9e8c183

attempt to make ts info / info debug backwards cmmpatable

3f65936

sjpotter force-pushed the v5-search-broken branch from 6ef31a1 to 3f65936 Compare October 8, 2024 08:35

sjpotter added 11 commits October 8, 2024 12:41

update to handle 'warning' in profile data return

839e14b

attempt to make test more reliable

91ed69e

fix hello test to test if modules is an array

2d18b36

clean up rejection fix for clientkill

31d61b8

fix ts.info/debug again to hopefully be cleaner

25fdd32

small cleanups

59a27cc

more cleanup

bf11c35

transformTuplesReply can be a BlobStringReply or SimpleStringReply

1a5ac15

update ft.info to actually work + with newest options

16950d0

small tweak for stopWords

e1b935c

Merge remote-tracking branch 'upstream/v5' into v5-search-broken

1f346f8

sjpotter closed this Oct 28, 2024

module commands to be marked as unstable and require clients to opt in #2766

module commands to be marked as unstable and require clients to opt in #2766

Conversation

sjpotter commented May 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leibale Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leibale Jun 26, 2024 •

edited

Loading