redis / node-redis

Redis Node.js client
https://redis.js.org/
MIT License
16.95k stars 1.89k forks source link

4.1.0 publish breaks when publishing an integer #2116

Open anshul-kai opened 2 years ago

anshul-kai commented 2 years ago

This was working fine up until 4.0.6 but starting in version 4.1.0, publishing integers throws the following error.

/node_modules/@redis/client/dist/lib/client/RESP2/encoder.js:20
            throw new TypeError('Invalid argument type');
                  ^

TypeError: Invalid argument type
    at encodeCommand (/node_modules/@redis/client/dist/lib/client/RESP2/encoder.js:20:19)
    at RedisCommandsQueue.getCommandToSend (/node_modules/@redis/client/dist/lib/client/commands-queue.js:187:45)
    at Commander._RedisClient_tick (/node_modules/@redis/client/dist/lib/client/index.js:429:76)
    at Commander._RedisClient_sendCommand (/node_modules/@redis/client/dist/lib/client/index.js:413:82)
    at Commander.commandsExecutor (/node_modules/@redis/client/dist/lib/client/index.js:167:154)
    at Commander.BaseClass.<computed> [as publish] (/node_modules/@redis/client/dist/lib/commander.js:8:29)

Environment:

byron70 commented 2 years ago

Same environment as a-koka, except for Redis Server Version: 6 on AWS Elasticache.

The setEx command fail on this line if the value being sent is an array. This previously worked without conversion of the array to a string.

client.setEx('myKey', 300, [100,200])

The encoderCommand(), is receiving the ttl of 300 as '300' (a string), so there is some conversion happening in the middle of all that, but not for the value.

I guess in reality, my usage is really incorrect. I actually recall being surprised that the array was converted to a delimited list in the previous version.

leibale commented 2 years ago

@a-koka @byron70 in previous versions (4.0.0-4.0.6) some values were incorrectly converted into strings, which could cause unexpected behaviors (see #1898 for example). I forgot to add that to the release notes.. :man_facepalming:

anshul-kai commented 2 years ago

I'm not sure if I follow this thoroughly. Publishing integer values has worked for the longest time until 4.1.0. Are we saying that publishing integers is no longer supported?

redisPubClient.publish('channel', 1) used to work but now has to be written as redisPubClient.publish('channel', '1')

tugtugtug commented 2 years ago

@leibale this is definitely a regression, the addCommand, hSet are also now broken with integer/boolean values. If https://github.com/redis/node-redis/issues/1898 complains about converting undefined, null to empty strings, shouldn't that particular error be fixed instead of completely stopping the conversion of any non-buffer, non-string arguments?

patel-kalp commented 2 years ago
import * as redis from 'redis';
import session from 'express-session';
import connectRedis from 'connect-redis';

const RedisStore = connectRedis(session);
const redisClient = redis.createClient();
await redisClient.connect();

app.use(
        session({
            name: 'sessionId',
            store: new RedisStore({
                client: redisClient as any,
                disableTouch: true,
            }),
            cookie: {
                maxAge: 31536000000,
                sameSite: "lax",
                secure: false
            },
            saveUninitialized: false,
            secret: "test",
            resave: false
        })
)

I am not sure if I am doing this right, but something similar has worked for me before. So now should I be making maxAge a string? @leibale

moisesbites commented 2 years ago

In this version 4.1.0, Date values are not working anymore too . Before, they worked fine.

moisesbites commented 2 years ago

Hi, some solution for this issue?

musta20 commented 2 years ago

hi facing the same issue solution yet

younggeun0 commented 2 years ago

I got the same error. It works for me adding createClient option(legecyMode: true) and connect() together.

import connectRedis from "connect-redis";
...
    const redisClient = createClient({
        legacyMode: true,
    });
   await redisClient.connect();
moisesbites commented 2 years ago

I got the same error. It works for me adding createClient option(legecyMode: true) and connect() together.

import connectRedis from "connect-redis";
...
    const redisClient = createClient({
        legacyMode: true,
    });
   await redisClient.connect();

For me, this workaround it's not working... I use many resources from v4.

For while, I'm converting every value to string.

dragonlobster commented 2 years ago

please help my production is broken because of this update... other bugs also are introduced because of the update, i am reverting back to the working version

moisesbites commented 2 years ago

please help my production is broken because of this update

@dragonlobster for while, convert to string every value on or before command db.set... when you to execute command get, convert the returned string to correct datatype...

devel0perx commented 2 years ago

@a-koka i am using redis 7 and node 16 having same problem but it work sometime and give above error What to do ?

leibale commented 2 years ago

Make sure to convert the channel and message into strings or buffers

zelucena commented 2 years ago

I found the same issue while trying to upgrade Node ACL2 to Redis 4.3.0. sAdd stopped working with integers as well. Stack trace:

 Uncaught TypeError: Invalid argument type
      at encodeCommand (node_modules\@redis\client\dist\lib\client\RESP2\encoder.js:17:19)
      at RedisCommandsQueue.getCommandToSend (node_modules\@redis\client\dist\lib\client\commands-queue.js:187:45)
      at Commander._RedisClient_tick (node_modules\@redis\client\dist\lib\client\index.js:440:76)
      at Commander.multiExecutor (node_modules\@redis\client\dist\lib\client\index.js:246:86)
      at Commander.exec (node_modules\@redis\client\dist\lib\client\multi-command.js:84:175)
      at RedisBackend.end (lib\redis-backend.js:36:46)
      at RedisBackend.tryCatcher (node_modules\bluebird\js\release\util.js:16:23)
      at RedisBackend.ret [as endAsync] (eval at makeNodePromisifiedEval (node_modules\bluebird\js\release\promisify.js:184:12), <anonymous>:13:39)
      at Acl.allow (lib\acl.js:337:29)
      at C:\Users\jose.lucena\Downloads\node_acl-master\node_acl-master\lib\acl.js:766:59
      at tryCatcher (node_modules\bluebird\js\release\util.js:16:23)
      at Object.gotValue (node_modules\bluebird\js\release\reduce.js:168:18)
      at Object.gotAccum (node_modules\bluebird\js\release\reduce.js:155:25)
      at Object.tryCatcher (node_modules\bluebird\js\release\util.js:16:23)
      at Promise._settlePromiseFromHandler (node_modules\bluebird\js\release\promise.js:547:31)
      at Promise._settlePromise (node_modules\bluebird\js\release\promise.js:604:18)
      at Promise._settlePromise0 (node_modules\bluebird\js\release\promise.js:649:10)
      at Promise._settlePromises (node_modules\bluebird\js\release\promise.js:729:18)
      at Promise._fulfill (node_modules\bluebird\js\release\promise.js:673:18)
      at C:\Users\jose.lucena\Downloads\node_acl-master\node_acl-master\node_modules\bluebird\js\release\nodeback.js:42:21
      at RedisBackend.end (lib\redis-backend.js:37:13)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
moisesbites commented 2 years ago

I found the same issue while trying to upgrade Node ACL2 to Redis 4.3.0. sAdd stopped working with integers as well. Stack trace:

 Uncaught TypeError: Invalid argument type
      at encodeCommand (node_modules\@redis\client\dist\lib\client\RESP2\encoder.js:17:19)
      at RedisCommandsQueue.getCommandToSend (node_modules\@redis\client\dist\lib\client\commands-queue.js:187:45)
      at Commander._RedisClient_tick (node_modules\@redis\client\dist\lib\client\index.js:440:76)
      at Commander.multiExecutor (node_modules\@redis\client\dist\lib\client\index.js:246:86)
      at Commander.exec (node_modules\@redis\client\dist\lib\client\multi-command.js:84:175)
      at RedisBackend.end (lib\redis-backend.js:36:46)
      at RedisBackend.tryCatcher (node_modules\bluebird\js\release\util.js:16:23)
      at RedisBackend.ret [as endAsync] (eval at makeNodePromisifiedEval (node_modules\bluebird\js\release\promisify.js:184:12), <anonymous>:13:39)
      at Acl.allow (lib\acl.js:337:29)
      at C:\Users\jose.lucena\Downloads\node_acl-master\node_acl-master\lib\acl.js:766:59
      at tryCatcher (node_modules\bluebird\js\release\util.js:16:23)
      at Object.gotValue (node_modules\bluebird\js\release\reduce.js:168:18)
      at Object.gotAccum (node_modules\bluebird\js\release\reduce.js:155:25)
      at Object.tryCatcher (node_modules\bluebird\js\release\util.js:16:23)
      at Promise._settlePromiseFromHandler (node_modules\bluebird\js\release\promise.js:547:31)
      at Promise._settlePromise (node_modules\bluebird\js\release\promise.js:604:18)
      at Promise._settlePromise0 (node_modules\bluebird\js\release\promise.js:649:10)
      at Promise._settlePromises (node_modules\bluebird\js\release\promise.js:729:18)
      at Promise._fulfill (node_modules\bluebird\js\release\promise.js:673:18)
      at C:\Users\jose.lucena\Downloads\node_acl-master\node_acl-master\node_modules\bluebird\js\release\nodeback.js:42:21
      at RedisBackend.end (lib\redis-backend.js:37:13)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

Convert to string before send to redis client, reconvert to datatype after get from redis. It's the solution, for while.

DDDDDanica commented 2 years ago

I got the same error. It works for me adding createClient option(legecyMode: true) and connect() together.

import connectRedis from "connect-redis";
...
    const redisClient = createClient({
        legacyMode: true,
    });
   await redisClient.connect();

Thank you !!! and this should be the fix and connect-redis did not support redis@v4 completely.

https://github.com/tj/connect-redis

Known compatible and tested clients:

[redis](https://github.com/NodeRedis/node-redis) (v3, v4 with legacyMode: true)
[ioredis](https://github.com/luin/ioredis)
[redis-mock](https://github.com/yeahoffline/redis-mock) for testing.

It would be great if this is specified in migration guide ~

kasvith commented 1 year ago

This is a serious error :(