Better handle regex in pcli query watch
#4361
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cratelyn
added
the
A-client
cratelyn
reviewed
May 9, 2024
| _ => Some( | ||
| regex::bytes::RegexBuilder::new(&request.nv_key_regex) | ||
| .size_limit(MAX_REGEX_LEN) | ||
| .unicode(false) |
There was a problem hiding this comment.
maybe we should add a comment here, remarking why the non-verifiable storage keys may not be valid unicode, compared to the verifiable storage keys?
There was a problem hiding this comment.
(i see this was already there, so this is not blocking)
cratelyn
added a commit
that referenced
this pull request
May 9, 2024
see discussion from #4361, here: #4361 (comment)
cratelyn
reviewed
May 9, 2024
cratelyn
approved these changes
May 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
This changes the
pcli query watchcommand to fix a bug and improve usability.Previously, the
nv_key_regexwas implemented incorrectly. This PR also makes the watch command disable watching the other storage type if only a single regex is supplied.There is a slight drawback in the current implementation in that you cannot enable filtering the watch command for one of the regexes while supplying an empty regex for the other to view all matches, however changing this would require a change to the proto interface:
rpc Watch(WatchRequest) returns (stream WatchResponse);Issue ticket number and link
Closes #4360
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: