Skip to content

fix: return False from is_legal_dimension instead of raising TypeError - #3760

Open
shashvat-singham wants to merge 1 commit into
milvus-io:masterfrom
shashvat-singham:fix/is-legal-dimension-typeerror
Open

fix: return False from is_legal_dimension instead of raising TypeError#3760
shashvat-singham wants to merge 1 commit into
milvus-io:masterfrom
shashvat-singham:fix/is-legal-dimension-typeerror

Conversation

@shashvat-singham

Copy link
Copy Markdown

What

is_legal_dimension only catches ValueError around its int() call, but int() raises TypeError — not ValueError — for a type it cannot convert at all. So the exception escapes check_pass_param instead of being reported as an illegal parameter:

>>> from pymilvus.client.check import check_pass_param
>>> check_pass_param(dimension=None)
TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'

Same for [], {} and any other non-numeric object. Every sibling validator handles the identical input cleanly, which is what makes this look unintended rather than deliberate:

>>> check_pass_param(topk=None)
ParamError: <ParamError: (code=1, message=`topk` value None is illegal)>

is_legal_port has the same int()-in-a-try shape but guards with isinstance(port, (str, int)) first, so it is unaffected.

Fix

Catch TypeError alongside ValueError so the function returns False and check_pass_param raises ParamError like every other parameter.

Tests

Added test_check_pass_param_invalid_dimension to tests/unit/test_check.py, parametrised over None, [], {} and object(). All four fail on master with TypeError and pass with the change.

$ python -m pytest tests/unit/test_check.py -q
1 failed, 39 passed

The one failure is TestGetCommit::test_get_commit, which asserts on git metadata that isn't present in my shallow clone — it fails identically on a clean master here, unrelated to this change.

Note

I deliberately kept this to the reachable case. is_correct_date_str has the same except ValueError shape and would also raise on non-str input, but its only caller (parser_range_date) checks isinstance(date, str) first, so it isn't reachable today — happy to tighten it too if you'd prefer them consistent.

Separately, is_legal_dimension currently also returns True for 1.5, True and "8" since int() accepts all of them. That's a wider behaviour question than a crash fix, so I left it alone; let me know if you want a follow-up.

int() raises TypeError, not ValueError, for a type it cannot convert at
all, so is_legal_dimension only caught half of its failure cases. For
None, a list or a dict the TypeError propagated out of check_pass_param
instead of being turned into a ParamError:

    >>> check_pass_param(dimension=None)
    TypeError: int() argument must be a string, a bytes-like object or a
    real number, not 'NoneType'

Every sibling validator reports the same input cleanly, e.g.

    >>> check_pass_param(topk=None)
    ParamError: <ParamError: (code=1, message=`topk` value None is illegal)>

Catch TypeError as well so the checker reports an illegal parameter.

Signed-off-by: shashvat-singham <shashvat.singham@gmail.com>
@sre-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shashvat-singham
To complete the pull request process, please assign xuanyang-cn after the PR has been reviewed.
You can assign the PR to them by writing /assign @xuanyang-cn in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot

Copy link
Copy Markdown

Welcome @shashvat-singham! It looks like this is your first PR to milvus-io/pymilvus 🎉

@mergify

mergify Bot commented Aug 15, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants