Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FT.SEARCH: Cannot read properties of undefined (reading 'length') #2772

Open
nxps opened this issue Jun 4, 2024 · 9 comments · May be fixed by #2814
Open

FT.SEARCH: Cannot read properties of undefined (reading 'length') #2772

nxps opened this issue Jun 4, 2024 · 9 comments · May be fixed by #2814
Assignees
@sjpotter
Labels
Bug

Comments

@nxps
Copy link

nxps commented Jun 4, 2024

Description

Usage: client.ft.search(index, query, option)
Exception: Cannot read properties of undefined (reading 'length')
Error location: https://github.com/redis/node-redis/blob/master/packages/search/lib/commands/SEARCH.ts
while (i < tuples.length) {

The debug log shows that the reply value is [100, [] ]

export type SearchRawReply = Array<any>;

export function transformReply(reply: SearchRawReply, withoutDocuments: boolean): SearchReply {
    const documents = [];
    let i = 1;
    while (i < reply.length) {
        documents.push({
            id: reply[i++],
            value: withoutDocuments ? Object.create(null) : documentValue(reply[i++])
        });
    }

    return {
        total: reply[0],
        documents
    };
}

function documentValue(tuples: any) {
    const message = Object.create(null);

    let i = 0;
    while (i < tuples.length) {
        const key = tuples[i++],
            value = tuples[i++];
        if (key === '$') { // might be a JSON reply
            try {
                Object.assign(message, JSON.parse(value));
                continue;
            } catch {
                // set as a regular property if not a valid JSON
            }
        }

        message[key] = value;
    }

    return message;
}

Node.js Version

v18.19.1

Redis Server Version

6.2.6

Node Redis Version

4.6.14

Platform

macOS

Logs

"TypeError: Cannot read properties of undefined (reading 'length')
    at documentValue (/Users/my/test/node_modules/@redis/search/dist/commands/SEARCH.js:30:23)
    at Object.transformReply (/Users/my/test/node_modules/@redis/search/dist/commands/SEARCH.js:18:61)
    at transformCommandReply (/Users/my/test/node_modules/@redis/client/dist/lib/commander.js:90:20)
    at Commander.commandsExecutor (/Users/my/test/node_modules/@redis/client/dist/lib/client/index.js:190:54)
@nxps nxps added the Bug label Jun 4, 2024
@dmaier-redislabs
Copy link
Contributor

Hi,

sorry for the late reply. We will take a look and come back to you.

Regards,
David

@sjpotter
Copy link
Contributor

sjpotter commented Jul 2, 2024 •
edited
Loading

is it possible to have the raw result (i.e. a redis-cli output) so that we can see what's going on?

edit: also the arguments as used to ft.search() could be useful, as the only thing I can imagine happening is that we aren't getting back results in the expected order of

<total # of doc's in search>
<doc-id>
<array of key/value pairs>
<doc-id>
<array of key/value pairs>
....

if for example it's

<total # of docs in search>
<doc-id>
<doc-id>
<doc-id>
....

the error would make sense, if the code expected the first response (as its now trying to parse the "next" doc-id as an array, when it isn't, but this should only happen if one specifies a RETURN option of an empty array.

But I have no idea how it could get into that state, I've run through different permutations of commands and can't duplicate it, so more info is desired if possible.

@CaptainTux
Copy link

I am having the same issue, often when using "*" as a query

@sjpotter
Copy link
Contributor

sjpotter commented Aug 7, 2024

I am having the same issue, often when using "*" as a query

if you can duplicate the query via a raw redis-cli output, it be useful.

@CaptainTux
Copy link

I am having the same issue, often when using "*" as a query

if you can duplicate the query via a raw redis-cli output, it be useful.

In the CLI it works fine
and I also think it's because of expired keys which haven't been deleted yet
because without expiring keys I don't get the error.
I have a workaround for now which is not expiring the indexed keys, and creating a new key for expiration, listen to the event, and then delete the actual key, which seems to work but isn't pretty

@CaptainTux
Copy link

I did some research, it might be related to this issue: redis/redis#12992
so perhaps this library should do a null check and skip null entries

@sjpotter
Copy link
Contributor

sjpotter commented Aug 7, 2024 •
edited
Loading

this makes me think its a bug in the ft.search api with a expiration/eviction race that will be really ugly to directly fix in clients due to the buggy api (though I have an idea I'll mention), and has to be fixed in the product. will forward to them.

ex: ft.search does a search. it gets a number of documents ids back. so that's the first number in the array.

then it returns a single doc id, and then an array of key value pairs.

i'm guessing, that what it does is that it opens up the key associated with the doc-id and extract the key/value pairs then. however, what happens if that key is expired/evicted. this fails, and (my guess) is that it just goes onto to the next doc-id (though already returning this doc-id, but without any content).

however, the client expect an array to be there, which its not.

So if this were to be fixed in the client what would have to be done:

we need to look at i/i+1 (what should be a doc-id/key-array pair) together, if i+1 isn't an array, continue loop over returned array by just incrementing 1, but not returning anything to user. If it is an array, existing code will work as expected, so increment by 2 to move to next possible pair.

@leibale what do you think?

@leibale
Copy link
Collaborator

leibale commented Aug 7, 2024

@sjpotter
if this is the intended behavior - we should not change/"hide" it on the client side.. but I don't think it is since there is nothing the user can do with this null.. how does knowing there was a key that matched the query help the user?!

I think that you/we need to talk to the search team and see whats going on..

@sjpotter
Copy link
Contributor

sjpotter commented Aug 8, 2024

ok, this is documented on ft.search page, its not the next doc-id, it is still in pairs, but it returns a null instead of an array.

https://redis.io/docs/latest/commands/ft.search/

If a relevant key expires while a query is running, an attempt to load the key's value will return a null array. However, the key is still counted in the total number of results.

@sjpotter sjpotter linked a pull request Aug 8, 2024 that will close this issue
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sjpotter sjpotter

Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

understand that null responses can be returned for the key/value array sjpotter/node-redis
5 participants
@sjpotter @leibale @nxps @CaptainTux @dmaier-redislabs