Copilot commented on code in PR #14281: URL: https://github.com/apache/gravitino/pull/14281#discussion_r4411019722
########## core/src/main/java/org/apache/gravitino/meta/DomainEntity.java: ########## @@ -0,0 +1,200 @@ +/* + * 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. + */ +package org.apache.gravitino.meta; + +import com.google.common.base.Preconditions; +import com.google.common.collect.Maps; +import java.util.Collections; +import java.util.Map; +import java.util.Optional; +import javax.annotation.Nullable; +import lombok.AccessLevel; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import lombok.experimental.Accessors; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Audit; +import org.apache.gravitino.Auditable; +import org.apache.gravitino.Entity; +import org.apache.gravitino.EntityFieldLimits; +import org.apache.gravitino.Field; +import org.apache.gravitino.HasIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.domain.Domain; +import org.apache.gravitino.tag.TagValue; + +/** + * The stored form of a {@link Domain}. It holds the domain's data only; the operations on {@link + * Domain} are served by the domain manager. + * + * <p>The backing tag is stored with the domain as the ID of the tag and an optional value; the tag + * name is read from the tag when the domain is read. If the tag is deleted, the ID stays on the + * domain but has no name, so the domain has no backing tag. The parent is not stored with the + * domain: it comes from the sub-domain relation and is set on the entity when it is read. + */ +@Getter +@Accessors(fluent = true) +@EqualsAndHashCode +@ToString +public class DomainEntity implements Entity, Auditable, HasIdentifier { + + /** The unique id of the domain entity. */ + public static final Field ID = + Field.required("id", Long.class, "The unique id of the domain entity."); + + /** The name of the domain entity. */ + public static final Field NAME = + Field.required("name", "The name of the domain entity.", EntityFieldLimits.MAX_NAME_LENGTH); + + /** The comment of the domain entity. */ + public static final Field COMMENT = + Field.optional( + "comment", "The comment of the domain entity.", EntityFieldLimits.MAX_COMMENT_LENGTH); + + /** The properties of the domain entity. */ + public static final Field PROPERTIES = + Field.optional("properties", Map.class, "The properties of the domain entity."); + + /** The status of the domain entity. */ + public static final Field STATUS = + Field.required("status", Domain.Status.class, "The status of the domain entity."); + + /** The ID of the backing tag of the domain entity. */ + public static final Field BACKING_TAG_ID = + Field.optional( + "backing_tag_id", Long.class, "The ID of the backing tag of the domain entity."); + + /** The value of the backing tag of the domain entity. */ + public static final Field BACKING_TAG_VALUE = + Field.optional( + "backing_tag_value", + "The value of the backing tag of the domain entity.", + EntityFieldLimits.MAX_TAG_VALUE_LENGTH); + + /** The audit details of the domain entity. */ + public static final Field AUDIT_INFO = + Field.required("audit_info", Audit.class, "The audit details of the domain entity."); + + private final Long id; + private final String name; + private final Namespace namespace; + @Nullable private final String comment; + private final Map<String, String> properties; + private final Domain.Status status; + + @Getter(AccessLevel.NONE) + @Nullable + private final String parent; + + /** The ID of the backing tag, or null if the domain has none. */ + @Nullable private final Long backingTagId; + + /** The name of the backing tag, or null if the domain has none or the tag is deleted. */ + @Getter(AccessLevel.NONE) + @Nullable + private final String backingTagName; + + /** The value of the backing tag, or null if the tag is bound without a value. */ + @Nullable private final String backingTagValue; + + private final Audit auditInfo; + + @lombok.Builder(setterPrefix = "with", toBuilder = true) + private DomainEntity( + Long id, + String name, + Namespace namespace, + @Nullable String comment, + Map<String, String> properties, + Domain.Status status, + @Nullable String parent, + @Nullable Long backingTagId, + @Nullable String backingTagName, + @Nullable String backingTagValue, + Audit auditInfo) { + this.id = id; + this.name = name; + this.namespace = namespace; + this.comment = comment; + this.properties = properties; + this.status = status; + this.parent = parent; + this.backingTagId = backingTagId; + // The name is read from the bound tag, so it follows the ID: clearing the ID unbinds the tag. + this.backingTagName = backingTagId == null ? null : backingTagName; + this.backingTagValue = backingTagValue; + this.auditInfo = auditInfo; + validate(); + Preconditions.checkArgument( + backingTagId != null || backingTagValue == null, + "The backing tag value of domain %s needs a backing tag ID", + name); + // A tag bound without a value is stored as NULL, so the value is never blank. + Preconditions.checkArgument( + backingTagValue == null || StringUtils.isNotBlank(backingTagValue), + "The backing tag value of domain %s must not be blank", + name); + } + Review Comment: `validate()` is called but no `validate` method is defined in `DomainEntity` (and the class doesn't extend a base class that would provide it). This will fail compilation. Add a private `validate()` method (consistent with other `*Entity` implementations) or remove the call and inline the required field/limit checks here. ########## core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/DomainMetaBaseSQLProvider.java: ########## @@ -0,0 +1,302 @@ +/* + * 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. + */ +package org.apache.gravitino.storage.relational.mapper.provider.base; + +import static org.apache.gravitino.storage.relational.mapper.DomainMetaMapper.DOMAIN_TABLE_NAME; + +import java.util.List; +import org.apache.gravitino.storage.relational.mapper.MetalakeMetaMapper; +import org.apache.gravitino.storage.relational.mapper.TagMetaMapper; +import org.apache.gravitino.storage.relational.mapper.provider.DatabaseTimeSQL; +import org.apache.gravitino.storage.relational.po.DomainPO; +import org.apache.ibatis.annotations.Param; + +/** The SQL of the {@code domain_meta} table shared by all backends. */ +public class DomainMetaBaseSQLProvider { + + private static final String DOMAIN_COLUMNS = + "dm.domain_id as domainId, dm.domain_name as domainName," + + " dm.metalake_id as metalakeId," + + " dm.domain_comment as comment," + + " dm.domain_status as status," + + " dm.backing_tag_id as backingTagId," + + " dm.backing_tag_value as backingTagValue," + + " dm.properties as properties," + + " dm.audit_info as auditInfo," + + " dm.current_version as currentVersion," + + " dm.last_version as lastVersion," + + " dm.deleted_at as deletedAt"; + + // Reads take the backing tag name from the live tag, so a deleted tag reads as no backing tag. + // Locking reads do not join: they must lock the domain row only, and PostgreSQL rejects FOR + // UPDATE on the nullable side of an outer join. + private static final String DOMAIN_COLUMNS_WITH_BACKING_TAG_NAME = + DOMAIN_COLUMNS + ", bt.tag_name as backingTagName"; + + private static final String BACKING_TAG_JOIN = + " LEFT JOIN " + + TagMetaMapper.TAG_TABLE_NAME + + " bt ON dm.backing_tag_id = bt.tag_id AND dm.metalake_id = bt.metalake_id" + + " AND bt.deleted_at = 0"; + + /** + * Returns SQL that lists the active domains of a metalake. + * + * @param metalakeName The metalake name. + * @return The SQL. + */ + public String listDomainPOsByMetalake(@Param("metalakeName") String metalakeName) { + return "SELECT " + + DOMAIN_COLUMNS_WITH_BACKING_TAG_NAME + + " FROM " + + DOMAIN_TABLE_NAME + + " dm JOIN " + + MetalakeMetaMapper.TABLE_NAME + + " mm ON dm.metalake_id = mm.metalake_id" + + BACKING_TAG_JOIN + + " WHERE mm.metalake_name = #{metalakeName} AND dm.deleted_at = 0 AND mm.deleted_at = 0"; + } + + /** + * Returns SQL that selects an active domain by metalake name and domain name. + * + * @param metalakeName The metalake name. + * @param domainName The domain name. + * @return The SQL. + */ + public String selectDomainMetaByMetalakeAndName( + @Param("metalakeName") String metalakeName, @Param("domainName") String domainName) { + return "SELECT " + + DOMAIN_COLUMNS_WITH_BACKING_TAG_NAME + + " FROM " + + DOMAIN_TABLE_NAME + + " dm JOIN " + + MetalakeMetaMapper.TABLE_NAME + + " mm ON dm.metalake_id = mm.metalake_id" + + BACKING_TAG_JOIN + + " WHERE mm.metalake_name = #{metalakeName} AND dm.domain_name = #{domainName}" + + " AND dm.deleted_at = 0 AND mm.deleted_at = 0"; + } + + /** + * Returns SQL that selects an active domain by metalake ID and domain name. + * + * @param metalakeId The metalake ID. + * @param domainName The domain name. + * @return The SQL. + */ + public String selectDomainMetaByMetalakeIdAndName( + @Param("metalakeId") Long metalakeId, @Param("domainName") String domainName) { + return "SELECT " + + DOMAIN_COLUMNS + + " FROM " + + DOMAIN_TABLE_NAME + + " dm WHERE dm.metalake_id = #{metalakeId} AND dm.domain_name = #{domainName}" + + " AND dm.deleted_at = 0"; + } + + /** + * Returns SQL that selects and exclusively locks an active domain by its natural key. + * + * @param metalakeId The metalake ID. + * @param domainName The domain name. + * @return The SQL. + */ + public String selectDomainMetaByMetalakeIdAndNameForUpdate( + @Param("metalakeId") Long metalakeId, @Param("domainName") String domainName) { + return selectDomainMetaByMetalakeIdAndName(metalakeId, domainName) + " FOR UPDATE"; + } + + /** + * Returns SQL that selects an active domain by ID. + * + * @param domainId The domain ID. + * @return The SQL. + */ + public String selectDomainMetaById(@Param("domainId") Long domainId) { + return "SELECT " + + DOMAIN_COLUMNS_WITH_BACKING_TAG_NAME + + " FROM " + + DOMAIN_TABLE_NAME + + " dm" + + BACKING_TAG_JOIN + + " WHERE dm.domain_id = #{domainId} AND dm.deleted_at = 0"; + } + + /** + * Returns SQL that selects and exclusively locks an active domain by ID. + * + * @param domainId The domain ID. + * @return The SQL. + */ + public String selectDomainMetaByIdForUpdate(@Param("domainId") Long domainId) { + return "SELECT " + + DOMAIN_COLUMNS + + " FROM " + + DOMAIN_TABLE_NAME + + " dm WHERE dm.domain_id = #{domainId} AND dm.deleted_at = 0 FOR UPDATE"; + } + + /** + * Returns SQL that lists the active domains with the given IDs. + * + * @param domainIds The domain IDs. + * @return The SQL. + */ + public String listDomainPOsByDomainIds(@Param("domainIds") List<Long> domainIds) { + return "<script>" + + "SELECT " + + DOMAIN_COLUMNS + + " FROM " + + DOMAIN_TABLE_NAME + + " dm WHERE dm.deleted_at = 0" + + " AND dm.domain_id IN (" + + "<foreach collection='domainIds' item='domainId' separator=','>" + + "#{domainId}" + + "</foreach>" + + ")" Review Comment: When `domainIds` is empty, the generated SQL becomes `... IN ()`, which is invalid SQL and will fail at runtime if this mapper is called directly with an empty list. Add an `<if>` guard in the script (returning a `WHERE 1=0` query) or enforce non-empty lists at the mapper boundary. ########## core/src/main/java/org/apache/gravitino/storage/relational/po/DomainPO.java: ########## @@ -0,0 +1,200 @@ +/* + * 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. + */ +package org.apache.gravitino.storage.relational.po; + +import static org.apache.gravitino.storage.relational.utils.POConverters.DEFAULT_DELETED_AT; +import static org.apache.gravitino.storage.relational.utils.POConverters.INIT_VERSION; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.google.common.base.Preconditions; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import lombok.experimental.Accessors; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.domain.Domain; +import org.apache.gravitino.json.JsonUtils; +import org.apache.gravitino.meta.AuditInfo; +import org.apache.gravitino.meta.DomainEntity; + +/** + * The persistent object of a row in {@code domain_meta}. The backing tag name is not a column of + * the row: reads that join the live tag fill it, and it is null when the domain has no backing tag + * or the tag is deleted. + * + * <p>A tag bound without a value has a NULL {@code backing_tag_value}, while {@code + * tag_relation_meta.tag_value} stores the empty string for an assignment without a value. A lookup + * from tag assignments to the domains they back must map the empty string to NULL instead of + * comparing the two columns directly. + */ +@Getter +@Accessors(fluent = true) +@EqualsAndHashCode +@ToString +public class DomainPO { + + private Long domainId; + private String domainName; + private Long metalakeId; + private String comment; + private String status; + private Long backingTagId; + private String backingTagName; + private String backingTagValue; + private String properties; + private String auditInfo; + private Long currentVersion; + private Long lastVersion; + private Long deletedAt; + + /** Creates an empty domain persistent object, used by MyBatis to map result rows. */ + public DomainPO() {} + + @lombok.Builder(setterPrefix = "with") + private DomainPO( + Long domainId, + String domainName, + Long metalakeId, + String comment, + String status, + Long backingTagId, + String backingTagValue, + String properties, + String auditInfo, + Long currentVersion, + Long lastVersion, + Long deletedAt) { + Preconditions.checkArgument(domainId != null, "domainId cannot be null"); + Preconditions.checkArgument(StringUtils.isNotBlank(domainName), "domainName cannot be blank"); + Preconditions.checkArgument(metalakeId != null, "metalakeId cannot be null"); + Preconditions.checkArgument(StringUtils.isNotBlank(status), "status cannot be blank"); + Preconditions.checkArgument( + backingTagId != null || backingTagValue == null, + "backingTagValue cannot be set without backingTagId"); + Preconditions.checkArgument(StringUtils.isNotBlank(auditInfo), "auditInfo cannot be blank"); + Preconditions.checkArgument(currentVersion != null, "currentVersion cannot be null"); + Preconditions.checkArgument(lastVersion != null, "lastVersion cannot be null"); + Preconditions.checkArgument(deletedAt != null, "deletedAt cannot be null"); + + this.domainId = domainId; + this.domainName = domainName; + this.metalakeId = metalakeId; + this.comment = comment; + this.status = status; + this.backingTagId = backingTagId; + this.backingTagValue = backingTagValue; + this.properties = properties; + this.auditInfo = auditInfo; + this.currentVersion = currentVersion; + this.lastVersion = lastVersion; + this.deletedAt = deletedAt; + } + + /** + * Creates the first version of the persistent object of a new domain. + * + * @param domainEntity The domain entity. + * @param metalakeId The ID of the metalake of the domain. + * @return The domain persistent object. + */ + public static DomainPO initializeDomainPO(DomainEntity domainEntity, Long metalakeId) { + return toDomainPO(domainEntity, domainEntity.id(), metalakeId, INIT_VERSION); + } + + /** + * Creates the next version of the persistent object of a domain from its updated entity. + * + * @param oldDomainPO The domain persistent object observed by the caller. + * @param newDomainEntity The updated domain entity. + * @return The domain persistent object with the next version. + */ + public static DomainPO updateDomainPO(DomainPO oldDomainPO, DomainEntity newDomainEntity) { + Long nextVersion = oldDomainPO.currentVersion() + 1; + return toDomainPO( + newDomainEntity, oldDomainPO.domainId(), oldDomainPO.metalakeId(), nextVersion); + } + + /** + * Converts a domain persistent object to a domain entity. The parent is not stored in the domain + * row, so it is not set. The backing tag name is set only when the read joined the live tag. + * + * @param domainPO The domain persistent object. + * @param namespace The namespace of the domain. + * @return The domain entity. + */ + @SuppressWarnings("unchecked") + public static DomainEntity fromDomainPO(DomainPO domainPO, Namespace namespace) { + try { + return DomainEntity.builder() + .withId(domainPO.domainId()) + .withName(domainPO.domainName()) + .withNamespace(namespace) + .withComment(domainPO.comment()) + .withStatus(Domain.Status.valueOf(domainPO.status())) + .withBackingTagId(domainPO.backingTagId()) + .withBackingTagName(domainPO.backingTagName()) + .withBackingTagValue(domainPO.backingTagValue()) + .withProperties(JsonUtils.anyFieldMapper().readValue(domainPO.properties(), Map.class)) + .withAuditInfo( + JsonUtils.anyFieldMapper().readValue(domainPO.auditInfo(), AuditInfo.class)) + .build(); + } catch (JsonProcessingException e) { + throw new RuntimeException("Failed to deserialize domain PO", e); + } + } Review Comment: `readValue(domainPO.properties(), ...)` and `readValue(domainPO.auditInfo(), ...)` will throw `IllegalArgumentException` if the DB column value is `NULL` (schema defaults allow `properties` to be NULL). Currently only `JsonProcessingException` is caught, so a NULL column can surface as an unexpected runtime exception. Handle NULL explicitly (e.g., treat NULL as `null`/empty map) and/or broaden the catch to include `IllegalArgumentException` with a clearer failure message. ########## core/src/main/java/org/apache/gravitino/utils/NamespaceUtil.java: ########## @@ -424,6 +434,19 @@ public static void checkJobTemplate(Namespace namespace) { namespace); } + /** + * Check if the given domain namespace is legal, throw an {@link IllegalNamespaceException} if + * it's illegal. + * + * @param namespace The domain namespace + */ + public static void checkDomain(Namespace namespace) { + check( + namespace != null && namespace.length() == 3, + "Domain namespace must be non-null and have 3 levels, the input namespace is %s", Review Comment: `checkDomain` only validates the namespace length, but does not enforce that level(1) is the system catalog reserved name and level(2) is the domain schema name. This makes `NameIdentifierUtil.checkDomain` accept unrelated 3-level namespaces, which can mis-route reads/writes. Tighten this check to validate the expected `<metalake>.system.domain` structure (similar to other `check*` methods in this class). -- 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]
