Copilot commented on code in PR #11058: URL: https://github.com/apache/gravitino/pull/11058#discussion_r3233798900
########## 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)) + Review Comment: `UserDTO` is hashable (`__hash__`) but contains mutable state (`_roles` is a list) and `roles()` exposes the internal list for non-empty roles. If a `UserDTO` is used as a dict key/set element and the returned list is mutated, the object's hash/equality semantics can break (e.g., lookup failures). To fix: make the DTO truly immutable (as described in the PR) by storing roles as an immutable type (e.g., tuple) and/or returning a defensive copy consistently; also consider making the dataclass frozen and ensuring the builder copies input role collections. ########## 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: Review Comment: `_user` defaults to `None` but is annotated as `UserDTO`, and `user()` is annotated to return `UserDTO`. This makes type hints inaccurate and can hide `None` flows from static analysis. Update the annotations to `Optional[UserDTO]` (and correspondingly `user() -> Optional[UserDTO]`), or remove the `None` default and rely on `infer_missing` behavior explicitly via validation. ########## 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 [] Review Comment: `UserDTO` is hashable (`__hash__`) but contains mutable state (`_roles` is a list) and `roles()` exposes the internal list for non-empty roles. If a `UserDTO` is used as a dict key/set element and the returned list is mutated, the object's hash/equality semantics can break (e.g., lookup failures). To fix: make the DTO truly immutable (as described in the PR) by storing roles as an immutable type (e.g., tuple) and/or returning a defensive copy consistently; also consider making the dataclass frozen and ensuring the builder copies input role collections. ########## clients/client-python/gravitino/client/gravitino_metalake.py: ########## @@ -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) Review Comment: The `add_user` parameter is named `user` but represents a user *name* (string), which is ambiguous in a public API and inconsistent with the method name (it could read like passing a User object). Rename the parameter to `name` or `user_name` (and update the precondition message accordingly) to make the API clearer. -- 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]
