MPT-18457 MPT API Client - Missing type for in RQL query#219
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
jentyk
force-pushed
the
fix/MPT-18457
branch
2 times, most recently
from
efa8423 to
d11599d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/rql/query_builder.py`:
- Line 10: Remove Iterable from the broad QueryValue alias and split it into a
scalar alias and a collection alias: define QueryScalar = str | bool | dt.date |
dt.datetime | Numeric and a separate QueryCollection type (e.g. list | tuple |
set of QueryScalar or a Sequence/Collection[...] from collections.abc) and
replace usages of QueryValue accordingly; keep QueryCollection for the "in" /
list operators and use QueryScalar for eq/ne/lt/... paths, and update the
runtime checks around the variable value and op (the branches that reference
constants.LIST and isinstance(value, list | tuple | set)) to validate against
QueryCollection instead of the old Iterable alias so scalar operators no longer
accept arbitrary iterables and the WPS221 lint error is resolved.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mpt_api_client/rql/query_builder.py (1)
2-10:⚠️ Potential issue | 🔴 Critical
QueryValuenow causes runtimeTypeErrorinisinstancepaths; split scalar vs list types.Line 10 adds
Iterable[str]intoQueryValue, andQueryValueis used in runtimeisinstance(...)checks (Line 105, Line 143). That raisesTypeError: isinstance() argument 2 cannot be a parameterized generic, which matches the failing pipeline traces.💡 Proposed fix
from decimal import Decimal from typing import Any, Self, override Numeric = int | float | Decimal - -QueryValue = str | bool | dt.date | dt.datetime | Numeric | Iterable[str] # noqa:WPS221 +QueryScalarValue = str | bool | dt.date | dt.datetime | Numeric +QueryListValue = list[QueryScalarValue] | tuple[QueryScalarValue, ...] | set[QueryScalarValue] +QueryValue = QueryScalarValue +QueryArgumentValue = QueryScalarValue | QueryListValue @@ -def parse_kwargs(query_dict: dict[str, QueryValue]) -> list[str]: # noqa: WPS231 +def parse_kwargs(query_dict: dict[str, QueryArgumentValue]) -> list[str]: # noqa: WPS231 @@ - **kwargs: QueryValue, + **kwargs: QueryArgumentValue, @@ -def query_value_str(value: Any) -> str: +def query_value_str(value: Any) -> str: """Converts a value to string for use in RQL queries.""" - if isinstance(value, QueryValue): + if isinstance(value, QueryScalarValue): value = RQLValue(value) @@ def rql_encode(op: str, value: Any) -> str: @@ - if op not in constants.LIST and isinstance(value, QueryValue | RQLValue | RQLProperty): + if op not in constants.LIST and isinstance(value, QueryScalarValue | RQLValue | RQLProperty): return query_value_str(value)#!/bin/bash set -euo pipefail # Verify root cause exists in current tree (read-only checks). rg -nP '^\s*QueryValue\s*=.*Iterable\[[^]]+\]' mpt_api_client/rql/query_builder.py rg -nP '\bisinstance\([^)]*,\s*QueryValue(\s*\||\s*\))' mpt_api_client/rql/query_builder.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/rql/query_builder.py` around lines 2 - 10, QueryValue currently includes a parameterized generic (Iterable[str]) which causes TypeError when used in runtime isinstance checks; split the alias into a scalar union and a non-parameterized iterable alias, then update the isinstance checks to use those concrete symbols. Concretely: replace QueryValue with two aliases such as QueryScalarValue = str | bool | dt.date | dt.datetime | Numeric and QueryIterableValue = Iterable (or Iterable[Any]/collections.abc.Iterable without type parameters), or define QueryListValue = Iterable[str] only for type hints, and update any runtime isinstance(...) use (the checks around the previous QueryValue usage) to test against QueryScalarValue's concrete types (e.g., (str, bool, dt.date, dt.datetime, int, float, Decimal)) or check isinstance(value, collections.abc.Iterable) for lists; keep the type-only parameterized Iterable[str] for typing annotations but never pass a parameterized generic into isinstance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 2-10: QueryValue currently includes a parameterized generic
(Iterable[str]) which causes TypeError when used in runtime isinstance checks;
split the alias into a scalar union and a non-parameterized iterable alias, then
update the isinstance checks to use those concrete symbols. Concretely: replace
QueryValue with two aliases such as QueryScalarValue = str | bool | dt.date |
dt.datetime | Numeric and QueryIterableValue = Iterable (or
Iterable[Any]/collections.abc.Iterable without type parameters), or define
QueryListValue = Iterable[str] only for type hints, and update any runtime
isinstance(...) use (the checks around the previous QueryValue usage) to test
against QueryScalarValue's concrete types (e.g., (str, bool, dt.date,
dt.datetime, int, float, Decimal)) or check isinstance(value,
collections.abc.Iterable) for lists; keep the type-only parameterized
Iterable[str] for typing annotations but never pass a parameterized generic into
isinstance.
d11599d to
ee54717
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mpt_api_client/rql/query_builder.py (1)
2-10:⚠️ Potential issue | 🔴 CriticalPipeline failure: mypy requires type parameters for
Iterable.The CI is failing with
Missing type parameters for generic type 'Iterable' [type-arg]. However, adding a type parameter (e.g.,Iterable[str]) will cause a runtime TypeError on line 143 becauseisinstance()cannot accept parameterized generics.The previous review comment's suggested fix remains the correct approach—split into scalar and collection aliases:
💡 Recommended refactor
-from collections.abc import Iterable from decimal import Decimal from typing import Any, Self, override Numeric = int | float | Decimal - -QueryValue = str | bool | dt.date | dt.datetime | Numeric | Iterable # noqa:WPS221 +QueryScalarValue = str | bool | dt.date | dt.datetime | Numeric +QueryListValue = list[QueryScalarValue] | tuple[QueryScalarValue, ...] | set[QueryScalarValue] +QueryValue = QueryScalarValue # For scalar operators (eq, ne, lt, etc.) +QueryArgumentValue = QueryScalarValue | QueryListValue # For kwargs accepting bothThen update function signatures and runtime checks accordingly:
parse_kwargs: useQueryArgumentValueRQLQuery.__init__(**kwargs): useQueryArgumentValue- Line 143: use
QueryScalarValuein the isinstance check- Line 145: keep
list | tuple | setfor the collection branch,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/rql/query_builder.py` around lines 2 - 10, Define separate scalar and collection aliases instead of using a parameterless Iterable: add QueryScalarValue = str | bool | dt.date | dt.datetime | Numeric and QueryCollectionValue = list | tuple | set and then define QueryArgumentValue = QueryScalarValue | QueryCollectionValue; update function/type annotations to use QueryArgumentValue (e.g., parse_kwargs and RQLQuery.__init__), change the runtime isinstance check that currently uses Iterable to check against QueryScalarValue (for scalars) and keep the collection branch using list | tuple | set so isinstance() calls remain valid at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mpt_api_client/rql/query_builder.py`:
- Around line 2-10: Define separate scalar and collection aliases instead of
using a parameterless Iterable: add QueryScalarValue = str | bool | dt.date |
dt.datetime | Numeric and QueryCollectionValue = list | tuple | set and then
define QueryArgumentValue = QueryScalarValue | QueryCollectionValue; update
function/type annotations to use QueryArgumentValue (e.g., parse_kwargs and
RQLQuery.__init__), change the runtime isinstance check that currently uses
Iterable to check against QueryScalarValue (for scalars) and keep the collection
branch using list | tuple | set so isinstance() calls remain valid at runtime.
d881b4f to
344faf2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mpt_api_client/rql/query_builder.py (1)
10-10:⚠️ Potential issue | 🟠 MajorUnresolved from prior review: split scalar vs collection aliases instead of widening
QueryValue.Line 10 still broadens
QueryValuetoIterable, so scalar operators (eq/ne/lt/...) now accept list/tuple/set and serialize them instead of failing fast. That changes behavior beyond thein/outuse case.Proposed refactor
-from collections.abc import Iterable from decimal import Decimal from typing import Any, Self, override Numeric = int | float | Decimal - -QueryValue = str | bool | dt.date | dt.datetime | Numeric | Iterable # type: ignore[type-arg] # noqa: WPS221 +QueryScalarValue = str | bool | dt.date | dt.datetime | Numeric +QueryListValue = list[QueryScalarValue] | tuple[QueryScalarValue, ...] | set[QueryScalarValue] +QueryValue = QueryScalarValue +QueryArgumentValue = QueryScalarValue | QueryListValue-def parse_kwargs(query_dict: dict[str, QueryValue]) -> list[str]: # noqa: WPS231 +def parse_kwargs(query_dict: dict[str, QueryArgumentValue]) -> list[str]: # noqa: WPS231def __init__( # noqa: WPS211 self, namespace_: str | None = None, # noqa: WPS120 - **kwargs: QueryValue, + **kwargs: QueryArgumentValue, ) -> None:#!/bin/bash set -euo pipefail # Verify alias split and where each alias is used rg -n "^(Numeric|QueryScalarValue|QueryListValue|QueryArgumentValue|QueryValue)\s*=" mpt_api_client/rql/query_builder.py # Verify scalar operators still accept scalar-only alias rg -n "def (eq|ne|lt|le|gt|ge|like|ilike)\(self, value:" mpt_api_client/rql/query_builder.py # Verify kwargs/parser accept scalar + collection arguments rg -n "def parse_kwargs|def __init__\(" mpt_api_client/rql/query_builder.py # Verify runtime branch separation remains explicit rg -n "if op not in constants\.LIST|if op in constants\.LIST" mpt_api_client/rql/query_builder.pyAs per coding guidelines, "Typing and type aliases... project config enforces strict typing and mypy checks; ensure proper imports and type hints conform to strict settings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/rql/query_builder.py` at line 10, The QueryValue alias was widened to include Iterable which lets scalar operators accept collections; split the alias into explicit QueryScalarValue (str | bool | dt.date | dt.datetime | Numeric) and QueryListValue (Iterable[QueryScalarValue]) and use a union QueryArgumentValue = QueryScalarValue | QueryListValue (or keep QueryValue as the union) so scalar-only methods (eq, ne, lt, le, gt, ge, like, ilike) accept only QueryScalarValue signatures while in/out and list-handling code uses QueryListValue; update type hints in the eq/ne/... method signatures, parse_kwargs and __init__ to accept the correct aliases, and ensure runtime branches still check operators against constants.LIST where needed (e.g., in serialize/parse code) to preserve behavior and satisfy mypy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mpt_api_client/rql/query_builder.py`:
- Line 10: The QueryValue alias was widened to include Iterable which lets
scalar operators accept collections; split the alias into explicit
QueryScalarValue (str | bool | dt.date | dt.datetime | Numeric) and
QueryListValue (Iterable[QueryScalarValue]) and use a union QueryArgumentValue =
QueryScalarValue | QueryListValue (or keep QueryValue as the union) so
scalar-only methods (eq, ne, lt, le, gt, ge, like, ilike) accept only
QueryScalarValue signatures while in/out and list-handling code uses
QueryListValue; update type hints in the eq/ne/... method signatures,
parse_kwargs and __init__ to accept the correct aliases, and ensure runtime
branches still check operators against constants.LIST where needed (e.g., in
serialize/parse code) to preserve behavior and satisfy mypy.
jentyk
force-pushed
the
fix/MPT-18457
branch
from
344faf2 to
95e98b3
Compare
|
Currently we do not expect QueryValue to be a list, dict or Iterable, in that case (for example when we check if a value is IN a list we do it like the example below. Would it be better to change it to: from |
|
Also: Is this not what you want? or ? |
jentyk
force-pushed
the
fix/MPT-18457
branch
from
95e98b3 to
f05a3f8
Compare
I didn't notice this. Thanks for spotting. I think the issue I found was coming from a different place. Have a look at the new version. |
jentyk
force-pushed
the
fix/MPT-18457
branch
from
f05a3f8 to
b5da65a
Compare
Can you provide an example in a form of a unit test that validate those changes and covers a potential user case? Currently QueryValue is always expected to be a Scalar. I do not comprehend why we need to pass an Iterator where we expect a Scalar. Thank you so much |
I added a test, have a look. The test is just an example and should be deleted I do not see a point of keeping it |
jentyk
force-pushed
the
fix/MPT-18457
branch
from
f9d59de to
2a284e6
Compare
jentyk
force-pushed
the
fix/MPT-18457
branch
from
2a284e6 to
d4b605a
Compare
albertsola
left a comment
albertsola
left a comment
There was a problem hiding this comment.
Thanks, I see the problem you are solving!
jentyk
force-pushed
the
fix/MPT-18457
branch
from
d4b605a to
b5da65a
Compare
|
3 participants
Closes MPT-18457