Copilot commented on code in PR #11109: URL: https://github.com/apache/gravitino/pull/11109#discussion_r3273793337
########## clients/client-python/tests/unittests/dto/responses/test_group_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 + +import json as _json +import unittest + +from gravitino.dto.authorization.group_dto import GroupDTO +from gravitino.dto.responses.group_response import ( + GroupListResponse, + GroupNamesListResponse, + GroupResponse, +) + + +class TestGroupResponses(unittest.TestCase): + def test_group_response(self): + group_dto = GroupDTO.builder().with_name("group1").build() + resp = GroupResponse(0, group_dto) Review Comment: This test constructs `GroupDTO` without audit info (`GroupDTO.builder().with_name(...).build()`), which conflicts with `tests/unittests/dto/test_group_dto.py::test_builder_no_audit_raises` expecting audit to be mandatory. Align the response tests with the chosen DTO contract (either always provide audit in test DTOs, or relax the builder/test expectation). ########## clients/client-python/gravitino/dto/authorization/group_dto.py: ########## @@ -0,0 +1,91 @@ +# 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.group import Group +from gravitino.dto.audit_dto import AuditDTO + + +@dataclass_json +@dataclass +class GroupDTO(Group): + """Represents a Group Data Transfer Object (DTO).""" + + _name: str = field(metadata=config(field_name="name")) + _roles: tuple[str, ...] = field( + default_factory=tuple, 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, GroupDTO): + 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() -> GroupDTO.Builder: + return GroupDTO.Builder() + + def name(self) -> str: + return self._name + + def roles(self) -> list[str]: + return list(self._roles) if self._roles else [] + + def audit_info(self) -> Optional[AuditDTO]: + return self._audit + + class Builder: + """Helper class to build a GroupDTO object.""" + + def __init__(self) -> None: + self._name: str = "" + self._roles: tuple[str, ...] = () + self._audit: Optional[AuditDTO] = None + + def with_name(self, name: str) -> GroupDTO.Builder: + self._name = name + return self + + def with_roles(self, roles: list[str]) -> GroupDTO.Builder: + if roles is not None: + self._roles = tuple(roles) + return self + + def with_audit(self, audit: AuditDTO) -> GroupDTO.Builder: + self._audit = audit + return self + + def build(self) -> GroupDTO: + if not self._name: + raise ValueError("name cannot be null or empty") + return GroupDTO(self._name, self._roles, self._audit) Review Comment: `GroupDTO.Builder.build()` only validates `_name`, but `tests/unittests/dto/test_group_dto.py::test_builder_no_audit_raises` expects a failure when audit is missing. This currently makes the unit tests inconsistent and likely failing. Either (a) enforce non-null audit in `build()` (and/or `__post_init__`) or (b) update the tests/spec to allow groups without audit; please keep behavior consistent with the expected contract of `Group`/`Auditable`. ########## clients/client-python/tests/unittests/dto/responses/test_group_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 + +import json as _json +import unittest + +from gravitino.dto.authorization.group_dto import GroupDTO +from gravitino.dto.responses.group_response import ( + GroupListResponse, + GroupNamesListResponse, + GroupResponse, +) + + +class TestGroupResponses(unittest.TestCase): + def test_group_response(self): + group_dto = GroupDTO.builder().with_name("group1").build() + resp = GroupResponse(0, group_dto) + + resp.validate() + + ser_json = _json.dumps(resp.to_dict()) + deser_dict = _json.loads(ser_json) + + self.assertEqual(group_dto, resp.group()) + self.assertEqual(0, deser_dict["code"]) + self.assertIsNotNone(deser_dict.get("group")) + self.assertEqual("group1", deser_dict["group"]["name"]) + + def test_group_response_validate_no_group(self): + resp = GroupResponse(0, None) + with self.assertRaises(ValueError): + resp.validate() + + def test_group_names_list_response(self): + names = ["group1", "group2"] + resp = GroupNamesListResponse(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(2, len(deser_dict["names"])) + self.assertListEqual(names, deser_dict["names"]) + + def test_group_list_response(self): + group1 = GroupDTO.builder().with_name("group1").with_roles(["r1"]).build() + group2 = GroupDTO.builder().with_name("group2").build() + Review Comment: `test_group_list_response` builds `GroupDTO` instances without audit for `group1`/`group2`, which will break if audit is required (as implied by `test_group_dto.py::test_builder_no_audit_raises`). Update these fixtures to match the intended `GroupDTO` invariants. ########## clients/client-python/tests/integration/test_group_management.py: ########## @@ -0,0 +1,123 @@ +# 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 logging +import os +import uuid + +from gravitino import GravitinoAdminClient, GravitinoClient +from gravitino.exceptions.base import ( + GroupAlreadyExistsException, + NoSuchGroupException, +) +from tests.integration.integration_test_env import IntegrationTestEnv + +logger = logging.getLogger(__name__) + + +class TestGroupManagement(IntegrationTestEnv): + _metalake_name: str = f"test_group_metalake_{uuid.uuid4().hex[:8]}" + _gravitino_admin_client: GravitinoAdminClient = None + _gravitino_client: GravitinoClient = None + + @classmethod + def setUpClass(cls): + cls._get_gravitino_home() + conf_path = os.path.join(cls.gravitino_home, "conf", "gravitino.conf") + cls._reset_conf( + {"gravitino.authorization.enable": "true"}, conf_path + ) + cls._append_conf( + {"gravitino.authorization.enable": "true"}, conf_path Review Comment: `setUpClass()` enables `gravitino.authorization.enable=true` but does not configure `gravitino.authorization.serviceAdmins`. The server-side default authorization impl is Jcasbin (`Configs.AUTHORIZATION_IMPL`) and `AccessControlManager.isServiceAdmin()` calls `serviceAdmins.contains(...)`, which can NPE (and/or cause all privileged ops to be denied) when serviceAdmins isn’t set. Add a service-admin user in the test config (e.g., `gravitino.authorization.serviceAdmins = anonymous` or whatever principal the Python client uses) to make these integration tests reliable. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
