Skip to content

Fix/v4 reviews #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wangxingjun778 merged 4 commits into from
Jun 5, 2026
Merged

Fix/v4 reviews #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d73429
[Fix] OpenAPI client audit fixes: pagination, retry, auth, body handling
wangxingjun778 Jun 5, 2026
8dc1953
[Test] Expand integration test coverage and add run_all.py runner
wangxingjun778 Jun 5, 2026
0ff5b78
[Fix] Catch RetryError in legacy client and fix integration test params
wangxingjun778 Jun 5, 2026
1cb8280
[Fix] Scope pagination empty-page test by owner to avoid huge global …
wangxingjun778 Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/modelscope_hub/_legacy_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
UPLOAD_BLOB_READ_TIMEOUT,
UPLOAD_RETRY_ALLOWED_METHODS,
)
from .errors import InvalidParameter, NetworkError, RequestTimeoutError, raise_for_status
from .errors import InvalidParameter, NetworkError, RequestTimeoutError, ServerError, raise_for_status
from .utils.logger import get_logger

logger = get_logger("legacy_api")
Expand Down Expand Up @@ -179,6 +179,8 @@ def _request(
timeout=timeout or self._timeout,
stream=stream,
)
except requests.exceptions.RetryError as exc:
raise ServerError(f"Max retries exceeded: {exc}") from exc
except requests.ConnectionError as exc:
raise NetworkError(f"Connection failed: {exc}") from exc
except requests.Timeout as exc:
Expand Down
53 changes: 43 additions & 10 deletions src/modelscope_hub/_openapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from .constants import API_MAX_RETRIES, API_TIMEOUT, OPENAPI_PREFIX
from .errors import (
AuthenticationError,
InvalidParameter,
NetworkError,
RateLimitError,
RequestTimeoutError,
Expand All @@ -50,6 +51,9 @@
# HTTP methods that are safe to retry without risking duplicate side-effects.
_IDEMPOTENT_METHODS: frozenset[str] = frozenset({"GET", "HEAD", "OPTIONS", "PUT", "DELETE"})

# POST endpoints that are semantically idempotent (deploy/stop are state transitions).
_RETRYABLE_POST_PATHS: frozenset[str] = frozenset({"/deploy", "/stop", "/undeploy"})

# Errors that warrant a transparent retry.
_RETRYABLE_EXC: tuple[type[BaseException], ...] = (
NetworkError, ServerError, RateLimitError,
Expand Down Expand Up @@ -225,8 +229,12 @@ def _request(
else:
return self._decode(response, unwrap=unwrap)

# Retry policy: only for idempotent methods on transient errors.
if attempt >= attempts or method_upper not in _IDEMPOTENT_METHODS:
# Retry policy: idempotent methods + known-idempotent POST paths.
is_retryable = (
method_upper in _IDEMPOTENT_METHODS
or (method_upper == "POST" and any(path.endswith(p) for p in _RETRYABLE_POST_PATHS))
)
if attempt >= attempts or not is_retryable:
break
backoff = min(2 ** (attempt - 1), 16)
_logger.debug(
Expand Down Expand Up @@ -271,14 +279,18 @@ def list_models(
owner: str | None = None,
sort: str | None = None,
page_number: int = 1,
page_size: int = 20,
page_size: int = 10,
filters: Filters = None,
) -> JSON:
"""``GET /models`` — list models with pagination and filters.

Supported filter keys: ``task``, ``library``, ``model_type``,
``custom_tag``, ``license``, ``deploy``.
"""
if page_number * page_size > 3000:
raise InvalidParameter(
f"page_number * page_size must be <= 3000 (got {page_number * page_size})."
)
Comment on lines +290 to +293
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 5, 2026

Choose a reason for hiding this comment

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

medium

The pagination parameters page_number and page_size are multiplied to check if they exceed the limit of 3000. However, if either parameter is negative or zero, the check page_number * page_size > 3000 can be bypassed (e.g., -1 * 10 = -10 <= 3000). To prevent sending invalid pagination values to the backend, we should explicitly validate that both page_number and page_size are positive integers (greater than or equal to 1).

        if page_number < 1 or page_size < 1:
            raise InvalidParameter("page_number and page_size must be positive integers.")
        if page_number * page_size > 3000:
            raise InvalidParameter(
                f"page_number * page_size must be <= 3000 (got {page_number * page_size})."
            )

params = self._merge_params(
{
"search": search,
Expand All @@ -305,10 +317,14 @@ def list_datasets(
owner: str | None = None,
sort: str | None = None,
page_number: int = 1,
page_size: int = 20,
page_size: int = 10,
filters: Filters = None,
) -> JSON:
"""``GET /datasets`` — list datasets. Filter keys: ``task``, ``license``."""
if page_number * page_size > 3000:
raise InvalidParameter(
f"page_number * page_size must be <= 3000 (got {page_number * page_size})."
)
Comment on lines +324 to +327
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 5, 2026

Choose a reason for hiding this comment

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

medium

The pagination parameters page_number and page_size are multiplied to check if they exceed the limit of 3000. However, if either parameter is negative or zero, the check page_number * page_size > 3000 can be bypassed (e.g., -1 * 10 = -10 <= 3000). To prevent sending invalid pagination values to the backend, we should explicitly validate that both page_number and page_size are positive integers (greater than or equal to 1).

        if page_number < 1 or page_size < 1:
            raise InvalidParameter("page_number and page_size must be positive integers.")
        if page_number * page_size > 3000:
            raise InvalidParameter(
                f"page_number * page_size must be <= 3000 (got {page_number * page_size})."
            )

params = self._merge_params(
{
"search": search,
Expand Down Expand Up @@ -379,14 +395,18 @@ def list_skills(
*,
search: str | None = None,
page_number: int = 1,
page_size: int = 20,
page_size: int = 10,
filters: Filters = None,
) -> JSON:
"""``GET /skills`` — list skills.

Filter keys: ``developer``, ``category``, ``license``, ``custom_tag``,
``owner``.
"""
if page_number * page_size > 3000:
raise InvalidParameter(
f"page_number * page_size must be <= 3000 (got {page_number * page_size})."
)
Comment on lines +406 to +409
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 5, 2026

Choose a reason for hiding this comment

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

medium

The pagination parameters page_number and page_size are multiplied to check if they exceed the limit of 3000. However, if either parameter is negative or zero, the check page_number * page_size > 3000 can be bypassed (e.g., -1 * 10 = -10 <= 3000). To prevent sending invalid pagination values to the backend, we should explicitly validate that both page_number and page_size are positive integers (greater than or equal to 1).

        if page_number < 1 or page_size < 1:
            raise InvalidParameter("page_number and page_size must be positive integers.")
        if page_number * page_size > 3000:
            raise InvalidParameter(
                f"page_number * page_size must be <= 3000 (got {page_number * page_size})."
            )

params = self._merge_params(
{
"search": search,
Expand Down Expand Up @@ -427,7 +447,7 @@ def create_studio(self, payload: CreateStudioPayload | Mapping[str, Any]) -> JSO

def get_studio(self, owner: str, repo_name: str) -> JSON:
"""``GET /studios/{owner}/{repo_name}`` — fetch Studio metadata."""
return self._request("GET", f"/studios/{owner}/{repo_name}", require_token=False)
return self._request("GET", f"/studios/{owner}/{repo_name}")

def deploy_studio(
self,
Expand All @@ -439,12 +459,12 @@ def deploy_studio(
return self._request(
"POST",
f"/studios/{owner}/{repo_name}/deploy",
json_body=dict(payload or {}),
json_body=dict(payload) if payload else None,
)

def stop_studio(self, owner: str, repo_name: str) -> JSON:
"""``POST /studios/{owner}/{repo_name}/stop`` — stop a running Studio."""
return self._request("POST", f"/studios/{owner}/{repo_name}/stop")
return self._request("POST", f"/studios/{owner}/{repo_name}/stop", json_body=None)

def get_studio_logs(
self,
Expand Down Expand Up @@ -523,15 +543,28 @@ def list_mcp_servers(
*,
search: str | None = None,
page_number: int = 1,
page_size: int = 20,
page_size: int = 10,
filter: Mapping[str, Any] | None = None,
extra: Mapping[str, Any] | None = None,
) -> JSON:
"""``PUT /mcp/servers`` — discover MCP servers (JSON body, not query)."""
"""``PUT /mcp/servers`` — discover MCP servers (JSON body, not query).

Parameters
----------
filter : dict, optional
Nested filter object. Supported keys: ``category``, ``is_hosted``.
"""
if page_number * page_size > 100:
raise InvalidParameter(
f"page_number * page_size must be <= 100 for MCP servers (got {page_number * page_size})."
)
Comment on lines +557 to +560
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 5, 2026

Choose a reason for hiding this comment

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

medium

The pagination parameters page_number and page_size are multiplied to check if they exceed the limit of 100. However, if either parameter is negative or zero, the check page_number * page_size > 100 can be bypassed (e.g., -1 * 10 = -10 <= 100). To prevent sending invalid pagination values to the backend, we should explicitly validate that both page_number and page_size are positive integers (greater than or equal to 1).

        if page_number < 1 or page_size < 1:
            raise InvalidParameter("page_number and page_size must be positive integers.")
        if page_number * page_size > 100:
            raise InvalidParameter(
                f"page_number * page_size must be <= 100 for MCP servers (got {page_number * page_size})."
            )

body: dict[str, Any] = {
"search": search,
"page_number": page_number,
"page_size": page_size,
}
if filter:
body["filter"] = dict(filter)
if extra:
body.update(extra)
body = {k: v for k, v in body.items() if v is not None}
Expand Down
8 changes: 6 additions & 2 deletions src/modelscope_hub/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ def _normalize_visibility(visibility: int | str | Visibility | None) -> int | No

_PAGED_ITEM_KEYS = (
"items", "list", "data", "results",
"models", "datasets", "skills", "servers",
"models", "datasets", "skills", "servers", "mcp_server_list",
"Models", "Datasets", "Skills", "Servers",
)
_PAGED_META_KEYS = frozenset({
Expand Down Expand Up @@ -824,7 +824,7 @@ def list_repos(
payload = self.openapi.list_mcp_servers(
search=search,
page_number=page_number, page_size=page_size,
extra=clean_filters or None,
filter=clean_filters or None,
)
elif rt is RepoType.STUDIO:
raise NotSupportedError(
Expand All @@ -834,6 +834,10 @@ def list_repos(
raise NotSupportedError(f"list_repos not supported for {rt}")

items, total, page, size = self._extract_paged(payload)
# MCP response omits page_number/page_size — use requested values.
if rt is RepoType.MCP:
page = page_number
size = page_size
infos = [self._repo_info_from_payload(item, rt) for item in items]
return PagedResult(items=infos, total_count=total, page_number=page, page_size=size)

Expand Down
Loading