Copilot commented on code in PR #11210: URL: https://github.com/apache/gravitino/pull/11210#discussion_r3297926627
########## clients/client-python/gravitino/dto/authorization/securable_object_dto.py: ########## @@ -0,0 +1,119 @@ +# 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 List, Optional + +from dataclasses_json import config, dataclass_json + +from gravitino.api.authorization.privileges import Privilege +from gravitino.api.authorization.securable_objects import SecurableObject +from gravitino.api.metadata_object import MetadataObject +from gravitino.dto.authorization.privilege_dto import PrivilegeDTO + + +def _encode_metadata_type(type_: MetadataObject.Type) -> str: + return type_.value + + +def _decode_metadata_type(val: str) -> MetadataObject.Type: + return MetadataObject.Type(val) + + +@dataclass_json +@dataclass +class SecurableObjectDTO(SecurableObject): + """Data transfer object representing a securable object.""" + + _full_name: str = field(metadata=config(field_name="fullName")) + _type: MetadataObject.Type = field( + metadata=config( + field_name="type", + encoder=_encode_metadata_type, + decoder=_decode_metadata_type, + ) + ) + _privileges: List[PrivilegeDTO] = field( + default_factory=list, metadata=config(field_name="privileges") + ) + + def __post_init__(self): + if self._full_name and "." in self._full_name: + index = self._full_name.rfind(".") + self._parent = self._full_name[:index] + self._name = self._full_name[index + 1 :] + else: + self._parent = None + self._name = self._full_name if self._full_name else "" + + def parent(self) -> Optional[str]: + return self._parent + + def name(self) -> str: + return self._name + + def full_name(self) -> str: + return self._full_name + + def type(self) -> MetadataObject.Type: + return self._type + + def privileges(self) -> List[Privilege]: + return list(self._privileges) if self._privileges else [] + + def __eq__(self, other: object) -> bool: + if not isinstance(other, SecurableObject): + return False + return self._full_name == other.full_name() and self._type == other.type() + + def __hash__(self) -> int: + return hash((self._full_name, self._type)) Review Comment: `SecurableObjectDTO.__eq__` currently ignores privileges and only compares `full_name` and `type`. This makes two securable objects with different privilege sets compare equal (and also makes equality inconsistent with `SecurableObjects.SecurableObjectImpl.__eq__`, which includes privileges). Consider including privileges in the equality check (e.g., compare the privilege collections ignoring order) and update `__hash__` accordingly so it stays consistent with `__eq__`. ########## clients/client-python/gravitino/dto/authorization/privilege_dto.py: ########## @@ -0,0 +1,110 @@ +# 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.api.authorization.privileges import Privilege +from gravitino.api.metadata_object import MetadataObject + + +def _encode_privilege_name(name: Privilege.Name) -> str: + return name.name.lower() + + +def _decode_privilege_name(val: str) -> Privilege.Name: + return Privilege.Name[val.upper()] + + +def _encode_condition(condition: Privilege.Condition) -> str: + return condition.value.lower() + + +def _decode_condition(val: str) -> Privilege.Condition: + return Privilege.Condition(val.upper()) + + +@dataclass_json +@dataclass +class PrivilegeDTO(Privilege): + """Data transfer object representing a privilege.""" + + _name: Privilege.Name = field( + metadata=config( + field_name="name", + encoder=_encode_privilege_name, + decoder=_decode_privilege_name, + ) + ) + _condition: Privilege.Condition = field( + metadata=config( + field_name="condition", + encoder=_encode_condition, + decoder=_decode_condition, + ) + ) + + def name(self) -> Privilege.Name: + return self._name + + def simple_string(self) -> str: + return f"{self._condition.value} {self._name.name.lower().replace('_', ' ')}" + + def condition(self) -> Privilege.Condition: + return self._condition + + def can_bind_to(self, obj_type: MetadataObject.Type) -> bool: + return True + Review Comment: `PrivilegeDTO.can_bind_to()` currently always returns `True`, which violates the `Privilege.can_bind_to` contract (many privileges are only valid for specific `MetadataObject.Type`s). This can lead to incorrect client-side validation/logic when DTOs are used as `Privilege` instances. Consider implementing the same binding rules as the concrete `Privilege` implementations (e.g., by delegating to `Privileges.*` helpers based on `self._name`) and adjusting the unit test accordingly. ########## clients/client-python/tests/unittests/dto/test_privilege_dto.py: ########## @@ -0,0 +1,79 @@ +# 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 json +import unittest + +from gravitino.api.authorization.privileges import Privilege +from gravitino.api.authorization.privileges import Privileges +from gravitino.dto.authorization.privilege_dto import PrivilegeDTO + + +class TestPrivilegeDTO(unittest.TestCase): + def test_create_privilege_dto(self): + dto = PrivilegeDTO(Privilege.Name.CREATE_FILESET, Privilege.Condition.ALLOW) + self.assertEqual(Privilege.Name.CREATE_FILESET, dto.name()) + self.assertEqual(Privilege.Condition.ALLOW, dto.condition()) + + def test_simple_string(self): + dto = PrivilegeDTO(Privilege.Name.CREATE_FILESET, Privilege.Condition.ALLOW) + self.assertEqual("ALLOW create fileset", dto.simple_string()) + + def test_can_bind_to(self): + dto = PrivilegeDTO(Privilege.Name.CREATE_FILESET, Privilege.Condition.ALLOW) + self.assertTrue(dto.can_bind_to(None)) + Review Comment: `test_can_bind_to` passes `None` for `obj_type` and asserts `True`, which effectively locks in the current (overly permissive) `PrivilegeDTO.can_bind_to` behavior. Since `Privilege.can_bind_to` is defined in terms of a `MetadataObject.Type`, consider updating this test to use real `MetadataObject.Type` values and to assert the correct binding rules for the specific privilege name. ########## clients/client-python/gravitino/dto/authorization/role_dto.py: ########## @@ -0,0 +1,119 @@ +# 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.role import Role +from gravitino.api.authorization.securable_objects import SecurableObject +from gravitino.dto.audit_dto import AuditDTO +from gravitino.dto.authorization.securable_object_dto import SecurableObjectDTO + + +@dataclass_json +@dataclass +class RoleDTO(Role): + """Represents a Role Data Transfer Object (DTO).""" + + _name: str = field(metadata=config(field_name="name")) + _properties: Optional[dict[str, str]] = field( + default=None, metadata=config(field_name="properties") + ) + _securable_objects: list[SecurableObjectDTO] = field( + default_factory=list, metadata=config(field_name="securableObjects") + ) + _audit: Optional[AuditDTO] = field( + default=None, metadata=config(field_name="audit") + ) + + def __eq__(self, other: object) -> bool: + if not isinstance(other, RoleDTO): + return False + return ( + self._name == other._name + and self._properties == other._properties + and self._audit == other._audit + ) + + def __hash__(self) -> int: + return hash( + ( + self._name, + frozenset(self._properties.items()) if self._properties else None, + self._audit, + ) + ) Review Comment: `RoleDTO.__eq__`/`__hash__` do not take `_securable_objects` into account. As a result, two roles with the same name/properties/audit but different securable objects (privileges) will compare equal and collide in hashed collections. Consider including the securable objects (order-insensitive) in both equality and hashing, or explicitly documenting why they are intentionally excluded. -- 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]
