Add SIMPLE_STRING reply type to the Module API

[rjd15372]

This commit adds the SIMPLE_STRING reply type to the Module API, which is required for scripts to process correctly the reply type of commands called inside scripts.

Before this change, commands like PING or SET, which return "OK" as a simple string reply, would be returned as string reply to scripts. To allow the support of the Lua engine as an external module, we need to distinguish between simple string and string replies to keep backward compatibility.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.41%. Comparing base (7f8c5b6) to head (0481b6f).

Files with missing lines	Patch %	Lines
src/module.c	0.00%	14 Missing ⚠️
src/call_reply.c	0.00%	8 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2804      +/-   ##
============================================
+ Coverage     72.40%   72.41%   +0.01%     
============================================
  Files           128      128              
  Lines         70270    70280      +10     
============================================
+ Hits          50880    50895      +15     
+ Misses        19390    19385       -5     

Files with missing lines	Coverage Δ
src/call_reply.c	0.00% <0.00%> (ø)
src/module.c	9.78% <0.00%> (-0.01%)	⬇️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[zuiderkwast]

Good point. Yeah I suppose we need this in some form. See comment below.

[zuiderkwast]

We need this logic to keep backward compatibility with existing modules and with lua scripts?

• Most modules receive all string replies as bulk strings from VM_Call, including the PONG reply.
• Modules implementing a scripting engine need to be aware that if they use ValkeyModule_Call from a scripting engine callback (i.e. when a script is running) the +PONG reply is a simple string.
• Scripts that call module-implemented commands that in turn call ValkeyModule_Call, now the module will receive a simple string in this scenario? Let's MYMOD.PING is a module command that is just a proxy to the built-in PING command using ValkeyModule_Call("PING"). If it's called from a client as MYMOD.PING the module receives a bulk string PONG, but if it's called from a script like EVAL 'server.call("MYMOD.PING")' 0, the module receives the +PONG as a simple string, just because scriptIsRunning() is true in this case. It's not intended, right?

Do we need to keep track of which client is doing what here?

Should we instead do something like a module capability or flag to ValkeyModule_Call that enables simple string replies for the particular ValkeyModule_Call regardless of script is running?

[rjd15372]

You're right. We don't want the mymod.ping to behave differently when it's called within a script.

I think having a new flag for the ValkeyModule_Call is the way to go, and is more robust than relying solely on the scriptIsRunning() function.

[rjd15372]

I'm going to open a PR that introduces the new flag. And then after merging the flag PR, I update this PR to use the flag.

[zuiderkwast]

Yeah, good. Are there more reply type differences a module may want to be aware of? If we add them at the same time, we can use the same flag to enable all the more detailed reply types.

[rjd15372]

This was the only reply type missing for Lua to work as a module.

[zuiderkwast]

Ok, but more generically, if a module wants to simulate the behaviour of any existing reply, is there anything more missing here...

Looking at what we have for VM_ReplyWithXXX, the only thing I can see missing is the difference between "null array" and "null bulk string" that is present in RESP 2. A null array is encoded as an array with length -1 and a null string as a string with length -1.

/* Reply to the client with a null array, simply null in RESP3,
 * null array in RESP2.
 *
 * Note: In RESP3 there's no difference between Null reply and
 * NullArray reply, so to prevent ambiguity it's better to avoid
 * using this API and use ValkeyModule_ReplyWithNull instead.
 *
 * The function always returns VALKEYMODULE_OK. */
int VM_ReplyWithNullArray(ValkeyModuleCtx *ctx) {
    client *c = moduleGetReplyClient(ctx);
    if (c == NULL) return VALKEYMODULE_OK;
    addReplyNullArray(c);
    return VALKEYMODULE_OK;
}

[zuiderkwast]

Let's not care about different kinds of nulls. 😄

[rjd15372]

Opened #2818. Instead of a new VM_Call flag, I decided to add a new ValkeyModuleContext flag, since it's the context that is used all around.

[rjd15372]

This PR is currently rebased on top of #2818, therefore will only be merged after #2818 is merged.