Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support session property with comma or equality sign #407

Merged
merged 4 commits into from
Mar 13, 2019
Merged

Support session property with comma or equality sign #407

merged 4 commits into from
Mar 13, 2019

Conversation

findepi
Copy link
Member

@findepi findepi commented Mar 7, 2019 •
edited
Loading

No description provided.

@findepi findepi requested a review from electrum March 7, 2019 13:44
@cla-bot cla-bot bot added the cla-signed label Mar 7, 2019
@martint
Copy link
Member

martint commented Mar 7, 2019

They may not rely on that, but it’s perfectly legal for someone to do within the constraints of the HTTP protocol.

I think a proper fix for this is to come up with a way to encode or escape those kinds of characters.

@findepi
Copy link
Member Author

findepi commented Mar 7, 2019

This would make Presto server side code consistent with client side:
https://github.com/prestosql/presto/blob/4a8665c47458ae4c9b02404ea6d93691b1b2d9a4/presto-client/src/main/java/io/prestosql/client/StatementClientV1.java#L413-L414

@findepi
Copy link
Member Author

findepi commented Mar 7, 2019 •
edited
Loading

I think a proper fix for this is to come up with a way to encode or escape those kinds of characters.

Do you have anything specific on your mind? Or any standard i should refer to?
I thought about running values through base64 before handing them over to the client.

@findepi findepi force-pushed the findepi/master/support-session-property-with-comma-or-equality-sign-301648 branch 4 times, most recently from dff326e to 854b30c Compare March 7, 2019 16:05
@findepi
Copy link
Member Author

findepi commented Mar 7, 2019

@martint @electrum i changed the approach.

@findepi
Copy link
Member Author

findepi commented Mar 7, 2019

i still need to verify this doesn't break ODBC.

kokosing
kokosing reviewed Mar 8, 2019
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would squash three last commits.

Also we need to make sure what SIMBA JDBC is using for , and ....

presto-client/src/main/java/io/prestosql/client/StatementClientV1.java
@@ -177,7 +177,7 @@ private Request buildQueryRequest(ClientSession session, String query)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too long commit message

presto-main/src/main/java/io/prestosql/server/HttpRequestSessionContext.java
properties.put(nameValue.get(0), urlDecode(nameValue.get(1)));
}
catch (IllegalArgumentException e) {
throw badRequest(format("Invalid %s header: %s", headerName, e));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please have a test for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly would you like to test?
Note that i added a test case in TestHttpRequestSessionContext

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a method

properties.put(nameValue.get(0), decodeHeaderValue(nameValue.get(1), headerName));

That way the catch (IllegalArgumentException) has the smallest possible scope

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! In this case, the com.google.common.collect.ImmutableMap.Builder#put(K, V) cannot really throw IllegalArgumentException, so i will leave this as it is.

presto-jdbc/src/test/java/io/prestosql/jdbc/TestJdbcConnection.java Show resolved Hide resolved
@findepi
Copy link
Member Author

findepi commented Mar 11, 2019

Also we need to make sure what SIMBA JDBC is using for , and ....

From what i can see Simba JDBC neither does urlEncode nor urlDecode.
Thus, it does not know the data it is handling contains , (or whatever), because to the driver it will appear as containing %2C.
At the same time, the driver doesn't seem to have an API to set session properties directly, so it can safely treat them as opaque values.

@findepi findepi force-pushed the findepi/master/support-session-property-with-comma-or-equality-sign-301648 branch 2 times, most recently from fcb972f to 731dee6 Compare March 11, 2019 10:26
electrum
electrum approved these changes Mar 12, 2019
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Please remember to squash before merging.

presto-jdbc/src/test/java/io/prestosql/jdbc/TestJdbcConnection.java Outdated
@@ -186,6 +187,28 @@ public void testSession()
assertThat(listSession(connection))
.contains("join_distribution_type|BROADCAST|PARTITIONED")
.contains("exchange_compression|true|false");

try (Statement statement = connection.createStatement()) {
statement.execute("SET ROLE admin"); // Required to be able to `SET SESSION hive....`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

// setting Hive session properties requires the admin role
statement.execute("SET ROLE admin");

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

presto-main/src/main/java/io/prestosql/server/HttpRequestSessionContext.java
properties.put(nameValue.get(0), urlDecode(nameValue.get(1)));
}
catch (IllegalArgumentException e) {
throw badRequest(format("Invalid %s header: %s", headerName, e));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a method

properties.put(nameValue.get(0), decodeHeaderValue(nameValue.get(1), headerName));

That way the catch (IllegalArgumentException) has the smallest possible scope

@electrum
Copy link
Member

This will break any older (Java) clients that were directly setting session properties containing percent signs (%), but that seems unlikely, and the easy solution is to upgrade the client.

@findepi
Copy link
Member Author

findepi commented Mar 12, 2019

This will break any older (Java) clients that were directly setting session properties containing percent signs (%), but that seems unlikely,

I am aware. Fortunately, there are very few properties than can contain arbitrary stuff.
We could provide a fallback mechanism (if urlDecode fails then use orig value), but that could mask bugs, so i would prefer not to.

and the easy solution is to upgrade the client.

exactly

@findepi
Copy link
Member Author

findepi commented Mar 13, 2019

Please remember to squash before merging.

Sure, i squashed the fixup.

@findepi findepi force-pushed the findepi/master/support-session-property-with-comma-or-equality-sign-301648 branch from 8929a25 to f227530 Compare March 13, 2019 08:43
@findepi findepi merged commit 1bb6303 into trinodb:master Mar 13, 2019
@findepi findepi deleted the findepi/master/support-session-property-with-comma-or-equality-sign-301648 branch March 13, 2019 08:43
@findepi findepi added this to the 306 milestone Mar 13, 2019
@findepi findepi mentioned this pull request Mar 13, 2019
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electrum electrum electrum approved these changes

@martint martint martint approved these changes

@kokosing kokosing kokosing approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
306
Development

Successfully merging this pull request may close these issues.
4 participants
@findepi @martint @electrum @kokosing