danitseitlin / redis-modules-sdk-ts

A Software development kit for easier connection and execution of Redis Modules commands.
Apache License 2.0
185 stars 29 forks source link

redisearch.search returns different shapes when result count is above/below limit parameter #189

Closed RShohoney closed 2 years ago

RShohoney commented 2 years ago

When performing searches against redisearch, I'm seeing an inconsistency with the shapes of the return. In the case of no results, the response is just a number === 0, which I actually think should be [0] so as to follow the shape for when there are responses.

The bigger problem is as follows:

The corresponding request shape is this:

redisearch.search(
  indexName, 
  query,
  {
    scorer: 'BM25',
    withScores: true,
    limit: {
      first: 0,
      num: 50,
    }
  }
});

I tried messing with the parseSearchQueries option, and it seems to be somewhat related. It would change which responses would break with my handling, vs not.

It also seems possible that WITHSCORES is causing issues. It appears that this line makes assumptions about the length of each collection of strings in the response array being length 2, rather than accounting for additional parameters being present due to additional options being passed: https://github.com/danitseitlin/redis-modules-sdk-ts/blob/081eebcddbdc7550920603d6827b512011a201f4/modules/module.base.ts#L126

Let me know if you need more information, I'll provide all that I can.

danitseitlin commented 2 years ago

Can you add reproduction steps please? I need more than the search query, I also need the index created so I can reproduce

RShohoney commented 2 years ago

These tests on my fork should help show the issues I'm observing. https://github.com/RShohoney/redis-modules-sdk-ts/blob/rshohoney--unhandled-extra-return-properties/tests/redisearch.ts#L466

danitseitlin commented 2 years ago

Hi, I'm unavailable this week, will take a look in the next week.

RShohoney commented 2 years ago

As an aside, I think it might be a good idea to allow users to opt out of request parsing, and instead get back the raw response from RediSearch. What do you think?

danitseitlin commented 2 years ago

@RShohoney I agree, please open a separate issue for that request, I will get to it once once off vacation