dengc367 commented on PR #14239: URL: https://github.com/apache/gravitino/pull/14239#issuecomment-6491343262
Following up on the term/concept split (comment above), three points from checking the code that I think are worth settling before this lands. **1. The `*_DEFINITION` privileges do not exist yet.** `Privilege.java` has 43 privileges and none of them is `*_DEFINITION`. `MANAGE_` exists only for security objects (`MANAGE_USERS`, `MANAGE_GROUPS`, `MANAGE_GRANTS`), never for a metadata object. The only upstream occurrence of `MANAGE_DEFINITIONS` is this PR. So the design's statement that "the 2.0 privilege model uses the DEFINITION privileges for every Define object" refers to something not in the repository. If that model is planned, could you link it? Otherwise the design is building on an unlanded scheme. The three tiers this design needs map 1:1 onto the shape Semantic Model already uses today: | This design | Existing Semantic Model shape | | --- | --- | | `CREATE_DEFINITION` — create, see and edit own drafts | `CREATE_SEMANTIC_MODEL` + `OWNER` | | `VIEW_DEFINITION` — read approved and deprecated | `USE_SEMANTIC_MODEL` | | `MANAGE_DEFINITIONS` — edit any, set status, delete | `MODIFY_SEMANTIC_MODEL` | `OWNER` is already part of the modify expression, so "edit your own term" needs no extra privilege, and `CREATE_` / `USE_` / `MODIFY_` + `OWNER` is a family that exists and is tested. Renaming to `CREATE_TERM` / `USE_TERM` / `MODIFY_TERM` would keep your semantics without introducing a family nothing implements. Domain's two-level `MANAGE_` / `USE_` does not fit here, because it cannot express "create without being able to approve". That distinction is the point of a multi-contributor term base. **2. The three-state status is right — I would keep it.** `DRAFT` / `APPROVED` / `DEPRECATED` is the correct call for a term: terms are referenced by bindings and by users' vocabulary, so "deprecated but still resolvable" is a real state that a delete cannot express. For context, the ontology design settled on Domain's two-state `DRAFT` / `PUBLISHED` instead (PR #14310). Not to differ, but because a vocabulary has no per-item reviewer to record. The two objects end up with different state sets for a reason: a term is reviewed, a vocabulary is published. It would help if both documents said so explicitly, so it does not later read as inconsistency. **3. Term privileges on a domain would duplicate Domain's privileges.** The design grants `CREATE_DEFINITION` / `VIEW_DEFINITION` / `MANAGE_DEFINITIONS` "on the metalake or a domain (inherited by subdomains)". Domain already defines `MANAGE_DOMAIN` / `USE_DOMAIN` on exactly those objects. A domain would then carry two parallel privilege sets, and an administrator would have to configure both before anyone could manage its terms. Suggestion: define the term privileges on the metalake only, and let the domain leg reuse the privileges that already exist — `USE_DOMAIN` to see a domain's terms, `MANAGE_DOMAIN` to approve them. That gives "a Finance domain manager approves Finance terms" without a second scheme, and it also removes the need for `TermScopeResolver` to walk domain parents for a second privilege family. Happy to be wrong on any of these. The first point is a factual check; the other two are trade-offs. -- 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]
