Copilot commented on code in PR #13784:
URL: https://github.com/apache/gravitino/pull/13784#discussion_r4246932433
##########
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";
Review Comment:
These constants are correct but a bit ambiguous: `FORM_ENCODED_SPACE` reads
like a full encoding scheme rather than 'space encoded as plus'. Consider
renaming to something more explicit (e.g., `SPACE_AS_PLUS` /
`SPACE_AS_PERCENT_20`) to make the intent immediately clear to readers.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestTableOperations.java:
##########
@@ -508,6 +508,32 @@ public void testCreatePartitionedTable() {
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.
+ 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());
Review Comment:
The JAX-RS `Response` should be closed to avoid leaking underlying resources
in tests. Wrap the request/assertions in a try-with-resources block (or ensure
`resp.close()` is called in a finally block), especially as this pattern may be
executed many times across the test suite.
--
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]
