-
Notifications
You must be signed in to change notification settings - Fork 956
support negative filtering for client command filters #2378
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
base: unstable
Are you sure you want to change the base?
Conversation
Signed-off-by: zhaozhao.zz <[email protected]>
Signed-off-by: zhaozhao.zz <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #2378 +/- ##
============================================
+ Coverage 71.41% 71.53% +0.11%
============================================
Files 123 123
Lines 67132 67282 +150
============================================
+ Hits 47942 48129 +187
+ Misses 19190 19153 -37
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces comprehensive negative filtering support for CLIENT LIST
and CLIENT KILL
commands, adding 14 new NOT-* filter types that exclude clients matching specified criteria instead of including them. The implementation allows combining positive and negative filters for more granular client management.
Key changes:
- Added 14 new negative filter types (NOT-ID, NOT-MAXAGE, NOT-TYPE, etc.) for both CLIENT LIST and CLIENT KILL commands
- Extended the clientFilter structure to support parallel negative filtering fields
- Updated command documentation and argument definitions to reflect the new capabilities
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/networking.c | Core implementation of negative filtering logic in clientFilter struct and parsing/matching functions |
tests/unit/introspection.tcl | Comprehensive test coverage for all new negative filter types and combinations |
src/commands/client-list.json | Command specification updates for CLIENT LIST negative filters |
src/commands/client-kill.json | Command specification updates for CLIENT KILL negative filters |
src/commands.def | Command definition updates including argument counts and structures |
c->flag.unix_socket || c->flag.readonly || | ||
c->flag.no_evict || c->flag.no_touch || | ||
c->flag.import_source) { | ||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for the 'N' flag check is inverted. When any flag is present, it should return 0 (no match for 'N'), but when no flags are present, it should return 1 (match for 'N'). The current logic returns 0 when flags are present but doesn't return 1 when no flags are present.
} | |
} | |
return 1; /* Explicitly return 1 when no flags are present */ |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One high level comment, but the code looked mostly good. I liked some of the other improvements you made as well.
!c->flag.no_evict && !c->flag.no_touch && | ||
!c->flag.import_source) { | ||
return 1; /* Matches 'N' */ | ||
if (c->flag.replica || c->flag.primary || c->flag.pubsub || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you changed this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'd also like to know this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is a bugfix, witch the bug can lead to the test case fails:
# Test CLIENT LIST with NOT-FLAGS filter
test {CLIENT LIST with NOT-FLAGS filter} {
set c1 [valkey_client]
$c1 readonly
set cl [r client list not-flags N]
assert_match "*flags=r*" $cl
assert_no_match "*flags=N*" $cl
catch {$c1 close}
}
with the old code, the READONLY client cannot match the condition:
case 'N': /* Check for no flags */
if (!c->flag.replica && !c->flag.primary && !c->flag.pubsub &&
!c->flag.multi && !c->flag.blocked && !c->flag.tracking &&
!c->flag.tracking_broken_redir && !c->flag.tracking_bcast &&
!c->flag.dirty_cas && !c->flag.close_after_reply &&
!c->flag.unblocked && !c->flag.close_asap &&
!c->flag.unix_socket && !c->flag.readonly &&
!c->flag.no_evict && !c->flag.no_touch &&
!c->flag.import_source) {
return 1; /* Matches 'N' */
}
break;
default:
/* Invalid flag, return false */
return 0;
}
}
/* If the loop completes, the client matches the flag filter */
return 1;
}
but return the wrong "1" after the loop completes.
Signed-off-by: zhaozhao.zz <[email protected]>
introduce negative filters for
CLIENT LIST
andCLIENT KILL
commands:NOT-ID
: Excludes clients in the IDs setNOT-TYPE
: Excludes clients of the specified typeNOT-ADDR
: Excludes clients of the specified address and portNOT-LADDR
: Excludes clients connected to the specified local address and portNOT-USER
: Excludes clients of the specified userNOT-FLAGS
: Excludes clients with the specified flag stringNOT-NAME
: Excludes clients with the specified nameNOT-LIB-NAME
: Excludes clients using the specified library nameNOT-LIB-VER
: Excludes clients with the specified library versionNOT-DB
: Excludes clients with the specified database IDNOT-CAPA
: Excludes clients with the specified capabilitiesNOT-IP
: Excludes clients with the specified IP addressclose #1936
and fix the matching algorithm for flag 'N'.