Skip to content

address_filter: Fix parsing of 'all'#2889

Merged
smalis-msft merged 3 commits intomicrosoft:mainfrom
smalis-msft:addressfilterallbug
Mar 9, 2026
Merged

address_filter: Fix parsing of 'all'#2889
smalis-msft merged 3 commits intomicrosoft:mainfrom
smalis-msft:addressfilterallbug

Conversation

@smalis-msft
Copy link
Contributor

@smalis-msft smalis-msft commented Mar 5, 2026

"all" should be treated as a special case here, not just a subrange. Otherwise it'll conflict with any other ranges. Fix parsing to handle this properly. Vibe coded with Claude

@smalis-msft smalis-msft requested a review from a team as a code owner March 5, 2026 16:18
Copilot AI review requested due to automatic review settings March 5, 2026 16:18
@smalis-msft smalis-msft enabled auto-merge (squash) March 5, 2026 16:18
Copy link
Contributor

Copilot AI left a 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 fixes a bug in AddressFilter::from_str where the "all" keyword was incorrectly compared against the full input string s instead of the current range token inside the comma-splitting loop. This caused "all" to be treated as a range within the loop, where it would conflict with the overlap checking logic when combined with other ranges. The fix moves the "all" check outside the loop as a top-level special case.

Changes:

  • Restructured from_str to handle "all" as a top-level special case before the comma-splitting loop, fixing the s == "all" vs range == "all" bug.
  • Added comprehensive unit tests covering all parsing paths: "all", "?", single address, range, compound filter, error cases (end-before-start, overlapping, missing prefix, nested all), and empty filter.

You can also share your feedback on Copilot code review. Take the survey.

@smalis-msft smalis-msft merged commit 11ec637 into microsoft:main Mar 9, 2026
80 of 82 checks passed
@smalis-msft smalis-msft deleted the addressfilterallbug branch March 9, 2026 19:12
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