Skip to content

fix(query): accept a bare string for include_vector - #2174

Open
chrikrah wants to merge 1 commit into
weaviate:mainfrom
chrikrah:fix/include-vector-str
Open

chrikrah wants to merge 1 commit into
weaviate:mainfrom
chrikrah:fix/include-vector-str

Conversation

@chrikrah

Copy link
Copy Markdown

No open issue covers this.

INCLUDE_VECTOR is Union[bool, str, List[str]], and the validator accepts bool, str and Sequence, so one named vector passed as a string is a valid call. _MetadataQuery.from_public at grpc.py:147 handles only bool and list, so a str matches neither branch and MetadataRequest asks for nothing:

$ .venv/bin/python repro.py
from_public(None, True         ) -> vector=True   vectors=None
from_public(None, 'named_vec'  ) -> vector=False  vectors=None      <- nothing requested
from_public(None, ['named_vec']) -> vector=False  vectors=['named_vec']
_QueryOptions.from_input(include_vector='named_vec').include_vector = True

The caller gets no vector and no error, while _QueryOptions records True, so the response parser is told to expect one. The fix normalises str to [str] at the top of from_public.

$ .venv/bin/python -m pytest test mock_tests -q -p no:randomly
594 passed, 1 skipped in 69.85s

Baseline on eb5546a is 591 passed, 1 skipped, so the delta is the three new cases. Reverting grpc.py and keeping them gives 2 failed, 592 passed.

The one existing test for this argument, test_queries.py:100, asserts include_vector=42 raises. The str path was untested.

I did not confirm this against a running Weaviate: no Docker here, so integration/ and journey_tests/ did not run. The proof is on the request-construction side, which needs no server, because a request that asks for no vector cannot return one. ruff is not installed in this venv, so I did not lint.

@dirkkul you and @g-despot merge most of the client changes here, and CODEOWNERS covers only .github/ and ci/. Worth a str overload on the type alias instead, or is normalising enough?

INCLUDE_VECTOR is Union[bool, str, List[str]], and the validator accepts
bool, str and Sequence, so passing one named vector as a string is a valid
call. _MetadataQuery.from_public handled only bool and list, so a str matched
neither branch: MetadataRequest asked for no vector at all and the caller got
none back, with no error. _QueryOptions meanwhile recorded include_vector=True,
so the response parser was told to expect one.

Normalises str to [str] at the top of from_public.

@orca-security-eu orca-security-eu Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed SAST high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca

@weaviate-git-bot

Copy link
Copy Markdown

To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Forum?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants