Fix the issue on REST API primaryPath encoding (Quarkus Reactive REST)
#2658
Conversation
| @@ -216,7 +218,7 @@ protected String endpointPathForRowGetWith(String ksName, String tableName) { | |||
| protected String endpointPathForRowByPK(String ksName, String tableName, Object... primaryKeys) { | |||
| StringBuilder sb = new StringBuilder(String.format("/v2/keyspaces/%s/%s", ksName, tableName)); | |||
| for (Object key : primaryKeys) { | |||
| sb.append('/').append(key); | |||
| sb.append('/').append(URLEncoder.encode(key.toString(), StandardCharsets.UTF_8)); | |||
There was a problem hiding this comment.
Without this fix un-encoded / would be added leading to (true) failures.
|
Blocked by now-reported bug: |
|
Underlying Quarkus bug fixed via quarkusio/quarkus#34809 -- we'll see which release will include the fix to resolve issue at our end. |
| @@ -2631,7 +2631,12 @@ public void escapedChar() { | |||
| .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) | |||
| .queryParam("raw", true) | |||
| .when() | |||
| .get(BASE_PATH + "/{document-id}/a%5C.b", DEFAULT_NAMESPACE, DEFAULT_COLLECTION, id) | |||
| .get( | |||
| BASE_PATH + "/{document-id}/{leaf}", | |||
There was a problem hiding this comment.
Earlier code was trying to pre-encode path but it really needs to be left to framework to do with template.
This may have "worked" with conjuction with bug Quarkus 3.2.1 fixed but is incorrect, leading to double-encoding by caller and then mismatching on server side.
| DEFAULT_NAMESPACE, | ||
| DEFAULT_COLLECTION, | ||
| id, | ||
| "a\\.b") |
There was a problem hiding this comment.
In this case need first doubling for Java compiler, but then \ itself handled by Docs API server side to avoid interpreting following . as path separator.
tatu-at-datastax
changed the title
primaryPath encoding (Quarkus Reactive REST)
What this PR does:
Adds testing, fix, for the issue where primary key segment contains encoded
/as part of key; this must not be used as segment separator but included in segment. DropWizard (SGv1/REST) handled this correctly, Quarkus seems to have problems.Also updates Quarkus to version 3.2.1 which has fix for:
quarkusio/quarkus#34586
which is the underlying bug.
Which issue(s) this PR fixes:
N/A
Checklist