Add escape for management key #4363
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/databend/databend/FLTk2rWATjRtJDCecyQxhaRCM75Y |
|
Thanks for the contribution! Please review the labels and make any necessary changes. |
|
@mergify update |
✅ Branch has been successfully updatedHey, I reacted but my real name is @Mergifyio |
|
We can move the common function |
|
Hi, @zhang2014 Could I have a question about why we need unescape_for_key function in cluster_mgr.rs? |
unescape_for_key for
No, it's immutable. |
|
@mergify update |
✅ Branch has been successfully updatedHey, I reacted but my real name is @Mergifyio |
|
|
||
| use std::string::FromUtf8Error; | ||
|
|
||
| pub fn escape_for_key(key: &str) -> Result<String, FromUtf8Error> { |
There was a problem hiding this comment.
Is it possible to add some comments about this escape?
Like the raw string is, now we escape to what?
Thanks.
There was a problem hiding this comment.
Also,some unit test cases for this string_func module is prefered.
There was a problem hiding this comment.
Have add comment and unit test.
| while index < bytes.len() { | ||
| match bytes[index] { | ||
| b'%' => { | ||
| let mut num = unhex(bytes[index + 1]) * 16; |
There was a problem hiding this comment.
Should we check the index out of range if the index is already the last byte? If we can guarantee % does not appear in the last position, we should leave a comment here.
There was a problem hiding this comment.
For me I prefer to use iterator here so that we just call next() and peek() do stuff, and it's idiomatic rust way.
There was a problem hiding this comment.
hould we check the index out of range if the index is already the last byte? If we can guarantee
%does not appear in the last position, we should leave a comment here.
This function is only used with reverse case of escape_for_key. I think there is no need to check the input. I have leave a comment in the code.
For me I prefer to use iterator here so that we just call
next()andpeek()do stuff, and it's idiomatic rust way.
In this function, the iterator sometimes needs to move 3 steps. Using index would be easier.
|
@mergify update |
☑️ Nothing to do
Hey, I reacted but my real name is @Mergifyio |
| use common_exception::Result; | ||
|
|
||
| #[test] | ||
| fn test_progress() -> Result<()> { |
There was a problem hiding this comment.
How about adding the slash case?
@drmingdrmer expected your suggestions on the escape case :)
There was a problem hiding this comment.
@BohuTANG
Looks good to me.
And it does not look like affecting me at all. 🤔
There was a problem hiding this comment.
@BohuTANG Thanks for your advise. I have changed the test case.
This reverts commit 1de96c6.
|
Thank you @Blacksmith96 ! |
I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/
Summary
Summary about this PR
Changelog
Related Issues
#3792
Test Plan
Unit Tests
Stateless Tests