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.

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

update _valid_key_re #119

Closed
wants to merge 1 commit into from
Closed

Conversation

dongweiming
Copy link

@dongweiming dongweiming commented Jan 22, 2019

ref: #118

Is it more appropriate to use this?

In [8]: _valid_key_re = re.compile(b'^[^\x00-\x20\x7f]{1,250}$')                                                  

In [9]: _valid_key_re.match(b'123')                                                                               
Out[9]: <_sre.SRE_Match object; span=(0, 3), match=b'123'>

In [10]: _valid_key_re.match(b'abc')                                                                               
Out[10]: <_sre.SRE_Match object; span=(0, 3), match=b'abc'>

In [11]: _valid_key_re.match(bytes('中文', 'utf-8'))                                                               
Out[11]: <_sre.SRE_Match object; span=(0, 6), match=b'\xe4\xb8\xad\xe6\x96\x87'>

In [12]: _valid_key_re.match(bytes('こんにちは', 'utf-8'))                                                         
Out[12]: <_sre.SRE_Match object; span=(0, 15), match=b'\xe3\x81\x93\xe3\x82\x93\xe3\x81\xab\xe3\x81\xa1>

In [13]: _valid_key_re.match(bytes('안녕하세요', 'utf-8'))                                                         
Out[13]: <_sre.SRE_Match object; span=(0, 15), match=b'\xec\x95\x88\xeb\x85\x95\xed\x95\x98\xec\x84\xb8>

In [14]: _valid_key_re.match(b' ')      

In [15]: _valid_key_re.match(bytes('!@#', 'utf-8'))                                                                
Out[15]: <_sre.SRE_Match object; span=(0, 3), match=b'!@#'>

Reviewed-by: dongweiming <[email protected]>
@codecov
Copy link

codecov bot commented Jan 22, 2019

Codecov Report

Merging #119 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #119   +/-   ##
======================================
  Coverage    91.5%   91.5%           
======================================
  Files           5       5           
  Lines         259     259           
  Branches       38      38           
======================================
  Hits          237     237           
  Misses         11      11           
  Partials       11      11
Impacted Files Coverage Δ
aiomcache/client.py 88.29% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75d44b2...e0d524a. Read the comment docs.

@dongweiming
Copy link
Author

@fafhrd91 @asvetlov please review, thanks!

@rexzhang
Copy link
Contributor

rexzhang commented Sep 8, 2020

i have merged your PR to my project rexzhang@27c737d

@@ -39,7 +39,7 @@ def __init__(self, host, port=11211, *,
# key supports ascii sans space and control chars
# \x21 is !, right after space, and \x7e is -, right before DEL
Copy link
Member

Choose a reason for hiding this comment

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

Doc line needs updating.

@@ -39,7 +39,7 @@ def __init__(self, host, port=11211, *,
# key supports ascii sans space and control chars
# \x21 is !, right after space, and \x7e is -, right before DEL
# also 1 <= len <= 250 as per the spec
_valid_key_re = re.compile(b'^[\x21-\x7e]{1,250}$')
_valid_key_re = re.compile(b'^[^\x00-\x20\x7f]{1,250}$')
Copy link
Member

Choose a reason for hiding this comment

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

As per the above comment and the memcached documention, "the key must not include
control characters or whitespace."

Therefore this will need some more work to allow non-ascii characters.

@Dreamsorcerer
Copy link
Member

Fixed with #240 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants