fix: object single index mapping #127
Conversation
WalkthroughThis pull request introduces modifications to the JSON template parsing system, focusing on enhancing the handling of array filters and path expressions. The changes primarily affect the Changes
Sequence DiagramsequenceDiagram
participant Parser as JsonTemplateParser
participant Handler as handleArrayFilterExpr
participant Expressions as Expressions
Parser->>Handler: Parse array filter
Handler-->>Expressions: Determine expression type
alt Is single element array index
Handler->>Expressions: Create SelectorExpression
else Standard array filter
Handler->>Expressions: Create ArrayFilterExpression
end
Expressions-->>Parser: Return appropriate expression
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🪛 GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)src/parser.ts[warning] 634-634: 🧾 Statement is not covered [warning] 635-635: 🧾 Statement is not covered [warning] 636-636: 🧾 Statement is not covered [warning] 637-637: 🧾 Statement is not covered [warning] 638-638: 🧾 Statement is not covered [warning] 639-639: 🧾 Statement is not covered [warning] 633-639: 🌿 Branch is not covered 🔇 Additional comments (6)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Coverage report
Show files with reduced coverage 🔻
Test suite run success217 tests passing in 7 suites. Report generated by 🧪jest coverage report action from 1cbc1db |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![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)
test/scenarios/mappings/object_mappings.json (1)
24-24: Consider standardizing the notation style.There's an inconsistency in the notation style:
- Line 24 uses bracket notation:
$.properties3['custom']- Line 29 uses dot notation:
$.properties3.custom.fooWhile both notations are valid, maintaining a consistent style throughout the mappings would improve readability and maintainability.
Also applies to: 28-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
src/parser.ts(1 hunks)src/types.ts(2 hunks)test/scenarios/mappings/object_mappings.json(1 hunks)test/scenarios/paths/data.ts(1 hunks)test/scenarios/paths/simple_path.jt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/scenarios/paths/simple_path.jt
🧰 Additional context used
🪛 GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
src/parser.ts
[warning] 637-637: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 638-638: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 639-639: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 640-640: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 641-641: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 642-642: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 643-643: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 644-644: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 645-645: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 636-645: 🌿 Branch is not covered
Warning! Not covered branch
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check for formatting & lint errors
- GitHub Check: Run Tests
🔇 Additional comments (6)
src/parser.ts (2)
625-650: Add unit tests for the newhandleArrayFilterExprmethodThe newly introduced
handleArrayFilterExprmethod is not covered by unit tests, especially the code path where aSelectorExpressionis returned when the filter is a string literal (lines 637-645). Adding tests for these scenarios will ensure the method behaves correctly and prevent future regressions.Would you like me to help create unit tests to cover these cases or open a new issue to track this task?
🧰 Tools
🪛 GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
[warning] 637-637: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 638-638: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 639-639: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 640-640: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 641-641: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 642-642: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 643-643: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 644-644: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 645-645: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 636-645: 🌿 Branch is not covered
Warning! Not covered branch
652-663: Ensure all usages can handle the newSelectorExpressionreturn typeThe
parseArrayFilterExprmethod now returns aSelectorExpressionin addition toArrayFilterExpressionandObjectFilterExpression. Please verify that all places whereparseArrayFilterExpris called can handle the new return type without causing any runtime errors or unexpected behavior.If necessary, update downstream code to handle the
SelectorExpressionand consider adding unit tests to cover these new scenarios.test/scenarios/paths/data.ts (1)
191-191: Validate the updated test output forsimple_path.jtThe
outputarray for thesimple_path.jttemplate has been modified to[1, 1, [4], 2, 1]. Please verify that this change accurately reflects the expected behavior of the template after your updates. If this change is intentional, ensure that it aligns with the modifications in the parsing logic and update any related documentation or comments accordingly.src/types.ts (2)
Line range hint
223-228: Verify integration of the newPathExpressioninterfaceA new
PathExpressioninterface has been added to define the structure of path expressions. Please ensure that all relevant parts of the codebase are updated to utilize this interface where appropriate, and that there are no inconsistencies or missed updates that could lead to type errors or unexpected behavior.
240-240: Check for impacts due to change inproptype inSelectorExpressionThe
propproperty inSelectorExpressionhas been modified toOmit<Token, 'range'>, omitting therangeproperty. Ensure that any code accessingpropdoes not rely on therangeproperty and that this change does not introduce any issues in expression handling or parsing logic.test/scenarios/mappings/object_mappings.json (1)
24-24: LGTM! The change aligns with the PR objective.The modification from dot notation to bracket notation appears to be the intended fix for the object single index mapping issue mentioned in PR #127.
Let's verify consistency with other mapping files:
✅ Verification successful
The notation change is intentional and correct
The use of bracket notation (
$.properties3['custom']) specifically addresses the object single index mapping issue, while dot notation remains appropriate for nested property access ($.properties3.custom.foo). This mixed notation pattern is intentional and doesn't require standardization across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar property access patterns in other mapping files # to ensure consistent notation style across the codebase # Search for similar property access patterns in JSON files rg -t json '"output":\s*".*\.(custom|properties\d+)"' # Search for bracket notation patterns rg -t json '"output":\s*".*\[[\'\"].*[\'\"]"'Length of output: 247
Script:
#!/bin/bash # First, find all JSON mapping files echo "=== JSON Mapping Files ===" fd -e json -e JSON mapping # Search for dot notation access patterns echo -e "\n=== Dot Notation Patterns ===" rg -t json '"output":\s*"\$\.[^"]*\.[^"]*"' -A 1 # Search for bracket notation access patterns echo -e "\n=== Bracket Notation Patterns ===" rg -t json '"output":\s*"\$\.[^"]*\[[\'\""][^\]]*[\'\""]\]"' -A 1Length of output: 8300
Script:
#!/bin/bash # Search for bracket notation patterns with proper escaping echo "=== Bracket Notation Patterns ===" rg -t json "output.*\[['\"].*['\"]" -A 1 # Also search specifically for the custom property pattern echo -e "\n=== Custom Property Patterns ===" rg -t json "custom" test/scenarios/mappings/object_mappings.json -A 1Length of output: 594
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
===========================================
- Coverage 100.00% 91.63% -8.37%
===========================================
Files 14 18 +4
Lines 4565 5176 +611
Branches 1082 1142 +60
===========================================
+ Hits 4565 4743 +178
- Misses 0 423 +423
- Partials 0 10 +10 ☔ View full report in Codecov by Sentry. |
koladilip
force-pushed
the
fix/mapping-issues
branch
from
f56c361 to
1cbc1db
Compare
|
|
Checked test cases mostly |
What are the changes introduced in this PR?
when path
$.a["b"]used in the output mapping is not working as expected.What is the related Linear task?
Resolves INT-3049
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new generic utility introduced or modified. Please explain the changes.
N/A
Any technical or performance related pointers to consider with the change?
N/A
@coderabbitai review
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
Is the PR limited to 10 file changes?
Is the PR limited to one linear task?
Are relevant unit and component test-cases added?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
Release Notes
New Features
PathExpressiontype for more flexible path handlingImprovements
SelectorExpressionTest Updates