fix: Enforce Typed Decorators and Fix Type Errors #26627
Conversation
Remove the `reportUntypedFunctionDecorator` hint from `pyrightconfig.json` and fix all resulting type errors. This change enforces stricter type checking for function decorators by removing the setting that suppressed warnings for untyped decorators. To resolve the newly surfaced errors, the following changes were made: - Added explicit type annotations (`Callable`, `ParamSpec`, `TypeVar`, `overload`) to all custom decorators in `api/controllers/console/wraps.py`, `api/controllers/console/app/wraps.py`, `api/controllers/console/datasets/wraps.py`, `api/controllers/inner_api/plugin/wraps.py`, and `api/controllers/service_api/wraps.py`. - Corrected the usage of decorators in `api/controllers/service_api/dataset/dataset.py` and `api/controllers/service_api/workspace/models.py`. - Added type hints to error handlers in `api/libs/external_api.py`. - Removed unused imports that were identified during the process. All type-checking errors have been resolved, and the backend unit tests pass.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request focuses on enforcing typed decorators by removing reportUntypedFunctionDecorator from pyrightconfig.json and fixing the resulting type errors across several files. The changes are well-executed, primarily involving the addition of type hints (Callable, ParamSpec, TypeVar, overload) to decorators and error handlers. This significantly improves the codebase's type safety and maintainability.
My review identified a few areas for improvement in api/controllers/service_api/dataset/dataset.py, where a newly injected tenant_id parameter is consistently left unused in favor of current_user.current_tenant_id. While this doesn't cause a functional bug, using the injected parameter directly would improve code clarity and consistency. I've provided suggestions to address this.
dosubot
bot
added
the
size:L
There was a problem hiding this comment.
Pull Request Overview
Enforces stricter type checking for function decorators by removing the reportUntypedFunctionDecorator hint from pyrightconfig.json and fixing all resulting type errors.
- Removed
reportUntypedFunctionDecoratorsetting to enable strict type checking for decorators - Added comprehensive type annotations using
overload,Callable,ParamSpec, andTypeVarto all custom decorators - Fixed decorator usage patterns and removed unused imports in service API controllers
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/pyrightconfig.json | Removed type checking suppression for untyped function decorators |
| api/libs/external_api.py | Added type annotations and # type: ignore comments to error handlers |
| api/controllers/service_api/wraps.py | Added comprehensive type annotations with overloads to decorators |
| api/controllers/service_api/workspace/models.py | Fixed decorator usage by using DatasetApiResource base class |
| api/controllers/service_api/dataset/dataset.py | Removed redundant decorator usage and fixed method signatures |
| api/controllers/inner_api/plugin/wraps.py | Added type annotations with overloads to plugin decorators |
| api/controllers/console/wraps.py | Added type annotations to all console decorators |
| api/controllers/console/datasets/wraps.py | Added type annotations with overloads to dataset decorators |
| api/controllers/console/app/wraps.py | Added type annotations with overloads to app decorators |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
cc @laipz8200 any advice? |
|
get_app_model is being raised by pyright and basedpyright, but not by pyrelfy... well. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request does a great job of enforcing stricter type checking for decorators, which significantly improves code quality and maintainability. The changes are extensive and well-executed across multiple files by adding explicit type annotations. However, I've found a critical runtime bug in the implementation of the validate_dataset_token decorator when applied to instance methods. The argument injection logic incorrectly handles the self argument, which will lead to runtime errors. Please see my detailed comment for the fix.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request focuses on enforcing typed decorators by removing reportUntypedFunctionDecorator and fixing the resulting type errors. The changes are well-executed, adding explicit type annotations to decorators across multiple files, which significantly improves type safety and code clarity. My review includes a few suggestions to further enhance this effort by adding missing return type hints to some inner decorator functions for consistency. I also identified a couple of instances of bare except clauses that could be made more specific to improve code robustness. Overall, this is a valuable contribution to improving the codebase's quality.
| def cloud_edition_billing_enabled(view: Callable[P, R]): | ||
| def cloud_edition_billing_enabled(view: Callable[P, R]) -> Callable[P, R]: | ||
| @wraps(view) | ||
| def decorated(*args: P.args, **kwargs: P.kwargs): |
There was a problem hiding this comment.
The inner decorated function is missing its return type hint. To maintain consistency with the other typed decorators in this file, you should add -> R.
| def decorated(*args: P.args, **kwargs: P.kwargs): | |
| def decorated(*args: P.args, **kwargs: P.kwargs) -> R: |
| def cloud_edition_billing_resource_check(resource: str) -> Callable[[Callable[P, R]], Callable[P, R]]: | ||
| def interceptor(view: Callable[P, R]) -> Callable[P, R]: | ||
| @wraps(view) | ||
| def decorated(*args: P.args, **kwargs: P.kwargs): |
There was a problem hiding this comment.
The inner decorated function is missing its return type hint. To maintain consistency with the other typed decorators in this file, you should add -> R.
| def decorated(*args: P.args, **kwargs: P.kwargs): | |
| def decorated(*args: P.args, **kwargs: P.kwargs) -> R: |
| def cloud_edition_billing_knowledge_limit_check(resource: str) -> Callable[[Callable[P, R]], Callable[P, R]]: | ||
| def interceptor(view: Callable[P, R]) -> Callable[P, R]: | ||
| @wraps(view) | ||
| def decorated(*args: P.args, **kwargs: P.kwargs): |
There was a problem hiding this comment.
The inner decorated function is missing its return type hint. To maintain consistency with the other typed decorators in this file, you should add -> R.
| def decorated(*args: P.args, **kwargs: P.kwargs): | |
| def decorated(*args: P.args, **kwargs: P.kwargs) -> R: |
| def is_allow_transfer_owner(view: Callable[P, R]): | ||
| def is_allow_transfer_owner(view: Callable[P, R]) -> Callable[P, R]: | ||
| @wraps(view) | ||
| def decorated(*args: P.args, **kwargs: P.kwargs): |
There was a problem hiding this comment.
The inner decorated function is missing its return type hint. To maintain consistency with the other typed decorators in this file, you should add -> R.
| def decorated(*args: P.args, **kwargs: P.kwargs): | |
| def decorated(*args: P.args, **kwargs: P.kwargs) -> R: |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request does a great job of enforcing stricter type checking for decorators, which significantly improves code quality and maintainability. The use of overload for decorators that can be called with or without arguments is well-implemented. The refactoring in api/controllers/service_api/dataset/dataset.py and api/controllers/service_api/workspace/models.py to apply decorators at the class level is a good simplification.
I've identified a few decorators in api/controllers/console/wraps.py where the innermost function is still missing a return type annotation. Adding these will make the typing more complete and consistent with the rest of the changes in this PR. Overall, this is a solid improvement.
asukaminato0721
force-pushed
the
fix/enforce-typed-decorators
branch
4 times, most recently
from
6464b8f to
f0565d3
Compare
asukaminato0721
force-pushed
the
fix/enforce-typed-decorators
branch
2 times, most recently
from
2437a4e to
19f68f2
Compare
asukaminato0721
force-pushed
the
fix/enforce-typed-decorators
branch
from
3f72031 to
f72d82a
Compare
asukaminato0721
force-pushed
the
fix/enforce-typed-decorators
branch
from
f72d82a to
625ec8d
Compare
asukaminato0721
changed the title
1 participant
Remove the
reportUntypedFunctionDecoratorhint frompyrightconfig.jsonand fix all resulting type errors.This change enforces stricter type checking for function decorators by removing the setting that suppressed warnings for untyped decorators.
To resolve the newly surfaced errors, the following changes were made:
Callable,ParamSpec,TypeVar,overload) to all custom decorators inapi/controllers/console/wraps.py,api/controllers/console/app/wraps.py,api/controllers/console/datasets/wraps.py,api/controllers/inner_api/plugin/wraps.py, andapi/controllers/service_api/wraps.py.api/controllers/service_api/dataset/dataset.pyandapi/controllers/service_api/workspace/models.py.api/libs/external_api.py.All type-checking errors have been resolved, and the backend unit tests pass.
Important
Fixes #<issue number>.part of #26412
Summary
Screenshots
Checklist
dev/reformat(backend) andcd web && npx lint-staged(frontend) to appease the lint gods