This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new f900dc599d [Cherry-pick to branch-1.3] [#13781] fix(client): Encode
spaces as %20 in REST path segments (#13784) (#13811)
f900dc599d is described below
commit f900dc599d8cafced87b422c8cb616cae6d77800
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Wed Oct 14 08:17:59 2026 +0800
[Cherry-pick to branch-1.3] [#13781] fix(client): Encode spaces as %20 in
REST path segments (#13784) (#13811)
**Cherry-pick Information:**
- Original commit: de25ce3bd82771d746253606156877988caeafb1
- Target branch: `branch-1.3`
- Status: ✅ Clean cherry-pick (no conflicts)
---------
Co-authored-by: Yuhui <[email protected]>
---
.../gravitino/client/TestRelationalCatalog.java | 24 ++++++++++++++++++++
.../java/org/apache/gravitino/rest/RESTUtils.java | 24 ++++++++++++++------
.../org/apache/gravitino/rest/TestRESTUtils.java | 10 +++++++--
.../server/web/rest/TestTableOperations.java | 26 ++++++++++++++++++++++
.../integration/test/hive/SparkHiveCatalogIT.java | 13 +++++------
5 files changed, 81 insertions(+), 16 deletions(-)
diff --git
a/clients/client-java/src/test/java/org/apache/gravitino/client/TestRelationalCatalog.java
b/clients/client-java/src/test/java/org/apache/gravitino/client/TestRelationalCatalog.java
index 64feaa121c..7c11f94707 100644
---
a/clients/client-java/src/test/java/org/apache/gravitino/client/TestRelationalCatalog.java
+++
b/clients/client-java/src/test/java/org/apache/gravitino/client/TestRelationalCatalog.java
@@ -816,6 +816,30 @@ public class TestRelationalCatalog extends TestBase {
Assertions.assertTrue(ex.getMessage().contains("table not found"));
}
+ @Test
+ public void testLoadTableWithSpaceInName() throws JsonProcessingException {
+ NameIdentifier tableId = NameIdentifier.of("schema1", "table 1");
+ Namespace fullNamespace = Namespace.of(metalakeName, catalogName,
tableId.namespace().level(0));
+ // MockServer matches the raw request path, so the space must be sent as
%20.
+ String tablePath =
+ withSlash(RelationalCatalog.formatTableRequestPath(fullNamespace) +
"/table%201");
+ ColumnDTO[] columns =
+ new ColumnDTO[] {createMockColumn("col1", Types.StringType.get(),
"comment1")};
+ TableDTO expectedTable =
+ createMockTable(
+ "table 1",
+ columns,
+ "comment",
+ Collections.emptyMap(),
+ new Partitioning[0],
+ DistributionDTO.NONE,
+ SortOrderDTO.EMPTY_SORT);
+ buildMockResource(Method.GET, tablePath, null, new
TableResponse(expectedTable), SC_OK);
+
+ Table table = catalog.asTableCatalog().loadTable(tableId);
+ Assertions.assertEquals("table 1", table.name());
+ }
+
@Test
public void testRenameTable() throws JsonProcessingException {
NameIdentifier tableId = NameIdentifier.of("schema1", "table1");
diff --git a/common/src/main/java/org/apache/gravitino/rest/RESTUtils.java
b/common/src/main/java/org/apache/gravitino/rest/RESTUtils.java
index 8b95669cee..bdcb873f8d 100644
--- a/common/src/main/java/org/apache/gravitino/rest/RESTUtils.java
+++ b/common/src/main/java/org/apache/gravitino/rest/RESTUtils.java
@@ -44,6 +44,8 @@ public class RESTUtils {
private static final Joiner.MapJoiner FORM_JOINER =
Joiner.on("&").withKeyValueSeparator("=");
private static final Splitter.MapSplitter FORM_SPLITTER =
Splitter.on("&").withKeyValueSeparator("=");
+ private static final String FORM_ENCODED_SPACE = "+";
+ private static final String PERCENT_ENCODED_SPACE = "%20";
private RESTUtils() {}
@@ -75,7 +77,7 @@ public class RESTUtils {
ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
formData.forEach(
(key, value) ->
- builder.put(encodeString(String.valueOf(key)),
encodeString(String.valueOf(value))));
+ builder.put(formEncode(String.valueOf(key)),
formEncode(String.valueOf(value))));
return FORM_JOINER.join(builder.build());
}
@@ -93,7 +95,8 @@ public class RESTUtils {
}
/**
- * URL encode a string.
+ * URL encode a string. Spaces are encoded as {@code %20}, so the result is
valid both as a path
+ * segment and as a query or form value.
*
* @param toEncode The string to encode.
* @return The URL encoded string.
@@ -102,11 +105,9 @@ public class RESTUtils {
*/
public static String encodeString(String toEncode) {
Preconditions.checkArgument(toEncode != null, "Invalid string to encode:
null");
- try {
- return URLEncoder.encode(toEncode, StandardCharsets.UTF_8.name());
- } catch (UnsupportedEncodingException e) {
- throw new GravitinoRuntimeException("Failed to encode string: %s",
toEncode);
- }
+ // URLEncoder follows the form encoding and turns a space into '+', which
is a literal '+'
+ // in a path segment. A literal '+' in the input is already encoded as %2B
at this point.
+ return formEncode(toEncode).replace(FORM_ENCODED_SPACE,
PERCENT_ENCODED_SPACE);
}
/**
@@ -169,4 +170,13 @@ public class RESTUtils {
}
throw new IOException("No available port in the range: " + portRangeStart
+ ":" + portRangeEnd);
}
+
+ // Encodes in application/x-www-form-urlencoded format, in which a space
becomes '+'.
+ private static String formEncode(String toEncode) {
+ try {
+ return URLEncoder.encode(toEncode, StandardCharsets.UTF_8.name());
+ } catch (UnsupportedEncodingException e) {
+ throw new GravitinoRuntimeException("Failed to encode string: %s",
toEncode);
+ }
+ }
}
diff --git a/common/src/test/java/org/apache/gravitino/rest/TestRESTUtils.java
b/common/src/test/java/org/apache/gravitino/rest/TestRESTUtils.java
index 6402a60a5d..465a3331df 100644
--- a/common/src/test/java/org/apache/gravitino/rest/TestRESTUtils.java
+++ b/common/src/test/java/org/apache/gravitino/rest/TestRESTUtils.java
@@ -45,6 +45,11 @@ class TestRESTUtils {
String expected = "key1=value1&key2=value2";
assertEquals(expected, RESTUtils.encodeFormData(formData));
+ // Form data keeps the form encoding, in which a space becomes '+'.
+ assertEquals(
+ "scope=read+write&secret=a%2Bb",
+ RESTUtils.encodeFormData(ImmutableMap.of("scope", "read write",
"secret", "a+b")));
+
Map<Object, Object> emptyFormData = ImmutableMap.of();
assertEquals("", RESTUtils.encodeFormData(emptyFormData));
}
@@ -67,8 +72,9 @@ class TestRESTUtils {
void testEncodeString() {
assertEquals("test", RESTUtils.encodeString("test"));
assertEquals("", RESTUtils.encodeString(""));
- /* not %20 as you might expect */
- assertEquals("hello+world", RESTUtils.encodeString("hello world"));
+ assertEquals("hello%20world", RESTUtils.encodeString("hello world"));
+ assertEquals("a%2Bb", RESTUtils.encodeString("a+b"));
+ assertEquals("a+b c", RESTUtils.decodeString(RESTUtils.encodeString("a+b
c")));
assertThrows(IllegalArgumentException.class, () ->
RESTUtils.encodeString(null));
}
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestTableOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestTableOperations.java
index 4d11ccb61e..2c8d0fdff1 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestTableOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestTableOperations.java
@@ -508,6 +508,32 @@ public class TestTableOperations extends
BaseOperationsTest {
Assertions.assertArrayEquals(partitioningWithAssignments,
tableResp.getTable().partitioning());
}
+ @Test
+ public void testLoadTableWithSpaceInName() {
+ when(dispatcher.loadTable(any()))
+ .thenAnswer(
+ invocation -> {
+ NameIdentifier ident = invocation.getArgument(0);
+ return mockTable(
+ ident.name(),
+ new Column[] {mockColumn("col1", Types.StringType.get())},
+ "mock comment",
+ ImmutableMap.of(),
+ new Transform[0]);
+ });
+
+ // The client builds request paths with RESTUtils.encodeString, so the
server must decode the
+ // encoded name back to the original one.
+ try (Response resp =
+ target(tablePath(metalake, catalog, schema) +
RESTUtils.encodeString("table 1"))
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .get()) {
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
resp.getStatus());
+ Assertions.assertEquals("table 1",
resp.readEntity(TableResponse.class).getTable().name());
+ }
+ }
+
@Test
public void testLoadTable() {
Column[] columns =
diff --git
a/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/hive/SparkHiveCatalogIT.java
b/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/hive/SparkHiveCatalogIT.java
index e5393b525c..ba1c68cb3a 100644
---
a/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/hive/SparkHiveCatalogIT.java
+++
b/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/hive/SparkHiveCatalogIT.java
@@ -39,6 +39,7 @@ import
org.apache.gravitino.spark.connector.integration.test.util.SparkTableInfo
import
org.apache.gravitino.spark.connector.integration.test.util.SparkTableInfoChecker;
import org.apache.hadoop.fs.Path;
import org.apache.spark.sql.AnalysisException;
+import org.apache.spark.sql.catalyst.analysis.PartitionsAlreadyExistException;
import org.apache.spark.sql.connector.catalog.TableCatalog;
import org.apache.spark.sql.types.DataTypes;
import org.junit.jupiter.api.Assertions;
@@ -185,13 +186,11 @@ public abstract class SparkHiveCatalogIT extends
SparkCommonIT {
Assertions.assertEquals(1, nullPartitionInfo.size());
Assertions.assertTrue(String.valueOf(nullPartitionInfo.get(0)[0]).startsWith("dt="));
- Exception exception =
- Assertions.assertThrows(
- Exception.class,
- () -> {
- sql("ALTER TABLE " + tableName + " ADD PARTITION (age_p1=21,
age_p2='twenty one')");
- });
- Assertions.assertTrue(exception.getMessage().contains("Partition already
exists"));
+ // Spark checks partitionExists before adding a partition and raises its
own exception, so
+ // assert the type rather than the message.
+ Assertions.assertThrows(
+ PartitionsAlreadyExistException.class,
+ () -> sql("ALTER TABLE " + tableName + " ADD PARTITION (age_p1=21,
age_p2='twenty one')"));
}
@ParameterizedTest
