This is an automated email from the ASF dual-hosted git repository.
mchades pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new fa2ba52269 [#14697] fix(common): validate view representation types
(#14698)
fa2ba52269 is described below
commit fa2ba52269d1798fd794f491fb0d807d7f25b925
Author: mchades <[email protected]>
AuthorDate: Fri Nov 6 20:28:08 2026 +0800
[#14697] fix(common): validate view representation types (#14698)
### What changes were proposed in this pull request?
Preserve the supplied representation `type` and require it to equal
`sql` during validation. Reject invalid create and `replaceView`
requests before dispatch, while preserving storage deserialization of
representations without a type.
### Why are the changes needed?
Invalid types currently fall back to SQL and bypass validation, contrary
to the existing OpenAPI contract.
Fixes #14697
### Does this PR introduce _any_ user-facing change?
Yes. Missing, null, blank, unknown, or incorrectly cased representation
types now return HTTP 400. Valid SQL requests remain supported.
### How was this patch tested?
- 74 targeted tests passed across representation/View DTOs, DTO
converters, `TestViewPO`, and `TestViewOperations`.
- New regression tests failed before the fix; HTTP create tests observed
200 instead of 400.
- HTTP tests verify rejection of both create and replace requests
without invoking the dispatcher.
- Common/server Spotless checks and `git diff --check` passed.
---
.../gravitino/dto/rel/RepresentationDTO.java | 2 +
.../gravitino/dto/rel/SQLRepresentationDTO.java | 17 +++++++-
.../gravitino/dto/rel/TestRepresentationDTO.java | 23 ++++++++++
.../server/web/rest/TestViewOperations.java | 51 ++++++++++++++++++++++
4 files changed, 92 insertions(+), 1 deletion(-)
diff --git
a/common/src/main/java/org/apache/gravitino/dto/rel/RepresentationDTO.java
b/common/src/main/java/org/apache/gravitino/dto/rel/RepresentationDTO.java
index 6db44da3c4..bc1776ce1d 100644
--- a/common/src/main/java/org/apache/gravitino/dto/rel/RepresentationDTO.java
+++ b/common/src/main/java/org/apache/gravitino/dto/rel/RepresentationDTO.java
@@ -27,6 +27,8 @@ import org.apache.gravitino.rel.SQLRepresentation;
/**
* A DTO mirroring {@link org.apache.gravitino.rel.Representation}.
Representation DTOs are
* serialized with a {@code type} discriminator so Jackson can deserialize
into the correct subtype.
+ *
+ * <p>Request validation must call {@link #validate()} to reject missing or
invalid types.
*/
@JsonIgnoreProperties(ignoreUnknown = true)
@JsonTypeInfo(
diff --git
a/common/src/main/java/org/apache/gravitino/dto/rel/SQLRepresentationDTO.java
b/common/src/main/java/org/apache/gravitino/dto/rel/SQLRepresentationDTO.java
index 995f638b30..992707b073 100644
---
a/common/src/main/java/org/apache/gravitino/dto/rel/SQLRepresentationDTO.java
+++
b/common/src/main/java/org/apache/gravitino/dto/rel/SQLRepresentationDTO.java
@@ -22,6 +22,7 @@ import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Preconditions;
import java.util.Objects;
+import javax.annotation.Nullable;
import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.rel.Representation;
@@ -32,8 +33,10 @@ import org.apache.gravitino.rel.Representation;
@JsonIgnoreProperties(ignoreUnknown = true)
public final class SQLRepresentationDTO extends RepresentationDTO {
+ // Preserve the supplied discriminator so validation can reject missing or
invalid types.
@JsonProperty("type")
- private final String type = Representation.TYPE_SQL;
+ @Nullable
+ private String type;
@JsonProperty("dialect")
private String dialect;
@@ -46,6 +49,7 @@ public final class SQLRepresentationDTO extends
RepresentationDTO {
}
private SQLRepresentationDTO(String dialect, String sql) {
+ this.type = Representation.TYPE_SQL;
this.dialect = dialect;
this.sql = sql;
}
@@ -59,7 +63,17 @@ public final class SQLRepresentationDTO extends
RepresentationDTO {
return new Builder();
}
+ /**
+ * Returns the supplied representation type.
+ *
+ * <p>Before successful validation, a deserialized instance may contain a
null or invalid type.
+ * Instances created by the builder or successfully validated by {@link
#validate()} return {@link
+ * Representation#TYPE_SQL}.
+ *
+ * @return The supplied type, or {@code null} if it was omitted or
explicitly null.
+ */
@Override
+ @Nullable
public String type() {
return type;
}
@@ -84,6 +98,7 @@ public final class SQLRepresentationDTO extends
RepresentationDTO {
@Override
public void validate() throws IllegalArgumentException {
+ Preconditions.checkArgument(Representation.TYPE_SQL.equals(type),
"\"type\" must be \"sql\"");
Preconditions.checkArgument(
StringUtils.isNotBlank(dialect), "\"dialect\" field is required and
cannot be empty");
Preconditions.checkArgument(
diff --git
a/common/src/test/java/org/apache/gravitino/dto/rel/TestRepresentationDTO.java
b/common/src/test/java/org/apache/gravitino/dto/rel/TestRepresentationDTO.java
index 2e8c53717d..8fbc033e76 100644
---
a/common/src/test/java/org/apache/gravitino/dto/rel/TestRepresentationDTO.java
+++
b/common/src/test/java/org/apache/gravitino/dto/rel/TestRepresentationDTO.java
@@ -25,6 +25,8 @@ import org.apache.gravitino.rel.Representation;
import org.apache.gravitino.rel.SQLRepresentation;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
public class TestRepresentationDTO {
@@ -39,12 +41,33 @@ public class TestRepresentationDTO {
RepresentationDTO deserialized =
JsonUtils.objectMapper().readValue(json, RepresentationDTO.class);
+ deserialized.validate();
Assertions.assertInstanceOf(SQLRepresentationDTO.class, deserialized);
Assertions.assertEquals(Representation.TYPE_SQL, deserialized.type());
Assertions.assertEquals("spark", ((SQLRepresentationDTO)
deserialized).dialect());
Assertions.assertEquals("SELECT 1", ((SQLRepresentationDTO)
deserialized).sql());
}
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "",
+ "\"type\":null,",
+ "\"type\":\"\",",
+ "\"type\":\" \",",
+ "\"type\":\"notsql\",",
+ "\"type\":\"SQL\","
+ })
+ public void testRejectInvalidRepresentationType(String typeField) throws
JsonProcessingException {
+ String json = "{" + typeField + "\"dialect\":\"trino\",\"sql\":\"SELECT
1\"}";
+ RepresentationDTO representation =
+ JsonUtils.objectMapper().readValue(json, RepresentationDTO.class);
+
+ IllegalArgumentException exception =
+ Assertions.assertThrows(IllegalArgumentException.class,
representation::validate);
+ Assertions.assertEquals("\"type\" must be \"sql\"",
exception.getMessage());
+ }
+
@Test
public void testRepresentationDTOConversion() {
Representation representation =
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestViewOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestViewOperations.java
index d0971646a5..02f5fa1699 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestViewOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestViewOperations.java
@@ -80,6 +80,8 @@ import org.glassfish.jersey.test.TestProperties;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
@@ -188,6 +190,55 @@ public class TestViewOperations extends BaseOperationsTest
{
Mockito.verifyNoInteractions(dispatcher);
}
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "",
+ "\"type\":null,",
+ "\"type\":\"\",",
+ "\"type\":\" \",",
+ "\"type\":\"notsql\",",
+ "\"type\":\"SQL\","
+ })
+ public void testRejectInvalidRepresentationTypesBeforeDispatch(String
typeField) {
+ View view = mockView("v", "comment", ImmutableMap.of(), "trino", "SELECT
1");
+ when(dispatcher.createView(any(), any(), any(), any(), any(), any(),
any())).thenReturn(view);
+ when(dispatcher.alterView(any(),
any(ViewChange[].class))).thenReturn(view);
+ String representation = "{" + typeField +
"\"dialect\":\"trino\",\"sql\":\"SELECT 1\"}";
+ String create = "{\"name\":\"v\",\"representations\":[" + representation +
"]}";
+
+ try (Response response =
+ target(viewPath(metalake, catalog, schema))
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept(VND_V1_JSON)
+ .post(Entity.json(create))) {
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ ErrorResponse error = response.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
error.getCode());
+ Assertions.assertEquals(IllegalArgumentException.class.getSimpleName(),
error.getType());
+ Assertions.assertTrue(error.getMessage().contains("\"type\" must be
\"sql\""));
+ }
+
+ String replace =
+ """
+ {"updates":[{"@type":"setProperty","property":"k","value":"v"},
+ {"@type":"replaceView","representations":[%s]}]}
+ """
+ .formatted(representation);
+ try (Response response =
+ target(viewPath(metalake, catalog, schema) + "/v")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept(VND_V1_JSON)
+ .put(Entity.json(replace))) {
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ ErrorResponse error = response.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
error.getCode());
+ Assertions.assertEquals(IllegalArgumentException.class.getSimpleName(),
error.getType());
+ Assertions.assertTrue(error.getMessage().contains("\"type\" must be
\"sql\""));
+ }
+ Mockito.verifyNoInteractions(dispatcher);
+ }
+
@Test
public void testListViews() {
NameIdentifier view1 = NameIdentifier.of(metalake, catalog, schema,
"view1");
