jerryshao commented on code in PR #13784:
URL: https://github.com/apache/gravitino/pull/13784#discussion_r4248192037
##########
common/src/main/java/org/apache/gravitino/rest/RESTUtils.java:
##########
@@ -103,7 +106,10 @@ public static Map<String, String> decodeFormData(String
formString) {
public static String encodeString(String toEncode) {
Preconditions.checkArgument(toEncode != null, "Invalid string to encode:
null");
try {
- return URLEncoder.encode(toEncode, StandardCharsets.UTF_8.name());
+ // 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 URLEncoder.encode(toEncode, StandardCharsets.UTF_8.name())
+ .replace(FORM_ENCODED_SPACE, PERCENT_ENCODED_SPACE);
Review Comment:
[Question] `encodeString` is not only used for path segments:
`encodeFormData` (line 76 of this file) runs both keys and values through it,
and its single production caller is the OAuth2 token request -
`HTTPClient.toFormEncoding` -> `new
StringEntity(RESTUtils.encodeFormData(formData))`
(`clients/client-java/src/main/java/org/apache/gravitino/client/HTTPClient.java:949-950`),
called from `OAuth2ClientUtil.clientCredentialsRequest`, which puts the
space-joined `scope` into the form
(`clients/client-java/src/main/java/org/apache/gravitino/client/OAuth2ClientUtil.java:106`,
`toScope` at :144-145).
So after this change a request body sent to a third-party identity provider
becomes `scope=read%20write` instead of `scope=read+write` (same for a client
secret containing a space). Every conforming
`application/x-www-form-urlencoded` parser decodes `%20`, so this is very
likely fine in practice, but the serializer form the spec prescribes is `+`,
and the PR description only mentions path segments. Was this collateral change
intended? If you want to keep the blast radius at the path, one option is to
leave `encodeString` form-encoding and add a dedicated path-segment encoder
(`encodePathSegment`) that the client path builders use, keeping
`encodeFormData` on the old behaviour.
Verified by: reading RESTUtils.java in full and grepping every
`encodeFormData`/`postForm` caller in `clients/client-java/src/main/java` - the
OAuth2 token endpoint is the only consumer.
##########
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"))
Review Comment:
[Nit] This proves the server decodes `%20`, but it bypasses the layer that
the reported bug actually flows through: the real client hands the encoded path
to `HTTPClient.buildUri`, which rebuilds it with `new URIBuilder(baseUri)`
(`clients/client-java/src/main/java/org/apache/gravitino/client/HTTPClient.java:284-292`).
A regression there - a URI builder that re-encodes `%` or normalises the path
- would leave this test green while the Java client stays broken. A cheap guard
would be a `clients/client-java` test that loads a table whose name contains a
space against the MockServer harness those tests already use
(`clients/client-java/src/test/java/org/apache/gravitino/client/TestBase.java`)
and asserts the path the server actually received, or an IT that creates and
loads such a table.
Not blocking: I checked httpcore5 5.3.4 (the pinned version) directly and
the raw path is preserved verbatim, with and without query parameters, so the
fix does work today.
Verified by: reading `HTTPClient.buildUri` and running `new
URIBuilder("http://localhost:8090/api/.../tables/hello%20world";).build()`
against the pinned httpcore5 version.
--
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]
