Copilot commented on code in PR #10982: URL: https://github.com/apache/gravitino/pull/10982#discussion_r3201887242
########## common/src/main/java/org/apache/gravitino/dto/rel/ViewDTO.java: ########## @@ -0,0 +1,260 @@ +/* + * 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.rel; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.base.Preconditions; +import java.util.Collections; +import java.util.Map; +import javax.annotation.Nullable; +import lombok.EqualsAndHashCode; +import lombok.ToString; +import org.apache.gravitino.dto.AuditDTO; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.Representation; +import org.apache.gravitino.rel.View; + +/** Represents a View DTO (Data Transfer Object). */ +@JsonIgnoreProperties(ignoreUnknown = true) +@EqualsAndHashCode +@ToString +public class ViewDTO implements View { + + @JsonProperty("name") + private String name; + + @JsonProperty("comment") + @Nullable + private String comment; + + @JsonProperty("columns") + private ColumnDTO[] columns; + + @JsonProperty("representations") + private RepresentationDTO[] representations; + + @JsonProperty("defaultCatalog") + @Nullable + private String defaultCatalog; + + @JsonProperty("defaultSchema") + @Nullable + private String defaultSchema; + + @JsonProperty("properties") + @Nullable + private Map<String, String> properties; + + @JsonProperty("audit") + private AuditDTO audit; + + private ViewDTO() {} + + private ViewDTO( + String name, + @Nullable String comment, + ColumnDTO[] columns, + RepresentationDTO[] representations, + @Nullable String defaultCatalog, + @Nullable String defaultSchema, + @Nullable Map<String, String> properties, + AuditDTO audit) { + this.name = name; + this.comment = comment; + this.columns = columns; + this.representations = representations; + this.defaultCatalog = defaultCatalog; + this.defaultSchema = defaultSchema; + this.properties = properties; + this.audit = audit; + } + + @Override + public String name() { + return name; + } + + @Override + public String comment() { + return comment; + } + + @Override + public Column[] columns() { + return columns; + } + + @Override + public Representation[] representations() { + return representations; + } Review Comment: ViewDTO.columns() returns the raw `columns` field, which can be null when deserialized from JSON (e.g., if the `columns` property is omitted). The View interface contract says implementations should return an empty array rather than null, so this can lead to NPEs in callers. Return an empty array when `columns` is null (and consider the same for `representations`). ########## common/src/main/java/org/apache/gravitino/dto/rel/ViewDTO.java: ########## @@ -0,0 +1,260 @@ +/* + * 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.rel; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.base.Preconditions; +import java.util.Collections; +import java.util.Map; +import javax.annotation.Nullable; +import lombok.EqualsAndHashCode; +import lombok.ToString; +import org.apache.gravitino.dto.AuditDTO; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.Representation; +import org.apache.gravitino.rel.View; + +/** Represents a View DTO (Data Transfer Object). */ +@JsonIgnoreProperties(ignoreUnknown = true) +@EqualsAndHashCode +@ToString +public class ViewDTO implements View { + + @JsonProperty("name") + private String name; + + @JsonProperty("comment") + @Nullable + private String comment; + + @JsonProperty("columns") + private ColumnDTO[] columns; + + @JsonProperty("representations") + private RepresentationDTO[] representations; + + @JsonProperty("defaultCatalog") + @Nullable + private String defaultCatalog; + + @JsonProperty("defaultSchema") + @Nullable + private String defaultSchema; + + @JsonProperty("properties") + @Nullable + private Map<String, String> properties; + + @JsonProperty("audit") + private AuditDTO audit; + + private ViewDTO() {} + + private ViewDTO( + String name, + @Nullable String comment, + ColumnDTO[] columns, + RepresentationDTO[] representations, + @Nullable String defaultCatalog, + @Nullable String defaultSchema, + @Nullable Map<String, String> properties, + AuditDTO audit) { + this.name = name; + this.comment = comment; + this.columns = columns; + this.representations = representations; + this.defaultCatalog = defaultCatalog; + this.defaultSchema = defaultSchema; + this.properties = properties; + this.audit = audit; + } + + @Override + public String name() { + return name; + } + + @Override + public String comment() { + return comment; + } + + @Override + public Column[] columns() { + return columns; + } + + @Override + public Representation[] representations() { + return representations; + } + + @Override + public String defaultCatalog() { + return defaultCatalog; + } + + @Override + public String defaultSchema() { + return defaultSchema; + } + + @Override + public Map<String, String> properties() { + return properties == null ? Collections.emptyMap() : properties; + } + + @Override + public AuditDTO auditInfo() { + return audit; + } + + /** + * Creates a new {@link Builder} to build a {@link ViewDTO}. + * + * @return A new builder instance. + */ + public static Builder builder() { + return new Builder(); + } + + /** Builder class for constructing {@link ViewDTO} instances. */ + public static class Builder { + + private String name; + @Nullable private String comment; + private ColumnDTO[] columns; + private RepresentationDTO[] representations; + @Nullable private String defaultCatalog; + @Nullable private String defaultSchema; + @Nullable private Map<String, String> properties; + private AuditDTO audit; + + private Builder() {} + + /** + * Sets the view name. + * + * @param name The view name. + * @return This builder. + */ + public Builder withName(String name) { + this.name = name; + return this; + } + + /** + * Sets the view comment. + * + * @param comment The view comment. + * @return This builder. + */ + public Builder withComment(@Nullable String comment) { + this.comment = comment; + return this; + } + + /** + * Sets the view columns. + * + * @param columns The view output columns. + * @return This builder. + */ + public Builder withColumns(ColumnDTO[] columns) { + this.columns = columns; + return this; + } + + /** + * Sets the view representations. + * + * @param representations The view representations. + * @return This builder. + */ + public Builder withRepresentations(RepresentationDTO[] representations) { + this.representations = representations; + return this; + } + + /** + * Sets the default catalog used to resolve unqualified identifiers in view representations. + * + * @param defaultCatalog The default catalog, or {@code null} if not set. + * @return This builder. + */ + public Builder withDefaultCatalog(@Nullable String defaultCatalog) { + this.defaultCatalog = defaultCatalog; + return this; + } + + /** + * Sets the default schema used to resolve unqualified identifiers in view representations. + * + * @param defaultSchema The default schema, or {@code null} if not set. + * @return This builder. + */ + public Builder withDefaultSchema(@Nullable String defaultSchema) { + this.defaultSchema = defaultSchema; + return this; + } + + /** + * Sets the view properties. + * + * @param properties The view properties. + * @return This builder. + */ + public Builder withProperties(@Nullable Map<String, String> properties) { + this.properties = properties; + return this; + } + + /** + * Sets the audit information for the view. + * + * @param audit The audit information. + * @return This builder. + */ + public Builder withAudit(AuditDTO audit) { + this.audit = audit; + return this; + } + + /** + * Builds a new {@link ViewDTO}. + * + * @return The constructed instance. + * @throws IllegalArgumentException If required fields are missing. + */ + public ViewDTO build() { + Preconditions.checkArgument(name != null && !name.isEmpty(), "name cannot be null or empty"); + Preconditions.checkArgument(audit != null, "audit cannot be null"); + return new ViewDTO( + name, + comment, + columns == null ? new ColumnDTO[0] : columns, + representations, + defaultCatalog, + defaultSchema, + properties, + audit); Review Comment: ViewDTO.Builder.build() does not validate that `representations` is non-null/non-empty, even though View requires at least one representation. As written, it can build an invalid ViewDTO that later fails response validation or triggers NPEs. Add a Preconditions check for `representations != null && representations.length > 0` (and ideally validate each element). ########## common/src/main/java/org/apache/gravitino/dto/requests/ViewCreateRequest.java: ########## @@ -0,0 +1,134 @@ +/* + * 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.requests; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.base.Preconditions; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import javax.annotation.Nullable; +import lombok.Builder; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import lombok.extern.jackson.Jacksonized; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.dto.rel.ColumnDTO; +import org.apache.gravitino.dto.rel.RepresentationDTO; +import org.apache.gravitino.dto.rel.SQLRepresentationDTO; +import org.apache.gravitino.rest.RESTRequest; + +/** Represents a request to create a view. */ +@Getter +@EqualsAndHashCode +@ToString +@Builder +@Jacksonized +@JsonIgnoreProperties(ignoreUnknown = true) +public class ViewCreateRequest implements RESTRequest { + + @JsonProperty("name") + private final String name; + + @JsonProperty("comment") + @Nullable + private final String comment; + + @JsonProperty("columns") + private final ColumnDTO[] columns; + + @JsonProperty("representations") + private final RepresentationDTO[] representations; + + @JsonProperty("defaultCatalog") + @Nullable + private final String defaultCatalog; + + @JsonProperty("defaultSchema") + @Nullable + private final String defaultSchema; + + @JsonProperty("properties") + @Nullable + private final Map<String, String> properties; + + /** Default constructor for Jackson deserialization. */ + public ViewCreateRequest() { + this(null, null, null, null, null, null, null); + } + + /** + * Creates a new {@link ViewCreateRequest}. + * + * @param name The name of the view. + * @param comment The comment of the view. + * @param columns The output columns of the view. + * @param representations The representations of the view. + * @param defaultCatalog The default catalog used to resolve unqualified identifiers in view + * representations. + * @param defaultSchema The default schema used to resolve unqualified identifiers in view + * representations. + * @param properties The properties of the view. + */ + public ViewCreateRequest( + String name, + @Nullable String comment, + ColumnDTO[] columns, + RepresentationDTO[] representations, + @Nullable String defaultCatalog, + @Nullable String defaultSchema, + @Nullable Map<String, String> properties) { + this.name = name; + this.comment = comment; + this.columns = columns; + this.representations = representations; + this.defaultCatalog = defaultCatalog; + this.defaultSchema = defaultSchema; + this.properties = properties; + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + StringUtils.isNotBlank(name), "\"name\" field is required and cannot be empty"); + Preconditions.checkArgument( + representations != null && representations.length > 0, + "\"representations\" field is required and cannot be empty"); + Arrays.stream(representations) + .forEach( + rep -> { + Preconditions.checkArgument(rep != null, "representation must not be null"); + rep.validate(); + }); + + Set<String> seenDialects = new HashSet<>(); + Arrays.stream(representations) + .filter(rep -> rep instanceof SQLRepresentationDTO) + .map(rep -> ((SQLRepresentationDTO) rep).dialect()) + .forEach( + dialect -> + Preconditions.checkArgument( + seenDialects.add(dialect), + "Duplicate SQL representation dialect: %s", + dialect)); + } Review Comment: ViewCreateRequest.validate() currently validates name and representations but never validates `columns` (or null entries within it). A request with `columns` containing null/invalid ColumnDTOs can slip through and then fail later during conversion/dispatch. Validate the `columns` array when present (check elements non-null and call ColumnDTO.validate()). ########## common/src/main/java/org/apache/gravitino/dto/requests/ViewUpdateRequest.java: ########## @@ -0,0 +1,260 @@ +/* + * 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.requests; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import com.google.common.base.Preconditions; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; +import javax.annotation.Nullable; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.dto.rel.ColumnDTO; +import org.apache.gravitino.dto.rel.RepresentationDTO; +import org.apache.gravitino.dto.rel.SQLRepresentationDTO; +import org.apache.gravitino.dto.util.DTOConverters; +import org.apache.gravitino.rel.ViewChange; +import org.apache.gravitino.rest.RESTRequest; + +/** Represents a request to update a view. */ +@JsonIgnoreProperties(ignoreUnknown = true) +@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY) +@JsonSubTypes({ + @JsonSubTypes.Type(value = ViewUpdateRequest.RenameViewRequest.class, name = "rename"), + @JsonSubTypes.Type(value = ViewUpdateRequest.SetViewPropertyRequest.class, name = "setProperty"), + @JsonSubTypes.Type( + value = ViewUpdateRequest.RemoveViewPropertyRequest.class, + name = "removeProperty"), + @JsonSubTypes.Type(value = ViewUpdateRequest.ReplaceViewRequest.class, name = "replaceView") +}) +public interface ViewUpdateRequest extends RESTRequest { + + /** + * The view change represented by this request. + * + * @return An instance of {@link ViewChange}. + */ + ViewChange viewChange(); + + /** Represents a request to rename a view. */ + @EqualsAndHashCode + @ToString + @Getter + class RenameViewRequest implements ViewUpdateRequest { + + @JsonProperty("newName") + private final String newName; + + /** + * Constructor for RenameViewRequest. + * + * @param newName The new name of the view. + */ + public RenameViewRequest(String newName) { + this.newName = newName; + } + + /** Default constructor for Jackson deserialization. */ + public RenameViewRequest() { + this(null); + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + StringUtils.isNotBlank(newName), "\"newName\" field is required and cannot be empty"); + } + + @Override + public ViewChange viewChange() { + return ViewChange.rename(newName); + } + } + + /** Represents a request to set a property of a view. */ + @EqualsAndHashCode + @ToString + @Getter + class SetViewPropertyRequest implements ViewUpdateRequest { + + @JsonProperty("property") + private final String property; + + @JsonProperty("value") + private final String value; + + /** + * Constructor for SetViewPropertyRequest. + * + * @param property The property to set. + * @param value The value of the property. + */ + public SetViewPropertyRequest(String property, String value) { + this.property = property; + this.value = value; + } + + /** Default constructor for Jackson deserialization. */ + public SetViewPropertyRequest() { + this(null, null); + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + StringUtils.isNotBlank(property), "\"property\" field is required and cannot be empty"); + Preconditions.checkArgument(value != null, "\"value\" field is required and cannot be null"); + } + + @Override + public ViewChange viewChange() { + return ViewChange.setProperty(property, value); + } + } + + /** Represents a request to remove a property of a view. */ + @EqualsAndHashCode + @ToString + @Getter + class RemoveViewPropertyRequest implements ViewUpdateRequest { + + @JsonProperty("property") + private final String property; + + /** + * Constructor for RemoveViewPropertyRequest. + * + * @param property The property to remove. + */ + public RemoveViewPropertyRequest(String property) { + this.property = property; + } + + /** Default constructor for Jackson deserialization. */ + public RemoveViewPropertyRequest() { + this(null); + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + StringUtils.isNotBlank(property), "\"property\" field is required and cannot be empty"); + } + + @Override + public ViewChange viewChange() { + return ViewChange.removeProperty(property); + } + } + + /** + * Represents a request to atomically replace the body (columns, representations, default catalog, + * default schema and comment) of a view. View name and properties are not affected. + */ + @EqualsAndHashCode + @ToString + @Getter + class ReplaceViewRequest implements ViewUpdateRequest { + + @JsonProperty("columns") + private final ColumnDTO[] columns; + + @JsonProperty("representations") + private final RepresentationDTO[] representations; + + @JsonProperty("defaultCatalog") + @Nullable + private final String defaultCatalog; + + @JsonProperty("defaultSchema") + @Nullable + private final String defaultSchema; + + @JsonProperty("comment") + @Nullable + private final String comment; + + /** + * Constructor for ReplaceViewRequest. + * + * @param columns The new output columns of the view. + * @param representations The new representations of the view. + * @param defaultCatalog The new default catalog, or {@code null} to unset it. + * @param defaultSchema The new default schema, or {@code null} to unset it. + * @param comment The new comment, or {@code null} to unset it. + */ + public ReplaceViewRequest( + ColumnDTO[] columns, + RepresentationDTO[] representations, + @Nullable String defaultCatalog, + @Nullable String defaultSchema, + @Nullable String comment) { + this.columns = columns; + this.representations = representations; + this.defaultCatalog = defaultCatalog; + this.defaultSchema = defaultSchema; + this.comment = comment; + } + + /** Default constructor for Jackson deserialization. */ + public ReplaceViewRequest() { + this(null, null, null, null, null); + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + representations != null && representations.length > 0, + "\"representations\" field is required and cannot be empty"); + Arrays.stream(representations) + .forEach( + rep -> { + Preconditions.checkArgument(rep != null, "representation must not be null"); + rep.validate(); + }); + + Set<String> seenDialects = new HashSet<>(); + Arrays.stream(representations) + .filter(rep -> rep instanceof SQLRepresentationDTO) + .map(rep -> ((SQLRepresentationDTO) rep).dialect()) + .forEach( + dialect -> + Preconditions.checkArgument( + seenDialects.add(dialect), + "Duplicate SQL representation dialect: %s", + dialect)); + } Review Comment: ReplaceViewRequest.validate() validates representations but does not validate `columns` when provided. This allows invalid/null ColumnDTO entries and can cause failures later when the ViewChange is applied. Validate the columns array (non-null elements, ColumnDTO.validate()) similarly to TableCreateRequest. ########## common/src/main/java/org/apache/gravitino/dto/responses/ViewResponse.java: ########## @@ -0,0 +1,66 @@ +/* + * 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.rel.ViewDTO; + +/** Represents a response for a view. */ +@Getter +@ToString +@EqualsAndHashCode(callSuper = true) +public class ViewResponse extends BaseResponse { + + @JsonProperty("view") + private final ViewDTO view; + + /** + * Creates a new ViewResponse. + * + * @param view The view DTO object. + */ + public ViewResponse(ViewDTO view) { + super(0); + this.view = view; + } + + /** Default constructor used by Jackson deserializer. */ + public ViewResponse() { + super(); + this.view = null; + } + + @Override + public void validate() throws IllegalArgumentException { + super.validate(); + + Preconditions.checkArgument(view != null, "view must not be null"); + Preconditions.checkArgument( + StringUtils.isNotBlank(view.name()), "view 'name' must not be null and empty"); Review Comment: The validation error message says "must not be null and empty"; this is grammatically incorrect and potentially confusing. Change it to "must not be null or empty" (and keep wording consistent with other response validators). -- 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]
