Copilot commented on code in PR #9929: URL: https://github.com/apache/gravitino/pull/9929#discussion_r2785527868
########## core/src/main/java/org/apache/gravitino/auth/PrincipalMapperFactory.java: ########## @@ -0,0 +1,56 @@ +/* + * 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.auth; + +/** Factory class for creating {@link PrincipalMapper} instances. */ +public class PrincipalMapperFactory { + + private PrincipalMapperFactory() {} + + /** + * Creates a principal mapper based on the type and configuration. + * + * @param mapperType "regex" for built-in regex mapper, or fully qualified class name for custom + * mapper + * @param regexPattern the regex pattern (only used when mapperType is "regex") + * @return a configured PrincipalMapper instance + * @throws IllegalArgumentException if the mapper cannot be created + */ + public static PrincipalMapper create(String mapperType, String regexPattern) { + if ("regex".equalsIgnoreCase(mapperType)) { + return new RegexPrincipalMapper(regexPattern); + } Review Comment: `mapperType` is not validated; if it is null/blank, `equalsIgnoreCase` will throw NPE or `Class.forName` will be called with an invalid name. Add an explicit argument check (e.g., Preconditions/checkArgument) for non-null, non-blank `mapperType` and a clearer exception message. ########## core/src/main/java/org/apache/gravitino/auth/RegexPrincipalMapper.java: ########## @@ -0,0 +1,100 @@ +/* + * 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.auth; + +import java.security.Principal; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.apache.gravitino.UserPrincipal; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Regex-based principal mapper that extracts username using regex patterns with capturing groups. + * + * <p>This implementation is thread-safe as Pattern.matcher() creates thread-local Matcher + * instances. + */ +public class RegexPrincipalMapper implements PrincipalMapper { + + private static final Logger LOG = LoggerFactory.getLogger(RegexPrincipalMapper.class); + + private final Pattern pattern; + + /** + * Creates a new regex principal mapper. + * + * @param patternStr the regex pattern with a capturing group (required) + * @throws IllegalArgumentException if the pattern string has invalid regex syntax + */ + public RegexPrincipalMapper(String patternStr) { + if (patternStr == null || patternStr.isEmpty()) { + throw new IllegalArgumentException("Pattern string cannot be null or empty"); + } + this.pattern = Pattern.compile(patternStr); + LOG.info("Initialized RegexPrincipalMapper with pattern: {}", patternStr); + } + + /** + * Maps a principal string to a Principal using the configured regex pattern. + * + * @param principal the principal string to map + * @return a Principal containing the extracted username from the first capturing group, or the + * original principal if no match + * @throws IllegalArgumentException if pattern matching fails + */ + @Override + public Principal map(String principal) { + if (principal == null) { + return null; + } + + try { + Matcher matcher = pattern.matcher(principal); + + // If pattern matches and has at least one capturing group, return the first group + if (matcher.find() && matcher.groupCount() >= 1) { + String extracted = matcher.group(1); + // Return extracted value if it's not null and not empty, otherwise original principal + String username = (extracted != null && !extracted.isEmpty()) ? extracted : principal; + return new UserPrincipal(username); + } + + // If pattern doesn't match or has no capturing groups, return original principal + return new UserPrincipal(principal); + + } catch (Exception e) { + String message = + String.format( + "Error applying regex pattern '%s' to principal '%s'", pattern.pattern(), principal); + LOG.error("{}: {}", message, e.getMessage()); + throw new IllegalArgumentException(message, e); + } Review Comment: This logs the full principal value on mapping failures, which can unintentionally expose user identities (and potentially other sensitive principal formats) in server logs. Consider redacting/masking the principal in the log message (or logging at DEBUG), and avoid catching generic `Exception`—catch the specific runtime exceptions you expect from regex/matcher usage. ```suggestion Matcher matcher = pattern.matcher(principal); // If pattern matches and has at least one capturing group, return the first group if (matcher.find() && matcher.groupCount() >= 1) { String extracted = matcher.group(1); // Return extracted value if it's not null and not empty, otherwise original principal String username = (extracted != null && !extracted.isEmpty()) ? extracted : principal; return new UserPrincipal(username); } // If pattern doesn't match or has no capturing groups, return original principal return new UserPrincipal(principal); ``` ########## catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/converter/HiveDatabaseConverter.java: ########## @@ -58,6 +58,7 @@ public static HiveSchema fromHiveDB(Database db) { .build(); return hiveSchema; } + /** * Add a comment on lines L57 to L65Add diff commentMarkdown input: edit mode * selected.WritePreviewHeadingBoldItalicQuoteCodeLinkUnordered listNumbered listTask Review Comment: There appears to be accidental, non-Javadoc text committed into the source (likely pasted from a code review UI). This will ship into artifacts and also breaks documentation quality. Please delete these lines and keep only meaningful JavaDoc. ########## core/src/main/java/org/apache/gravitino/auth/RegexPrincipalMapper.java: ########## @@ -0,0 +1,100 @@ +/* + * 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.auth; + +import java.security.Principal; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.apache.gravitino.UserPrincipal; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Regex-based principal mapper that extracts username using regex patterns with capturing groups. + * + * <p>This implementation is thread-safe as Pattern.matcher() creates thread-local Matcher + * instances. + */ +public class RegexPrincipalMapper implements PrincipalMapper { + + private static final Logger LOG = LoggerFactory.getLogger(RegexPrincipalMapper.class); + + private final Pattern pattern; + + /** + * Creates a new regex principal mapper. + * + * @param patternStr the regex pattern with a capturing group (required) + * @throws IllegalArgumentException if the pattern string has invalid regex syntax + */ + public RegexPrincipalMapper(String patternStr) { + if (patternStr == null || patternStr.isEmpty()) { + throw new IllegalArgumentException("Pattern string cannot be null or empty"); + } + this.pattern = Pattern.compile(patternStr); + LOG.info("Initialized RegexPrincipalMapper with pattern: {}", patternStr); + } + + /** + * Maps a principal string to a Principal using the configured regex pattern. + * + * @param principal the principal string to map + * @return a Principal containing the extracted username from the first capturing group, or the + * original principal if no match + * @throws IllegalArgumentException if pattern matching fails + */ + @Override + public Principal map(String principal) { + if (principal == null) { + return null; Review Comment: Returning null from `PrincipalMapper.map` makes downstream code prone to NPEs and contradicts the interface contract that maps a principal string to a Principal (and typically throws for invalid inputs). Prefer throwing `IllegalArgumentException` for null/empty input or returning a `UserPrincipal` consistently. ```suggestion * @throws IllegalArgumentException if the principal string is null or empty, or if pattern * matching fails */ @Override public Principal map(String principal) { if (principal == null || principal.isEmpty()) { throw new IllegalArgumentException("Principal string cannot be null or empty"); ``` ########## core/src/main/java/org/apache/gravitino/EntityStore.java: ########## @@ -165,6 +167,35 @@ <E extends Entity & HasIdentifier> E update( <E extends Entity & HasIdentifier> E get(NameIdentifier ident, EntityType entityType, Class<E> e) throws NoSuchEntityException, IOException; + /** + * Batch get the entity from the underlying storage. + * + * @param idents the unique identifier of the entity + * @param entityType the general type of the entity + * @param clazz the entity class instance + * @param <E> the class of entity + * @return the entity retrieved from the underlying storage + * @throws NoSuchEntityException if the entity does not exist + */ + <E extends Entity & HasIdentifier> List<E> batchGet( + List<NameIdentifier> idents, EntityType entityType, Class<E> clazz); Review Comment: The existing `get(...)` API declares `throws NoSuchEntityException, IOException`, but the new `batchGet(List<...>)` does not declare any checked exceptions. That inconsistency forces implementers either to wrap checked exceptions or change error semantics. Align the batch API by declaring the same checked exceptions (or clearly documenting that it never throws them and how missing entities are represented). ########## catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java: ########## @@ -620,6 +625,29 @@ public void testConnection( } } + /** + * Load view metadata from the Iceberg catalog. + * + * <p>Delegates to the underlying Iceberg REST catalog to load view metadata. + * + * @param ident The identifier of the view to load. + * @return The loaded view metadata. + * @throws NoSuchViewException If the view does not exist. + */ + @Override + public View loadView(NameIdentifier ident) throws NoSuchViewException { + try { + LoadViewResponse response = + icebergCatalogWrapper.loadView( + IcebergCatalogWrapperHelper.buildIcebergTableIdentifier(ident)); + + return IcebergView.fromLoadViewResponse(response, ident.name()); + } catch (Exception e) { + throw new NoSuchViewException( + e, "Failed to load view %s from Iceberg catalog: %s", ident, e.getMessage()); Review Comment: Catching all exceptions and converting them into `NoSuchViewException` can mask real failures (network/auth/misconfiguration) as 'not found'. Only translate the specific Iceberg 'not found' exception(s) to `NoSuchViewException`, and let other exceptions propagate as appropriate (or wrap them in a more accurate exception type). ```suggestion } catch (org.apache.iceberg.exceptions.NoSuchViewException e) { throw new NoSuchViewException( e, "View %s does not exist in Iceberg catalog", ident); ``` ########## common/src/main/java/org/apache/gravitino/dto/responses/FunctionResponse.java: ########## @@ -0,0 +1,70 @@ +/* + * 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.dto.responses; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.base.Preconditions; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.dto.function.FunctionDTO; + +/** Response for function operations. */ +@Getter +@ToString +@EqualsAndHashCode(callSuper = true) +public class FunctionResponse extends BaseResponse { + + @JsonProperty("function") + private final FunctionDTO function; + + /** Constructor for FunctionResponse. */ + public FunctionResponse() { + super(0); + this.function = null; + } + + /** + * Constructor for FunctionResponse. + * + * @param function the function DTO object. + */ + public FunctionResponse(FunctionDTO function) { + super(0); + this.function = function; + } + + /** + * Validates the response. + * + * @throws IllegalArgumentException if the response is invalid. + */ + @Override + public void validate() throws IllegalArgumentException { + super.validate(); + Preconditions.checkArgument(function != null, "function must not be null"); + Preconditions.checkArgument( + StringUtils.isNotBlank(function.name()), "function 'name' must not be null and empty"); + Preconditions.checkArgument( + function.functionType() != null, "function 'functionType' must not be null"); + Preconditions.checkArgument( + function.definitions() != null, "function 'definitions' must not be null"); Review Comment: `FunctionDTO.definitions()` returns an empty array when the underlying field is null, so `function.definitions() != null` will always pass and won't actually validate presence of definitions. If the response requires definitions, validate `function.definitions().length > 0` (or validate the DTO field directly). ```suggestion function.definitions().length > 0, "function 'definitions' must not be empty"); ``` ########## common/src/main/java/org/apache/gravitino/dto/responses/FunctionListResponse.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.dto.responses; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.base.Preconditions; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import org.apache.gravitino.dto.function.FunctionDTO; + +/** Response wrapper for multiple functions. */ +@Getter +@ToString +@EqualsAndHashCode(callSuper = true) +public class FunctionListResponse extends BaseResponse { + + @JsonProperty("functions") + private FunctionDTO[] functions; + + /** + * Creates a response containing multiple functions. + * + * @param functions Function array payload. + */ + public FunctionListResponse(FunctionDTO[] functions) { + super(0); + this.functions = functions; + } + + private FunctionListResponse() { Review Comment: The no-arg constructor is private. For Jackson (and similar serializers) response types typically need a public or at least package-visible no-arg constructor unless you have explicit configuration allowing private access. Consider making this constructor public/package-private to avoid deserialization failures. ```suggestion FunctionListResponse() { ``` ########## core/src/main/java/org/apache/gravitino/GravitinoEnv.java: ########## @@ -270,7 +274,16 @@ public FunctionDispatcher functionDispatcher() { } /** - * Get the PartitionDispatcher associated with the Gravitino environment. + * Get the ViewDispatcher associated with the Gravitino environment. + * + * @return The ViewDispatcher instance. + */ + public ViewDispatcher viewDispatcher() { + return viewDispatcher; + } + + /** + * * Get the PartitionDispatcher associated with the Gravitino environment. Review Comment: The JavaDoc line starts with an extra `*` (`* * Get...`), which is a typo and will render oddly in generated docs. Remove the extra asterisk so the comment reads normally. ```suggestion * Get the PartitionDispatcher associated with the Gravitino environment. ``` ########## core/src/main/java/org/apache/gravitino/auxiliary/GravitinoAuxiliaryService.java: ########## @@ -38,8 +38,10 @@ public interface GravitinoAuxiliaryService { /** * @param config GravitinoServer will pass the config with prefix * `gravitino.auxService.{shortName}.` to aux server + * @param auxMode whether the service is running as an auxiliary service (true) or standalone + * (false) */ - void serviceInit(Map<String, String> config); + void serviceInit(Map<String, String> config, boolean auxMode); Review Comment: Changing an interface method signature is a breaking change for all auxiliary service implementations. If backward compatibility is required, consider adding a default overload `serviceInit(Map<String, String> config)` that delegates to the new method with an appropriate default for `auxMode`. ########## clients/client-python/gravitino/dto/responses/metadata_object_list_response.py: ########## @@ -0,0 +1,59 @@ +# 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 + +from gravitino.dto.metadata_object_dto import MetadataObjectDTO +from gravitino.dto.responses.base_response import BaseResponse +from gravitino.utils.precondition import Precondition + + Review Comment: This response class uses `dataclasses_json.config(...)` metadata on fields, but it is missing the `@dataclass_json` decorator. Without it, `from_json`/`to_json` (and related helpers) won't work as expected for this DTO. Add `@dataclass_json` to match the other response DTO patterns. ```suggestion from dataclasses_json import config, dataclass_json from gravitino.dto.metadata_object_dto import MetadataObjectDTO from gravitino.dto.responses.base_response import BaseResponse from gravitino.utils.precondition import Precondition @dataclass_json ``` ########## api/src/main/java/org/apache/gravitino/authorization/Privileges.java: ########## @@ -1237,4 +1257,66 @@ public boolean canBindTo(MetadataObject.Type type) { return type == MetadataObject.Type.METALAKE || type == MetadataObject.Type.JOB_TEMPLATE; } } + + /** The privilege to create a view. */ + public static class CreateView extends GenericPrivilege<CreateView> { + private static final CreateView ALLOW_INSTANCE = + new CreateView(Condition.ALLOW, Name.CREATE_VIEW); + private static final CreateView DENY_INSTANCE = + new CreateView(Condition.DENY, Name.CREATE_VIEW); + + private CreateView(Condition condition, Name name) { + super(condition, name); + } + + /** + * @return The instance with allow condition of the privilege. + */ + public static CreateView allow() { + return ALLOW_INSTANCE; + } + + /** + * @return The instance with deny condition of the privilege. + */ + public static CreateView deny() { + return DENY_INSTANCE; + } + + @Override + public boolean canBindTo(MetadataObject.Type type) { + return SCHEMA_SUPPORTED_TYPES.contains(type); + } Review Comment: `CreateView.canBindTo` currently uses `SCHEMA_SUPPORTED_TYPES`, which (per earlier definition) includes `FILESET`. That contradicts the added tests in `TestSecurableObjects` expecting `createView.canBindTo(FILESET)` to be false. Define a dedicated supported-type set for CREATE_VIEW (likely METALAKE/CATALOG/SCHEMA only) and use that here. -- 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]
