You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "nija-at (via GitHub)" <gi...@apache.org> on 2023/04/19 13:16:11 UTC
[GitHub] [spark] nija-at opened a new pull request, #40853: [SPARK-43192] [CONNECT] Remove user agent validations
nija-at opened a new pull request, #40853:
URL: https://github.com/apache/spark/pull/40853
### Why are the changes needed?
The current validation on charset is restrictive. It does not allow
blank space and digits. It's common for user agent strings to contain
these characters.
Secondly, it restricts the length to stay under 200 characters. While
not common, 200+ user agent strings are sent over the wire.
Remove this restriction from the client and let the service enforce
length limits.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Unit tests
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on a diff in pull request #40853: [SPARK-43192] [CONNECT] Remove user agent validations
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #40853:
URL: https://github.com/apache/spark/pull/40853#discussion_r1171329246
##########
python/pyspark/sql/connect/client.py:
##########
@@ -269,18 +269,9 @@ def userAgent(self) -> str:
user_agent : str
The user_agent parameter specified in the connection string,
or "_SPARK_CONNECT_PYTHON" when not specified.
+ The returned value will be percent encoded.
"""
- user_agent = self.params.get(ChannelBuilder.PARAM_USER_AGENT, "_SPARK_CONNECT_PYTHON")
- allowed_chars = string.ascii_letters + string.punctuation
- if len(user_agent) > 200:
Review Comment:
should we not assert on some length? maybe 1204?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #40853: [SPARK-43192] [CONNECT] Remove user agent charset validation
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #40853:
URL: https://github.com/apache/spark/pull/40853#issuecomment-1517326940
Merged to master.
(since the last change is basically no-op)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] grundprinzip commented on pull request #40853: [SPARK-43192] [CONNECT] Remove user agent validations
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #40853:
URL: https://github.com/apache/spark/pull/40853#issuecomment-1515755055
There is no server-side validation. The limit to 200 characters was mostly something that was done as a simple protection mechanism. We've looked into different specifications for what could be part of the user agent and the conclusion was to not have a limit at this point.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #40853: [SPARK-43192] [CONNECT] Remove user agent charset validation
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #40853: [SPARK-43192] [CONNECT] Remove user agent charset validation
URL: https://github.com/apache/spark/pull/40853
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] amaliujia commented on pull request #40853: [SPARK-43192] [CONNECT] Remove user agent validations
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on PR #40853:
URL: https://github.com/apache/spark/pull/40853#issuecomment-1515459219
Just curious
```
Remove this restriction from the client and let the service enforce
length limits.
```
Is the server already enforcing the length limits or you plan to have a follow-up to add that.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] nija-at commented on a diff in pull request #40853: [SPARK-43192] [CONNECT] Remove user agent validations
Posted by "nija-at (via GitHub)" <gi...@apache.org>.
nija-at commented on code in PR #40853:
URL: https://github.com/apache/spark/pull/40853#discussion_r1172252726
##########
python/pyspark/sql/connect/client.py:
##########
@@ -269,18 +269,9 @@ def userAgent(self) -> str:
user_agent : str
The user_agent parameter specified in the connection string,
or "_SPARK_CONNECT_PYTHON" when not specified.
+ The returned value will be percent encoded.
"""
- user_agent = self.params.get(ChannelBuilder.PARAM_USER_AGENT, "_SPARK_CONNECT_PYTHON")
- allowed_chars = string.ascii_letters + string.punctuation
- if len(user_agent) > 200:
Review Comment:
I've set a limit of 2048. Hopefully, that's large enough while having "some" protection.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40853: [SPARK-43192] [CONNECT] Remove user agent charset validation
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40853:
URL: https://github.com/apache/spark/pull/40853#discussion_r1173179658
##########
python/pyspark/sql/connect/client.py:
##########
@@ -269,18 +267,15 @@ def userAgent(self) -> str:
user_agent : str
The user_agent parameter specified in the connection string,
or "_SPARK_CONNECT_PYTHON" when not specified.
+ The returned value will be percent encoded.
"""
user_agent = self.params.get(ChannelBuilder.PARAM_USER_AGENT, "_SPARK_CONNECT_PYTHON")
- allowed_chars = string.ascii_letters + string.punctuation
- if len(user_agent) > 200:
- raise SparkConnectException(
- "'user_agent' parameter cannot exceed 200 characters in length"
- )
- if set(user_agent).difference(allowed_chars):
+ ua_len = len(urllib.parse.quote(user_agent))
+ if ua_len > 2048:
raise SparkConnectException(
- "Only alphanumeric and common punctuations are allowed for 'user_agent'"
+ f"'user_agent' parameter should not exceed 2048 characters, found {len} characters."
)
- return user_agent
+ return self.params.get(ChannelBuilder.PARAM_USER_AGENT, "_SPARK_CONNECT_PYTHON")
Review Comment:
Why don't we just return `user_agent`?
```suggestion
return user_agent
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org