Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
There was a problem hiding this comment.
Pull request overview
Updates RediSearch index rotation in the om repositories to avoid fragile FT.INFO error-string parsing (which broke on Redis 8), by switching discovery to FT._LIST and centralizing the logic in a shared helper.
Changes:
- Replaced per-repository
CreateAndAliasIndexlogic inHashRepositoryandJSONRepositorywith a sharedcreateAndAliasIndexhelper. - Implemented version discovery via
FT._LIST, selecting the nextidx_vNbased on the max existing version and then dropping all prior versioned indexes. - Updated repositories to call the new helper and adjusted imports accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
om/indexes.go |
New shared helper implementing FT._LIST-based version selection, alias update, and cleanup of old versioned indexes. |
om/hash.go |
Switched HashRepository.CreateAndAliasIndex to use the shared helper. |
om/json.go |
Switched JSONRepository.CreateAndAliasIndex to use the shared helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| newIndex := idx + "_v1" | ||
| if len(currVers) > 0 { | ||
| newIndex = fmt.Sprintf("%s_v%d", idx, slices.Max(currVers)+1) | ||
| } | ||
|
|
||
| // Create the new index | ||
| if err := client.Do(ctx, cmdFn(createCmd(newIndex).Schema())).Error(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := client.Do(ctx, client.B().FtAliasupdate().Alias(idx).Index(newIndex).Build()).Error(); err != nil { | ||
| return fmt.Errorf("failed to update alias: %w", err) | ||
| } | ||
|
|
||
| for _, ver := range currVers { | ||
| currIdx := fmt.Sprintf("%s_v%d", idx, ver) | ||
| if err := client.Do(ctx, client.B().FtDropindex().Index(currIdx).Build()).Error(); err != nil { | ||
| return fmt.Errorf("failed to drop old index %q: %w", currIdx, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
This flow can't be exercised using the OM alone so I'm conflicted about whether it's worth testing.
|
Hi @yelly, could you also add tests for Redis 8? |
|
ping @yelly, could you also add tests for Redis 8? |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Hi sorry got pulled away from this for a bit. |
Fixes #969.
The current implementation fails tests when running against Redis 8 (presumably the version in which the error message from
FT.INFOchanged).Introduced a new implementation for
CreateAndAliasIndexwhich does the following:FT._LIST.The implementation is shared between Hash and JSON repositories to avoid duplication.
The new implementation passes the tests against Redis 8 and redis-stack.
Note
Medium Risk
Changes the RediSearch index migration/aliasing flow for both Hash and JSON repositories, including dropping all previously versioned indexes, which could impact deployments if index naming differs or drop semantics change. Risk is moderate and isolated to index management paths.
Overview
Fixes
CreateAndAliasIndexfor Redis 8 by replacing theFT.INFO/error-message-based alias detection with a shared implementation that usesFT._LISTto discover existing*_vNindexes.Both
HashRepositoryandJSONRepositorynow create the next versioned index,FT.ALIASUPDATEthe alias to it, and then drop all prior versioned indexes via the new helper inom/indexes.go.Reviewed by Cursor Bugbot for commit 1434867. Bugbot is set up for automated code reviews on this repo. Configure here.