diff --git a/docs/api/exceptions.rst b/docs/api/exceptions.rst index 63f2a4e9..8fb5fb53 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 8283afad..ce70fe8a 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** | + +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. + +`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 -@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: + +```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`. 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. -| 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 several 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`. + +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 + +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 661beb0d..c2885e87 100644 --- a/redisvl/extensions/cache/llm/semantic.py +++ b/redisvl/extensions/cache/llm/semantic.py @@ -12,7 +12,9 @@ ) from redisvl.extensions.constants import ( 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, @@ -36,6 +38,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 +69,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 +93,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 +193,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 ( @@ -261,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 cb58c98c..c31bb7ef 100644 --- a/redisvl/extensions/constants.py +++ b/redisvl/extensions/constants.py @@ -33,3 +33,18 @@ # 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." +) + +# 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 efbd64f7..7566dab1 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, @@ -16,8 +17,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 +33,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 +54,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 +79,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 @@ -73,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: @@ -89,7 +115,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 8664da43..eccad4e9 100644 --- a/redisvl/extensions/message_history/semantic_history.py +++ b/redisvl/extensions/message_history/semantic_history.py @@ -4,6 +4,8 @@ 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, @@ -20,9 +22,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 +43,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 +69,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 +127,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 @@ -128,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: @@ -144,7 +176,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 6ae7ea01..16472615 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -7,7 +7,11 @@ 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, + EXTERNAL_INDEX_LIFECYCLE_CONFLICT, + ROUTE_VECTOR_FIELD_NAME, +) from redisvl.extensions.router.schema import ( DistanceAggregationMethod, Route, @@ -42,6 +46,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 +63,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 +78,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 +132,7 @@ def __init__( redis_client=redis_client, ) + self._create_index = create_index self._initialize_index( redis_client, redis_url, @@ -113,7 +141,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 +157,20 @@ 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) + 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",), @@ -169,6 +216,8 @@ def from_existing( redis_url=resolved_redis_url, 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: @@ -200,11 +249,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 +455,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 +623,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`. @@ -597,13 +672,17 @@ 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. - 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.""" + if not self._create_index: + raise ValueError(EXTERNAL_INDEX_LIFECYCLE_CONFLICT) self._index.clear() self.routes = [] @@ -836,7 +915,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 +953,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 +962,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 +979,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 1b8c4706..0930383c 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 00000000..c77b1838 --- /dev/null +++ b/tests/unit/test_extension_create_index_flag.py @@ -0,0 +1,254 @@ +""" +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): + name = kwargs.pop("name", kind) + if kind == "cache": + return SemanticCache( + name=name, vectorizer=vectorizer, redis_client=client, **kwargs + ) + if kind == "history": + return MessageHistory(name=name, redis_client=client, **kwargs) + if kind == "semantic_history": + return SemanticMessageHistory( + name=name, + vectorizer=vectorizer, + redis_client=client, + **kwargs, + ) + return SemanticRouter( + name=name, + 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 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 + # 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: + @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 + # 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()