jerryshao commented on code in PR #12625: URL: https://github.com/apache/gravitino/pull/12625#discussion_r4119886019
########## common/src/main/java/org/apache/gravitino/dto/semantic/SemanticModelDTO.java: ########## @@ -0,0 +1,206 @@ +/* + * 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.semantic; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonPropertyOrder; +import com.google.common.base.Preconditions; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import javax.annotation.Nullable; +import lombok.EqualsAndHashCode; +import lombok.ToString; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.dto.AuditDTO; +import org.apache.gravitino.semantic.SemanticModel; +import org.apache.gravitino.semantic.SemanticModelDefinition; + +/** DTO for a schema-scoped Semantic Model. */ +@EqualsAndHashCode +@ToString +@JsonInclude(JsonInclude.Include.NON_NULL) +@JsonPropertyOrder({"name", "comment", "definition", "properties", "audit"}) +public class SemanticModelDTO implements SemanticModel { + + @JsonProperty("name") + private String name; + + @Nullable + @JsonProperty("comment") + private String comment; + + @JsonProperty("definition") + private SemanticModelDefinitionDTO definition; + + @JsonProperty("properties") + private Map<String, String> properties; + + @JsonProperty("audit") + private AuditDTO audit; + + private SemanticModelDTO() {} + + private SemanticModelDTO( + String name, + @Nullable String comment, + SemanticModelDefinitionDTO definition, + @Nullable Map<String, String> properties, + AuditDTO audit) { + this.name = name; + this.comment = comment; + this.definition = definition; + this.properties = immutableProperties(properties); + this.audit = audit; + } + + @Override + public String name() { + return name; + } + + @Override + @Nullable + public String comment() { + return comment; + } + + /** + * Returns whether this DTO contains a Semantic Model definition. + * + * @return {@code true} if the definition is present, otherwise {@code false}. + */ + public boolean hasDefinition() { + return definition != null; + } + + @Override + public SemanticModelDefinition definition() { + Preconditions.checkArgument(definition != null, "definition must not be null"); + return definition.toDefinition(); + } + + @Override + public Map<String, String> properties() { + return immutableProperties(properties); + } + + @Override + public AuditDTO auditInfo() { + return audit; + } + + /** + * Creates a builder for a Semantic Model DTO. + * + * @return A new builder. + */ + public static Builder builder() { + return new Builder(); + } + + /** Builder for {@link SemanticModelDTO}. */ + public static final class Builder { + + private String name; + @Nullable private String comment; + private SemanticModelDefinitionDTO definition; + @Nullable private Map<String, String> properties; + private AuditDTO audit; + + private Builder() {} + + /** + * Sets the Semantic Model name. + * + * @param name The Semantic Model name. + * @return This builder. + */ + public Builder withName(String name) { + this.name = name; + return this; + } + + /** + * Sets the Semantic Model comment. + * + * @param comment The comment, or {@code null} if it is not set. + * @return This builder. + */ + public Builder withComment(@Nullable String comment) { + this.comment = comment; + return this; + } + + /** + * Sets the Semantic Model definition. + * + * @param definition The complete definition DTO. + * @return This builder. + */ + public Builder withDefinition(SemanticModelDefinitionDTO definition) { + this.definition = definition; + return this; + } + + /** + * Sets the Gravitino-specific Semantic Model properties. + * + * @param properties The properties, or {@code null} if none are set. + * @return This builder. + */ + public Builder withProperties(@Nullable Map<String, String> properties) { + this.properties = properties; + return this; + } + + /** + * Sets the Semantic Model audit information. + * + * @param audit The audit information. + * @return This builder. + */ + public Builder withAudit(AuditDTO audit) { + this.audit = audit; + return this; + } + + /** + * Builds a Semantic Model DTO. + * + * @return The Semantic Model DTO. + * @throws IllegalArgumentException If a required field is missing. + */ + public SemanticModelDTO build() { + Preconditions.checkArgument(StringUtils.isNotBlank(name), "name cannot be null or empty"); + Preconditions.checkArgument(definition != null, "definition cannot be null"); + Preconditions.checkArgument(audit != null, "audit cannot be null"); + + return new SemanticModelDTO(name, comment, definition, properties, audit); Review Comment: [Question] `build()` now stores the caller's definition DTO directly - two side effects worth confirming. Removing the `toDefinition()`/`fromDefinition()` round trip removes the extra traversal, which was the point. It also changes two things that the new tests now pin as intended, so I want to check they are intended rather than incidental: 1. No nested validation at build time. `TestSemanticModelDTO.java:151-161` now asserts that a definition with `new DatasetDTO[] {null}` builds successfully, and `TestSemanticModelResponse.java:94-99` asserts such a payload passes `SemanticModelResponse.validate()`. A client would therefore accept the response and fail later, from `definition()` at line 95, at whatever call site first touches it. That is consistent with other response DTOs being shallow, and the server side cannot produce it (`DTOConverters.toDTO` always builds the definition via `fromDefinition`, so it is well formed by construction) - but it does mean `validate()` is now a presence check only. 2. No defensive copy. The DTO now aliases the caller's `SemanticModelDefinitionDTO`, which holds mutable arrays (`DatasetDTO[]` and friends). No current caller mutates one after `build()` - the only production caller is `DTOConverters.toDTO`, which passes a freshly created instance - so this is latent, not a bug today. If both are intended, no change needed; a short comment on `build()` saying the definition is stored as given and validated on access would save the next reader the same trip. Verified by: read `build()`, `definition()` and `hasDefinition()` on this head; read the only production caller `DTOConverters.toDTO(SemanticModel)` (`common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java:330-338`); grepped `SemanticModelDTO` repo-wide - the only other references are the three test classes; read the two new `assertDoesNotThrow` assertions. Not run - the build rejects JDK 21. ########## common/src/main/java/org/apache/gravitino/dto/requests/SemanticModelCreateRequest.java: ########## @@ -0,0 +1,102 @@ +/* + * 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.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonPropertyOrder; +import com.google.common.base.Preconditions; +import java.util.Map; +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.semantic.SemanticModelDefinitionDTO; +import org.apache.gravitino.rest.RESTRequest; +import org.apache.gravitino.semantic.SemanticModelDefinition; + +/** Represents a request to create a Semantic Model. */ +@Getter +@EqualsAndHashCode +@ToString +@Builder +@Jacksonized +@JsonInclude(JsonInclude.Include.NON_NULL) +@JsonPropertyOrder({"name", "comment", "definition", "properties"}) +public class SemanticModelCreateRequest implements RESTRequest { + + @JsonProperty("name") + private final String name; + + @Nullable + @JsonProperty("comment") + private final String comment; + + @JsonProperty("definition") + private final SemanticModelDefinitionDTO definition; + + @Nullable + @JsonProperty("properties") + private final Map<String, String> properties; + + /** Default constructor for Jackson deserialization. */ + public SemanticModelCreateRequest() { + this(null, null, null, null); + } + + /** + * Creates a Semantic Model create request. + * + * @param name The Semantic Model name. + * @param comment The comment, or {@code null} if it is not set. + * @param definition The required Semantic Model definition. + * @param properties The Gravitino-specific properties, or {@code null} if none are set. + */ + public SemanticModelCreateRequest( + String name, + @Nullable String comment, + SemanticModelDefinitionDTO definition, + @Nullable Map<String, String> properties) { + this.name = name; + this.comment = comment; + this.definition = definition; + this.properties = properties; + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + StringUtils.isNotBlank(name), "\"name\" field is required and cannot be empty"); + Preconditions.checkArgument( + definition != null, "\"definition\" field is required and cannot be null"); + } Review Comment: [Question] `validate()` no longer covers the definition contents - deliberate for the client contract too? Dropping `toDefinition()` from here was the right fix for the double conversion, and the REST path is unaffected: `SemanticModelOperations.java:95` calls `toDefinition()` inside the same `Utils.doAs` try block, so a malformed definition still maps to 400 through the `IllegalArgumentException` branch at `ExceptionHandlers.java:1023`, and `testCreateSemanticModelErrors` (`TestSemanticModelOperations.java:254-276`) still proves it. What changes is the meaning of `RESTRequest.validate()` for anyone who is not this resource. The Java client convention is to call `validate()` on a request before putting it on the wire, so once the client PR lands, a caller passing a dataset array with a null element will now get a server 400 instead of a local `IllegalArgumentException`. If that is the intent, fine - it matches `SchemaCreateRequest.validate()`, which is also shallow. If you would rather keep local failure, the cheap version is to keep the conversion out of `validate()` (as now) and have the client call `toDefinition()` itself before sending. Not blocking either way; mostly worth being explicit about so the client PR does not re-add the eager call and reintroduce the double conversion. Verified by: read `validate()` and `toDefinition()` here on this head; traced the REST call order at `SemanticModelOperations.java:92-102`; confirmed the 400 branch in `SemanticModelExceptionHandler` and the surviving REST cases in `TestSemanticModelOperations`; compared against `SchemaCreateRequest.validate()`. Not run - the build rejects JDK 21. ########## common/src/main/java/org/apache/gravitino/dto/responses/SemanticModelResponse.java: ########## @@ -0,0 +1,65 @@ +/* + * 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.semantic.SemanticModelDTO; + +/** Represents a response containing one Semantic Model. */ +@Getter +@EqualsAndHashCode(callSuper = true) +@ToString +public class SemanticModelResponse extends BaseResponse { + + @JsonProperty("semanticModel") + private final SemanticModelDTO semanticModel; + + /** Default constructor for Jackson deserialization. */ + public SemanticModelResponse() { + super(); + this.semanticModel = null; + } + + /** + * Creates a successful Semantic Model response. + * + * @param semanticModel The Semantic Model DTO. + */ + public SemanticModelResponse(SemanticModelDTO semanticModel) { + super(0); + this.semanticModel = semanticModel; + } + + @Override + public void validate() throws IllegalArgumentException { + super.validate(); + Preconditions.checkArgument(semanticModel != null, "semanticModel must not be null"); + Preconditions.checkArgument( + StringUtils.isNotBlank(semanticModel.name()), + "semanticModel 'name' must not be null or empty"); + Preconditions.checkArgument(semanticModel.hasDefinition(), "definition must not be null"); Review Comment: [Nit] Message does not match the two it sits between. The neighbouring preconditions read `semanticModel 'name' must not be null or empty` (line 60) and `semanticModel 'audit' must not be null` (line 63); this one is bare `definition must not be null`, so a client surfacing the failure loses the object prefix the other two carry. `semanticModel 'definition' must not be null` would line it up. `TestSemanticModelResponse.java:88-90` asserts the current string exactly, so that assertion needs updating in the same change. Verified by: read all four preconditions in this `validate()` on this head, and read the matching assertions in `TestSemanticModelResponse` (lines 84-93), which pin all three messages verbatim. ########## server/src/test/java/org/apache/gravitino/server/web/rest/TestSemanticModelOperations.java: ########## @@ -0,0 +1,495 @@ +/* + * 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.server.web.rest; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.time.Instant; +import java.util.Map; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.client.Entity; +import javax.ws.rs.core.Application; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.catalog.SemanticModelDispatcher; +import org.apache.gravitino.dto.requests.SemanticModelCreateRequest; +import org.apache.gravitino.dto.responses.ErrorConstants; +import org.apache.gravitino.dto.responses.ErrorResponse; +import org.apache.gravitino.dto.responses.SemanticModelResponse; +import org.apache.gravitino.dto.semantic.DatasetDTO; +import org.apache.gravitino.dto.semantic.SemanticModelDefinitionDTO; +import org.apache.gravitino.exceptions.ConnectionFailedException; +import org.apache.gravitino.exceptions.ForbiddenException; +import org.apache.gravitino.exceptions.IllegalSemanticModelException; +import org.apache.gravitino.exceptions.MetalakeNotInUseException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.exceptions.NoSuchSemanticModelException; +import org.apache.gravitino.exceptions.SemanticModelAlreadyExistsException; +import org.apache.gravitino.meta.AuditInfo; +import org.apache.gravitino.meta.SemanticModelEntity; +import org.apache.gravitino.rest.RESTUtils; +import org.apache.gravitino.semantic.AIContext; +import org.apache.gravitino.semantic.CustomExtension; +import org.apache.gravitino.semantic.DataType; +import org.apache.gravitino.semantic.Dataset; +import org.apache.gravitino.semantic.DialectExpression; +import org.apache.gravitino.semantic.Dialects; +import org.apache.gravitino.semantic.Expression; +import org.apache.gravitino.semantic.Field; +import org.apache.gravitino.semantic.Metric; +import org.apache.gravitino.semantic.SemanticModel; +import org.apache.gravitino.semantic.SemanticModelDefinition; +import org.apache.gravitino.utils.NameIdentifierUtil; +import org.apache.gravitino.utils.NamespaceUtil; +import org.glassfish.jersey.internal.inject.AbstractBinder; +import org.glassfish.jersey.server.ResourceConfig; +import org.glassfish.jersey.test.TestProperties; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +/** Tests the Semantic Model create/load REST resource and its error mappings. */ +public class TestSemanticModelOperations extends BaseOperationsTest { + + private static final String VND_V1_JSON = "application/vnd.gravitino.v1+json"; + + private static class MockServletRequestFactory extends ServletRequestFactoryBase { + @Override + public HttpServletRequest get() { + return mock(HttpServletRequest.class); + } + } + + private final SemanticModelDispatcher dispatcher = mock(SemanticModelDispatcher.class); + private final String metalake = "semantic_model_metalake"; + private final String catalog = "semantic_model_catalog"; + private final String schema = "semantic_model_schema"; + private final Namespace namespace = NamespaceUtil.ofSemanticModel(metalake, catalog, schema); + + /** {@inheritDoc} */ + @Override + protected Application configure() { + try { + forceSet( + TestProperties.CONTAINER_PORT, String.valueOf(RESTUtils.findAvailablePort(2000, 3000))); + } catch (IOException e) { + throw new RuntimeException(e); + } + + ResourceConfig resourceConfig = new ResourceConfig(); + resourceConfig.register(SemanticModelOperations.class); + resourceConfig.register( + new AbstractBinder() { + @Override + protected void configure() { + bind(dispatcher).to(SemanticModelDispatcher.class).ranked(2); + bindFactory(MockServletRequestFactory.class).to(HttpServletRequest.class); + } + }); + return resourceConfig; + } + + @BeforeEach + void resetDispatcher() { + reset(dispatcher); + } + + @Test + void testLoadSemanticModelReturnsCompleteDefinitionWithoutWrites() { + NameIdentifier ident = semanticModelIdentifier("sales"); + SemanticModel semanticModel = semanticModel("sales", "Sales definitions"); + when(dispatcher.loadSemanticModel(ident)).thenReturn(semanticModel); + + Response response = get(semanticModelPath() + "/sales"); + + Assertions.assertEquals(Response.Status.OK.getStatusCode(), response.getStatus()); + SemanticModelResponse body = response.readEntity(SemanticModelResponse.class); + body.validate(); + Assertions.assertEquals("sales", body.getSemanticModel().name()); + Assertions.assertEquals("Sales definitions", body.getSemanticModel().comment()); + Assertions.assertEquals(semanticModel.definition(), body.getSemanticModel().definition()); + Assertions.assertEquals("orders", body.getSemanticModel().definition().datasets()[0].name()); + Assertions.assertEquals( + "order_total", body.getSemanticModel().definition().metrics()[0].name()); + Assertions.assertEquals( + "acme", body.getSemanticModel().definition().customExtensions()[0].vendorName()); + Assertions.assertEquals(Map.of("domain", "sales"), body.getSemanticModel().properties()); + Assertions.assertEquals("tester", body.getSemanticModel().auditInfo().creator()); + verify(dispatcher).loadSemanticModel(ident); + verifyNoMoreInteractions(dispatcher); + } + + @Test + void testLoadSemanticModelNotFound() { + NameIdentifier ident = semanticModelIdentifier("sales"); + doThrow(new NoSuchSemanticModelException("sales does not exist")) + .when(dispatcher) + .loadSemanticModel(ident); + + assertError( + get(semanticModelPath() + "/sales"), + Response.Status.NOT_FOUND, + ErrorConstants.NOT_FOUND_CODE, + NoSuchSemanticModelException.class.getSimpleName(), + "sales does not exist"); + } + + @Test + void testLoadSemanticModelNotInUse() { + NameIdentifier ident = semanticModelIdentifier("sales"); + doThrow(new MetalakeNotInUseException("metalake is not in use")) + .when(dispatcher) + .loadSemanticModel(ident); + + assertError( + get(semanticModelPath() + "/sales"), + Response.Status.CONFLICT, + ErrorConstants.NOT_IN_USE_CODE, + MetalakeNotInUseException.class.getSimpleName(), + "metalake is not in use"); + } Review Comment: [Nit] The not-in-use case is covered on load only; create reaches the same check. This test pins the fix well. The asymmetry is that `createSemanticModel` runs the identical path - `SemanticModelOperationDispatcher.checkRelationalCatalog()` (`core/src/main/java/org/apache/gravitino/catalog/SemanticModelOperationDispatcher.java:97`, the load side being line 81) -> `CatalogManager.loadCatalog()` -> `BaseCatalog.checkMetalakeInUse()` - and additionally goes through `schemaDispatcher.loadSchema` (line 99), which can raise `CatalogNotInUseException`. Both resolve through the same new branch, so the risk of a real regression is low; but a create-side twin (or a `CatalogNotInUseException` variant) would cover the other `NotInUseException` subclass and the other endpoint for about four lines. Verified by: read this test and `testCreateSemanticModelErrors` on this head - no create case throws a `NotInUseException` subclass; re-read the dispatcher call path cited above; confirmed `CatalogNotInUseException` and `MetalakeNotInUseException` both extend `NotInUseException` (`api/src/main/java/org/apache/gravitino/exceptions/NotInUseException.java:25`). -- 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]
