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.

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

DOC-4345 added testable JSON index/query example #2864

Open
wants to merge 4 commits into
base: emb-examples
Choose a base branch
from

Conversation

andy-stark-redis
Copy link

@andy-stark-redis andy-stark-redis commented Nov 8, 2024

Description

DOC-4345

The other client library pages have an JSON index/query example so we should have one for Node.js. Also, these examples are currently just ordinary text in the client pages but I am adding them to the client repos to make them testable.

Describe your pull request here

Adds the example code in the emb-examples branch. I'll add the corresponding text to the doc page when this PR is merged.


Checklist

  • Is the new or changed code fully tested?

@andy-stark-redis andy-stark-redis marked this pull request as ready for review November 8, 2024 13:11
@andy-stark-redis andy-stark-redis self-assigned this Nov 8, 2024
// REMOVE_START
try {
await client.ft.dropIndex('idx:users', { DD: true });
} catch{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this is that dropIndex generally throws an error if you try to drop an index that doesn't exist (at least in the clients for other languages). It seemed to work OK without the try...catch locally, but I got an error from the CI when I made the PR. I assumed the try...catch was required and it fixed the error. If I'm doing this the wrong way or misinterpreting the build error then please let me know.

await client.ft.dropIndex('idx:users', { DD: true });
} catch{}

await client.del('user:1', 'user:2', 'user:3');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FT.DROPINDEX idx:users DD already delete the keys, why are we deleting them again?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Often, there isn't an existing index (hence the try...catch as I mentioned in the previous reply), so presumably the keys don't get deleted in this case. However, keys like user:1, etc, are common in docs and test code. If user:1 exists but isn't a JSON key then you'll get the "wrong kind of value" error. If you think I'm being overly cautious here then I'll remove the client.del here, but I have seen this kind of thing happen occasionally.


// STEP_START create_data
var user1 = {
name: "Paul John",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use ' instead of "
use const/let instead of var
add missing ;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found/replaced these throughout the file now.

// STEP_END

// STEP_START add_data
var user1Added = await client.json.set('user:1', '$', user1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no reason to make 3 round trips for 3 SET, you can do:

const [user1Reply, user2Reply, user3Reply] = await Promise.all([
  client.json.set('user:1', '$', user1),
  client.json.set('user:2', '$', user2),
  client.json.set('user:3', '$', user3)
]);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good tip, thanks.

// REMOVE_START
assert.strictEqual(3, citiesResult.total);

citiesResult.documents.sort((a, b)=> a.id < b.id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we sorting on the client side and not using RediSearch it self?!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just for the test code. For other client examples I've written, the order of the documents returned is not deterministic, so you need to sort before using the asserts, otherwise you get an occasionally flaky test. The reason I don't add the sort option to the command is simply that the other client examples don't use the sort option here - just keeping them all consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants