Copilot commented on code in PR #10888:
URL: https://github.com/apache/gravitino/pull/10888#discussion_r3154565135
##########
core/src/main/java/org/apache/gravitino/tag/TagManager.java:
##########
@@ -173,10 +173,14 @@ public Tag alterTag(String metalake, String name,
TagChange... changes)
throw new NoSuchTagException(
"Tag with name %s under metalake %s does not exist", name,
metalake);
} catch (EntityAlreadyExistsException e) {
- throw new RuntimeException(
- String.format(
- "Trying to alter tag %s under metalake %s, but the new
name already exists",
- name, metalake));
+ String newName =
+ Arrays.stream(changes)
+ .filter(c -> c instanceof TagChange.RenameTag)
+ .map(c -> ((TagChange.RenameTag) c).getNewName())
+ .findFirst()
+ .orElse(name);
+ throw new TagAlreadyExistsException(
+ e, "Tag with name %s under metalake %s already exists",
newName, metalake);
Review Comment:
In the rename-conflict path, `newName` is derived using `findFirst()` over
`TagChange.RenameTag`, but `updateTagEntity` applies the *last* rename if
multiple rename changes are passed. This can produce an incorrect error message
(and potentially mislead callers). Consider validating that at most one rename
change is provided, or compute `newName` using the same “last rename wins”
logic as `updateTagEntity`.
##########
design-docs/gravitino-function-privilege.md:
##########
@@ -0,0 +1,163 @@
+<!--
+ 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.
+-->
+
+# Design of Function Privilege Control in Gravitino
+
+## Background
+
+Apache Gravitino provides a unified function registry that allows users to
define custom functions (UDFs) once and share them across multiple compute
engines (Trino, Spark, Flink). Gravitino currently supports full function
metadata management — register, list, get, alter, drop — but **does not yet
have privilege control for functions**.
+
+The existing Gravitino access control framework covers catalogs, schemas,
tables, views, topics, filesets, models, tags, policies, and jobs. Each object
type has well-defined privilege types (e.g., `CREATE_TABLE`, `SELECT_TABLE`,
`MODIFY_TABLE`) with privilege inheritance through the metalake → catalog →
schema hierarchy. Functions are the notable gap in this model:
+
+- **No function privilege types** are defined in `Privilege.Name` — any
authenticated user can register, execute, alter, or drop any function.
+- **No function visibility control** — `listFunctions` returns all functions
regardless of the user's permissions.
+- **No ownership tracking** — `FunctionHookDispatcher` is missing, so function
ownership is not set on creation.
+
+---
+
+## Goals
+
+1. **Integrate with Existing Access Control Framework**: Define function
privilege types that follow established Gravitino naming conventions and
privilege inheritance patterns.
+
+2. **Function Visibility Control**: Users should only see functions they have
privileges on. `listFunctions` and `getFunction` should filter results based on
user permissions, consistent with how tables and filesets handle visibility.
+
+3. **Ownership Tracking**: Functions should have owners, set automatically on
registration and manageable through Gravitino's existing ownership mechanism.
+
+4. **Backward Compatibility**: Existing function management APIs remain
unchanged. Privilege enforcement is additive — when authorization is disabled,
behavior is identical to current functionality.
+
+---
+
+## Non-Goals
+
+1. **DEFINER/INVOKER Security Mode**: Gravitino's function metadata model does
not currently include a `securityType` field. Adding security mode support to
functions is a separate metadata model concern and is outside the scope of this
privilege design.
+
+2. **New Function Management Capabilities**: This design adds privilege
control on top of the existing function management API. No new CRUD operations
or metadata model changes are introduced.
+
+3. **Per-Definition Privilege Control**: Privileges are defined at the
function level, not at the individual function definition (overload) level. A
user who can execute a function can execute any of its definitions.
+
+4. **Built-in Function Privilege Control**: Built-in functions provided by
compute engines are not managed by Gravitino and are outside the scope of this
design.
+
+---
+
+## Proposal
+
+### Privilege Types
+
+Three privilege types are defined for functions, following established
Gravitino privilege naming conventions:
+
+| Privilege | Securable Object Levels | Description
|
+|---------------------|-------------------------------------|---------------------------------------------------------------|
+| `REGISTER_FUNCTION` | Metalake, Catalog, Schema | Permission to
register new functions |
+| `EXECUTE_FUNCTION` | Metalake, Catalog, Schema, Function | Permission to
execute/invoke a function and view its metadata |
+| `MODIFY_FUNCTION` | Metalake, Catalog, Schema, Function | Permission to
alter a function's metadata |
+
+**Naming rationale:**
+
+- `REGISTER_FUNCTION` — Consistent with `REGISTER_MODEL` and the
`registerFunction` API method. Gravitino uses "register" for managed metadata
objects (functions, models) to distinguish from "create" used for delegated
objects (tables, views, schemas).
+- `EXECUTE_FUNCTION` — The universally adopted privilege name across all
surveyed systems (Databricks UC, Trino, MySQL, PostgreSQL, OceanBase,
Hologres). Analogous to `SELECT_TABLE` / `SELECT_VIEW` in semantics (the
fundamental "use" privilege), but `EXECUTE` is the standard term for functions.
+- `MODIFY_FUNCTION` — Consistent with `MODIFY_TABLE`. Covers alter operations
on function metadata (comment, definitions, properties). Drop operations
require function ownership, following the same pattern as table and fileset
drop (see `TableOperations.java` and `FilesetOperations.java` where DROP uses
owner-only expressions).
+
+**Privilege inheritance:** Privileges granted at metalake, catalog, or schema
level cascade to all functions within that scope, consistent with existing
behavior for tables and filesets.
+
+**Deny privileges:** Each privilege has a corresponding deny form
(`DENY_REGISTER_FUNCTION`, `DENY_EXECUTE_FUNCTION`, `DENY_MODIFY_FUNCTION`)
following the existing deny privilege pattern.
+
+---
+
+### Securable Object Hierarchy
+
+Functions follow the existing three-level namespace hierarchy:
+
+```
+metalake
+ └── catalog
+ └── schema
+ └── function
+```
+
+This is consistent with tables, filesets, and other schema-scoped objects.
Functions can be registered under any catalog type that supports schemas
(relational, Hive, Iceberg, etc.). A new `MetadataObject.Type.FUNCTION` type
and `SecurableObjects.ofFunction()` factory method are added.
+
+**Privilege applicability by level:**
+
+| Securable Object | REGISTER_FUNCTION | EXECUTE_FUNCTION | MODIFY_FUNCTION |
+|------------------|-------------------|------------------|-----------------|
+| Metalake | ✅ | ✅ | ✅ |
+| Catalog | ✅ | ✅ | ✅ |
+| Schema | ✅ | ✅ | ✅ |
+| Function | — | ✅ | ✅ |
+
Review Comment:
Same table-formatting issue here: the leading `||` results in an extra empty
column when rendered. Use single `|` delimiters for proper markdown table
rendering.
##########
design-docs/gravitino-function-privilege.md:
##########
@@ -0,0 +1,163 @@
+<!--
+ 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.
+-->
+
+# Design of Function Privilege Control in Gravitino
+
+## Background
+
+Apache Gravitino provides a unified function registry that allows users to
define custom functions (UDFs) once and share them across multiple compute
engines (Trino, Spark, Flink). Gravitino currently supports full function
metadata management — register, list, get, alter, drop — but **does not yet
have privilege control for functions**.
+
+The existing Gravitino access control framework covers catalogs, schemas,
tables, views, topics, filesets, models, tags, policies, and jobs. Each object
type has well-defined privilege types (e.g., `CREATE_TABLE`, `SELECT_TABLE`,
`MODIFY_TABLE`) with privilege inheritance through the metalake → catalog →
schema hierarchy. Functions are the notable gap in this model:
+
+- **No function privilege types** are defined in `Privilege.Name` — any
authenticated user can register, execute, alter, or drop any function.
+- **No function visibility control** — `listFunctions` returns all functions
regardless of the user's permissions.
+- **No ownership tracking** — `FunctionHookDispatcher` is missing, so function
ownership is not set on creation.
+
+---
+
+## Goals
+
+1. **Integrate with Existing Access Control Framework**: Define function
privilege types that follow established Gravitino naming conventions and
privilege inheritance patterns.
+
+2. **Function Visibility Control**: Users should only see functions they have
privileges on. `listFunctions` and `getFunction` should filter results based on
user permissions, consistent with how tables and filesets handle visibility.
+
+3. **Ownership Tracking**: Functions should have owners, set automatically on
registration and manageable through Gravitino's existing ownership mechanism.
+
+4. **Backward Compatibility**: Existing function management APIs remain
unchanged. Privilege enforcement is additive — when authorization is disabled,
behavior is identical to current functionality.
+
+---
+
+## Non-Goals
+
+1. **DEFINER/INVOKER Security Mode**: Gravitino's function metadata model does
not currently include a `securityType` field. Adding security mode support to
functions is a separate metadata model concern and is outside the scope of this
privilege design.
+
+2. **New Function Management Capabilities**: This design adds privilege
control on top of the existing function management API. No new CRUD operations
or metadata model changes are introduced.
+
+3. **Per-Definition Privilege Control**: Privileges are defined at the
function level, not at the individual function definition (overload) level. A
user who can execute a function can execute any of its definitions.
+
+4. **Built-in Function Privilege Control**: Built-in functions provided by
compute engines are not managed by Gravitino and are outside the scope of this
design.
+
+---
+
+## Proposal
+
+### Privilege Types
+
+Three privilege types are defined for functions, following established
Gravitino privilege naming conventions:
+
+| Privilege | Securable Object Levels | Description
|
+|---------------------|-------------------------------------|---------------------------------------------------------------|
+| `REGISTER_FUNCTION` | Metalake, Catalog, Schema | Permission to
register new functions |
+| `EXECUTE_FUNCTION` | Metalake, Catalog, Schema, Function | Permission to
execute/invoke a function and view its metadata |
+| `MODIFY_FUNCTION` | Metalake, Catalog, Schema, Function | Permission to
alter a function's metadata |
+
Review Comment:
The markdown tables use a leading double pipe (`|| ...`) which creates an
empty first column in CommonMark/GFM table parsing. Replace with single `|`
delimiters so the tables render correctly.
--
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]
