From 3e40a2e319d05ba9a067c3d2c407878e45bf6097 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Wed, 12 Aug 2026 18:44:41 +0200 Subject: [PATCH 1/2] feat: let callers opt out of index creation with create_index=False Every extension checks whether its index exists while being constructed, and that check is `FT.INFO`. A credential assembled from `+@read +@write` is denied `FT.INFO` and `FT.CREATE` together -- neither command is in either category, and measured on Redis 8.0.6 through 8.8.1 the mapping is identical -- so such a role cannot construct `SemanticCache`, `MessageHistory`, `SemanticMessageHistory` or `SemanticRouter` at all, even against an index it can query perfectly well: RedisSearchError: Error while fetching llmcache index info: User has no permissions to run the 'FT.INFO' command There was no way to ask for less. `overwrite=False` is the *reason* the check runs -- `create()` calls `exists()` first and consults `overwrite` only afterwards -- `drop` is not a constructor parameter at all, and `overwrite=True` is strictly worse, since it proceeds to `FT.DROPINDEX`. `SearchIndex` exposes no lifecycle seam either, and each constructor calls `create()` inline with no hook to subclass around. `create_index=False` lets the caller state what RedisVL cannot ask: the index exists. It skips the existence check, the schema comparison against the live index, and creation, so the constructor issues no index command at all. It is rejected together with `overwrite=True`, which asks for the opposite. Deliberately not a runtime probe. `FT.SEARCH` would be permitted where `FT.INFO` is not, but its reply is identical for an index and for an alias pointing at one, so a probe would reinstate the `create(overwrite=True, drop=True)` -> `FT.DROPINDEX DD` data-loss path that #672 closed. A credential that cannot ask whether the index exists cannot create one either, so there is nothing to work out at runtime -- `exists()` keeps failing loudly instead. ## The invariant this had to fix first Constructor-time `create()` was the de-facto eager connect. `SearchIndex.client` returns the raw client and is `None` until the lazy `_redis_client` property runs, so skipping `create()` left ten `self._index.client` sites in `redisvl/extensions/` dereferencing `None` -- starting with the router's `route_config` write, which runs immediately after index setup. All ten now use `_redis_client`. Two of the `# type: ignore` comments they carried turned out to cover a real `scan_by_pattern` signature mismatch rather than the Optional, and are kept with explicit codes. ## Router semantics `create_index=False` means the index exists, is already seeded, and is not ours to rewrite, so the router also skips writing route references and the stored `route_config`. Rewriting that blob from an unverified local route list would truncate a shared router's routes, and `JSON.SET` is `@write`, so a restricted credential can do it. `_update_router_state()` stays armed: `add_route()` and `remove_route()` are the caller acting deliberately -- but that consequence is now stated in `add_route()`'s own docstring and in the constructor's, not only in the guide, since the docstring is the reference for anyone who never opens it. `SemanticRouter.from_existing()` now threads the flag through. It reads the stored config with `JSON.GET` and reaches `FT.INFO` only via the constructor, so with `create_index=False` it issues no index command and becomes the way to attach to a router under a restricted credential. The flag is popped before `_split_from_existing_kwargs`, which retains only `SearchIndex` init kwargs and would otherwise pass it to the Redis client constructor -- where `SearchIndex.__init__` discards unknown kwargs silently, making the mistake invisible. Separately, `routes=[]` now raises a useful error when matching instead of `max() arg is an empty sequence`. That is unconditional: emptiness is legal on either path, and a flag about index ownership should not decide it. ## Tests - `tests/unit/test_extension_create_index_flag.py` -- 17 cases. The contract is "no index command at all", so they assert on the client: a `MagicMock` records every call, and `ft()` is the gate every `FT.*` command passes through. All four constructors reach zero recorded calls. Two cases pin the lazy-connect invariant above by driving `drop(id=...)` and `get_route_references()` as the first operation -- reverting any of the ten conversions otherwise breaks no test. Others pin that the flag survives as instance state, that it never reaches the router's stored config, and that `from_existing()` still verifies by default. The default construction path is not re-tested here; the existing 189 integration tests already fail if `create()` is dropped. - One integration test round-trips `store()`/`check()` through a cache built with `create_index=False` under a real `+@read +@write -@dangerous` ACL user -- the customer's rule, so the destructive commands it denies stay denied -- with the premise pinned (`FT.INFO` must raise `NoPermissionError` for that user) and the negative alongside it: without the flag the same credential raises `RedisSearchError` naming `ft.info`, with `NoPermissionError` chained. ## Docs `docs/user_guide/installation.md`'s ACL section is restructured. Four statements were falsified by this change or were already wrong: that a credential needs `@search` at all, that all four extensions always call `create()`, that enumeration is the only thing an `-@admin` rule breaks, and the advice to grant `FT.CREATE`. The operation table gains a `+@read +@write` column, the command-to-category mapping is labelled as measured rather than documented, and the wrapped error text appears verbatim under its own heading -- an H3, so it has an anchor to link to. The pre-existing key-permission material was rewritten as its own section rather than dropped, and corrected while there: partial key-pattern overlap is denied exactly like no overlap, not filtered down to the readable subset, and `FT.CREATE` is not key-checked at all, so a credential can create an index it cannot query. A new subsection covers what the flag gives up. An absent index fails loudly, and a vector dimension mismatch does too -- but only once the index holds a document, which a freshly provisioned index will not. A wrong prefix, an `ON JSON` index written as hashes, and a differing datatype or distance metric are silent. The tell is `FT.INFO`'s `key_type`, `prefixes` and `attributes`, not `hash_indexing_failures`, which stays `0` because those keys were never indexing candidates. Router provisioning gets its own subsection, since preparing one for this mode needs embedded reference vectors rather than a hand-written `FT.CREATE`. Two corrections worth calling out: `clear()` is not uniform -- only `SemanticCache.clear()` avoids `FT.INFO`, while the other three delegate to `SearchIndex.clear()`, which calls `info()` first -- and Redis Cloud's predefined Read-Write rule reads as `@read`/`@write`-shaped from its published description, so it is a candidate for this problem rather than immune to it. `docs/api/exceptions.rst` gains one cross-reference: when the credential genuinely cannot run `FT.INFO`, the permission error is not something to handle. ## Not in scope `create_index=False` restrains construction only; `delete()` and `clear()` stay armed. Coupling ownership to the flag is the coherent next step, and the flag is stored as instance state so it can be added without another parameter. `adk_redis` needs a matching field on `RedisVLCacheProviderConfig` before this reaches callers who construct through that provider. --- docs/api/exceptions.rst | 4 + docs/user_guide/installation.md | 104 +++++++-- redisvl/extensions/cache/llm/semantic.py | 91 ++++++-- redisvl/extensions/constants.py | 7 + .../message_history/message_history.py | 25 ++- .../message_history/semantic_history.py | 47 +++- redisvl/extensions/router/semantic.py | 90 +++++++- tests/integration/test_llmcache.py | 65 +++++- .../unit/test_extension_create_index_flag.py | 208 ++++++++++++++++++ 9 files changed, 582 insertions(+), 59 deletions(-) create mode 100644 tests/unit/test_extension_create_index_flag.py diff --git a/docs/api/exceptions.rst b/docs/api/exceptions.rst index 63f2a4e90..8fb5fb53d 100644 --- a/docs/api/exceptions.rst +++ b/docs/api/exceptions.rst @@ -158,6 +158,10 @@ there raises ``redis.exceptions.NoPermissionError`` itself rather than a wrapped :class:`RedisSearchError`. See :doc:`/user_guide/installation` for the ACL categories RedisVL needs. +When the credential genuinely cannot run ``FT.INFO``, this error is not something to +handle: construct the extension with ``create_index=False`` instead, which skips the +existence check entirely. See :doc:`/user_guide/installation`. + Telling "the index is missing" apart from other failures -------------------------------------------------------- diff --git a/docs/user_guide/installation.md b/docs/user_guide/installation.md index 8283afad6..74fef50d7 100644 --- a/docs/user_guide/installation.md +++ b/docs/user_guide/installation.md @@ -186,30 +186,102 @@ The Sentinel URL format supports: ## Redis permissions (ACLs) -RedisVL works through Redis Search commands, so a connecting credential needs the `@search` ACL category, or the individual `FT.*` commands. Reading an index additionally requires key permissions covering its prefix: the [ACL documentation](https://redis.io/docs/latest/operate/oss_and_stack/management/security/acl/#command-categories) describes this rule for creating, modifying, and reading an index, and in practice `FT.INFO`, `FT.SEARCH`, and `FT.AGGREGATE` are denied when the index prefix falls outside the allowed key patterns. `FT.CREATE` is not checked this way, so a credential can create an index it is then unable to read. +RedisVL reaches Redis through Redis Search commands, but not all of them need the `@search` category. Querying and loading work under an ordinary `+@read +@write` role; it is the commands that inspect or manage an index — `FT.INFO`, `FT.CREATE`, `FT._LIST` — that need `@search` or an explicit grant. -One command needs more than `@search`. Redis tags `FT._LIST` as `@admin` as well as `@search` and `@slow`, and ACL rules are applied left to right — so a rule that grants search access and then takes back administrative commands, such as `+@search -@admin` or `+@all -@admin`, denies it: +The command-to-category mapping below was measured against live servers rather than quoted from published documentation, which does not list ACL categories per `FT.*` command. It was identical on Redis 8.0.6, 8.2.7, 8.4.5, 8.6.4, 8.8.0 and 8.8.1. Check your own deployment with `COMMAND INFO ft.info`, `ACL CAT search`, or `ACL DRYRUN FT.INFO `. + +### What each operation needs + +| Operation | Redis command | `+@all -@admin` | `+@read +@write` | +|---|---|---|---| +| `index.query()`, `index.search()`, `index.aggregate()` | `FT.SEARCH`, `FT.AGGREGATE` | Yes | Yes | +| `index.load()` | `HSET` or `JSON.SET` (needs key access) | Yes | Yes | +| `index.exists()`, `index.info()`, `index.clear()`, `SearchIndex.from_existing()`, `rvl index info`, `rvl stats` | `FT.INFO` | Yes | **No** | +| `index.create()` | `FT.CREATE` | Yes | **No** | +| `index.delete()`, `rvl index delete`, `rvl index destroy` | `FT.DROPINDEX` | Yes | Yes | +| Enumerating indexes (see below) | `FT._LIST` | **No** | **No** | + +Every `Yes` above assumes key patterns that cover the index prefix — see [Key permissions](#key-permissions) — and `-@dangerous` layered on the second column additionally denies `FT.DROPINDEX`, so `index.delete()` becomes `No`. An SVS-VAMANA schema needs more than `FT.CREATE`: `index.create()` first probes capabilities with `INFO` (`@slow @dangerous`) and `MODULE LIST` (`@admin @slow @dangerous`), so both `-@admin` and `-@dangerous` policies break creation for those schemas. + +Two of the rows above deserve their own explanation. + +`FT._LIST` is tagged `@admin` as well as `@search` and `@slow`, and ACL rules are applied left to right — so a rule that grants search access and then takes back administrative commands, such as `+@search -@admin` or `+@all -@admin`, denies it: ```text User has no permissions to run the 'FT._LIST' command ``` -Note that `FT._LIST` does not *require* `@admin`: granting `+@search` on its own permits it. Only rules that subtract `@admin` after granting search are affected. To keep such a policy and still enumerate indexes, grant the command back explicitly with `+ft._list`. +`FT._LIST` does not *require* `@admin`: granting `+@search` on its own permits it. Only rules that subtract `@admin` after granting search are affected. To keep such a policy and still enumerate indexes, grant the command back explicitly with `+ft._list`. Enumeration is reached by `SearchIndex.listall()` and `AsyncSearchIndex.listall()`, by `rvl index listall`, and by the migration entry points that discover indexes for you: `rvl migrate helper`, `rvl migrate wizard` when no `-i/--index` is given, and `rvl migrate batch-plan --pattern`. + +`FT.DROPINDEX` is tagged `@dangerous` and `@write` as well as `@search`, so a policy that subtracts `@dangerous` denies `index.delete()`, `rvl index delete`, and `rvl index destroy`. + +### Roles built from `@read` and `@write` + +`FT.INFO` is in neither `@read` nor `@write` — its only category is `@search` — and `FT.CREATE` is the same. So an application role assembled from `+@read +@write` — a natural least-privilege shape — can query and load, but cannot ask whether an index exists and cannot create one. Note that subtracting `@dangerous` is not what causes this: `+@all -@dangerous` permits both. The commands are simply never granted. + +Every extension constructor checks whether its index exists, so under such a credential all of them fail while being constructed: + +```text +RedisSearchError: Error while fetching llmcache index info: +User has no permissions to run the 'FT.INFO' command +``` + +### "no permissions to run the 'FT.INFO' command" + +RedisVL does not guess its way around this. A credential that cannot ask whether the index exists also cannot create one, so there is nothing useful to infer — instead, tell RedisVL that the index is already there: + +```python +cache = SemanticCache( + name="llmcache", + redis_url="redis://localhost:6379", + create_index=False, +) +``` + +`create_index=False` is available on `SemanticCache`, `MessageHistory`, `SemanticMessageHistory` and `SemanticRouter`. It skips the existence check, the comparison of your schema against the live index, and index creation — the constructor issues no index command at all. Pass it when the index is managed externally, or when the credential cannot run `FT.INFO`. It cannot be combined with `overwrite=True`, which asks for the opposite. + +A `SearchIndex` used directly needs nothing special: build it with `from_dict()` or `from_yaml()`, then load and query. Two of its methods stay unavailable, because both read index metadata: `from_existing()`, which reconstructs a schema out of Redis, and `clear()`, which starts by calling `info()`. + +The flag also skips the SVS-VAMANA capability probe described above, since that runs inside `create()`. + +### Provisioning a router + +`SemanticRouter` with `create_index=False` writes nothing at all: not the reference vectors for its routes, and not the stored route config that `SemanticRouter.from_existing()` reads. Preparing a router for this mode therefore means constructing it once with a privileged credential — a hand-written `FT.CREATE` is not enough, because the reference vectors have to be embedded and written too. Without them the router matches nothing, which looks like a distance-threshold problem rather than an empty index. + +Afterwards, `SemanticRouter.from_existing(name, create_index=False)` is the way to attach to it: it recovers the routes and thresholds with `JSON.GET` and needs no `FT.INFO`. Pass the full route set. Each route's distance threshold is applied from the local list, so a partial set silently narrows matching — and `add_route()` and `remove_route()` rewrite the stored config from that same list, so attaching with a subset and then adding a route drops the rest from the config every other client reads. -| Operation | Redis command | Permitted by `+@all -@admin` | -|---|---|---| -| `index.create()`, `index.exists()` | `FT.CREATE`, `FT.INFO` | Yes | -| `index.query()`, `index.search()`, `index.aggregate()` | `FT.SEARCH`, `FT.AGGREGATE` | Yes | -| `index.info()`, `rvl index info`, `rvl stats` | `FT.INFO` | Yes | -| `index.load()` | `HSET` or `JSON.SET` (needs `@write` and key access) | Yes | -| `index.delete()`, `rvl index delete`, `rvl index destroy` | `FT.DROPINDEX` | Yes | -| Enumerating indexes (see below) | `FT._LIST` | No | +### When the schema diverges -`SemanticCache`, `SemanticMessageHistory`, `MessageHistory`, and `SemanticRouter` each call `index.create()` while being constructed, so they are covered by the first row. +With `create_index=False` nothing verifies that the live index matches the schema you described. Some mismatches are loud on first use, and two are silent: -Index enumeration is the only thing an `-@admin` rule breaks. It is reached by `SearchIndex.listall()` and `AsyncSearchIndex.listall()`, by `rvl index listall`, and by the migration entry points that discover indexes for you: `rvl migrate helper`, `rvl migrate wizard` when no `-i/--index` is given, and `rvl migrate batch-plan --pattern`. +| Mismatch | What happens | +|---|---| +| The index does not exist | `RedisSearchError` on the first query, naming the missing index | +| Vector dimensions disagree | `Error parsing vector similarity query: query vector blob size (32) does not match index's expected size (16)` — but only once the index holds a document. On an empty index the same query returns nothing, so a freshly provisioned index hides this until the first write lands | +| The prefix does not cover your keys | **Silent.** Documents are written but never indexed, so queries return nothing, forever | +| The index is `ON JSON` and you write hashes (or the reverse) | **Silent**, the same way | +| The datatype or distance metric differs | **Silent.** Neither is restated by a query, so nothing compares them — results come back ranked by the index's metric, not yours | -Other categories gate different operations. `FT.DROPINDEX` is tagged `@dangerous` and `@write` as well as `@search`, so a policy that subtracts `@dangerous` denies `index.delete()`, `rvl index delete`, and `rvl index destroy`. +For the silent cases the tell is `FT.INFO`'s `key_type`, `prefixes` and `attributes` — not `hash_indexing_failures`, which stays `0` because those keys were never indexing candidates. Diagnosing it therefore needs a credential that can run `FT.INFO`. + +`create_index=False` restrains construction only, so `delete()` and `clear()` are as destructive as ever. Note that `clear()` differs across the extensions: `SemanticCache.clear()` is a `SCAN` plus `DEL` and works under any role that can write, while `MessageHistory`, `SemanticMessageHistory` and `SemanticRouter` delegate to `SearchIndex.clear()`, which calls `info()` first and therefore needs `FT.INFO`. + +### Key permissions + +Command categories are only half of it. Redis also scopes the search commands by key pattern: [the ACL documentation](https://redis.io/docs/latest/operate/oss_and_stack/management/security/acl/#command-categories) states that only users with access to a *superset* of the prefixes defined at index creation can create, modify, or read an index. + +Measured on 8.4.5 against an index prefixed `doc:`, with the command categories held constant at `+@all`: + +| Key patterns | `FT.SEARCH`, `FT.INFO`, `FT.AGGREGATE` | +|---|---| +| `~doc:*` (superset) | Permitted | +| `%R~doc:*` (read permission only) | Permitted | +| `~doc:1` (partial overlap) | `NOPERM User does not have the required permissions to query the index` | +| `~other:*` (no overlap) | The same denial | + +Partial overlap is worth emphasising: it fails exactly like no overlap at all, rather than returning the subset you can read. `FT.CREATE` is not checked this way, so a credential can create an index it is then unable to query. + +`create_index=False` does not help here — the very commands it lets you avoid are joined by the ones it cannot, so widen the key patterns instead. Outside of Redis Search, RedisVL identifies itself on connect with `CLIENT SETINFO`. That command is tagged `@connection` and `@slow`, and belongs to neither `@read` nor `@write`, so a rule built up from those categories never grants it. A credential that cannot run it still connects: identification only populates the `lib-name` field that `CLIENT LIST` and `CLIENT INFO` display, so a refusal is ignored (and logged, if you have configured logging at debug level). Grant `+client|setinfo` if you want RedisVL to appear as the connecting library there — note that this labels the connection RedisVL opens, while redis-py labels the rest of the pool as plain `redis-py`. @@ -222,4 +294,6 @@ Both manage ACLs through their own control plane rather than the `ACL SETUSER` c - **Redis Cloud** provides three predefined ACL rules that cannot be edited — Full-Access, Read-Write ("read and write commands and excludes dangerous commands"), and Read-Only — which you assign to a data access role. See [Configure permissions with Redis ACLs](https://redis.io/docs/latest/operate/rc/security/access-control/data-access-control/configure-acls/). Custom rules use the same syntax as above. - **Redis Software** ships one predefined ACL, Full Access, and you define others in the Cluster Manager UI or with a [`POST /v1/redis_acls`](https://redis.io/docs/latest/operate/rs/security/access-control/create-db-roles/) request. It [does not support every `ACL` command](https://redis.io/docs/latest/operate/rs/security/access-control/redis-acl-overview/#acl-command-support), nor nested selectors, nor `(` and `)` in key patterns. -Because the predefined rules' exact command sets are not published, confirm a credential against the database rather than inferring what its policy name implies. Redis Software's documentation uses `+@read +FT.INFO +FT.SEARCH` as an example rule, which is a good illustration: it permits querying and `index.exists()`, but not `index.create()` or index enumeration. Grant `FT.CREATE` explicitly when the application creates its own index, which `SemanticCache`, `SemanticMessageHistory`, `MessageHistory`, and `SemanticRouter` all do. +Because the predefined rules' exact command sets are not published, confirm a credential against the database rather than inferring what its policy name implies — `ACL DRYRUN FT.INFO ` answers it directly. Read the descriptions carefully before assuming you are unaffected: Read-Only allows read commands, and Read-Write "allows read and write commands and excludes dangerous commands", so both read as `@read`/`@write`-shaped — the shape that denies `FT.INFO` and `FT.CREATE` and wants `create_index=False`. Only Full-Access is clearly unaffected. + +Redis Software's documentation uses `+@read +FT.INFO +FT.SEARCH` as an example rule, which is a good illustration: it permits querying and `index.exists()`, but not `index.create()` or index enumeration. Grant `FT.CREATE` explicitly when the application creates its own index. When the index is provisioned for the application instead, leave it out and construct with `create_index=False`. diff --git a/redisvl/extensions/cache/llm/semantic.py b/redisvl/extensions/cache/llm/semantic.py index 661beb0d9..b0e5ab321 100644 --- a/redisvl/extensions/cache/llm/semantic.py +++ b/redisvl/extensions/cache/llm/semantic.py @@ -12,6 +12,7 @@ ) from redisvl.extensions.constants import ( CACHE_VECTOR_FIELD_NAME, + CREATE_INDEX_OVERWRITE_CONFLICT, ENTRY_ID_FIELD_NAME, INSERTED_AT_FIELD_NAME, METADATA_FIELD_NAME, @@ -36,6 +37,18 @@ logger = get_logger("[RedisVL]") +# Emitted when the caller did not choose a vectorizer and the index predates this +# call, so its vectors may have been written with the old default model. Remove +# this warning in future releases. +DEFAULT_VECTORIZER_CHANGED_WARNING = ( + "The default vectorizer has changed from `sentence-transformers/all-mpnet-base-v2` " + "to `redis/langcache-embed-v1` in version 0.6.0 of RedisVL. " + "For more information about this model, please refer to https://arxiv.org/abs/2504.02268 " + "or visit https://huggingface.co/redis/langcache-embed-v1. " + "To continue using the old vectorizer, please specify it explicitly in the constructor as: " + "vectorizer=HFTextVectorizer(model='sentence-transformers/all-mpnet-base-v2')" +) + class SemanticCache(BaseLLMCache): """Semantic Cache for Large Language Models.""" @@ -55,6 +68,7 @@ def __init__( redis_url: str = "redis://localhost:6379", connection_kwargs: dict[str, Any] = {}, overwrite: bool = False, + create_index: bool = True, **kwargs, ): """Semantic Cache for Large Language Models. @@ -78,13 +92,43 @@ def __init__( for the redis client. Defaults to empty {}. overwrite (bool): Whether or not to force overwrite the schema for the semantic cache index. Defaults to false. + create_index (bool): Whether RedisVL creates and validates the index. + When True, the constructor runs ``FT.INFO`` to check whether the + index exists, compares the live schema against this one, and runs + ``FT.CREATE`` if it is absent. When False it does none of these + and issues no index command at all: the index must already exist + with a compatible schema. A live index whose prefix or storage + type differs from this schema is not detected and produces empty + results rather than an error. Use this when the index is managed + externally, or when the credential cannot run ``FT.INFO``. See + :doc:`/user_guide/installation` for the ACL details. Defaults to + true. Raises: TypeError: If an invalid vectorizer is provided. TypeError: If the TTL value is not an int. ValueError: If the threshold is not between 0 and 2 (Redis COSINE distance). ValueError: If existing schema does not match new schema and overwrite is False. + ValueError: If both create_index is False and overwrite is True. + + .. code-block:: python + + from redisvl.extensions.cache.llm import SemanticCache + + # RedisVL creates the index if it is missing + cache = SemanticCache(name="llmcache", redis_url="redis://localhost:6379") + + # the index is managed externally, or this credential cannot run + # FT.INFO -- assume the index exists and issue no index command + cache = SemanticCache( + name="llmcache", + redis_url="redis://localhost:6379", + create_index=False, + ) """ + if not create_index and overwrite: + raise ValueError(CREATE_INDEX_OVERWRITE_CONFLICT) + # Call parent class with all shared parameters super().__init__( name=name, @@ -148,32 +192,35 @@ def __init__( # Check for existing cache index and handle schema mismatch self.overwrite = overwrite - if not self.overwrite and self._index.exists(): + self._create_index = create_index - if not vectorizer: - # user hasn't specified a vectorizer and an index already exists they're not overwriting - # raise a warning to inform users we changed the default embedding model - # remove this warning in future releases - logger.warning( - "The default vectorizer has changed from `sentence-transformers/all-mpnet-base-v2` " - "to `redis/langcache-embed-v1` in version 0.6.0 of RedisVL. " - "For more information about this model, please refer to https://arxiv.org/abs/2504.02268 " - "or visit https://huggingface.co/redis/langcache-embed-v1. " - "To continue using the old vectorizer, please specify it explicitly in the constructor as: " - "vectorizer=HFTextVectorizer(model='sentence-transformers/all-mpnet-base-v2')" - ) + if create_index: + if not self.overwrite and self._index.exists(): + if not vectorizer: + logger.warning(DEFAULT_VECTORIZER_CHANGED_WARNING) - existing_index = SearchIndex.from_existing( - name, redis_client=self._index._redis_client - ) - if existing_index.schema.to_dict() != self._index.schema.to_dict(): - raise ValueError( - f"Existing index {name} schema does not match the user provided schema for the semantic cache. " - "If you wish to overwrite the index schema, set overwrite=True during initialization." + existing_index = SearchIndex.from_existing( + name, redis_client=self._index._redis_client ) + if existing_index.schema.to_dict() != self._index.schema.to_dict(): + raise ValueError( + f"Existing index {name} schema does not match the user provided schema for the semantic cache. " + "If you wish to overwrite the index schema, set overwrite=True during initialization." + ) - # Create the search index in Redis - self._index.create(overwrite=self.overwrite, drop=False) + # Create the search index in Redis + self._index.create(overwrite=self.overwrite, drop=False) + else: + # The flag asserts the index already exists, which is exactly this + # warning's precondition -- so it still applies here. + if not vectorizer: + logger.warning(DEFAULT_VECTORIZER_CHANGED_WARNING) + + logger.debug( + f"create_index=False: assuming index {name!r} exists over prefix " + f"{schema.index.prefix!r} with {self._vectorizer.dims} vector " + "dimensions. Its schema is not verified." + ) def __repr__(self) -> str: return ( diff --git a/redisvl/extensions/constants.py b/redisvl/extensions/constants.py index cb58c98c9..bf2555e8b 100644 --- a/redisvl/extensions/constants.py +++ b/redisvl/extensions/constants.py @@ -33,3 +33,10 @@ # SemanticRouter ROUTE_VECTOR_FIELD_NAME: str = "vector" + +# Raised by every constructor that accepts both `create_index` and `overwrite`. +CREATE_INDEX_OVERWRITE_CONFLICT: str = ( + "create_index=False and overwrite=True contradict each other: overwrite asks " + "RedisVL to drop and recreate the index, which it cannot do when it is told " + "not to manage the index at all." +) diff --git a/redisvl/extensions/message_history/message_history.py b/redisvl/extensions/message_history/message_history.py index efbd64f74..c684d5a27 100644 --- a/redisvl/extensions/message_history/message_history.py +++ b/redisvl/extensions/message_history/message_history.py @@ -16,8 +16,11 @@ from redisvl.index import SearchIndex from redisvl.query import CountQuery, FilterQuery from redisvl.query.filter import Tag +from redisvl.utils.log import get_logger from redisvl.utils.utils import serialize +logger = get_logger(__name__) + class MessageHistory(BaseMessageHistory): @@ -29,6 +32,7 @@ def __init__( redis_client: Redis | None = None, redis_url: str = "redis://localhost:6379", connection_kwargs: dict[str, Any] = {}, + create_index: bool = True, **kwargs, ): """Initialize message history @@ -49,6 +53,16 @@ def __init__( redis_url (str, optional): The redis url. Defaults to redis://localhost:6379. connection_kwargs (Dict[str, Any]): The connection arguments for the redis client. Defaults to empty {}. + create_index (bool): Whether RedisVL creates the index. When False + the constructor issues no index command at all: the index must + already exist over this name and prefix. This class never + validates an existing index's schema, so nothing further is + verified either way -- and as elsewhere, a live index whose + prefix or storage type differs from this one is not detected and + produces empty results rather than an error. See + :class:`~redisvl.extensions.cache.llm.SemanticCache` for a worked + example, and :doc:`/user_guide/installation` for the ACL details. + Defaults to True. """ super().__init__(name, session_tag) @@ -64,7 +78,14 @@ def __init__( connection_kwargs=connection_kwargs or None, ) - self._index.create(overwrite=False) + self._create_index = create_index + if create_index: + self._index.create(overwrite=False) + else: + logger.debug( + f"create_index=False: assuming index {name!r} exists over prefix " + f"{prefix!r}." + ) self._default_session_filter = Tag(SESSION_FIELD_NAME) == self._session_tag @@ -89,7 +110,7 @@ def drop(self, id: str | None = None) -> None: if id is None: id = self.get_recent(top_k=1, raw=True)[0][ID_FIELD_NAME] # type: ignore - self._index.client.delete(self._index.key(id)) # type: ignore + self._index._redis_client.delete(self._index.key(id)) def count(self, session_tag=None): query = CountQuery( diff --git a/redisvl/extensions/message_history/semantic_history.py b/redisvl/extensions/message_history/semantic_history.py index 8664da43a..c3364b4e0 100644 --- a/redisvl/extensions/message_history/semantic_history.py +++ b/redisvl/extensions/message_history/semantic_history.py @@ -4,6 +4,7 @@ from redisvl.extensions.constants import ( CONTENT_FIELD_NAME, + CREATE_INDEX_OVERWRITE_CONFLICT, ID_FIELD_NAME, MESSAGE_VECTOR_FIELD_NAME, METADATA_FIELD_NAME, @@ -20,9 +21,12 @@ from redisvl.index import SearchIndex from redisvl.query import CountQuery, FilterQuery, RangeQuery from redisvl.query.filter import Tag +from redisvl.utils.log import get_logger from redisvl.utils.utils import deprecated_argument, serialize, validate_vector_dims from redisvl.utils.vectorize import BaseVectorizer, HFTextVectorizer +logger = get_logger(__name__) + class SemanticMessageHistory(BaseMessageHistory): @@ -38,6 +42,7 @@ def __init__( redis_url: str = "redis://localhost:6379", connection_kwargs: dict[str, Any] = {}, overwrite: bool = False, + create_index: bool = True, **kwargs, ): """Initialize message history with index @@ -63,10 +68,24 @@ def __init__( for the redis client. Defaults to empty {}. overwrite (bool): Whether or not to force overwrite the schema for the semantic message index. Defaults to false. + create_index (bool): Whether RedisVL creates and validates the index. + When False the constructor issues no index command at all: the + index must already exist with a compatible schema, and a live + index whose prefix or storage type differs is not detected -- + which produces empty results rather than an error. See + :class:`~redisvl.extensions.cache.llm.SemanticCache` for a worked + example, and :doc:`/user_guide/installation` for the ACL details. + Defaults to True. + + Raises: + ValueError: If both create_index is False and overwrite is True. The proposed schema will support a single vector embedding constructed from either the prompt or response in a single string. """ + if not create_index and overwrite: + raise ValueError(CREATE_INDEX_OVERWRITE_CONFLICT) + super().__init__(name, session_tag) prefix = prefix or name @@ -107,16 +126,24 @@ def __init__( ) # Check for existing message history index - if not overwrite and self._index.exists(): - existing_index = SearchIndex.from_existing( - name, redis_client=self._index.client - ) - if existing_index.schema.to_dict() != self._index.schema.to_dict(): - raise ValueError( - f"Existing index {name} schema does not match the user provided schema for the semantic message history. " - "If you wish to overwrite the index schema, set overwrite=True during initialization." + self._create_index = create_index + if create_index: + if not overwrite and self._index.exists(): + existing_index = SearchIndex.from_existing( + name, redis_client=self._index._redis_client ) - self._index.create(overwrite=overwrite, drop=False) + if existing_index.schema.to_dict() != self._index.schema.to_dict(): + raise ValueError( + f"Existing index {name} schema does not match the user provided schema for the semantic message history. " + "If you wish to overwrite the index schema, set overwrite=True during initialization." + ) + self._index.create(overwrite=overwrite, drop=False) + else: + logger.debug( + f"create_index=False: assuming index {name!r} exists over prefix " + f"{prefix!r} with {vectorizer.dims} vector dimensions. Its schema " + "is not verified." + ) self._default_session_filter = Tag(SESSION_FIELD_NAME) == self._session_tag @@ -144,7 +171,7 @@ def drop(self, id: str | None = None) -> None: if id is None: id = self.get_recent(top_k=1, raw=True)[0][ID_FIELD_NAME] # type: ignore - self._index.client.delete(self._index.key(id)) # type: ignore + self._index._redis_client.delete(self._index.key(id)) def count(self, session_tag=None): query = CountQuery( diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index 6ae7ea01b..e44ec3b61 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -7,7 +7,10 @@ from redis.commands.search.aggregation import AggregateRequest, AggregateResult, Reducer from redis.exceptions import ResponseError -from redisvl.extensions.constants import ROUTE_VECTOR_FIELD_NAME +from redisvl.extensions.constants import ( + CREATE_INDEX_OVERWRITE_CONFLICT, + ROUTE_VECTOR_FIELD_NAME, +) from redisvl.extensions.router.schema import ( DistanceAggregationMethod, Route, @@ -42,6 +45,9 @@ class SemanticRouter(BaseModel): """Configuration for routing behavior.""" _index: SearchIndex = PrivateAttr() + # A private attribute, never a field: it describes this instance's + # relationship to the index, and must not reach the stored route config. + _create_index: bool = PrivateAttr(default=True) model_config = ConfigDict(arbitrary_types_allowed=True) @@ -56,6 +62,7 @@ def __init__( redis_url: str = "redis://localhost:6379", overwrite: bool = False, connection_kwargs: dict[str, Any] = {}, + create_index: bool = True, **kwargs, ): """Initialize the SemanticRouter. @@ -70,7 +77,26 @@ def __init__( overwrite (bool, optional): Whether to overwrite existing index. Defaults to False. connection_kwargs (Dict[str, Any]): The connection arguments for the redis client. Defaults to empty {}. + create_index (bool, optional): Whether RedisVL creates and validates + the index. When False the constructor issues no index command at + all and writes nothing: the index must already exist, already + hold the reference vectors for ``routes``, and already have its + stored config, since none of that is written or verified. + ``routes`` must match what is indexed, because each route's + distance threshold is applied from this local list -- and + :meth:`add_route` rewrites the stored config from that same list, + so attaching with a partial set and then adding a route + truncates the config every other client reads. See + :class:`~redisvl.extensions.cache.llm.SemanticCache` for a worked + example of the flag, and :doc:`/user_guide/installation` for the + ACL details. Defaults to True. + + Raises: + ValueError: If both create_index is False and overwrite is True. """ + if not create_index and overwrite: + raise ValueError(CREATE_INDEX_OVERWRITE_CONFLICT) + dtype = kwargs.pop("dtype", None) index_kwargs = kwargs.pop("_index_kwargs", None) @@ -105,6 +131,7 @@ def __init__( redis_client=redis_client, ) + self._create_index = create_index self._initialize_index( redis_client, redis_url, @@ -113,7 +140,13 @@ def __init__( index_kwargs=index_kwargs, ) - self._index.client.json().set(f"{self.name}:route_config", f".", self.to_dict()) # type: ignore + if create_index: + # The stored config is the source of truth for from_existing(). With + # create_index=False the router does not own this index, so it is not + # ours to rewrite from a local route list we have not verified. + self._index._redis_client.json().set( + f"{self.name}:route_config", f".", self.to_dict() + ) @classmethod def from_existing( @@ -123,7 +156,17 @@ def from_existing( redis_url: str = "redis://localhost:6379", **kwargs, ) -> "SemanticRouter": - """Return SemanticRouter instance from existing index.""" + """Return SemanticRouter instance from existing index. + + Reads the stored route config with ``JSON.GET``, so unlike + :meth:`SearchIndex.from_existing` this needs no ``FT.INFO``. Pass + ``create_index=False`` to keep it that way through construction, which + makes this the way to attach to a router with a credential that cannot + run index-metadata commands. + """ + # Pulled out before the split below, which retains only SearchIndex init + # kwargs and would otherwise hand this to the Redis client constructor. + create_index = kwargs.pop("create_index", True) init_kwargs, connection_kwargs = _split_from_existing_kwargs( dict(kwargs), nested_connection_keys=("connection_kwargs",), @@ -169,6 +212,7 @@ def from_existing( redis_url=resolved_redis_url, redis_client=redis_client, connection_kwargs=connection_kwargs or None, + create_index=create_index, _index_kwargs={**init_kwargs, **index_kwargs} or None, ) except Exception: @@ -200,11 +244,24 @@ def _initialize_index( **(index_kwargs or {}), ) + if not self._create_index: + # The caller asserts the index exists, is seeded, and is not ours to + # rewrite -- so no existence check, no schema comparison, no + # FT.CREATE, and no route references written. The local `routes` have + # to match what is actually indexed, because the per-route FILTER in + # _distance_threshold_filter is built from them. + logger.debug( + f"create_index=False: assuming router index {self.name!r} exists " + f"and is already seeded with {len(self.routes)} routes. Its schema " + "is not verified." + ) + return + # Check for existing router index existed = self._index.exists() if not overwrite and existed: existing_index = SearchIndex.from_existing( - self.name, redis_client=self._index.client + self.name, redis_client=self._index._redis_client ) if existing_index.schema.to_dict() != self._index.schema.to_dict(): raise ValueError( @@ -393,6 +450,15 @@ def _get_route_matches( ) -> list[RouteMatch]: """Get route response from vector db""" + if not self.routes: + # Otherwise max() below raises "max() arg is an empty sequence", + # which says nothing about the actual problem. + raise ValueError( + f"Router {self.name!r} has no routes, so there is nothing to match " + "against. Add routes with add_route(), or construct the router " + "with a non-empty routes list." + ) + # what's interesting about this is that we only provide one distance_threshold for a range query not multiple # therefore you might take the max_threshold and further refine from there. distance_threshold = max(route.distance_threshold for route in self.routes) @@ -552,6 +618,10 @@ def route_many( def add_route(self, route: Route) -> str: """Add a new route to the SemanticRouter. + Note that this replaces the router's stored config with this instance's + route list, so a router constructed with a subset of the indexed routes + will drop the rest from the config that :meth:`from_existing` reads. + Embeds the route's references, writes them to the Redis index, appends the route to ``self.routes``, and persists the updated router config so the route survives :meth:`SemanticRouter.from_existing`. @@ -600,7 +670,7 @@ def delete(self) -> None: self._index.delete(drop=True) # The route config is stored as a standalone JSON key that is not # tracked by the search index, so it must be removed explicitly. - self._index.client.delete(f"{self.name}:route_config") # type: ignore + self._index._redis_client.delete(f"{self.name}:route_config") def clear(self) -> None: """Flush all routes from the semantic router index.""" @@ -836,7 +906,7 @@ def get_route_references( elif route_name: if not keys: pattern = self._route_pattern(self._index, route_name) - keys = scan_by_pattern(self._index.client, pattern) # type: ignore + keys = scan_by_pattern(self._index._redis_client, pattern) # type: ignore[arg-type,assignment] sep = self._index.key_separator queries = self._make_filter_queries( @@ -874,7 +944,7 @@ def delete_route_references( keys = [r[0]["id"] for r in res if len(r) > 0] elif not keys: pattern = self._route_pattern(self._index, route_name) - keys = scan_by_pattern(self._index.client, pattern) # type: ignore + keys = scan_by_pattern(self._index._redis_client, pattern) # type: ignore[arg-type,assignment] if not keys: raise ValueError(f"No references found for route {route_name}") @@ -883,7 +953,7 @@ def delete_route_references( for key in keys: route_name = key.split(":")[-2] to_be_deleted.append( - (route_name, convert_bytes(self._index.client.hgetall(key))) # type: ignore + (route_name, convert_bytes(self._index._redis_client.hgetall(key))) ) deleted = self._index.drop_keys(keys) @@ -900,4 +970,6 @@ def delete_route_references( def _update_router_state(self) -> None: """Update the router configuration in Redis.""" - self._index.client.json().set(f"{self.name}:route_config", f".", self.to_dict()) # type: ignore + self._index._redis_client.json().set( + f"{self.name}:route_config", f".", self.to_dict() + ) diff --git a/tests/integration/test_llmcache.py b/tests/integration/test_llmcache.py index 1b8c47067..0930383cc 100644 --- a/tests/integration/test_llmcache.py +++ b/tests/integration/test_llmcache.py @@ -7,8 +7,9 @@ import pytest from pydantic import ValidationError -from redis.exceptions import ConnectionError +from redis.exceptions import ConnectionError, NoPermissionError +from redisvl.exceptions import RedisSearchError from redisvl.extensions.cache.llm import SemanticCache from redisvl.index.index import AsyncSearchIndex, SearchIndex from redisvl.query.filter import Num, Tag, Text @@ -1158,3 +1159,65 @@ def test_cache_disconnect(redis_url, worker_id, hf_vectorizer): cache.disconnect() # We keep this index object around because it isn't lazily created assert cache._index.client is None + + +def test_create_index_false_works_under_a_read_write_acl( + cache, vectorizer, redis_url, acl_user, worker_id +): + """An application role must be able to use a cache it did not create. + + A credential assembled from `@read`/`@write` is denied `FT.INFO` and + `FT.CREATE` together -- neither is in either category -- so it cannot + construct a SemanticCache at all, even against an index it can query + perfectly well. `create_index=False` lets the caller assert what RedisVL is + unable to ask, and only a live server can show that the resulting cache + still stores and retrieves. + + The `cache` fixture supplies the pre-created index, standing in for the DBA + who provisions it. + """ + cache.store("What is the capital of France?", "Paris") + + with acl_user( + "~*", "&*", "+@read", "+@write", "-@dangerous", name="acl_cache_user" + ) as user: + credentials = {"username": user.username, "password": user.password} + + # Pin the premise: this role cannot ask whether the index exists. + restricted = user.connect() + with pytest.raises(NoPermissionError): + restricted.execute_command("FT.INFO", cache.index.name) + + restricted_cache = SemanticCache( + name=cache.index.name, + vectorizer=vectorizer, + distance_threshold=0.2, + redis_url=redis_url, + connection_kwargs=credentials, + create_index=False, + ) + + try: + hits = restricted_cache.check("What is the capital of France?") + assert hits and hits[0]["response"] == "Paris" + + # And it can write, so a cache miss populates the shared cache. + restricted_cache.store("Who wrote Hamlet?", "Shakespeare") + assert restricted_cache.check("Who wrote Hamlet?") + finally: + # This cache built its own client from the credentials, so the + # fixture does not track it -- and the user is about to be deleted. + restricted_cache.disconnect() + + # Without the flag, the same credential fails loudly at the existence + # check rather than degrading -- deliberately, since a credential that + # cannot ask whether the index exists cannot create one either. + with pytest.raises(RedisSearchError) as excinfo: + SemanticCache( + name=cache.index.name, + vectorizer=vectorizer, + redis_url=redis_url, + connection_kwargs=credentials, + ) + assert "ft.info" in str(excinfo.value).lower() + assert isinstance(excinfo.value.__cause__, NoPermissionError) diff --git a/tests/unit/test_extension_create_index_flag.py b/tests/unit/test_extension_create_index_flag.py new file mode 100644 index 000000000..75ef2e470 --- /dev/null +++ b/tests/unit/test_extension_create_index_flag.py @@ -0,0 +1,208 @@ +""" +Unit tests for `create_index=False` on the extension constructors. + +Every extension calls `create()` while being constructed, which runs `FT.INFO` +first. A credential assembled from `@read`/`@write` is denied `FT.INFO` and +`FT.CREATE` together, so it cannot construct any of them -- even against an +index that already exists and that it can query perfectly well. `create_index` +lets such a caller state what RedisVL is otherwise unable to ask. + +The contract is "no index command at all", so these tests assert on the Redis +client rather than on outcomes: the mock records every call, and `ft()` is the +gate every `FT.*` command passes through. +""" + +from unittest.mock import MagicMock, Mock + +import pytest +from redis import Redis +from redis.exceptions import NoPermissionError + +from redisvl.exceptions import RedisSearchError +from redisvl.extensions.cache.llm import SemanticCache +from redisvl.extensions.message_history import MessageHistory, SemanticMessageHistory +from redisvl.extensions.router import SemanticRouter +from redisvl.extensions.router.schema import Route +from redisvl.redis.connection import RedisConnectionFactory +from redisvl.utils.vectorize import CustomVectorizer + +ROUTE = Route(name="greeting", references=["hello"], distance_threshold=0.5) + + +@pytest.fixture +def vectorizer(): + """A stub vectorizer, so no model is downloaded and no API is called.""" + return CustomVectorizer(lambda text: [0.1, 0.2, 0.3]) + + +def _client() -> MagicMock: + return MagicMock(spec=Redis) + + +def _build(kind, client, vectorizer, **kwargs): + if kind == "cache": + return SemanticCache( + name="cache", vectorizer=vectorizer, redis_client=client, **kwargs + ) + if kind == "history": + return MessageHistory(name="history", redis_client=client, **kwargs) + if kind == "semantic_history": + return SemanticMessageHistory( + name="semantic_history", + vectorizer=vectorizer, + redis_client=client, + **kwargs, + ) + return SemanticRouter( + name="router", + routes=[ROUTE], + vectorizer=vectorizer, + redis_client=client, + **kwargs, + ) + + +ALL_KINDS = ["cache", "history", "semantic_history", "router"] +# MessageHistory has no `overwrite` parameter and absorbs one through **kwargs, +# so the contradiction guard cannot reach it. +OVERWRITE_KINDS = ["cache", "semantic_history", "router"] + + +class TestNoIndexCommandIsIssued: + @pytest.mark.parametrize("kind", ALL_KINDS) + def test_construction_issues_no_redis_command(self, kind, vectorizer): + client = _client() + _build(kind, client, vectorizer, create_index=False) + + client.ft.assert_not_called() + issued = [call.args[0] for call in client.execute_command.call_args_list] + assert [cmd for cmd in issued if str(cmd).upper().startswith("FT.")] == [] + # Deliberately stricter than the stated contract: nothing at all should + # touch Redis during construction, so a new eager call has to be + # justified here rather than slipping in. + assert client.mock_calls == [] + + @pytest.mark.parametrize("kind", ALL_KINDS) + def test_the_flag_is_kept_as_instance_state(self, kind, vectorizer): + # Three of the four never read it back, so without this a dead-store + # check prunes it -- and it is what a future "index not owned" notion + # would build on. + built = _build(kind, _client(), vectorizer, create_index=False) + assert built._create_index is False + + def test_router_writes_no_stored_config(self, vectorizer): + # The router's config blob is the source of truth for from_existing(), + # and is written from an unverified local route list. Rewriting it would + # truncate a shared router's routes. + client = _client() + router = _build("router", client, vectorizer, create_index=False) + client.json.assert_not_called() + # And the flag itself must never become part of that blob. + assert "create_index" not in router.to_dict() + + +class TestLazyConnectionAfterSkippingCreate: + """`create()` used to be the de-facto eager connect. + + `SearchIndex.client` stays `None` until the lazy `_redis_client` property + runs, so with the existence check skipped, any extension method reaching for + the raw client would dereference `None`. These pin the two shapes that broke. + """ + + def test_history_drop_connects_lazily(self, vectorizer, monkeypatch): + client = _client() + monkeypatch.setattr( + RedisConnectionFactory, "get_redis_connection", lambda **kwargs: client + ) + history = MessageHistory(name="history", create_index=False) + assert history._index.client is None # the precondition that broke + + history.drop(id="abc") + + client.delete.assert_called_once() + + def test_router_reference_lookup_connects_lazily(self, vectorizer, monkeypatch): + client = _client() + client.scan.return_value = (0, []) + monkeypatch.setattr( + RedisConnectionFactory, "get_redis_connection", lambda **kwargs: client + ) + router = SemanticRouter( + name="router", routes=[ROUTE], vectorizer=vectorizer, create_index=False + ) + assert router._index.client is None + + assert router.get_route_references(route_name="greeting") == [] + + +class TestContradictoryArguments: + @pytest.mark.parametrize("kind", OVERWRITE_KINDS) + def test_overwrite_with_create_index_false_is_rejected(self, kind, vectorizer): + with pytest.raises(ValueError, match="contradict"): + _build(kind, _client(), vectorizer, create_index=False, overwrite=True) + + +class TestRouterWithoutRoutes: + def test_empty_routes_raises_a_useful_error_when_matching(self, vectorizer): + # The guard is unconditional -- `routes=[]` is legal on either path, and + # the flag is about index ownership, not about route contents. Only the + # create_index=False case can be hermetic; the default path needs a live + # server and is covered by the integration suite. + router = SemanticRouter( + name="router", + routes=[], + vectorizer=vectorizer, + redis_client=_client(), + create_index=False, + ) + with pytest.raises(ValueError, match="no routes"): + router("hello") + + +class TestFromExisting: + def test_from_existing_issues_no_index_command(self, vectorizer, monkeypatch): + # SemanticRouter.from_existing reads the stored config with JSON.GET, so + # with the flag threaded it needs no FT.INFO -- which makes it the way to + # attach to a router under a credential that cannot run one. + stored = _build("router", _client(), vectorizer, create_index=False).to_dict() + monkeypatch.setattr( + "redisvl.utils.vectorize.vectorizer_from_dict", lambda _: vectorizer + ) + + # A real instance, so validate_sync_redis's issubclass check passes. + client = Redis.from_url("redis://localhost:6379") + client.client_setinfo = Mock() + client.ft = Mock() + client.json = Mock() + client.json.return_value.get.return_value = stored + + router = SemanticRouter.from_existing( + "router", redis_client=client, create_index=False + ) + + assert [route.name for route in router.routes] == ["greeting"] + client.ft.assert_not_called() + + def test_from_existing_still_verifies_by_default(self, vectorizer, monkeypatch): + # A stored route config proves a config key exists, not that the index + # does -- so the default must still check. + stored = _build("router", _client(), vectorizer, create_index=False).to_dict() + monkeypatch.setattr( + "redisvl.utils.vectorize.vectorizer_from_dict", lambda _: vectorizer + ) + + client = Redis.from_url("redis://localhost:6379") + client.client_setinfo = Mock() + client.json = Mock() + client.json.return_value.get.return_value = stored + # Stand in for the credential this feature exists for: the default path + # must reach FT.INFO, so a denial has to surface. + client.ft = Mock() + client.ft.return_value.info.side_effect = NoPermissionError( + "User acl_user has no permissions to run the 'FT.INFO' command" + ) + + with pytest.raises(RedisSearchError): + SemanticRouter.from_existing("router", redis_client=client) + + client.ft.assert_called() From 46459d6e23b9d7bad4ba6f55c8c64d97d240e527 Mon Sep 17 00:00:00 2001 From: Nitin Kanukolanu Date: Fri, 14 Aug 2026 13:16:17 -0400 Subject: [PATCH 2/2] fix: guard externally managed index lifecycle --- docs/user_guide/installation.md | 10 ++-- redisvl/extensions/cache/llm/semantic.py | 17 ++++++ redisvl/extensions/constants.py | 8 +++ .../message_history/message_history.py | 5 ++ .../message_history/semantic_history.py | 5 ++ redisvl/extensions/router/semantic.py | 9 ++++ .../unit/test_extension_create_index_flag.py | 54 +++++++++++++++++-- 7 files changed, 99 insertions(+), 9 deletions(-) diff --git a/docs/user_guide/installation.md b/docs/user_guide/installation.md index 74fef50d7..ce70fe8a5 100644 --- a/docs/user_guide/installation.md +++ b/docs/user_guide/installation.md @@ -201,7 +201,7 @@ The command-to-category mapping below was measured against live servers rather t | `index.delete()`, `rvl index delete`, `rvl index destroy` | `FT.DROPINDEX` | Yes | Yes | | Enumerating indexes (see below) | `FT._LIST` | **No** | **No** | -Every `Yes` above assumes key patterns that cover the index prefix — see [Key permissions](#key-permissions) — and `-@dangerous` layered on the second column additionally denies `FT.DROPINDEX`, so `index.delete()` becomes `No`. An SVS-VAMANA schema needs more than `FT.CREATE`: `index.create()` first probes capabilities with `INFO` (`@slow @dangerous`) and `MODULE LIST` (`@admin @slow @dangerous`), so both `-@admin` and `-@dangerous` policies break creation for those schemas. +Except for `FT.CREATE`, every `Yes` above assumes key patterns that cover the index prefix — see [Key permissions](#key-permissions). `FT.CREATE` is not checked against those patterns, so a credential can create an index it cannot query. Adding `-@dangerous` to the second column additionally denies `FT.DROPINDEX`, so `index.delete()` becomes `No`. An SVS-VAMANA schema needs more than `FT.CREATE`: `index.create()` first probes capabilities with `INFO` (`@slow @dangerous`) and `MODULE LIST` (`@admin @slow @dangerous`), so both `-@admin` and `-@dangerous` policies break creation for those schemas. Two of the rows above deserve their own explanation. @@ -217,7 +217,7 @@ User has no permissions to run the 'FT._LIST' command ### Roles built from `@read` and `@write` -`FT.INFO` is in neither `@read` nor `@write` — its only category is `@search` — and `FT.CREATE` is the same. So an application role assembled from `+@read +@write` — a natural least-privilege shape — can query and load, but cannot ask whether an index exists and cannot create one. Note that subtracting `@dangerous` is not what causes this: `+@all -@dangerous` permits both. The commands are simply never granted. +`FT.INFO` is in neither `@read` nor `@write` — its only category is `@search` — and `FT.CREATE` is the same. So an application role assembled from `+@read +@write -@dangerous` — a natural least-privilege shape for a runtime that must query and load but must not manage indexes — can query and load, but cannot ask whether an index exists and cannot create or drop one. Subtracting `@dangerous` is not what denies `FT.INFO` or `FT.CREATE`: `+@all -@dangerous` permits both. Those commands are simply never granted by `@read` or `@write`. Every extension constructor checks whether its index exists, so under such a credential all of them fail while being constructed: @@ -248,11 +248,11 @@ The flag also skips the SVS-VAMANA capability probe described above, since that `SemanticRouter` with `create_index=False` writes nothing at all: not the reference vectors for its routes, and not the stored route config that `SemanticRouter.from_existing()` reads. Preparing a router for this mode therefore means constructing it once with a privileged credential — a hand-written `FT.CREATE` is not enough, because the reference vectors have to be embedded and written too. Without them the router matches nothing, which looks like a distance-threshold problem rather than an empty index. -Afterwards, `SemanticRouter.from_existing(name, create_index=False)` is the way to attach to it: it recovers the routes and thresholds with `JSON.GET` and needs no `FT.INFO`. Pass the full route set. Each route's distance threshold is applied from the local list, so a partial set silently narrows matching — and `add_route()` and `remove_route()` rewrite the stored config from that same list, so attaching with a subset and then adding a route drops the rest from the config every other client reads. +Afterwards, `SemanticRouter.from_existing(name, create_index=False)` is the way to attach to it: it recovers the routes and thresholds with `JSON.GET` and needs no `FT.INFO`. The stored config must contain the full route set. Each route's distance threshold is applied from that recovered list, so an incomplete stored config silently narrows matching — and `add_route()` and `remove_route()` rewrite the stored config from the same list, so mutating an incomplete config permanently drops the omitted routes from the config every other client reads. ### When the schema diverges -With `create_index=False` nothing verifies that the live index matches the schema you described. Some mismatches are loud on first use, and two are silent: +With `create_index=False` nothing verifies that the live index matches the schema you described. Some mismatches are loud on first use, and several are silent: | Mismatch | What happens | |---|---| @@ -264,7 +264,7 @@ With `create_index=False` nothing verifies that the live index matches the schem For the silent cases the tell is `FT.INFO`'s `key_type`, `prefixes` and `attributes` — not `hash_indexing_failures`, which stays `0` because those keys were never indexing candidates. Diagnosing it therefore needs a credential that can run `FT.INFO`. -`create_index=False` restrains construction only, so `delete()` and `clear()` are as destructive as ever. Note that `clear()` differs across the extensions: `SemanticCache.clear()` is a `SCAN` plus `DEL` and works under any role that can write, while `MessageHistory`, `SemanticMessageHistory` and `SemanticRouter` delegate to `SearchIndex.clear()`, which calls `info()` first and therefore needs `FT.INFO`. +An extension constructed with `create_index=False` refuses index-wide `delete()` and `clear()` operations (and their async cache equivalents). This protects an externally managed index — including an index reached through an alias — from being destroyed through an attach-only instance. Targeted operations such as dropping a specific cache entry or message remain available. Perform lifecycle-wide destructive operations through the privileged provisioning path that owns the index. ### Key permissions diff --git a/redisvl/extensions/cache/llm/semantic.py b/redisvl/extensions/cache/llm/semantic.py index b0e5ab321..c2885e873 100644 --- a/redisvl/extensions/cache/llm/semantic.py +++ b/redisvl/extensions/cache/llm/semantic.py @@ -14,6 +14,7 @@ CACHE_VECTOR_FIELD_NAME, CREATE_INDEX_OVERWRITE_CONFLICT, ENTRY_ID_FIELD_NAME, + EXTERNAL_INDEX_LIFECYCLE_CONFLICT, INSERTED_AT_FIELD_NAME, METADATA_FIELD_NAME, PROMPT_FIELD_NAME, @@ -308,13 +309,29 @@ def set_threshold(self, distance_threshold: float) -> None: def delete(self) -> None: """Delete the cache and its index entirely.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) self._index.delete(drop=True) async def adelete(self) -> None: """Async delete the cache and its index entirely.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) aindex = await self._get_async_index() await aindex.delete(drop=True) + def clear(self) -> None: + """Clear all cache keys when RedisVL manages the index lifecycle.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + super().clear() + + async def aclear(self) -> None: + """Async clear all cache keys when RedisVL manages the index lifecycle.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) + await super().aclear() + def drop(self, ids: list[str] | None = None, keys: list[str] | None = None) -> None: """Drop specific entries from the cache by ID or Redis key. diff --git a/redisvl/extensions/constants.py b/redisvl/extensions/constants.py index bf2555e8b..c31bb7ef6 100644 --- a/redisvl/extensions/constants.py +++ b/redisvl/extensions/constants.py @@ -40,3 +40,11 @@ "RedisVL to drop and recreate the index, which it cannot do when it is told " "not to manage the index at all." ) + +# Raised when an extension attached to an externally managed index is asked to +# perform an index-wide destructive operation. +EXTERNAL_INDEX_LIFECYCLE_CONFLICT: str = ( + "Cannot delete or clear an index when create_index=False because RedisVL " + "does not manage that index's lifecycle. Use the externally managed " + "provisioning path to perform index-wide destructive operations." +) diff --git a/redisvl/extensions/message_history/message_history.py b/redisvl/extensions/message_history/message_history.py index c684d5a27..7566dab1f 100644 --- a/redisvl/extensions/message_history/message_history.py +++ b/redisvl/extensions/message_history/message_history.py @@ -4,6 +4,7 @@ from redisvl.extensions.constants import ( CONTENT_FIELD_NAME, + EXTERNAL_INDEX_LIFECYCLE_CONFLICT, ID_FIELD_NAME, METADATA_FIELD_NAME, ROLE_FIELD_NAME, @@ -94,10 +95,14 @@ def __repr__(self) -> str: def clear(self) -> None: """Clears the conversation message history.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) self._index.clear() def delete(self) -> None: """Clear all conversation keys and remove the search index.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) self._index.delete(drop=True) def drop(self, id: str | None = None) -> None: diff --git a/redisvl/extensions/message_history/semantic_history.py b/redisvl/extensions/message_history/semantic_history.py index c3364b4e0..eccad4e92 100644 --- a/redisvl/extensions/message_history/semantic_history.py +++ b/redisvl/extensions/message_history/semantic_history.py @@ -5,6 +5,7 @@ from redisvl.extensions.constants import ( CONTENT_FIELD_NAME, CREATE_INDEX_OVERWRITE_CONFLICT, + EXTERNAL_INDEX_LIFECYCLE_CONFLICT, ID_FIELD_NAME, MESSAGE_VECTOR_FIELD_NAME, METADATA_FIELD_NAME, @@ -155,10 +156,14 @@ def __repr__(self) -> str: def clear(self) -> None: """Clears the message history.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) self._index.clear() def delete(self) -> None: """Clear all message keys and remove the search index.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) self._index.delete(drop=True) def drop(self, id: str | None = None) -> None: diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index e44ec3b61..16472615b 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -9,6 +9,7 @@ from redisvl.extensions.constants import ( CREATE_INDEX_OVERWRITE_CONFLICT, + EXTERNAL_INDEX_LIFECYCLE_CONFLICT, ROUTE_VECTOR_FIELD_NAME, ) from redisvl.extensions.router.schema import ( @@ -167,6 +168,9 @@ def from_existing( # Pulled out before the split below, which retains only SearchIndex init # kwargs and would otherwise hand this to the Redis client constructor. create_index = kwargs.pop("create_index", True) + overwrite = kwargs.pop("overwrite", False) + if not create_index and overwrite: + raise ValueError(CREATE_INDEX_OVERWRITE_CONFLICT) init_kwargs, connection_kwargs = _split_from_existing_kwargs( dict(kwargs), nested_connection_keys=("connection_kwargs",), @@ -213,6 +217,7 @@ def from_existing( redis_client=redis_client, connection_kwargs=connection_kwargs or None, create_index=create_index, + overwrite=overwrite, _index_kwargs={**init_kwargs, **index_kwargs} or None, ) except Exception: @@ -667,6 +672,8 @@ def remove_route(self, route_name: str) -> None: def delete(self) -> None: """Delete the semantic router index and its persisted route config.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) self._index.delete(drop=True) # The route config is stored as a standalone JSON key that is not # tracked by the search index, so it must be removed explicitly. @@ -674,6 +681,8 @@ def delete(self) -> None: def clear(self) -> None: """Flush all routes from the semantic router index.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) self._index.clear() self.routes = [] diff --git a/tests/unit/test_extension_create_index_flag.py b/tests/unit/test_extension_create_index_flag.py index 75ef2e470..c77b18384 100644 --- a/tests/unit/test_extension_create_index_flag.py +++ b/tests/unit/test_extension_create_index_flag.py @@ -40,21 +40,22 @@ def _client() -> MagicMock: def _build(kind, client, vectorizer, **kwargs): + name = kwargs.pop("name", kind) if kind == "cache": return SemanticCache( - name="cache", vectorizer=vectorizer, redis_client=client, **kwargs + name=name, vectorizer=vectorizer, redis_client=client, **kwargs ) if kind == "history": - return MessageHistory(name="history", redis_client=client, **kwargs) + return MessageHistory(name=name, redis_client=client, **kwargs) if kind == "semantic_history": return SemanticMessageHistory( - name="semantic_history", + name=name, vectorizer=vectorizer, redis_client=client, **kwargs, ) return SemanticRouter( - name="router", + name=name, routes=[ROUTE], vectorizer=vectorizer, redis_client=client, @@ -142,6 +143,43 @@ def test_overwrite_with_create_index_false_is_rejected(self, kind, vectorizer): _build(kind, _client(), vectorizer, create_index=False, overwrite=True) +class TestExternalIndexLifecycle: + @pytest.mark.parametrize("kind", ALL_KINDS) + @pytest.mark.parametrize("method", ["clear", "delete"]) + def test_index_wide_mutation_is_rejected(self, kind, method, vectorizer): + client = _client() + extension = _build( + kind, + client, + vectorizer, + create_index=False, + name="production_alias", + ) + + with pytest.raises(ValueError, match="does not manage.*lifecycle"): + getattr(extension, method)() + + assert client.mock_calls == [] + + @pytest.mark.asyncio + @pytest.mark.parametrize("method", ["aclear", "adelete"]) + async def test_async_cache_index_wide_mutation_is_rejected( + self, method, vectorizer + ): + client = _client() + cache = SemanticCache( + name="production_alias", + vectorizer=vectorizer, + redis_client=client, + create_index=False, + ) + + with pytest.raises(ValueError, match="does not manage.*lifecycle"): + await getattr(cache, method)() + + assert client.mock_calls == [] + + class TestRouterWithoutRoutes: def test_empty_routes_raises_a_useful_error_when_matching(self, vectorizer): # The guard is unconditional -- `routes=[]` is legal on either path, and @@ -160,6 +198,14 @@ def test_empty_routes_raises_a_useful_error_when_matching(self, vectorizer): class TestFromExisting: + @pytest.mark.parametrize("with_client", [False, True]) + def test_from_existing_rejects_create_index_false_with_overwrite(self, with_client): + kwargs = {"redis_client": _client()} if with_client else {} + with pytest.raises(ValueError, match="contradict"): + SemanticRouter.from_existing( + "router", create_index=False, overwrite=True, **kwargs + ) + def test_from_existing_issues_no_index_command(self, vectorizer, monkeypatch): # SemanticRouter.from_existing reads the stored config with JSON.GET, so # with the flag threaded it needs no FT.INFO -- which makes it the way to