From 687f945fba5f9d0cb1635a16dd036091be4671f3 Mon Sep 17 00:00:00 2001 From: Sun Yuhan Date: Tue, 12 May 2026 21:03:17 +0800 Subject: [PATCH 1/3] [#10782] feat(client-python): add User authorization management Add User management capabilities to the Python client, supporting the same set of User operations already exposed by the Java client and REST API layer. This is part of a larger split of #10783, which introduced the full authorization model in a single PR. To make review easier, the change is being broken down by entity; subsequent PRs will add Group, Role, Privilege, SecurableObject and grant/revoke operations. Changes - Add User interface and UserDTO (Builder-based, immutable) - Add UserAddRequest / UserResponse / UserListResponse / UserNamesListResponse DTOs, plus a shared RemoveResponse - Add add_user / get_user / remove_user / list_users / list_user_names methods to GravitinoMetalake - Add UserErrorHandler to map User-related REST errors to NoSuchUserException, UserAlreadyExistsException, etc. - Add 28 unit tests covering DTO serialization, response parsing, GravitinoMetalake HTTP interactions (URL, method, body, error propagation) and the error-handler mapping table --- .../gravitino/api/authorization/__init__.py | 2 + .../gravitino/api/authorization/user.py | 47 +++++ .../gravitino/client/gravitino_metalake.py | 70 +++++++ .../gravitino/dto/authorization/user_dto.py | 89 +++++++++ .../dto/requests/user_add_request.py | 38 ++++ .../dto/responses/remove_response.py | 47 +++++ .../gravitino/dto/responses/user_response.py | 76 ++++++++ .../gravitino/exceptions/base.py | 8 + .../exceptions/handlers/user_error_handler.py | 68 +++++++ .../client/test_metalake_user_operations.py | 181 ++++++++++++++++++ .../dto/responses/test_user_response.py | 77 ++++++++ .../tests/unittests/dto/test_user_dto.py | 96 ++++++++++ .../tests/unittests/test_error_handler.py | 58 ++++++ 13 files changed, 857 insertions(+) create mode 100644 clients/client-python/gravitino/api/authorization/user.py create mode 100644 clients/client-python/gravitino/dto/authorization/user_dto.py create mode 100644 clients/client-python/gravitino/dto/requests/user_add_request.py create mode 100644 clients/client-python/gravitino/dto/responses/remove_response.py create mode 100644 clients/client-python/gravitino/dto/responses/user_response.py create mode 100644 clients/client-python/gravitino/exceptions/handlers/user_error_handler.py create mode 100644 clients/client-python/tests/unittests/client/test_metalake_user_operations.py create mode 100644 clients/client-python/tests/unittests/dto/responses/test_user_response.py create mode 100644 clients/client-python/tests/unittests/dto/test_user_dto.py diff --git a/clients/client-python/gravitino/api/authorization/__init__.py b/clients/client-python/gravitino/api/authorization/__init__.py index a593b8d584d..f1b0f5de987 100644 --- a/clients/client-python/gravitino/api/authorization/__init__.py +++ b/clients/client-python/gravitino/api/authorization/__init__.py @@ -20,9 +20,11 @@ from gravitino.api.authorization.privileges import Privileges from gravitino.api.authorization.role import Role from gravitino.api.authorization.securable_objects import SecurableObjects +from gravitino.api.authorization.user import User __all__ = [ "Role", "SecurableObjects", "Privileges", + "User", ] diff --git a/clients/client-python/gravitino/api/authorization/user.py b/clients/client-python/gravitino/api/authorization/user.py new file mode 100644 index 00000000000..9b1153f42c7 --- /dev/null +++ b/clients/client-python/gravitino/api/authorization/user.py @@ -0,0 +1,47 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +from abc import abstractmethod + +from gravitino.api.auditable import Auditable + + +class User(Auditable): + """The interface of a user. The user is a basic entity in the authorization system.""" + + @abstractmethod + def name(self) -> str: + """ + The name of the user. + + Returns: + str: The name of the user. + """ + raise NotImplementedError() + + @abstractmethod + def roles(self) -> list[str]: + """ + The roles of the user. A user can have multiple roles. + Every role binds several privileges. + + Returns: + list[str]: The role names of the user. + """ + raise NotImplementedError() diff --git a/clients/client-python/gravitino/client/gravitino_metalake.py b/clients/client-python/gravitino/client/gravitino_metalake.py index 9d865b20fbb..07d5059dc5e 100644 --- a/clients/client-python/gravitino/client/gravitino_metalake.py +++ b/clients/client-python/gravitino/client/gravitino_metalake.py @@ -19,6 +19,7 @@ from typing import Dict, List, Optional from gravitino.api.authorization.owner import Owner +from gravitino.api.authorization.user import User from gravitino.api.catalog import Catalog from gravitino.api.catalog_change import CatalogChange from gravitino.api.job.job_handle import JobHandle @@ -45,6 +46,7 @@ from gravitino.dto.requests.owner_set_request import OwnerSetRequest from gravitino.dto.requests.tag_create_request import TagCreateRequest from gravitino.dto.requests.tag_updates_request import TagUpdatesRequest +from gravitino.dto.requests.user_add_request import UserAddRequest from gravitino.dto.responses.catalog_list_response import CatalogListResponse from gravitino.dto.responses.catalog_response import CatalogResponse from gravitino.dto.responses.drop_response import DropResponse @@ -54,16 +56,23 @@ from gravitino.dto.responses.job_template_list_response import JobTemplateListResponse from gravitino.dto.responses.job_template_response import JobTemplateResponse from gravitino.dto.responses.owner_response import OwnerResponse +from gravitino.dto.responses.remove_response import RemoveResponse from gravitino.dto.responses.set_response import SetResponse from gravitino.dto.responses.tag_response import ( TagListResponse, TagNamesListResponse, TagResponse, ) +from gravitino.dto.responses.user_response import ( + UserListResponse, + UserNamesListResponse, + UserResponse, +) from gravitino.exceptions.handlers.catalog_error_handler import CATALOG_ERROR_HANDLER from gravitino.exceptions.handlers.job_error_handler import JOB_ERROR_HANDLER from gravitino.exceptions.handlers.owner_error_handler import OWNER_ERROR_HANDLER from gravitino.exceptions.handlers.tag_error_handler import TAG_ERROR_HANDLER +from gravitino.exceptions.handlers.user_error_handler import USER_ERROR_HANDLER from gravitino.rest.rest_utils import encode_string from gravitino.utils.http_client import HTTPClient from gravitino.utils.precondition import Precondition @@ -92,6 +101,10 @@ class GravitinoMetalake( API_METALAKES_TAG_PATH = "api/metalakes/{}/tags/{}" API_METALAKES_TAGS_PATH = "api/metalakes/{}/tags" + # Authorization paths + API_METALAKES_USERS_PATH = "api/metalakes/{}/users" + API_METALAKES_USER_PATH = "api/metalakes/{}/users/{}" + def __init__(self, metalake: MetalakeDTO = None, client: HTTPClient = None): super().__init__( _name=metalake.name(), @@ -767,3 +780,60 @@ def set_owner( ) set_resp = SetResponse.from_json(response.body, infer_missing=True) set_resp.validate() + + #################### + # User operations + #################### + + def add_user(self, user: str) -> User: + """Add a user to this metalake.""" + Precondition.check_string_not_empty(user, "user name must not be null or empty") + req = UserAddRequest(user) + req.validate() + url = self.API_METALAKES_USERS_PATH.format(encode_string(self.name())) + response = self.rest_client.post( + url, json=req, error_handler=USER_ERROR_HANDLER + ) + resp = UserResponse.from_json(response.body, infer_missing=True) + resp.validate() + return resp.user() + + def remove_user(self, user: str) -> bool: + """Remove a user from this metalake.""" + Precondition.check_string_not_empty(user, "user name must not be null or empty") + url = self.API_METALAKES_USER_PATH.format( + encode_string(self.name()), encode_string(user) + ) + response = self.rest_client.delete(url, error_handler=USER_ERROR_HANDLER) + remove_response = RemoveResponse.from_json(response.body, infer_missing=True) + remove_response.validate() + return remove_response.removed() + + def get_user(self, user: str) -> User: + """Get a user by name from this metalake.""" + Precondition.check_string_not_empty(user, "user name must not be null or empty") + url = self.API_METALAKES_USER_PATH.format( + encode_string(self.name()), encode_string(user) + ) + response = self.rest_client.get(url, error_handler=USER_ERROR_HANDLER) + resp = UserResponse.from_json(response.body, infer_missing=True) + resp.validate() + return resp.user() + + def list_users(self) -> list[User]: + """List all users with details under this metalake.""" + url = self.API_METALAKES_USERS_PATH.format(encode_string(self.name())) + response = self.rest_client.get( + url, params={"details": "true"}, error_handler=USER_ERROR_HANDLER + ) + resp = UserListResponse.from_json(response.body, infer_missing=True) + resp.validate() + return resp.users() + + def list_user_names(self) -> list[str]: + """List all user names under this metalake.""" + url = self.API_METALAKES_USERS_PATH.format(encode_string(self.name())) + response = self.rest_client.get(url, error_handler=USER_ERROR_HANDLER) + resp = UserNamesListResponse.from_json(response.body, infer_missing=True) + resp.validate() + return resp.names() diff --git a/clients/client-python/gravitino/dto/authorization/user_dto.py b/clients/client-python/gravitino/dto/authorization/user_dto.py new file mode 100644 index 00000000000..46761920ea2 --- /dev/null +++ b/clients/client-python/gravitino/dto/authorization/user_dto.py @@ -0,0 +1,89 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Optional + +from dataclasses_json import config, dataclass_json + +from gravitino.api.authorization.user import User +from gravitino.dto.audit_dto import AuditDTO + + +@dataclass_json +@dataclass +class UserDTO(User): + """Represents a User Data Transfer Object (DTO).""" + + _name: str = field(metadata=config(field_name="name")) + _roles: list[str] = field(default_factory=list, metadata=config(field_name="roles")) + _audit: Optional[AuditDTO] = field( + default=None, metadata=config(field_name="audit") + ) + + def __eq__(self, other: object) -> bool: + if not isinstance(other, UserDTO): + return False + return ( + self._name == other._name + and self._roles == other._roles + and self._audit == other._audit + ) + + def __hash__(self) -> int: + return hash((self._name, tuple(self._roles), self._audit)) + + @staticmethod + def builder() -> UserDTO.Builder: + return UserDTO.Builder() + + def name(self) -> str: + return self._name + + def roles(self) -> list[str]: + return self._roles if self._roles else [] + + def audit_info(self) -> Optional[AuditDTO]: + return self._audit + + class Builder: + """Helper class to build a UserDTO object.""" + + def __init__(self) -> None: + self._name: str = "" + self._roles: list[str] = [] + self._audit: Optional[AuditDTO] = None + + def with_name(self, name: str) -> UserDTO.Builder: + self._name = name + return self + + def with_roles(self, roles: list[str]) -> UserDTO.Builder: + if roles is not None: + self._roles = roles + return self + + def with_audit(self, audit: AuditDTO) -> UserDTO.Builder: + self._audit = audit + return self + + def build(self) -> UserDTO: + if not self._name: + raise ValueError("name cannot be null or empty") + return UserDTO(self._name, self._roles, self._audit) diff --git a/clients/client-python/gravitino/dto/requests/user_add_request.py b/clients/client-python/gravitino/dto/requests/user_add_request.py new file mode 100644 index 00000000000..df7ab9f9ab8 --- /dev/null +++ b/clients/client-python/gravitino/dto/requests/user_add_request.py @@ -0,0 +1,38 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +from dataclasses import dataclass, field + +from dataclasses_json import config, dataclass_json + +from gravitino.rest.rest_message import RESTRequest +from gravitino.utils.precondition import Precondition + + +@dataclass_json +@dataclass +class UserAddRequest(RESTRequest): + """Represents a request to add a user.""" + + _name: str = field(metadata=config(field_name="name")) + + def validate(self) -> None: + Precondition.check_string_not_empty( + self._name, "name is required and cannot be empty" + ) diff --git a/clients/client-python/gravitino/dto/responses/remove_response.py b/clients/client-python/gravitino/dto/responses/remove_response.py new file mode 100644 index 00000000000..300f466771f --- /dev/null +++ b/clients/client-python/gravitino/dto/responses/remove_response.py @@ -0,0 +1,47 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +from dataclasses import dataclass, field + +from dataclasses_json import config, dataclass_json + +from gravitino.dto.responses.base_response import BaseResponse +from gravitino.utils.precondition import Precondition + + +@dataclass_json +@dataclass +class RemoveResponse(BaseResponse): + """Represents a response for a remove operation.""" + + _removed: bool = field(metadata=config(field_name="removed")) + + def removed(self) -> bool: + return self._removed + + def validate(self) -> None: + """Validates the response. + + Raises: + IllegalArgumentException: If the removed field is not set. + """ + Precondition.check_argument( + self._removed is not None, + "Remove response must contain 'removed' field", + ) diff --git a/clients/client-python/gravitino/dto/responses/user_response.py b/clients/client-python/gravitino/dto/responses/user_response.py new file mode 100644 index 00000000000..b4cd5e33a69 --- /dev/null +++ b/clients/client-python/gravitino/dto/responses/user_response.py @@ -0,0 +1,76 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +from dataclasses import dataclass, field + +from dataclasses_json import config, dataclass_json + +from gravitino.dto.authorization.user_dto import UserDTO +from gravitino.dto.responses.base_response import BaseResponse +from gravitino.utils.precondition import Precondition + + +@dataclass_json +@dataclass +class UserResponse(BaseResponse): + """Represents a response for a user.""" + + _user: UserDTO = field(default=None, metadata=config(field_name="user")) + + def user(self) -> UserDTO: + return self._user + + def validate(self) -> None: + Precondition.check_argument( + self._user is not None, "User response should have a user" + ) + + +@dataclass_json +@dataclass +class UserListResponse(BaseResponse): + """Represents a response for a list of users with details.""" + + _users: list[UserDTO] = field( + default_factory=list, metadata=config(field_name="users") + ) + + def users(self) -> list[UserDTO]: + return self._users + + def validate(self) -> None: + Precondition.check_argument( + self._users is not None, "User list response should have users" + ) + + +@dataclass_json +@dataclass +class UserNamesListResponse(BaseResponse): + """Represents a response for a list of user names.""" + + _names: list[str] = field(default_factory=list, metadata=config(field_name="names")) + + def names(self) -> list[str]: + return self._names + + def validate(self) -> None: + Precondition.check_argument( + self._names is not None, "User names list response should have names" + ) diff --git a/clients/client-python/gravitino/exceptions/base.py b/clients/client-python/gravitino/exceptions/base.py index ced91b9fc9c..354d234f1a1 100644 --- a/clients/client-python/gravitino/exceptions/base.py +++ b/clients/client-python/gravitino/exceptions/base.py @@ -255,3 +255,11 @@ class NoSuchMetadataObjectException(NotFoundException): class RoleAlreadyExistsException(AlreadyExistsException): """Exception thrown when a role with specified name already exists.""" + + +class NoSuchUserException(NotFoundException): + """An exception thrown when a user is not found.""" + + +class UserAlreadyExistsException(AlreadyExistsException): + """An exception thrown when a user already exists.""" diff --git a/clients/client-python/gravitino/exceptions/handlers/user_error_handler.py b/clients/client-python/gravitino/exceptions/handlers/user_error_handler.py new file mode 100644 index 00000000000..61640b72858 --- /dev/null +++ b/clients/client-python/gravitino/exceptions/handlers/user_error_handler.py @@ -0,0 +1,68 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + + +from gravitino.constants.error import ErrorConstants +from gravitino.dto.responses.error_response import ErrorResponse +from gravitino.exceptions.base import ( + IllegalArgumentException, + MetalakeNotInUseException, + NoSuchMetalakeException, + NoSuchRoleException, + NoSuchUserException, + NotFoundException, + UserAlreadyExistsException, +) +from gravitino.exceptions.handlers.rest_error_handler import RestErrorHandler + + +class UserErrorHandler(RestErrorHandler): + """Error handler specific to User operations.""" + + def handle(self, error_response: ErrorResponse): + error_message = error_response.format_error_message() + code = error_response.code() + exception_type = error_response.type() + + if code == ErrorConstants.ILLEGAL_ARGUMENTS_CODE: + raise IllegalArgumentException(error_message) + + if code == ErrorConstants.NOT_FOUND_CODE: + if exception_type == NoSuchMetalakeException.__name__: + raise NoSuchMetalakeException(error_message) + + if exception_type == NoSuchUserException.__name__: + raise NoSuchUserException(error_message) + + if exception_type == NoSuchRoleException.__name__: + raise NoSuchRoleException(error_message) + + raise NotFoundException(error_message) + + if code == ErrorConstants.ALREADY_EXISTS_CODE: + raise UserAlreadyExistsException(error_message) + + if code == ErrorConstants.NOT_IN_USE_CODE: + raise MetalakeNotInUseException(error_message) + + if code == ErrorConstants.INTERNAL_ERROR_CODE: + raise RuntimeError(error_message) + + super().handle(error_response) + + +USER_ERROR_HANDLER = UserErrorHandler() diff --git a/clients/client-python/tests/unittests/client/test_metalake_user_operations.py b/clients/client-python/tests/unittests/client/test_metalake_user_operations.py new file mode 100644 index 00000000000..652cea6e637 --- /dev/null +++ b/clients/client-python/tests/unittests/client/test_metalake_user_operations.py @@ -0,0 +1,181 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import unittest +from unittest.mock import patch + +from gravitino.dto.authorization.user_dto import UserDTO +from gravitino.dto.requests.user_add_request import UserAddRequest +from gravitino.dto.responses.remove_response import RemoveResponse +from gravitino.dto.responses.user_response import ( + UserListResponse, + UserNamesListResponse, + UserResponse, +) +from gravitino.exceptions.base import ( + IllegalArgumentException, + NoSuchMetalakeException, + NoSuchUserException, + UserAlreadyExistsException, +) +from gravitino.exceptions.handlers.user_error_handler import USER_ERROR_HANDLER +from tests.unittests import mock_base + + +def _build_user_dto(name: str = "alice", roles: list | None = None) -> UserDTO: + return ( + UserDTO.builder() + .with_name(name) + .with_roles(roles if roles is not None else []) + .with_audit(mock_base.build_audit_info()) + .build() + ) + + +class TestMetalakeUserOperations(unittest.TestCase): + METALAKE_USERS_PATH = "api/metalakes/metalake_demo/users" + METALAKE_USER_PATH = "api/metalakes/metalake_demo/users/alice" + + def test_add_user(self): + metalake = mock_base.mock_load_metalake() + user = _build_user_dto() + mock_resp = mock_base.mock_http_response(UserResponse(0, user).to_json()) + + with patch( + "gravitino.utils.http_client.HTTPClient.post", return_value=mock_resp + ) as mock_post: + added = metalake.add_user("alice") + + self.assertEqual("alice", added.name()) + self.assertEqual([], added.roles()) + + mock_post.assert_called_once() + call_args = mock_post.call_args + self.assertEqual(self.METALAKE_USERS_PATH, call_args.args[0]) + self.assertIsInstance(call_args.kwargs["json"], UserAddRequest) + self.assertIn('"name": "alice"', call_args.kwargs["json"].to_json()) + self.assertIs(USER_ERROR_HANDLER, call_args.kwargs["error_handler"]) + + def test_add_user_already_exists(self): + metalake = mock_base.mock_load_metalake() + with patch( + "gravitino.utils.http_client.HTTPClient.post", + side_effect=UserAlreadyExistsException("user alice already exists"), + ): + with self.assertRaises(UserAlreadyExistsException): + metalake.add_user("alice") + + def test_add_user_empty_name_rejected(self): + metalake = mock_base.mock_load_metalake() + with self.assertRaises(IllegalArgumentException): + metalake.add_user("") + + def test_get_user(self): + metalake = mock_base.mock_load_metalake() + user = _build_user_dto(roles=["role_a", "role_b"]) + mock_resp = mock_base.mock_http_response(UserResponse(0, user).to_json()) + + with patch( + "gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp + ) as mock_get: + retrieved = metalake.get_user("alice") + + self.assertEqual("alice", retrieved.name()) + self.assertEqual(["role_a", "role_b"], retrieved.roles()) + + mock_get.assert_called_once() + self.assertEqual(self.METALAKE_USER_PATH, mock_get.call_args.args[0]) + self.assertIs( + USER_ERROR_HANDLER, mock_get.call_args.kwargs["error_handler"] + ) + + def test_get_user_not_found(self): + metalake = mock_base.mock_load_metalake() + with patch( + "gravitino.utils.http_client.HTTPClient.get", + side_effect=NoSuchUserException("no such user"), + ): + with self.assertRaises(NoSuchUserException): + metalake.get_user("alice") + + def test_remove_user(self): + metalake = mock_base.mock_load_metalake() + mock_resp = mock_base.mock_http_response(RemoveResponse(0, True).to_json()) + + with patch( + "gravitino.utils.http_client.HTTPClient.delete", return_value=mock_resp + ) as mock_delete: + self.assertTrue(metalake.remove_user("alice")) + + mock_delete.assert_called_once() + self.assertEqual(self.METALAKE_USER_PATH, mock_delete.call_args.args[0]) + self.assertIs( + USER_ERROR_HANDLER, mock_delete.call_args.kwargs["error_handler"] + ) + + def test_remove_user_returns_false_when_not_removed(self): + metalake = mock_base.mock_load_metalake() + mock_resp = mock_base.mock_http_response(RemoveResponse(0, False).to_json()) + + with patch( + "gravitino.utils.http_client.HTTPClient.delete", return_value=mock_resp + ): + self.assertFalse(metalake.remove_user("alice")) + + def test_list_users(self): + metalake = mock_base.mock_load_metalake() + users = [_build_user_dto("alice"), _build_user_dto("bob")] + mock_resp = mock_base.mock_http_response(UserListResponse(0, users).to_json()) + + with patch( + "gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp + ) as mock_get: + result = metalake.list_users() + + self.assertEqual(["alice", "bob"], [u.name() for u in result]) + + mock_get.assert_called_once_with( + self.METALAKE_USERS_PATH, + params={"details": "true"}, + error_handler=USER_ERROR_HANDLER, + ) + + def test_list_user_names(self): + metalake = mock_base.mock_load_metalake() + mock_resp = mock_base.mock_http_response( + UserNamesListResponse(0, ["alice", "bob"]).to_json() + ) + + with patch( + "gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp + ) as mock_get: + names = metalake.list_user_names() + + self.assertEqual(["alice", "bob"], names) + + mock_get.assert_called_once_with( + self.METALAKE_USERS_PATH, error_handler=USER_ERROR_HANDLER + ) + + def test_list_users_metalake_not_found(self): + metalake = mock_base.mock_load_metalake() + with patch( + "gravitino.utils.http_client.HTTPClient.get", + side_effect=NoSuchMetalakeException("metalake not found"), + ): + with self.assertRaises(NoSuchMetalakeException): + metalake.list_users() diff --git a/clients/client-python/tests/unittests/dto/responses/test_user_response.py b/clients/client-python/tests/unittests/dto/responses/test_user_response.py new file mode 100644 index 00000000000..e1070c63b7c --- /dev/null +++ b/clients/client-python/tests/unittests/dto/responses/test_user_response.py @@ -0,0 +1,77 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +import json as _json +import unittest + +from gravitino.dto.authorization.user_dto import UserDTO +from gravitino.dto.responses.user_response import ( + UserListResponse, + UserNamesListResponse, + UserResponse, +) + + +class TestUserResponses(unittest.TestCase): + def test_user_response(self): + user_dto = UserDTO.builder().with_name("user1").build() + resp = UserResponse(0, user_dto) + + resp.validate() + + ser_json = _json.dumps(resp.to_dict()) + deser_dict = _json.loads(ser_json) + + self.assertEqual(user_dto, resp.user()) + self.assertEqual(0, deser_dict["code"]) + self.assertIsNotNone(deser_dict.get("user")) + self.assertEqual("user1", deser_dict["user"]["name"]) + + def test_user_response_validate_no_user(self): + resp = UserResponse(0, None) + with self.assertRaises(ValueError): + resp.validate() + + def test_user_names_list_response(self): + names = ["user1", "user2", "user3"] + resp = UserNamesListResponse(0, names) + + resp.validate() + + ser_json = _json.dumps(resp.to_dict()) + deser_dict = _json.loads(ser_json) + + self.assertEqual(0, deser_dict["code"]) + self.assertEqual(3, len(deser_dict["names"])) + self.assertListEqual(names, deser_dict["names"]) + + def test_user_list_response(self): + user1 = UserDTO.builder().with_name("user1").with_roles(["r1"]).build() + user2 = UserDTO.builder().with_name("user2").build() + + resp = UserListResponse(0, [user1, user2]) + + ser_json = _json.dumps(resp.to_dict()) + deser_dict = _json.loads(ser_json) + + self.assertEqual(0, deser_dict["code"]) + self.assertEqual(2, len(deser_dict["users"])) + self.assertEqual("user1", deser_dict["users"][0]["name"]) + self.assertEqual(["r1"], deser_dict["users"][0]["roles"]) + self.assertEqual("user2", deser_dict["users"][1]["name"]) diff --git a/clients/client-python/tests/unittests/dto/test_user_dto.py b/clients/client-python/tests/unittests/dto/test_user_dto.py new file mode 100644 index 00000000000..d5cf2d93401 --- /dev/null +++ b/clients/client-python/tests/unittests/dto/test_user_dto.py @@ -0,0 +1,96 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import json as _json +import unittest + +from gravitino.dto.audit_dto import AuditDTO +from gravitino.dto.authorization.user_dto import UserDTO + + +class TestUserDTO(unittest.TestCase): + def test_create_user_dto(self): + audit = AuditDTO( + _creator="admin", + _create_time="2024-01-01T00:00:00Z", + ) + user_dto = ( + UserDTO.builder() + .with_name("test_user") + .with_roles(["role1", "role2"]) + .with_audit(audit) + .build() + ) + + ser_json = _json.dumps(user_dto.to_dict()).encode("utf-8") + deser_dict = _json.loads(ser_json) + self.assertEqual(deser_dict["name"], "test_user") + self.assertEqual(deser_dict["roles"], ["role1", "role2"]) + self.assertEqual(deser_dict["audit"]["creator"], "admin") + + def test_user_dto_without_roles(self): + user_dto = UserDTO.builder().with_name("test_user_no_roles").build() + + ser_json = _json.dumps(user_dto.to_dict()).encode("utf-8") + deser_dict = _json.loads(ser_json) + self.assertEqual(deser_dict["name"], "test_user_no_roles") + self.assertEqual(deser_dict["roles"], []) + + def test_user_dto_methods(self): + user_dto = ( + UserDTO.builder() + .with_name("method_user") + .with_roles(["admin_role"]) + .build() + ) + self.assertEqual(user_dto.name(), "method_user") + self.assertEqual(user_dto.roles(), ["admin_role"]) + self.assertIsNone(user_dto.audit_info()) + + def test_builder_empty_name_raises(self): + with self.assertRaises(ValueError): + UserDTO.builder().build() + + def test_equality_and_hash(self): + audit = AuditDTO(_creator="test", _create_time="2024-01-01T00:00:00Z") + + user1 = ( + UserDTO.builder() + .with_name("user1") + .with_roles(["r1"]) + .with_audit(audit) + .build() + ) + user2 = ( + UserDTO.builder() + .with_name("user1") + .with_roles(["r1"]) + .with_audit(audit) + .build() + ) + user3 = ( + UserDTO.builder() + .with_name("user2") + .with_roles(["r1"]) + .with_audit(audit) + .build() + ) + + self.assertEqual(user1, user2) + self.assertEqual(hash(user1), hash(user2)) + self.assertNotEqual(user1, user3) diff --git a/clients/client-python/tests/unittests/test_error_handler.py b/clients/client-python/tests/unittests/test_error_handler.py index a0133a7a974..ca7ceb00a19 100644 --- a/clients/client-python/tests/unittests/test_error_handler.py +++ b/clients/client-python/tests/unittests/test_error_handler.py @@ -34,8 +34,10 @@ NoSuchFilesetException, NoSuchMetalakeException, NoSuchPartitionException, + NoSuchRoleException, NoSuchSchemaException, NoSuchTableException, + NoSuchUserException, NotEmptyException, NotFoundException, NotInUseException, @@ -44,6 +46,7 @@ SchemaAlreadyExistsException, TableAlreadyExistsException, UnsupportedOperationException, + UserAlreadyExistsException, ) from gravitino.exceptions.handlers.catalog_error_handler import CATALOG_ERROR_HANDLER from gravitino.exceptions.handlers.credential_error_handler import ( @@ -57,6 +60,7 @@ from gravitino.exceptions.handlers.rest_error_handler import REST_ERROR_HANDLER from gravitino.exceptions.handlers.schema_error_handler import SCHEMA_ERROR_HANDLER from gravitino.exceptions.handlers.table_error_handler import TABLE_ERROR_HANDLER +from gravitino.exceptions.handlers.user_error_handler import USER_ERROR_HANDLER class TestErrorHandler(unittest.TestCase): @@ -418,3 +422,57 @@ def test_table_error_handler(self): TABLE_ERROR_HANDLER.handle( ErrorResponse.generate_error_response(Exception, "mock error") ) + + def test_user_error_handler(self): + with self.assertRaises(NoSuchMetalakeException): + USER_ERROR_HANDLER.handle( + ErrorResponse.generate_error_response( + NoSuchMetalakeException, "mock error" + ) + ) + + with self.assertRaises(NoSuchUserException): + USER_ERROR_HANDLER.handle( + ErrorResponse.generate_error_response(NoSuchUserException, "mock error") + ) + + with self.assertRaises(NoSuchRoleException): + USER_ERROR_HANDLER.handle( + ErrorResponse.generate_error_response(NoSuchRoleException, "mock error") + ) + + with self.assertRaises(UserAlreadyExistsException): + USER_ERROR_HANDLER.handle( + ErrorResponse.generate_error_response( + UserAlreadyExistsException, "mock error" + ) + ) + + with self.assertRaises(IllegalArgumentException): + USER_ERROR_HANDLER.handle( + ErrorResponse.generate_error_response( + IllegalArgumentException, "mock error" + ) + ) + + with self.assertRaises(MetalakeNotInUseException): + USER_ERROR_HANDLER.handle( + ErrorResponse.generate_error_response( + MetalakeNotInUseException, "mock error" + ) + ) + + with self.assertRaises(NotFoundException): + USER_ERROR_HANDLER.handle( + ErrorResponse.generate_error_response(NotFoundException, "mock error") + ) + + with self.assertRaises(RuntimeError): + USER_ERROR_HANDLER.handle( + ErrorResponse.generate_error_response(InternalError, "mock error") + ) + + with self.assertRaises(RESTException): + USER_ERROR_HANDLER.handle( + ErrorResponse.generate_error_response(Exception, "mock error") + ) From a893a020b42fef632b098cdf421bf90b1ef7f719 Mon Sep 17 00:00:00 2001 From: Sun Yuhan Date: Wed, 13 May 2026 20:34:25 +0800 Subject: [PATCH 2/3] [#10782] feat(client-python): address PR #11058 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add User delegate methods to GravitinoClient (add_user, get_user, remove_user, list_users, list_user_names) — matching the Java SDK pattern and addressing jerryshao's review comment - Make UserDTO._roles immutable (tuple internally, list externally) to preserve hash/equality semantics (Copilot feedback) - Add Optional[UserDTO] type hint to UserResponse._user field (Copilot feedback) - Expand docstrings on GravitinoMetalake User methods with Args/Returns/Raises, keeping `user` parameter name to match Java SDK - Add 7 new tests: UserDTO immutability, GravitinoClient delegation --- .../gravitino/client/gravitino_client.py | 69 +++++++++++++++++++ .../gravitino/client/gravitino_metalake.py | 56 +++++++++++++-- .../gravitino/dto/authorization/user_dto.py | 10 +-- .../gravitino/dto/responses/user_response.py | 5 +- .../client/test_metalake_user_operations.py | 61 ++++++++++++++++ .../tests/unittests/dto/test_user_dto.py | 10 +++ 6 files changed, 200 insertions(+), 11 deletions(-) diff --git a/clients/client-python/gravitino/client/gravitino_client.py b/clients/client-python/gravitino/client/gravitino_client.py index c1cd8e5b270..1503e265068 100644 --- a/clients/client-python/gravitino/client/gravitino_client.py +++ b/clients/client-python/gravitino/client/gravitino_client.py @@ -20,6 +20,7 @@ from typing import Dict, List, Optional from gravitino.api.authorization.owner import Owner +from gravitino.api.authorization.user import User from gravitino.api.catalog import Catalog from gravitino.api.catalog_change import CatalogChange from gravitino.api.job.job_handle import JobHandle @@ -369,3 +370,71 @@ def set_owner( UnsupportedOperationException: If the operation is not supported. """ self.get_metalake().set_owner(metadata_object, owner_name, owner_type) + + # User operations + + def add_user(self, user: str) -> User: + """Add a user to the metalake. + + Args: + user: The name of the user. + + Returns: + The added User object. + + Raises: + UserAlreadyExistsException: If a user with the same name already exists. + NoSuchMetalakeException: If the metalake does not exist. + """ + return self.get_metalake().add_user(user) + + def remove_user(self, user: str) -> bool: + """Remove a user from the metalake. + + Args: + user: The name of the user. + + Returns: + True if the user was removed, False if the user did not exist. + + Raises: + NoSuchMetalakeException: If the metalake does not exist. + """ + return self.get_metalake().remove_user(user) + + def get_user(self, user: str) -> User: + """Get a user by name from the metalake. + + Args: + user: The name of the user. + + Returns: + The User object. + + Raises: + NoSuchUserException: If the user does not exist. + NoSuchMetalakeException: If the metalake does not exist. + """ + return self.get_metalake().get_user(user) + + def list_users(self) -> list[User]: + """List all users with details under the metalake. + + Returns: + A list of User objects. + + Raises: + NoSuchMetalakeException: If the metalake does not exist. + """ + return self.get_metalake().list_users() + + def list_user_names(self) -> list[str]: + """List all user names under the metalake. + + Returns: + A list of user name strings. + + Raises: + NoSuchMetalakeException: If the metalake does not exist. + """ + return self.get_metalake().list_user_names() diff --git a/clients/client-python/gravitino/client/gravitino_metalake.py b/clients/client-python/gravitino/client/gravitino_metalake.py index 07d5059dc5e..4528aac5914 100644 --- a/clients/client-python/gravitino/client/gravitino_metalake.py +++ b/clients/client-python/gravitino/client/gravitino_metalake.py @@ -786,7 +786,18 @@ def set_owner( #################### def add_user(self, user: str) -> User: - """Add a user to this metalake.""" + """Add a user to this metalake. + + Args: + user: The name of the user. + + Returns: + The added User object. + + Raises: + UserAlreadyExistsException: If a user with the same name already exists. + NoSuchMetalakeException: If the metalake does not exist. + """ Precondition.check_string_not_empty(user, "user name must not be null or empty") req = UserAddRequest(user) req.validate() @@ -799,7 +810,17 @@ def add_user(self, user: str) -> User: return resp.user() def remove_user(self, user: str) -> bool: - """Remove a user from this metalake.""" + """Remove a user from this metalake. + + Args: + user: The name of the user. + + Returns: + True if the user was removed, False if the user did not exist. + + Raises: + NoSuchMetalakeException: If the metalake does not exist. + """ Precondition.check_string_not_empty(user, "user name must not be null or empty") url = self.API_METALAKES_USER_PATH.format( encode_string(self.name()), encode_string(user) @@ -810,7 +831,18 @@ def remove_user(self, user: str) -> bool: return remove_response.removed() def get_user(self, user: str) -> User: - """Get a user by name from this metalake.""" + """Get a user by name from this metalake. + + Args: + user: The name of the user. + + Returns: + The User object. + + Raises: + NoSuchUserException: If the user does not exist. + NoSuchMetalakeException: If the metalake does not exist. + """ Precondition.check_string_not_empty(user, "user name must not be null or empty") url = self.API_METALAKES_USER_PATH.format( encode_string(self.name()), encode_string(user) @@ -821,7 +853,14 @@ def get_user(self, user: str) -> User: return resp.user() def list_users(self) -> list[User]: - """List all users with details under this metalake.""" + """List all users with details under this metalake. + + Returns: + A list of User objects. + + Raises: + NoSuchMetalakeException: If the metalake does not exist. + """ url = self.API_METALAKES_USERS_PATH.format(encode_string(self.name())) response = self.rest_client.get( url, params={"details": "true"}, error_handler=USER_ERROR_HANDLER @@ -831,7 +870,14 @@ def list_users(self) -> list[User]: return resp.users() def list_user_names(self) -> list[str]: - """List all user names under this metalake.""" + """List all user names under this metalake. + + Returns: + A list of user name strings. + + Raises: + NoSuchMetalakeException: If the metalake does not exist. + """ url = self.API_METALAKES_USERS_PATH.format(encode_string(self.name())) response = self.rest_client.get(url, error_handler=USER_ERROR_HANDLER) resp = UserNamesListResponse.from_json(response.body, infer_missing=True) diff --git a/clients/client-python/gravitino/dto/authorization/user_dto.py b/clients/client-python/gravitino/dto/authorization/user_dto.py index 46761920ea2..da321985348 100644 --- a/clients/client-python/gravitino/dto/authorization/user_dto.py +++ b/clients/client-python/gravitino/dto/authorization/user_dto.py @@ -32,7 +32,9 @@ class UserDTO(User): """Represents a User Data Transfer Object (DTO).""" _name: str = field(metadata=config(field_name="name")) - _roles: list[str] = field(default_factory=list, metadata=config(field_name="roles")) + _roles: tuple[str, ...] = field( + default_factory=tuple, metadata=config(field_name="roles") + ) _audit: Optional[AuditDTO] = field( default=None, metadata=config(field_name="audit") ) @@ -57,7 +59,7 @@ def name(self) -> str: return self._name def roles(self) -> list[str]: - return self._roles if self._roles else [] + return list(self._roles) if self._roles else [] def audit_info(self) -> Optional[AuditDTO]: return self._audit @@ -67,7 +69,7 @@ class Builder: def __init__(self) -> None: self._name: str = "" - self._roles: list[str] = [] + self._roles: tuple[str, ...] = () self._audit: Optional[AuditDTO] = None def with_name(self, name: str) -> UserDTO.Builder: @@ -76,7 +78,7 @@ def with_name(self, name: str) -> UserDTO.Builder: def with_roles(self, roles: list[str]) -> UserDTO.Builder: if roles is not None: - self._roles = roles + self._roles = tuple(roles) return self def with_audit(self, audit: AuditDTO) -> UserDTO.Builder: diff --git a/clients/client-python/gravitino/dto/responses/user_response.py b/clients/client-python/gravitino/dto/responses/user_response.py index b4cd5e33a69..f90668c5e8b 100644 --- a/clients/client-python/gravitino/dto/responses/user_response.py +++ b/clients/client-python/gravitino/dto/responses/user_response.py @@ -18,6 +18,7 @@ from __future__ import annotations from dataclasses import dataclass, field +from typing import Optional from dataclasses_json import config, dataclass_json @@ -31,9 +32,9 @@ class UserResponse(BaseResponse): """Represents a response for a user.""" - _user: UserDTO = field(default=None, metadata=config(field_name="user")) + _user: Optional[UserDTO] = field(default=None, metadata=config(field_name="user")) - def user(self) -> UserDTO: + def user(self) -> Optional[UserDTO]: return self._user def validate(self) -> None: diff --git a/clients/client-python/tests/unittests/client/test_metalake_user_operations.py b/clients/client-python/tests/unittests/client/test_metalake_user_operations.py index 652cea6e637..3b22e9e35c6 100644 --- a/clients/client-python/tests/unittests/client/test_metalake_user_operations.py +++ b/clients/client-python/tests/unittests/client/test_metalake_user_operations.py @@ -18,6 +18,8 @@ import unittest from unittest.mock import patch +from gravitino.client.gravitino_client import GravitinoClient +from gravitino.client.gravitino_metalake import GravitinoMetalake from gravitino.dto.authorization.user_dto import UserDTO from gravitino.dto.requests.user_add_request import UserAddRequest from gravitino.dto.responses.remove_response import RemoveResponse @@ -179,3 +181,62 @@ def test_list_users_metalake_not_found(self): ): with self.assertRaises(NoSuchMetalakeException): metalake.list_users() + + +class TestGravitinoClientUserDelegates(unittest.TestCase): + """Verify that GravitinoClient correctly delegates User operations.""" + + def _make_client(self): + metalake = mock_base.mock_load_metalake() + client = GravitinoClient.__new__(GravitinoClient) + client._metalake = metalake + return client, metalake + + def test_client_add_user(self): + client, metalake = self._make_client() + user = _build_user_dto() + mock_resp = mock_base.mock_http_response(UserResponse(0, user).to_json()) + with patch( + "gravitino.utils.http_client.HTTPClient.post", return_value=mock_resp + ): + result = client.add_user("alice") + self.assertEqual("alice", result.name()) + + def test_client_get_user(self): + client, metalake = self._make_client() + user = _build_user_dto(roles=["r1"]) + mock_resp = mock_base.mock_http_response(UserResponse(0, user).to_json()) + with patch( + "gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp + ): + result = client.get_user("alice") + self.assertEqual(["r1"], result.roles()) + + def test_client_remove_user(self): + client, metalake = self._make_client() + mock_resp = mock_base.mock_http_response(RemoveResponse(0, True).to_json()) + with patch( + "gravitino.utils.http_client.HTTPClient.delete", return_value=mock_resp + ): + self.assertTrue(client.remove_user("alice")) + + def test_client_list_users(self): + client, metalake = self._make_client() + users = [_build_user_dto("alice"), _build_user_dto("bob")] + mock_resp = mock_base.mock_http_response(UserListResponse(0, users).to_json()) + with patch( + "gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp + ): + result = client.list_users() + self.assertEqual(["alice", "bob"], [u.name() for u in result]) + + def test_client_list_user_names(self): + client, metalake = self._make_client() + mock_resp = mock_base.mock_http_response( + UserNamesListResponse(0, ["alice", "bob"]).to_json() + ) + with patch( + "gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp + ): + result = client.list_user_names() + self.assertEqual(["alice", "bob"], result) diff --git a/clients/client-python/tests/unittests/dto/test_user_dto.py b/clients/client-python/tests/unittests/dto/test_user_dto.py index d5cf2d93401..74bf2b2e10e 100644 --- a/clients/client-python/tests/unittests/dto/test_user_dto.py +++ b/clients/client-python/tests/unittests/dto/test_user_dto.py @@ -94,3 +94,13 @@ def test_equality_and_hash(self): self.assertEqual(user1, user2) self.assertEqual(hash(user1), hash(user2)) self.assertNotEqual(user1, user3) + + def test_roles_returns_list_type(self): + user_dto = UserDTO.builder().with_name("test").with_roles(["r1"]).build() + self.assertIsInstance(user_dto.roles(), list) + + def test_roles_immutability(self): + user_dto = UserDTO.builder().with_name("test").with_roles(["r1", "r2"]).build() + roles = user_dto.roles() + roles.append("r3") + self.assertEqual(["r1", "r2"], user_dto.roles()) From e0bd2e0191fb1fd10aea737ae7a22fb551e124f9 Mon Sep 17 00:00:00 2001 From: Sun Yuhan Date: Thu, 14 May 2026 10:56:59 +0800 Subject: [PATCH 3/3] [#10782] fix(client-python): fix pylint errors in delegate tests Fix CI pylint failure in TestGravitinoClientUserDelegates: - Use patch.object(GravitinoClient, "get_metalake") instead of accessing _metalake directly (protected-access W0212) - Remove unused variable and import warnings (W0612, W0611) --- .../client/test_metalake_user_operations.py | 64 +++++++++++++------ 1 file changed, 45 insertions(+), 19 deletions(-) diff --git a/clients/client-python/tests/unittests/client/test_metalake_user_operations.py b/clients/client-python/tests/unittests/client/test_metalake_user_operations.py index 3b22e9e35c6..da0b67e53f7 100644 --- a/clients/client-python/tests/unittests/client/test_metalake_user_operations.py +++ b/clients/client-python/tests/unittests/client/test_metalake_user_operations.py @@ -19,7 +19,6 @@ from unittest.mock import patch from gravitino.client.gravitino_client import GravitinoClient -from gravitino.client.gravitino_metalake import GravitinoMetalake from gravitino.dto.authorization.user_dto import UserDTO from gravitino.dto.requests.user_add_request import UserAddRequest from gravitino.dto.responses.remove_response import RemoveResponse @@ -187,56 +186,83 @@ class TestGravitinoClientUserDelegates(unittest.TestCase): """Verify that GravitinoClient correctly delegates User operations.""" def _make_client(self): - metalake = mock_base.mock_load_metalake() client = GravitinoClient.__new__(GravitinoClient) - client._metalake = metalake - return client, metalake + return client def test_client_add_user(self): - client, metalake = self._make_client() + client = self._make_client() user = _build_user_dto() mock_resp = mock_base.mock_http_response(UserResponse(0, user).to_json()) - with patch( - "gravitino.utils.http_client.HTTPClient.post", return_value=mock_resp + with ( + patch.object( + GravitinoClient, + "get_metalake", + return_value=mock_base.mock_load_metalake(), + ), + patch( + "gravitino.utils.http_client.HTTPClient.post", return_value=mock_resp + ), ): result = client.add_user("alice") self.assertEqual("alice", result.name()) def test_client_get_user(self): - client, metalake = self._make_client() + client = self._make_client() user = _build_user_dto(roles=["r1"]) mock_resp = mock_base.mock_http_response(UserResponse(0, user).to_json()) - with patch( - "gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp + with ( + patch.object( + GravitinoClient, + "get_metalake", + return_value=mock_base.mock_load_metalake(), + ), + patch("gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp), ): result = client.get_user("alice") self.assertEqual(["r1"], result.roles()) def test_client_remove_user(self): - client, metalake = self._make_client() + client = self._make_client() mock_resp = mock_base.mock_http_response(RemoveResponse(0, True).to_json()) - with patch( - "gravitino.utils.http_client.HTTPClient.delete", return_value=mock_resp + with ( + patch.object( + GravitinoClient, + "get_metalake", + return_value=mock_base.mock_load_metalake(), + ), + patch( + "gravitino.utils.http_client.HTTPClient.delete", return_value=mock_resp + ), ): self.assertTrue(client.remove_user("alice")) def test_client_list_users(self): - client, metalake = self._make_client() + client = self._make_client() users = [_build_user_dto("alice"), _build_user_dto("bob")] mock_resp = mock_base.mock_http_response(UserListResponse(0, users).to_json()) - with patch( - "gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp + with ( + patch.object( + GravitinoClient, + "get_metalake", + return_value=mock_base.mock_load_metalake(), + ), + patch("gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp), ): result = client.list_users() self.assertEqual(["alice", "bob"], [u.name() for u in result]) def test_client_list_user_names(self): - client, metalake = self._make_client() + client = self._make_client() mock_resp = mock_base.mock_http_response( UserNamesListResponse(0, ["alice", "bob"]).to_json() ) - with patch( - "gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp + with ( + patch.object( + GravitinoClient, + "get_metalake", + return_value=mock_base.mock_load_metalake(), + ), + patch("gravitino.utils.http_client.HTTPClient.get", return_value=mock_resp), ): result = client.list_user_names() self.assertEqual(["alice", "bob"], result)