Fixed: ignore whitespace after comparison in filter expression #80
Conversation
WalkthroughThe update introduces version 0.10.1 in the changelog, which documents a fix for whitespace handling in filter expressions. A regular expression in the Changes
Sequence Diagram(s)sequenceDiagram
participant TestCase as JSONPathTest
participant JSONPathLib as JSONPath Library
participant Filter as QueryMatchFilter
participant Regex as Regex Engine
TestCase->>JSONPathLib: Execute query with extra whitespace
JSONPathLib->>Filter: Process filter expression
Filter->>Regex: Evaluate regex with updated lookahead
Regex-->>Filter: Return match result
Filter-->>JSONPathLib: Return filtered data
JSONPathLib-->>TestCase: Provide query output
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #80 +/- ##
============================================
+ Coverage 89.36% 89.86% +0.49%
- Complexity 202 223 +21
============================================
Files 11 11
Lines 395 444 +49
============================================
+ Hits 353 399 +46
- Misses 42 45 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
HI @SoftCreatR, the new regex was a bit too greedy and consumed whitespace after the comparator in filter strings, a small change to the regex |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
3-3: Consider following proper Markdown heading hierarchyThe current heading uses
###(h3) after the top-level#(h1), skipping the##(h2) level. According to markdown best practices, heading levels should only increment by one level at a time.-### 0.10.1 +## 0.10.1🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)src/Filters/QueryMatchFilter.php(1 hunks)tests/JSONPathTest.php(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (3)
CHANGELOG.md (1)
3-5: Changelog entry added correctlyThe new version entry 0.10.1 properly documents the bug fix for whitespace handling in filter expressions with boolean operations.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
src/Filters/QueryMatchFilter.php (1)
31-31: Excellent fix for the whitespace handling issueThe regex pattern has been improved to handle whitespace after the comparison value by:
- Adding
\s*to allow optional whitespace before logical operators- Making the lookahead non-capturing with
(?:...)instead of(...)since we don't need to capture this groupThis change successfully fixes the issue where whitespace following comparators was incorrectly processed.
tests/JSONPathTest.php (1)
327-340: Great test case additionThe new test case
testQueryMatchWhitespaceIgnoredproperly verifies the fix by:
- Including intentional whitespace around operators in the filter expression
- Testing a real-world use case with both category and author conditions
- Ensuring the correct result is returned despite the extra whitespace
This test provides good coverage for the regex change made in
QueryMatchFilter.php.
lucasnetau
force-pushed
the
fix-whitespace-handling
branch
from
390f3bb to
b74ad33
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
3-4: Adjust Heading Level for ConsistencyThe new version header is currently using a level-3 heading (
### 0.10.1), but since the file starts with a top-level heading (# Changelog), it is more appropriate to use a level-2 heading for version entries. This change will also resolve the MD001 markdownlint warning regarding heading increments.Proposed diff:
-### 0.10.1 +## 0.10.1🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
4-4: Clarify Changelog Message TerminologyThe changelog entry reads "Fixed ignore whitespace after comparison value in filter expression". For consistency with the PR title and description—which reference the "comparator"—consider updating the wording to use "comparator" instead of "comparison value".
Proposed diff:
-- Fixed ignore whitespace after comparison value in filter expression +- Fixed ignore whitespace after comparator in filter expression
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)src/Filters/QueryMatchFilter.php(1 hunks)tests/JSONPathTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/JSONPathTest.php
- src/Filters/QueryMatchFilter.php
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
lucasnetau
changed the title
|
Thank you :) |
3 participants
🔀 Pull Request
What does this PR do?
White space after the comparator value was consumed by the regex leading to invalid decoding of strings surrounded by single quotes.
This adds the optional whitespace to the positive lookahead. Additionally the lookahead match group is made non-capturing as it serves no purpose in further processing.
Test Plan
An additional test testQueryMatchWhitespaceIgnored was added to tests/JSONPathTest.php which has a filter string with deliberate additional whitespace added around the query.
Related PRs and Issues
I did not open an issue ticket for this issue.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests