Refactoring of QueryMatchFilter and provide greater support for JSONPath spec #79
Conversation
…o convert values into their scalar value
… 42 so should not be in expected results
…per section 2.3.5.2.2. Comparisons of the RFC
… More Info link to point to the RFC for queries sourced from there.
|
Lgtm. I wonder, if this resolves some of the open issues. |
Signed-off-by: Sascha Greuel <github@1-2.dev>
WalkthroughThe updates introduce a new release version 0.10.0 by adding an entry in the changelog and bumping the package version in composer.json. The query filtering logic in the core filter class has been refactored to support enhanced regex patterns, negation handling, and modular comparison methods. Multiple test files have been updated to adjust error handling, method signatures, and test data, while the GitHub Actions workflow now alters the error handling logic during test execution. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant QMF as QueryMatchFilter
participant RE as Regex Engine
participant CE as Comparison Methods
C->>QMF: Call filter(query)
QMF->>RE: Match negation & logical expressions
RE-->>QMF: Return regex match results
QMF->>QMF: Iterate through query parts
QMF->>CE: Invoke compareEquals / compareLessThan
CE-->>QMF: Return comparison result
QMF-->>C: Return final filtering outcome
Assessment against linked issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
CHANGELOG.md (2)
3-5: CHANGELOG entry placeholder added properlyThe new version entry has been added with a TBA placeholder, which is appropriate for tracking the upcoming changes. This will need to be updated with actual details before the final release.
Consider adding specific details about the QueryMatchFilter refactoring and JSONPath spec support mentioned in the PR objectives.
🧰 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)
3-3: Minor: Inconsistent heading levelStatic analysis flagged that heading levels should only increment by one level at a time. The file starts with
# Changelog(h1) and then jumps to### 0.10.0(h3) without an h2 in between.However, this follows the established pattern in the rest of the file, so it's just a note for consistency with markdown best practices rather than something that needs to be changed.
🧰 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 (4)
42-49: Negation logic introduction.
Your approach to detect and strip negation before further processing is sound. However, consider edge cases in whichlogicalexprmight be empty or malformed to avoid unexpected results.
52-69: Complex expression grouping.
Using string replacement with%groupX%placeholders effectively breaks down nested filtering logic. However, grouping can become costly for deeply nested or repeated expressions. Consider caching sub-results or reducing string replacements if performance becomes an issue.
71-84: More specific exceptions might help debugging.
Throwing aRuntimeExceptionis functional but consider a customFilterQueryExceptionor similar to better separate malformed filter errors from generic runtime issues.
186-217: Consider stricter comparisons.
While the partial type check is helpful ($type_a === $type_b), using==can still allow implicit type juggling. Consider fully strict checks (===) if alignment with RFC semantics permits.tests/data/baselineFailedQueries.txt (1)
21-28: Explicitly marking null-handling queries as XFAIL.
Marking these queries shows you’re intentionally acknowledging partial or unimplemented support for null semantics. Consider referencing an issue or roadmap if a fix is planned.Do you want help opening an issue to track these known failures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)composer.json(1 hunks)src/Filters/QueryMatchFilter.php(1 hunks)tests/QueryTest.php(6 hunks)tests/Traits/TestDataTrait.php(2 hunks)tests/data/baselineFailedQueries.txt(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/QueryTest.php (1)
src/JSONPathException.php (1) (1)
JSONPathException(13-16)
🪛 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 (15)
composer.json (1)
5-5: Version bump from 0.9.1 to 0.10.0 looks goodThe version increment from 0.9.1 to 0.10.0 aligns with the refactoring work described in the PR objectives. Following semantic versioning, this minor version bump appropriately signals new functionality while maintaining backward compatibility.
tests/Traits/TestDataTrait.php (2)
21-21: Updated PHPDoc to reflect proper exception typeThe
@throwsannotation has been correctly updated to reflect that the method now throws aRuntimeExceptioninstead of aJsonException.
31-35: Improved JSON error handling with try-catchThe error handling has been enhanced by:
- Using
JSON_THROW_ON_ERRORflag to makejson_decodethrow exceptions on error- Catching the specific
JsonExceptionand converting it to a more informativeRuntimeException- Including the original exception message in the new exception for better debugging
This is a more robust approach to error handling compared to the previous implementation.
tests/QueryTest.php (6)
51-55: Enhanced URL reference for RFC-based testsThe code now constructs a URL to the RFC specification document for test IDs that start with 'rfc_', which improves traceability to the standard. This change aligns with the PR objective of providing better support for the JSONPath specification.
77-81: Added handling for unexpectedly passing testsThis new code checks if a test that is expected to fail (listed in
self::$baselineFailedQueries) unexpectedly passes, and throws an appropriate exception. This is valuable for keeping the test suite accurate and ensuring that known failures are properly tracked. When these tests eventually pass due to implementation improvements, the base failure list should be updated.
105-111: Consolidated exception handlingThe exception handling for
JSONPathExceptionandRuntimeExceptionhas been consolidated into a single catch block, which simplifies the code without losing any functionality. The error message now includes more context, making debugging easier.
1311-1311: Fixed missing closing quote in test dataThe formatting of the expected result for the filter expression with not equals test has been fixed to properly close a quoted string.
1479-1479: Updated test expectation for script expressionThe expected result for the script expression test has been changed from an unknown value to
["fifth"], which appears to be the correct expected value based on the test description.
1560-1613: Added new RFC compliance tests for null value semanticsNine new test cases have been added to verify the implementation's handling of null values in JSONPath expressions, including:
- Null object values
- Null used as an array
- Null used as an object
- Null in array values
- Null existence checking
- Null comparisons
Some tests are marked as expected failures (XFAIL), indicating areas where the current implementation doesn't fully comply with the RFC. This is a significant improvement in test coverage and will guide future development of the JSONPath implementation.
These tests are directly related to the PR objective of improving JSONPath spec compliance. They will help ensure that the implementation correctly handles null values as specified in the RFC.
src/Filters/QueryMatchFilter.php (4)
9-10: Great use of strict typing.
Enablingstrict_types=1helps catch type-related bugs early and ensures more predictable behavior across the codebase.
16-21: Improved exception-based error handling.
IntroducingJsonExceptionandJSON_THROW_ON_ERRORensures transparent and consistent error handling for JSON operations.
25-27: Well-defined negation patterns.
Defining separate constants for wrapped vs. unwrapped negation clarifies the regex approach and improves maintainability.
170-177: Verify negation on null results.
If$comparisonResultisnull(e.g., when a prior check doesn’t set it), applying!nullyieldstrue. Confirm that$comparisonResultis guaranteed to be a boolean here or handle the null case explicitly.tests/data/baselineFailedQueries.txt (2)
1-4: New test items for bracket/dot notation and union checks.
Adding these baseline entries helps ensure a comprehensive test suite covering common notation edge cases and tautological filters.
31-31: Confirm expected behavior for “union_with_slice_and_number.”
This line is flagged as changed but the difference isn’t obvious. Double-check whether it’s truly modified in content or if it merely reflects a reformat.
| $comparisonResult = null; | ||
|
|
||
| if ($notNothing) { | ||
| $comparisonResult = match ($operator) { | ||
| null => AccessHelper::keyExists($node, $key, $this->magicIsAllowed) || (!$key), | ||
| "=", "==" => $this->compareEquals($selectedNode, $comparisonValue), | ||
| "!=", "!==", "<>" => !$this->compareEquals($selectedNode, $comparisonValue), | ||
| '=~' => @\preg_match($comparisonValue, $selectedNode), | ||
| '<' => $this->compareLessThan($selectedNode, $comparisonValue), |
There was a problem hiding this comment.
Potential risk with error-suppressed regex.
Using @\preg_match($comparisonValue, $selectedNode) may hide warnings or errors from invalid user-supplied patterns. This can raise security and debugging concerns. Consider removing error suppression or validating patterns ahead of time.
SoftCreatR
force-pushed
the
james-dev
branch
2 times, most recently
from
d761568 to
b67f7aa
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
============================================
+ 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:
|
It does. I'll go through tomorrow and mark the ones it closes. |
Excellent. Your pull request is slated for release as v0.10.0. I've already added a placeholder for these updates in the changelog. Please update the changelog with any additional details you deem necessary. Once that's done, I'll publish the new release as soon as possible. Thank you for your hard work :) |
|
Fixed issues have been marked for closing. I'll now update the Changelog. |
|
CHANGELOG updated. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
3-3: Adjust Heading Level for ConsistencyThe version header "### 0.10.0" on line 3 should increment by only one level from the top-level heading. Since "# Changelog" is an h1, consider updating "### 0.10.0" to "## 0.10.0" to conform with proper Markdown heading hierarchy.
🧰 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 (1)
CHANGELOG.md(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 (1)
CHANGELOG.md (1)
4-24: Comprehensive Changelog Entry for v0.10.0The new changelog entry clearly details all the fixes and enhancements (e.g., various filter expression adjustments, test corrections, and the addition of JSONPath-related NULL tests) in line with the PR objectives. The bullet points are well-organized and descriptive.
|
Awesome, thank you =) |
🔀 Pull Request
What does this PR do?
Refactoring of QueryMatchFilter and provide greater support for JSONPath spec
Queries now passing:
Test Plan
PHPUnit test all passing with additional tests from the RFC added (specifically around Null). Addition QueryTest now passing. No significant change in memory or time to run tests.
Before
After
Related PRs and Issues
This started out for me requiring logical AND filter and support for negation, and ended up with me refactoring the QueryMatchFilter to simplify the code while also making stricter in terms of comparisons per the RFC and not rely on PHP semantics for comparisons.
I've kept the commit history to show the development path, can squash if required or split into multiple PRs if preferred.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores