fix: keep vector fields in from_existing on RESP2 unified FT.INFO replies

[desty]

Fixes #712

Problem

With redis-py 8 on a RESP2 connection created with legacy_responses=False, FT.INFO puts vector params into the field's flags list:

{'identifier': 'h', 'attribute': 'h', 'type': 'VECTOR',
 'flags': ['algorithm', 'HNSW', 'data_type', 'FLOAT32', 'dim', 8,
           'distance_metric', 'L2', 'M', 24, 'ef_construction', 300, 'INDEXMISSING']}

The dict branch of parse_vector_attrs skipped flags entirely. It found no dims and returned None, and convert_index_info_to_schema then dropped the field with a bare continue. As a result, SearchIndex.from_existing() + create() silently rebuilt the index without its vector fields.

Changes

1. The dict branch of parse_vector_attrs now unpacks key/value pairs from flags. It skips valueless modifiers such as INDEXMISSING; I checked on Redis 8.4 that INDEXMISSING is appended as a bare token after the pairs. Top-level keys still take precedence, so RESP3 replies (params at top level, empty flags) behave as before.
2. When a vector field is skipped, a warning now names the field and the index, so the skip is no longer silent.

Testing

• Reproduced against redis:8.4 + redis-py 8.1.0 with a FLAT and an HNSW (m=24, ef_construction=300) vector field. Before this change, from_existing returned ['cat', 'text'] for protocol=2, legacy_responses=False. After it, all three client configurations (protocol=2, protocol=2, legacy_responses=False, protocol=3) recover the identical schema, including the HNSW params.
• New unit tests in tests/unit/test_convert_index_info.py: flags unpacking with a trailing INDEXMISSING, and the skip warning. Both fail on main and pass here.
• make format and make check-types pass. Running tests/unit (excluding test_mcp) plus tests/integration/test_connection.py gives the same result as on main: the pre-existing test_repr failures and google_genai optional-dependency errors, which are unrelated to this change, plus the 2 new tests passing.

AI disclosure: this change was prepared with an AI coding assistant (Claude Code). I reviewed the diff and ran the tests above.

---

Note

Medium Risk
Changes index schema recovery used by from_existing() and recreate flows; incorrect parsing could still drop or mis-specify vector fields, though behavior is narrowed with tests and explicit warnings.

Overview
Fixes silent loss of vector fields when introspecting indexes with redis-py 8 on RESP2 and legacy_responses=False, where FT.INFO puts vector settings in the field's flags list instead of top-level keys.

parse_vector_attrs now walks flags as known key/value pairs (via _VECTOR_PARAM_KEYS), skips valueless modifiers like INDEXMISSING, and tolerates unknown bare tokens without mis-pairing later params. Dim parsing no longer raises on bad values; unparseable vector fields are skipped with a warning naming the field and index. Integration coverage adds protocol=2, legacy_responses=False; unit tests cover HNSW recovery, unknown tokens, and skip/warn paths.

^{Reviewed by Cursor Bugbot for commit d4d7a2e. Bugbot is set up for automated code reviews on this repo. Configure here.}

[vishal-bala]

Thanks for your contribution! This looks good, but there's just a couple things I'd like to see addressed before we merge it.

[vishal-bala]

Good tests, and the first vector coverage this file has had. But they pin today's redis-py wire shape as a literal, so if it changes again they'll pass while the library breaks.

There's already an integration test for exactly this configuration that can't fire: test_client_from_existing_and_batch_search (tests/integration/test_search_index.py:624) asserts reopened.schema == index.schema on an index with a vector field, parametrised over {}, {"protocol": 3} and {"legacy_responses": False}. On redis-py 8 all three negotiate RESP3 (DEFAULT_RESP_VERSION = 3, redis/utils.py:283), so new-response-format duplicates protocol-3 and the RESP2-unified shape is never reached. Same in the async twin at tests/integration/test_async_search_index.py:546.

Could you add one param to each?

        pytest.param(
            {"protocol": 2, "legacy_responses": False},
            id="resp2-new-response-format",
            marks=pytest.mark.skipif(
                int(redis.__version__.split(".")[0]) < 8,
                reason="legacy_responses requires redis-py 8",
            ),
        ),

On main each file then gives 1 failed, 3 passed; on this branch, 8 passed. No new test function, and it runs on every redis-py 8.x matrix leg.

[desty]

Done in d4d7a2e. I added the resp2-new-response-format param ({"protocol": 2, "legacy_responses": False}, skipped on redis-py < 8) to test_client_from_existing_and_batch_search in both test_search_index.py and test_async_search_index.py. Locally on redis-py 8.1.0 / Redis 8.4, with main's connection.py the new param fails in both files (2 failed, 6 passed), and on this branch all 8 pass.

[vishal-bala]

The comment doesn't match the contents: as a general list it's missing CASESENSITIVE, NOSTEM and WITHSUFFIXTRIE (already enumerated in parse_attrs at line 492), and as a vector-specific list it's over-broad — Redis 8.4.7 rejects SORTABLE, NOINDEX, UNF and INDEXEMPTY on a VECTOR field, so only INDEXMISSING can occur.

More important: with a denylist, any token you haven't enumerated is consumed as a parameter name and swallows the one after it, shifting parity. Since distance_metric, algorithm and datatype all silently default below, the result is a wrong field rather than a failure:

["algorithm","FLAT","data_type","FLOAT32","dim",4,"NEWMOD","distance_metric","IP"]
  -> {'dims': 4, 'distance_metric': 'cosine', 'algorithm': 'flat', 'datatype': 'float32'}

IP index, recovered as COSINE, and IndexSchema.from_dict accepts it.

Inverting the test drops the need to track server vocabulary — pair up only the keys this function consumes, so an unknown token costs one skipped position and the walk re-syncs:

_VECTOR_PARAM_KEYS = frozenset(
    {"algorithm", "data_type", "datatype", "type", "dim", "dims", "distance_metric",
     "initial_cap", "block_size", "m", "ef_construction", "ef_runtime", "epsilon",
     "compression", "reduce", "graph_max_degree", "construction_window_size",
     "search_window_size", "training_threshold"}
)

with line 334 becoming if key not in _VECTOR_PARAM_KEYS or i + 1 >= len(flags):. Same approach as redis-py's own _INFO_ATTR_PAIR_KEYS.

I ran it: your two tests pass unchanged, 94 unit tests pass, all four client configs recover the full schema, +9/-1 lines. This is hardening, not a live bug — current servers emit only a trailing INDEXMISSING, and RediSearch appends bare tokens after the valued pairs by construction. If you'd rather keep the denylist, just fixing the comment is fine.

[desty]

Switched to the allowlist in d4d7a2e. Only keys in _VECTOR_PARAM_KEYS are paired with the next token, so an unknown bare token costs one position and the walk re-syncs. I also added your example as a unit test: ["algorithm","FLAT","data_type","FLOAT32","dim",4,"NEWMOD","distance_metric","IP"] now recovers distance_metric == "ip" (it fails on the previous revision). The misleading comment went away with the denylist.

[vishal-bala]

This loop is where dim stops coming from a trusted top-level key and starts coming from flags, which makes line 375 reachable — the one numeric conversion here that isn't wrapped, unlike m (418), ef_construction (424) and the SVS params through 476:

flags [... "dim","FUTUREMOD" ...]  -> ValueError: invalid literal for int() with base 10: 'FUTUREMOD'
flags [... "M","NOTANUMBER" ...]   -> OK, field returned with m omitted

Worth wrapping so a gap in dim takes the return None -> warn-and-skip path instead of escaping from_existing. The allowlist above doesn't cover this — dim is a known key, so it still consumes whatever follows.

[desty]

Wrapped in d4d7a2e. dim/dims now go through the same try/except (ValueError, TypeError) as the other numeric params. An unparseable value leaves dims unset, so the field takes the existing return None → warn-and-skip path. A new unit test covers ["algorithm","FLAT","dim","FUTUREMOD"]: the field is skipped with the warning, and nothing escapes convert_index_info_to_schema.

[vishal-bala]

Exactly what the issue asked for, but it won't reach the user it's for — and that isn't your fault.

get_logger attaches a NullHandler to the leaf logger (redisvl/utils/log.py:12-13), which makes Python count a handler as found and suppresses logging.lastResort, the stderr fallback for WARNING+ with no configuration:

plain script, no logging config  ->  nothing printed
same + logging.basicConfig()     ->  WARNING:redisvl.redis.connection:Skipping vector field 'emb' ...

So the skip is still silent for the issue's own reproduction, and the caplog test passes only because caplog installs a handler. Pre-existing and library-wide (23 get_logger sites, 89 warning/error calls) — I'm filing it separately, just flagging that the second half of #712 doesn't land yet.

Optional: the message could name the consequence — the schema is incomplete, and rebuilding from it with create(overwrite=True) drops the field. Keeping %r means the test still passes.

[desty]

Thanks for tracking the NullHandler issue separately. I also took the optional suggestion in d4d7a2e: the warning now says the recovered schema is incomplete and that recreating the index from it (e.g. create(overwrite=True)) drops the field. It still uses %r for the names, so the test is unchanged.

[vishal-bala]

This expected dict has no index_missing, which freezes a real loss into the suite: a vector field's INDEXMISSING is discarded on every protocol, so a from_existing -> create(overwrite=True) round trip rebuilds it without the modifier and ismissing(@emb) then errors.

It can't be fixed here — field_data puts a Python bool into the VECTOR attribute dict, so create() with a vector index_missing=True fails today with Invalid input of type: 'bool', and the server rejects the token inside the attribute block anyway (Bad arguments for algorithm FLAT). Fixing the read side alone would turn a quiet loss into a hard failure on rebuild.

So leave the behaviour — but a one-line comment noting the discard is deliberate would stop the next reader assuming it was missed. Separate issue coming.

[desty]

Added a comment above the expected dict in d4d7a2e. It says INDEXMISSING is deliberately not recovered as index_missing, because a vector field can't be recreated with it yet, so restoring it would turn a quiet loss into a failure on create().

[desty]

@vishal-bala Thanks for the detailed review. All five points are addressed in d4d7a2e (also rebased onto main), with replies on each thread. The allowlist and dim handling now have unit tests built from your examples, and the new resp2-new-response-format integration params fail on main and pass here. Ready for another look when you have time.