fix: address PR review comments on serialization helpers

thakoreh · thakoreh · commit 537ea60ec321 · 2026-03-03T23:27:19.000-05:00
- Sanitize Redis URLs by removing passwords before serialization - Add _sanitize_redis_url() helper function - Replace password with *** while preserving username - Move to_dict() and to_yaml() to BaseSearchIndex to avoid duplication - Handle None _redis_url by omitting from output instead of writing None - Move import yaml to top of file instead of inline import - Add overwrite parameter to to_yaml() for API consistency - When overwrite=False, raises FileExistsError if file exists - Update tests to cover new functionality Fixes review comments from Copilot/Cursor on PR redis#525
diff --git a/redisvl/index/index.py b/redisvl/index/index.py
@@ -21,6 +21,9 @@
     cast,
 )
 
+import yaml
+from urllib.parse import urlparse, urlunparse
+
 import redis.exceptions
 
 # Add missing imports
@@ -107,6 +110,38 @@
 _HYBRID_SEARCH_ERROR_MESSAGE = "Hybrid search is not available in this version of redis-py. Please upgrade to redis-py >= 7.1.0."
 
 
+def _sanitize_redis_url(url: Optional[str]) -> Optional[str]:
+    """Remove password from Redis URL for safe serialization.
+
+    Args:
+        url: Redis URL that may contain a password.
+
+    Returns:
+        URL with password replaced by '***', or None if url is None.
+
+    Example:
+        >>> _sanitize_redis_url("redis://:secret@localhost:6379")
+        'redis://***@localhost:6379'
+    """
+    if not url:
+        return None
+    try:
+        parsed = urlparse(url)
+        if parsed.password:
+            # Replace password with *** but keep username if present
+            if parsed.username:
+                netloc = f"{parsed.username}:***@{parsed.hostname}"
+            else:
+                netloc = f"***@{parsed.hostname}"
+            if parsed.port:
+                netloc = f"{netloc}:{parsed.port}"
+            return urlunparse(parsed._replace(netloc=netloc))
+        return url
+    except Exception:
+        # If parsing fails, return a safe placeholder
+        return "***"
+
+
 REQUIRED_MODULES_FOR_INTROSPECTION = [
     {"name": "search", "ver": 20810},
     {"name": "searchlight", "ver": 20810},
@@ -400,6 +435,60 @@ def key(self, id: str) -> str:
             key_separator=self.schema.index.key_separator,
         )
 
+    def to_dict(self, include_connection: bool = False) -> Dict[str, Any]:
+        """Serialize the index configuration to a dictionary.
+
+        Args:
+            include_connection (bool, optional): Whether to include connection
+                parameters. Defaults to False for security (passwords/URLs
+                are excluded by default).
+
+        Returns:
+            Dict[str, Any]: Dictionary representation of the index configuration.
+
+        Example:
+            >>> config = index.to_dict()
+            >>> new_index = SearchIndex.from_dict(config)
+        """
+        config = self.schema.to_dict()
+        if include_connection:
+            # Sanitize the Redis URL to remove password before serialization
+            sanitized_url = _sanitize_redis_url(self._redis_url)
+            if sanitized_url is not None:
+                config["_redis_url"] = sanitized_url
+            # Note: connection_kwargs may contain sensitive info
+            # Only include non-sensitive keys
+            safe_keys = {"decode_responses", "ssl", "socket_timeout", "socket_connect_timeout"}
+            connection_kwargs = getattr(self, "_connection_kwargs", {}) or {}
+            config["_connection_kwargs"] = {
+                k: v for k, v in connection_kwargs.items()
+                if k in safe_keys
+            }
+        return config
+
+    def to_yaml(self, path: str, include_connection: bool = False, overwrite: bool = True) -> None:
+        """Serialize the index configuration to a YAML file.
+
+        Args:
+            path (str): Path to write the YAML file.
+            include_connection (bool, optional): Whether to include connection
+                parameters. Defaults to False for security.
+            overwrite (bool, optional): Whether to overwrite existing file.
+                Defaults to True. If False and file exists, raises FileExistsError.
+
+        Example:
+            >>> index.to_yaml("schemas/my_index.yaml")
+
+        Raises:
+            FileExistsError: If overwrite=False and file already exists.
+        """
+        import os
+        if not overwrite and os.path.exists(path):
+            raise FileExistsError(f"File already exists: {path}")
+        config = self.to_dict(include_connection=include_connection)
+        with open(path, "w") as f:
+            yaml.dump(config, f, default_flow_style=False, sort_keys=False)
+
 
 class SearchIndex(BaseSearchIndex):
     """A search index class for interacting with Redis as a vector database.
@@ -1294,49 +1383,6 @@ def info(self, name: Optional[str] = None) -> Dict[str, Any]:
         index_name = name or self.schema.index.name
         return self._info(index_name, self._redis_client)
 
-    def to_dict(self, include_connection: bool = False) -> Dict[str, Any]:
-        """Serialize the index configuration to a dictionary.
-
-        Args:
-            include_connection (bool, optional): Whether to include connection
-                parameters. Defaults to False for security (passwords/URLs
-                are excluded by default).
-
-        Returns:
-            Dict[str, Any]: Dictionary representation of the index configuration.
-
-        Example:
-            >>> config = index.to_dict()
-            >>> new_index = SearchIndex.from_dict(config)
-        """
-        config = self.schema.to_dict()
-        if include_connection:
-            config["_redis_url"] = self._redis_url
-            # Note: connection_kwargs may contain sensitive info
-            # Only include non-sensitive keys
-            safe_keys = {"decode_responses", "ssl", "socket_timeout", "socket_connect_timeout"}
-            config["_connection_kwargs"] = {
-                k: v for k, v in self._connection_kwargs.items()
-                if k in safe_keys
-            }
-        return config
-
-    def to_yaml(self, path: str, include_connection: bool = False) -> None:
-        """Serialize the index configuration to a YAML file.
-
-        Args:
-            path (str): Path to write the YAML file.
-            include_connection (bool, optional): Whether to include connection
-                parameters. Defaults to False for security.
-
-        Example:
-            >>> index.to_yaml("schemas/my_index.yaml")
-        """
-        import yaml
-        config = self.to_dict(include_connection=include_connection)
-        with open(path, "w") as f:
-            yaml.dump(config, f, default_flow_style=False, sort_keys=False)
-
     def __enter__(self):
         return self
 
@@ -2294,49 +2340,6 @@ def disconnect_sync(self):
             return
         sync_wrapper(self.disconnect)()
 
-    def to_dict(self, include_connection: bool = False) -> Dict[str, Any]:
-        """Serialize the index configuration to a dictionary.
-
-        Args:
-            include_connection (bool, optional): Whether to include connection
-                parameters. Defaults to False for security (passwords/URLs
-                are excluded by default).
-
-        Returns:
-            Dict[str, Any]: Dictionary representation of the index configuration.
-
-        Example:
-            >>> config = index.to_dict()
-            >>> new_index = AsyncSearchIndex.from_dict(config)
-        """
-        config = self.schema.to_dict()
-        if include_connection:
-            config["_redis_url"] = self._redis_url
-            # Note: connection_kwargs may contain sensitive info
-            # Only include non-sensitive keys
-            safe_keys = {"decode_responses", "ssl", "socket_timeout", "socket_connect_timeout"}
-            config["_connection_kwargs"] = {
-                k: v for k, v in self._connection_kwargs.items()
-                if k in safe_keys
-            }
-        return config
-
-    def to_yaml(self, path: str, include_connection: bool = False) -> None:
-        """Serialize the index configuration to a YAML file.
-
-        Args:
-            path (str): Path to write the YAML file.
-            include_connection (bool, optional): Whether to include connection
-                parameters. Defaults to False for security.
-
-        Example:
-            >>> await index.to_yaml("schemas/my_index.yaml")
-        """
-        import yaml
-        config = self.to_dict(include_connection=include_connection)
-        with open(path, "w") as f:
-            yaml.dump(config, f, default_flow_style=False, sort_keys=False)
-
     async def __aenter__(self):
         return self
 
diff --git a/tests/unit/test_index_serialization.py b/tests/unit/test_index_serialization.py
@@ -59,6 +59,46 @@ def test_to_dict_with_connection(self, sample_schema):
         assert config["_connection_kwargs"]["ssl"] is True
         assert config["_connection_kwargs"]["socket_timeout"] == 30
 
+    def test_to_dict_sanitizes_password(self, sample_schema):
+        """Test to_dict() sanitizes passwords from redis_url."""
+        index = SearchIndex(
+            schema=sample_schema,
+            redis_url="redis://:mysecretpassword@localhost:6379",
+        )
+        
+        config = index.to_dict(include_connection=True)
+        
+        assert "_redis_url" in config
+        # Password should be sanitized
+        assert "mysecretpassword" not in config["_redis_url"]
+        assert "***" in config["_redis_url"]
+
+    def test_to_dict_sanitizes_password_with_username(self, sample_schema):
+        """Test to_dict() sanitizes passwords but keeps username."""
+        index = SearchIndex(
+            schema=sample_schema,
+            redis_url="redis://user:pass@localhost:6379",
+        )
+        
+        config = index.to_dict(include_connection=True)
+        
+        assert "_redis_url" in config
+        assert "pass" not in config["_redis_url"]
+        assert "user" in config["_redis_url"]
+        assert "***" in config["_redis_url"]
+
+    def test_to_dict_none_url_omitted(self, sample_schema):
+        """Test to_dict() omits _redis_url when it's None."""
+        index = SearchIndex(
+            schema=sample_schema,
+            redis_url=None,
+        )
+        
+        config = index.to_dict(include_connection=True)
+        
+        # _redis_url should be omitted, not set to None
+        assert "_redis_url" not in config
+
     def test_to_yaml_without_connection(self, sample_schema):
         """Test to_yaml() writes schema to YAML file."""
         index = SearchIndex(schema=sample_schema)
@@ -94,6 +134,35 @@ def test_to_yaml_with_connection(self, sample_schema):
         finally:
             Path(path).unlink()
 
+    def test_to_yaml_overwrite_false_raises(self, sample_schema):
+        """Test to_yaml() raises FileExistsError when overwrite=False."""
+        index = SearchIndex(schema=sample_schema)
+        
+        with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f:
+            path = f.name
+        
+        try:
+            with pytest.raises(FileExistsError):
+                index.to_yaml(path, overwrite=False)
+        finally:
+            Path(path).unlink()
+
+    def test_to_yaml_overwrite_true(self, sample_schema):
+        """Test to_yaml() overwrites when overwrite=True."""
+        index = SearchIndex(schema=sample_schema)
+        
+        with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f:
+            path = f.name
+        
+        try:
+            # Should not raise
+            index.to_yaml(path, overwrite=True)
+            
+            content = Path(path).read_text()
+            assert "test_index" in content
+        finally:
+            Path(path).unlink()
+
     def test_roundtrip_dict(self, sample_schema):
         """Test that to_dict() and from_dict() roundtrip correctly."""
         original_index = SearchIndex(
@@ -138,6 +207,31 @@ def test_to_dict_with_connection(self, sample_schema):
         # Password should not be included (not in safe_keys)
         assert "password" not in config.get("_connection_kwargs", {})
 
+    def test_to_dict_sanitizes_password(self, sample_schema):
+        """Test to_dict() sanitizes passwords from redis_url."""
+        index = AsyncSearchIndex(
+            schema=sample_schema,
+            redis_url="redis://:secret@localhost:6379",
+        )
+        
+        config = index.to_dict(include_connection=True)
+        
+        assert "_redis_url" in config
+        assert "secret" not in config["_redis_url"]
+        assert "***" in config["_redis_url"]
+
+    def test_to_dict_none_url_omitted(self, sample_schema):
+        """Test to_dict() omits _redis_url when it's None."""
+        index = AsyncSearchIndex(
+            schema=sample_schema,
+            redis_url=None,
+        )
+        
+        config = index.to_dict(include_connection=True)
+        
+        # _redis_url should be omitted, not set to None
+        assert "_redis_url" not in config
+
     def test_to_yaml(self, sample_schema):
         """Test to_yaml() writes schema to YAML file."""
         index = AsyncSearchIndex(schema=sample_schema)
@@ -153,6 +247,19 @@ def test_to_yaml(self, sample_schema):
         finally:
             Path(path).unlink()
 
+    def test_to_yaml_overwrite_false_raises(self, sample_schema):
+        """Test to_yaml() raises FileExistsError when overwrite=False."""
+        index = AsyncSearchIndex(schema=sample_schema)
+        
+        with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f:
+            path = f.name
+        
+        try:
+            with pytest.raises(FileExistsError):
+                index.to_yaml(path, overwrite=False)
+        finally:
+            Path(path).unlink()
+
     def test_roundtrip_dict(self, sample_schema):
         """Test that to_dict() and from_dict() roundtrip correctly."""
         original_index = AsyncSearchIndex(
