feat(spider-py): Add support for parsing TDL types into native Python types. - #188
Conversation
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
![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
🧹 Nitpick comments (3)
python/src/spider/type/utils.py (3)
15-21: Docstring tweaks: clarify input format and fix raises clauseClarify that the input must be a fully qualified dotted path and align the raises note with the actual parameter name.
- """ - Gets class by name. - :param name: - :return: - :raise: TypeError if `class_name` is not a valid class. - """ + """ + Gets a class by its fully qualified dotted name. + :param name: e.g., 'package.module.Class' or 'package.module.Outer.Inner'. + :return: Class object. + :raises TypeError: if the name does not resolve to a class. + """
3-5: Remove now-unused typing.cast import (post-refactor)If you adopt the runtime isinstance check suggested above, cast is no longer used.
-from typing import cast
15-30: Security: importing arbitrary module paths executes module-level codeIf names come from untrusted input (e.g., user-supplied TDL), import_module will execute import-time code. Consider:
- Restricting imports to an allowlist of modules/namespaces.
- Providing a resolver callback that maps allowed identifiers to classes.
- Failing fast if the module prefix is not in an approved set.
Please confirm whether inputs to get_class_by_name can be user-controlled. If so, we should add an allowlist or resolver strategy before merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
python/src/spider/type/tdl_convert.py(1 hunks)python/src/spider/type/tdl_parse.py(1 hunks)python/src/spider/type/tdl_type.py(1 hunks)python/src/spider/type/utils.py(1 hunks)python/tests/type/test_to_native.py(1 hunks)python/tests/type/test_to_tdl.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- python/tests/type/test_to_native.py
- python/src/spider/type/tdl_convert.py
- python/tests/type/test_to_tdl.py
- python/src/spider/type/tdl_parse.py
- python/src/spider/type/tdl_type.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
🔇 Additional comments (1)
python/src/spider/type/utils.py (1)
7-13: LGTM: fully qualified class name generation is correctUsing module + qualname is appropriate and preserves nested class paths.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/spider-py/src/spider_py/type/tdl_convert.py (1)
111-113: Guard theissubclasscall to avoid unexpected errors on non-class inputsWithout an
isinstance(native_type, type)guard, passing values like123,object()or a module intoto_tdl_type()makes the bareissubclass(native_type, Collection)raise Python’s built-in
TypeError: issubclass() arg 1 must be a class, bypassing your custom error message. Add a type check so only classes reachissubclass, yielding a consistentTypeError(f"{native_type} is not a valid TDL type.")when appropriate.• File:
python/spider-py/src/spider_py/type/tdl_convert.py, lines 111–113
• Replace:- if issubclass(native_type, Collection): + if isinstance(native_type, type) and issubclass(native_type, Collection): msg = f"{native_type} is not a valid TDL type." raise TypeError(msg)This ensures non-class inputs fall through (and can be handled or reported elsewhere) rather than triggering a misleading built-in exception.
🧹 Nitpick comments (16)
python/spider-py/src/spider_py/type/tdl_type.py (4)
3-7: Ensure typing_extensions is available or provide a safe runtime fallback for override.The module imports typing_extensions.override on Python 3.10. Either declare typing-extensions as a dependency (see pyproject suggestion) or provide a no-op fallback to avoid runtime ImportError.
Example import pattern:
try: from typing import override # Python 3.12+ except Exception: # noqa: BLE001 try: from typing_extensions import override # type: ignore[assignment] except Exception: # As last resort, define a no-op def override(func): # type: ignore[no-redef] return func
123-129: Surface clearer errors for invalid class names and rely on improved resolver.native_type delegates to get_class_by_name(self._name), which is good. Once utils.get_class_by_name supports nested classes and better diagnostics (see my comment there), this method will inherit those improvements. Consider referencing that utility’s message in the docstring.
No code change here if utils.py is updated as suggested; otherwise, add a brief format check (must contain a dot) to fail fast with a clearer message.
Please verify that TDL class strings may include nested class paths (e.g., pkg.mod.Outer.Inner) and add a test if so.
184-186: Use type_str() in the map key-type error for better UX.Printing the object directly yields a less helpful repr. Using type_str() makes the message user-facing and consistent with TDL syntax.
Apply:
- msg = f"{key_type} is not a supported type for map key." + msg = f"{key_type.type_str()} is not a supported type for map key."
194-197: Map generic native typing is fine; one caveat about “string-as-List”.dict[key.native_type(), value.native_type()] is correct. However, since TDL “string” is modelled as List, map keys representing strings will translate to list[Int8] at the typing level, which are not hashable at runtime. If these types are only used for type representation (not runtime enforcement), it’s fine; otherwise, consider a dedicated StringType that maps to bytes or str for hashability.
I can sketch a StringType with native_type -> bytes (or str) and adapt is_string accordingly if desired.
Confirm whether map keys will ever be realised at runtime to actual dict keys; if yes, we should ensure the key’s native type is hashable.
python/spider-py/src/spider_py/type/tdl_convert.py (2)
24-46: Deduplicate primitive mapping logic; delegate to the single source of truth.to_primitive_tdl_type reimplements the same mapping already provided by TypeDict/_to_primitive_tdl_type. Keep one mapping to prevent drift.
Replace the body with a simple dictionary lookup:
def to_primitive_tdl_type(native_type: type | GenericAlias) -> TdlType | None: - """ - Converts a native type to primitive TDL type. - :param native_type: - :return: Converted TDL primitive. None if `native_type` is not a supported primitive type. - """ - tdl_type: TdlType | None = None - if native_type is Int8: - tdl_type = Int8Type() - elif native_type is Int16: - tdl_type = Int16Type() - elif native_type is Int32: - tdl_type = Int32Type() - elif native_type is Int64: - tdl_type = Int64Type() - elif native_type is Float: - tdl_type = FloatType() - elif native_type is Double: - tdl_type = DoubleType() - elif native_type is bool: - tdl_type = BoolType() - return tdl_type + """ + Converts a native type to primitive TDL type. + :param native_type: + :return: The TDL primitive if supported; otherwise None. + """ + return TypeDict.get(native_type)If you prefer that this public function also raises on unsupported built-ins (int/float/str/complex/bytes), delegate directly to _to_primitive_tdl_type and update the docstring accordingly.
59-76: Good centralisation and guardrails for primitive resolution.Using TypeDict and explicitly erroring on built-in Python primitives makes misuses visible early. Consider expanding the message with guidance (e.g., “int -> Int32/Int64, float -> Double/Float, str -> List[Int8]”) if that helps users.
No code changes required; just a messaging improvement if desired.
python/spider-py/tests/type/test_to_native.py (5)
8-8: Import from the public API surface instead of the module path.Prefer importing
parse_tdl_typefromspider_py.typeto keep tests coupled to the public API and reduce churn if internals move.-from spider_py.type.tdl_parse import parse_tdl_type +from spider_py.type import parse_tdl_type
25-33: Parametrize primitive mappings to reduce repetition and improve diagnostics on failures.Parametrization makes failures pinpoint which mapping broke and shortens the test.
- def test_to_primitive_native_type(self) -> None: - """Test converting primitive TDL type to native type.""" - assert string_to_native("bool") is bool - assert string_to_native("double") is spider_py.Double - assert string_to_native("float") is spider_py.Float - assert string_to_native("int8") is spider_py.Int8 - assert string_to_native("int16") is spider_py.Int16 - assert string_to_native("int32") is spider_py.Int32 - assert string_to_native("int64") is spider_py.Int64 + @pytest.mark.parametrize( + ("tdl", "expected"), + [ + ("bool", bool), + ("double", spider_py.Double), + ("float", spider_py.Float), + ("int8", spider_py.Int8), + ("int16", spider_py.Int16), + ("int32", spider_py.Int32), + ("int64", spider_py.Int64), + ], + ) + def test_to_primitive_native_type(self, tdl: str, expected: type) -> None: + """Test converting primitive TDL type to native type.""" + assert string_to_native(tdl) is expected
35-39: Use identity comparison for classes.When asserting class resolution,
iscommunicates identity intent more clearly than==.- assert string_to_native(get_class_name(TestToNativeType)) == TestToNativeType + assert string_to_native(get_class_name(TestToNativeType)) is TestToNativeType
48-54: Confirm that List keys in Map are intentional (Python dict keys are unhashable if they are lists).The test accepts
Map<List<int8>, ...>which maps todict[list[Int8], ...]. In Python,listis unhashable and cannot be used as a dict key at runtime. If TDL intentionally allows non-hashable keys (e.g., because “Map” semantics differ from Python dicts), then this is fine. If not, we should reject such keys earlier (likely viais_map_key) and add a negative test.If you want to enforce hashable keys, I can propose a patch in
MapType.__init__and add a failing test like:with pytest.raises(TypeError): string_to_native("Map<List<int8>, int8>")Do you want me to open a follow-up issue/PR for this?
22-54: Add a few edge-case parsing tests (whitespace, invalid syntax, and case-sensitivity).These help harden the grammar and error paths without adding much maintenance overhead.
Examples (new tests to add; outside current diff):
@pytest.mark.parametrize( "s, expected", [ (" List < int8 > ", list[spider_py.Int8]), ("Map< int8 , double >", dict[spider_py.Int8, spider_py.Double]), ("List<List< int64 >>", list[list[spider_py.Int64]]), ], ) def test_whitespace_tolerance(s, expected): assert string_to_native(s) == expected @pytest.mark.parametrize("s", ["Map<int8 int8>", "List<>", "Map<,>", "Map<int8,>", "List"]) def test_invalid_strings_raise(s): with pytest.raises(TypeError): string_to_native(s) def test_case_sensitivity_of_keywords(): with pytest.raises(TypeError): string_to_native("list<int8>") # grammar requires capitalized List/Mappython/spider-py/src/spider_py/type/tdl_parse.py (5)
57-59: Renamekeytoelementinlist_typefor clarity.Minor naming nit: the parameter represents the element type, not a key.
- def list_type(self, key: TdlType) -> TdlType: + def list_type(self, element: TdlType) -> TdlType: """Transforms list node into List type.""" - return ListType(key) + return ListType(element)
62-66: Docstring should reflect both primitive and class outcomes.
base_typereturns either a primitive or aClassType.- def base_type(self, children: list[Token]) -> TdlType: - """Transforms primitive node into primitive type.""" + def base_type(self, children: list[Token]) -> TdlType: + """Transforms base node into a primitive or class TDL type.""" name = str(children[0]) if name in primitive_type_map: return primitive_type_map[name]() # type: ignore[abstract] return ClassType(name)
5-17: Avoid duplicating primitive mappings; reuse the converter fromtdl_convert.There is an existing primitive mapping in
tdl_convert. Reusing it prevents drift when adding new primitives and keeps a single source of truth.from spider_py.type.tdl_type import ( BoolType, ClassType, DoubleType, FloatType, Int8Type, Int16Type, Int32Type, Int64Type, ListType, MapType, TdlType, ) +from spider_py.type.tdl_convert import to_primitive_tdl_type @@ -primitive_type_map = { - "bool": BoolType, - "double": DoubleType, - "float": FloatType, - "int8": Int8Type, - "int16": Int16Type, - "int32": Int32Type, - "int64": Int64Type, -} +# Primitive resolution is delegated to `to_primitive_tdl_type` for consistency. @@ - def base_type(self, children: list[Token]) -> TdlType: - """Transforms base node into primitive type.""" - name = str(children[0]) - if name in primitive_type_map: - return primitive_type_map[name]() # type: ignore[abstract] - return ClassType(name) + def base_type(self, children: list[Token]) -> TdlType: + """Transforms base node into a primitive or class TDL type.""" + name = str(children[0]) + primitive = to_primitive_tdl_type(name) + return primitive if primitive is not None else ClassType(name)Also applies to: 32-40, 61-66
69-69: Optional: consider renaming the start rule fromtypeto avoid clashing with the Python built-in.This is purely stylistic. Using a name like
tdl_typeavoids shadowing in transformer methods and improves readability.
3-3: Consider caching parse results if this function is on a hot path.If
parse_tdl_typeis called frequently with repeated inputs (e.g., schema parsing), an LRU cache can materially reduce parse overhead. Ensure returnedTdlTypeinstances are immutable (they appear to be).+from functools import lru_cache @@ -def parse_tdl_type(string: str) -> TdlType: +@lru_cache(maxsize=1024) +def parse_tdl_type(string: str) -> TdlType:Also applies to: 72-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
python/spider-py/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
python/spider-py/pyproject.toml(1 hunks)python/spider-py/src/spider_py/type/__init__.py(1 hunks)python/spider-py/src/spider_py/type/tdl_convert.py(1 hunks)python/spider-py/src/spider_py/type/tdl_parse.py(1 hunks)python/spider-py/src/spider_py/type/tdl_type.py(12 hunks)python/spider-py/src/spider_py/type/utils.py(1 hunks)python/spider-py/tests/type/test_to_native.py(1 hunks)taskfiles/lint.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
python/spider-py/src/spider_py/type/tdl_parse.py (1)
python/spider-py/src/spider_py/type/tdl_type.py (11)
BoolType(96-105)ClassType(108-128)DoubleType(24-33)FloatType(36-45)Int8Type(48-57)Int16Type(60-69)Int32Type(72-81)Int64Type(84-93)ListType(131-147)MapType(174-196)TdlType(12-21)
python/spider-py/tests/type/test_to_native.py (4)
python/spider-py/src/spider_py/type/tdl_parse.py (1)
parse_tdl_type(72-84)python/spider-py/src/spider_py/type/utils.py (1)
get_class_name(7-12)python/spider-py/src/spider_py/type/tdl_type.py (11)
native_type(20-21)native_type(32-33)native_type(44-45)native_type(56-57)native_type(68-69)native_type(80-81)native_type(92-93)native_type(104-105)native_type(123-128)native_type(146-147)native_type(195-196)python/spider-py/src/spider_py/type/type.py (6)
Double(69-74)Float(61-66)Int8(29-34)Int16(37-42)Int32(45-50)Int64(53-58)
python/spider-py/src/spider_py/type/tdl_type.py (2)
python/spider-py/src/spider_py/type/type.py (6)
Double(69-74)Float(61-66)Int8(29-34)Int16(37-42)Int32(45-50)Int64(53-58)python/spider-py/src/spider_py/type/utils.py (1)
get_class_by_name(15-30)
python/spider-py/src/spider_py/type/__init__.py (1)
python/spider-py/src/spider_py/type/tdl_parse.py (1)
parse_tdl_type(72-84)
python/spider-py/src/spider_py/type/tdl_convert.py (2)
python/spider-py/src/spider_py/type/tdl_type.py (19)
native_type(20-21)native_type(32-33)native_type(44-45)native_type(56-57)native_type(68-69)native_type(80-81)native_type(92-93)native_type(104-105)native_type(123-128)native_type(146-147)native_type(195-196)TdlType(12-21)Int8Type(48-57)Int16Type(60-69)Int32Type(72-81)Int64Type(84-93)FloatType(36-45)DoubleType(24-33)BoolType(96-105)python/spider-py/src/spider_py/type/type.py (6)
Int8(29-34)Int16(37-42)Int32(45-50)Int64(53-58)Float(61-66)Double(69-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (6)
python/spider-py/src/spider_py/type/tdl_type.py (3)
19-22: Good abstraction: native_type() on the base TdlType.Introducing native_type in the ABC keeps resolution logic encapsulated per type. This aligns well with SOLID’s Open/Closed principle.
31-34: Primitive TDL → native type mappings look correct.Double/Float/Int8/16/32/64/Bool map cleanly to their corresponding runtime types in spider_py.type.type and bool. The implementation is straightforward and side-effect free.
Also applies to: 43-46, 56-57, 68-69, 80-81, 92-93, 104-105
145-148: List generic native typing is fine.Returning list[element_type.native_type()] is the right PEP 585 form and matches downstream usage.
python/spider-py/src/spider_py/type/tdl_convert.py (2)
85-107: GenericAlias handling for list/dict is correct and composable.Origin/args flow and recursive to_tdl_type calls cover nested generics well. The “no element/key/value type” checks are also clear.
118-124: String conversion helper is concise and correct.to_tdl_type_str correctly defers to to_tdl_type().type_str(), keeping a single conversion path.
python/spider-py/src/spider_py/type/__init__.py (1)
4-4: Re-export looks good and matches the new public API surface.
parse_tdl_typeis now exposed viaspider_py.type, and__all__reflects it alongside existing exports. This aligns with the tests and keeps the internal parser module encapsulated.Also applies to: 7-16
| "lark>=1.2.2", | ||
| "mariadb>=1.1.13", | ||
| "msgpack-types>=0.5.0", | ||
| ] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add missing runtime dependency for typing_extensions and consider constraining Lark’s major version.
- This repo imports typing_extensions.override (see python/spider-py/src/spider_py/type/tdl_type.py Line 6). On Python 3.10, typing_extensions is not in the stdlib, so the package must declare it as a dependency, otherwise imports will fail in clean environments.
- Optional: Since Lark follows semver and 2.x may include breaking changes, favour pinning an upper bound to avoid unexpected resolver picks.
Apply this diff to add typing-extensions and (optionally) cap Lark to <2:
dependencies = [
- "lark>=1.2.2",
+ "lark>=1.2.2,<2",
"mariadb>=1.1.13",
"msgpack-types>=0.5.0",
+ "typing-extensions>=4.8",
]Run this to confirm usage of typing_extensions across the repo and catch any other missing deps:
🏁 Script executed:
#!/bin/bash
rg -n --glob 'python/**' -C2 -e '\btyping_extensions\b'Length of output: 1312
Action Required: Add typing-extensions as a runtime dependency and constrain Lark’s major version
The code imports override from typing_extensions (see python/spider-py/src/spider_py/type/tdl_type.py:6), but typing-extensions isn’t declared in pyproject.toml. In a clean Python 3.10 environment this import will fail. Additionally, since Lark follows semver and a 2.x release may introduce breaking changes, it’s safer to pin an upper bound on its major version.
Please update python/spider-py/pyproject.toml accordingly:
dependencies = [
- "lark>=1.2.2",
+ "lark>=1.2.2,<2",
"mariadb>=1.1.13",
"msgpack-types>=0.5.0",
+ "typing-extensions>=4.8",
]- Confirm that any CI or packaging tooling picks up the new dependency.
- Optional: adjust the minimum version of
typing-extensionsif you need features introduced after 4.8.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "lark>=1.2.2", | |
| "mariadb>=1.1.13", | |
| "msgpack-types>=0.5.0", | |
| ] | |
| # python/spider-py/pyproject.toml | |
| dependencies = [ | |
| "lark>=1.2.2,<2", | |
| "mariadb>=1.1.13", | |
| "msgpack-types>=0.5.0", | |
| "typing-extensions>=4.8", | |
| ] |
🤖 Prompt for AI Agents
In python/spider-py/pyproject.toml around lines 12–15, add typing-extensions as
a runtime dependency and constrain Lark’s major version to avoid accidental 2.x
breakage; specifically add an entry for "typing-extensions" (optionally with a
minimum like ">=4.8" if you rely on newer features) to the dependencies list and
change the Lark entry to include an upper bound such as "lark>=1.2.2,<2.0.0";
after editing, ensure CI/packaging picks up the updated pyproject (e.g., by
re-running dependency install steps or updating lock files) so the new
dependency is propagated.
| def get_class_by_name(name: str) -> type: | ||
| """ | ||
| Gets class by name. | ||
| :param name: | ||
| :return: | ||
| :raise: TypeError if `class_name` is not a valid class. | ||
| """ | ||
| parts = name.split(".") | ||
| module_name = ".".join(parts[:-1]) | ||
| class_name = parts[-1] | ||
| try: | ||
| module = import_module(module_name) | ||
| return cast("type", getattr(module, class_name)) | ||
| except (ValueError, ModuleNotFoundError, AttributeError) as exc: | ||
| msg = f"{name} is not a valid TDL type." | ||
| raise TypeError(msg) from exc |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make class resolution robust to nested classes and improve diagnostics.
Current logic splits on the last dot and getattr’s a single attribute from the module. This fails for nested classes (e.g., pkg.mod.Outer.Inner) and for objects not actually being classes. It also conflates module import failures with attribute lookups in error messaging.
Refactor to resolve the longest importable module prefix, then getattr-chain the remaining components. Also validate the resolved object is a class and surface clearer messages:
def get_class_by_name(name: str) -> type:
"""
Gets class by name.
:param name:
:return:
:raise: TypeError if `class_name` is not a valid class.
"""
- parts = name.split(".")
- module_name = ".".join(parts[:-1])
- class_name = parts[-1]
- try:
- module = import_module(module_name)
- return cast("type", getattr(module, class_name))
- except (ValueError, ModuleNotFoundError, AttributeError) as exc:
- msg = f"{name} is not a valid TDL type."
- raise TypeError(msg) from exc
+ parts = name.split(".")
+ if len(parts) < 2:
+ raise TypeError(f"{name} is not a fully-qualified class path.")
+
+ last_exc: Exception | None = None
+ # Try longest module prefix first, then shorten until import succeeds.
+ for i in range(len(parts) - 1, 0, -1):
+ module_name = ".".join(parts[:i])
+ attrs = parts[i:]
+ try:
+ module = import_module(module_name)
+ except (ValueError, ModuleNotFoundError) as exc:
+ last_exc = exc
+ continue
+ obj: object = module
+ try:
+ for attr in attrs:
+ obj = getattr(obj, attr)
+ except AttributeError as exc:
+ last_exc = exc
+ continue
+ if not isinstance(obj, type):
+ raise TypeError(f"{name} resolved to {type(obj).__name__}, not a class.")
+ return cast("type", obj)
+
+ raise TypeError(f"{name} is not a valid class path.") from last_excConsider aligning the error text with the utility’s purpose, e.g., “valid class path” instead of “valid TDL type,” to reduce confusion.
If you’d like, I can add unit tests covering nested classes (Outer.Inner) resolution.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_class_by_name(name: str) -> type: | |
| """ | |
| Gets class by name. | |
| :param name: | |
| :return: | |
| :raise: TypeError if `class_name` is not a valid class. | |
| """ | |
| parts = name.split(".") | |
| module_name = ".".join(parts[:-1]) | |
| class_name = parts[-1] | |
| try: | |
| module = import_module(module_name) | |
| return cast("type", getattr(module, class_name)) | |
| except (ValueError, ModuleNotFoundError, AttributeError) as exc: | |
| msg = f"{name} is not a valid TDL type." | |
| raise TypeError(msg) from exc | |
| def get_class_by_name(name: str) -> type: | |
| """ | |
| Gets class by name. | |
| :param name: | |
| :return: | |
| :raise: TypeError if `class_name` is not a valid class. | |
| """ | |
| parts = name.split(".") | |
| if len(parts) < 2: | |
| raise TypeError(f"{name} is not a fully-qualified class path.") | |
| last_exc: Exception | None = None | |
| # Try longest module prefix first, then shorten until import succeeds. | |
| for i in range(len(parts) - 1, 0, -1): | |
| module_name = ".".join(parts[:i]) | |
| attrs = parts[i:] | |
| try: | |
| module = import_module(module_name) | |
| except (ValueError, ModuleNotFoundError) as exc: | |
| last_exc = exc | |
| continue | |
| obj: object = module | |
| try: | |
| for attr in attrs: | |
| obj = getattr(obj, attr) | |
| except AttributeError as exc: | |
| last_exc = exc | |
| continue | |
| if not isinstance(obj, type): | |
| raise TypeError(f"{name} resolved to {type(obj).__name__}, not a class.") | |
| return cast("type", obj) | |
| raise TypeError(f"{name} is not a valid class path.") from last_exc |
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Reviewed tdl_parse. Still working on the rest.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
python/spider-py/src/spider_py/type/utils.py (1)
15-31: Make resolution robust to nested classes and validate the resolved object is a class.Current logic splits once and does a single
getattr, which breaks for nested classes (e.g.,pkg.mod.Outer.Inner) and may return non-class attributes without validation. Recommend resolving the longest importable module, then getattr-chain the remainder, and validate the final object is a class. Also improve error messaging to reflect the utility’s purpose. This mirrors a prior suggestion on this PR thread.Apply this diff:
-def get_class_by_name(name: str) -> type: - """ - Gets class by name. - :param name: - :return: The type object identified by `name`. - :raise: TypeError if `class_name` is not a valid class. - """ - module_name, _, class_name = name.rpartition(".") - if "" == module_name: - msg = f"{name} does not contain a valid Python module." - raise TypeError(msg) - try: - module = import_module(module_name) - return cast("type", getattr(module, class_name)) - except (ValueError, ModuleNotFoundError, AttributeError) as exc: - msg = f"{name} is not a valid TDL type." - raise TypeError(msg) from exc +def get_class_by_name(name: str) -> type: + """ + Resolve a fully-qualified class path to a class object. + :param name: Fully-qualified path like 'pkg.mod.Outer.Inner'. + :return: The class object identified by `name`. + :raise TypeError: if `name` does not resolve to a class. + """ + parts = name.split(".") + if len(parts) < 2: + raise TypeError(f"{name} is not a fully-qualified class path.") + + last_exc: Exception | None = None + # Try longest module prefix first, then shorten until import succeeds. + for i in range(len(parts) - 1, 0, -1): + module_name = ".".join(parts[:i]) + attrs = parts[i:] + try: + module = import_module(module_name) + except (ValueError, ModuleNotFoundError) as exc: + last_exc = exc + continue + obj: object = module + try: + for attr in attrs: + obj = getattr(obj, attr) + except AttributeError as exc: + last_exc = exc + continue + if not isinstance(obj, type): + raise TypeError(f"{name} resolved to {type(obj).__name__}, not a class.") + return cast(type, obj) + + raise TypeError(f"{name} is not a valid class path.") from last_excIf you want, I can add unit tests covering:
- nested class resolution (Outer.Inner),
- non-class attributes (should error),
- ambiguous module vs. attribute errors.
🧹 Nitpick comments (4)
python/spider-py/src/spider_py/type/utils.py (4)
16-21: Docstring and error text clarity.Minor polish: the docstring refers to “
class_name” (not a parameter) and error text says “valid TDL type,” which is misleading for a utility that resolves class paths.If you keep the current implementation, apply this minimal diff to improve wording:
- """ - Gets class by name. - :param name: - :return: The type object identified by `name`. - :raise: TypeError if `class_name` is not a valid class. - """ + """ + Resolve a fully-qualified class path to a class object. + :param name: Fully-qualified path like 'pkg.mod.Class'. + :return: The class object identified by `name`. + :raise TypeError: if `name` does not resolve to a class. + """ @@ - msg = f"{name} is not a valid TDL type." + msg = f"{name} is not a valid class path."Also applies to: 30-31
23-25: Fix Yoda condition flagged by Ruff (SIM300).Style nit: prefer
module_name == ""over"" == module_name.If the current approach is kept, apply:
- if "" == module_name: + if module_name == "":
28-28: Use non-string type incast.
cast("type", ...)works, butcast(type, ...)is clearer and avoids unnecessary forward references.- return cast("type", getattr(module, class_name)) + return cast(type, getattr(module, class_name))
15-31: Confirm threat model; importing arbitrary modules from strings can be risky.If TDL strings are user-controlled,
import_modulewill execute module top-level code. Consider restricting permissible modules (e.g., allowlist prefixes) or requiring prior registration.Options:
- Accept an optional
allowed_prefixes: Sequence[str]argument and reject names outside it.- Resolve only within a curated registry map (name → class).
- Restrict resolution to already-imported modules in
sys.modules.I can provide a patch once you confirm whether input is trusted or needs sandboxing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
python/spider-py/src/spider_py/type/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
python/spider-py/src/spider_py/type/utils.py
23-23: Yoda condition detected
Rewrite as module_name == ""
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (1)
python/spider-py/src/spider_py/type/utils.py (1)
3-4: Imports look correct and minimal.
import_moduleandcastare used below; nothing extraneous here.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Minor comments. Otherwise lgtm.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(spider-py): Add support for parsing TDL types into native Python types.
2 participants
Description
Note
This PR depends on #187.
This PR:
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Tests
Chores