feat: merge filters and indexes #113
Conversation
WalkthroughThis update involves modifications across several components of the JSON template processing system, including Changes
Sequence Diagram(s)N/A Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Coverage report
Show files with reduced coverage 🔻
Test suite run success210 tests passing in 6 suites. Report generated by 🧪jest coverage report action from 51faf11 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
===========================================
- Coverage 100.00% 91.54% -8.46%
===========================================
Files 14 18 +4
Lines 4565 4981 +416
Branches 1082 1089 +7
===========================================
- Hits 4565 4560 -5
- Misses 0 412 +412
- Partials 0 9 +9 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
src/engine.ts (1)
Line range hint
15-18: Avoid usingFunctionas a type.The
Functiontype is used in multiple places. Prefer explicitly defining the function shape to avoid potential bugs.- private readonly fn: Function; - private constructor(fn: Function) { + private readonly fn: (data: unknown, bindings: Record<string, unknown>) => unknown; + private constructor(fn: (data: unknown, bindings: Record<string, unknown>) => unknown) { - private static compileAsSync(template: TemplateInput, options?: EngineOptions): Function { + private static compileAsSync(template: TemplateInput, options?: EngineOptions): (data: unknown, bindings: Record<string, unknown>) => unknown { - private static compileAsAsync(templateOrExpr: TemplateInput, options?: EngineOptions): Function { + private static compileAsAsync(templateOrExpr: TemplateInput, options?: EngineOptions): (data: unknown, bindings: Record<string, unknown>) => unknown {Also applies to: 22-22, 26-26, 35-35
src/parser.ts (1)
Line range hint
1524-1524: Avoid usingthisin a static context.Using
thisin a static context can be confusing. Consider using the class name instead.- return this.isArrayFilterExpressionSimple(part as ArrayFilterExpression); + return JsonTemplateParser.isArrayFilterExpressionSimple(part as ArrayFilterExpression);Also applies to: 1530-1530, 1537-1537
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/engine.ts (1 hunks)
- src/parser.ts (4 hunks)
- src/types.ts (1 hunks)
- src/utils/converter.ts (3 hunks)
- test/scenarios/mappings/all_features.json (1 hunks)
- test/scenarios/mappings/context_vars_mapping.json (1 hunks)
Additional context used
Biome
src/engine.ts
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 22-22: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 26-26: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 35-35: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
src/parser.ts
[error] 1524-1524: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 1530-1530: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 1537-1537: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 562-562: Do not add then to an object.
(lint/suspicious/noThenProperty)
GitHub Check: codecov/patch
src/utils/converter.ts
[warning] 120-121: src/utils/converter.ts#L120-L121
Added lines #L120 - L121 were not covered by tests
GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
src/utils/converter.ts
[warning] 120-120: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 121-121: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 119-119: 🌿 Branch is not covered
Warning! Not covered branch
[warning] 119-121: 🌿 Branch is not covered
Warning! Not covered branchsrc/parser.ts
[warning] 1580-1583: 🌿 Branch is not covered
Warning! Not covered branch
Additional comments not posted (13)
test/scenarios/mappings/context_vars_mapping.json (1)
7-7: Verify the correctness of the updated JSON path.The
frompath has been updated to$.a[*].#index.foo. Ensure that the updated path is correct and consistent with the rest of the logic.test/scenarios/mappings/all_features.json (8)
23-23: Verify the correctness of the updated JSON path.The
inputpath has been updated to$.products[*].name. Ensure that the updated path is correct and consistent with the rest of the logic.
27-27: Verify the correctness of the updated JSON path.The
frompath has been updated to$.products[*].category. Ensure that the updated path is correct and consistent with the rest of the logic.
31-31: Verify the correctness of the updated JSON path.The
inputpath has been updated to$.products[*].variations[*].size. Ensure that the updated path is correct and consistent with the rest of the logic.
35-35: Verify the correctness of the updated JSON path.The
inputpath has been updated to$.products[*].(@.price * @.quantity * (1 - $.discount / 100)). Ensure that the updated path is correct and consistent with the rest of the logic.
43-43: Verify the correctness of the updated JSON path.The
inputpath has been updated to$.products[*].variations[*].length. Ensure that the updated path is correct and consistent with the rest of the logic.
47-47: Verify the correctness of the updated JSON path.The
inputpath has been updated to$.products[*].variations[*].width. Ensure that the updated path is correct and consistent with the rest of the logic.
51-51: Verify the correctness of the updated JSON path.The
inputpath has been updated to$.products[*].variations[*].color. Ensure that the updated path is correct and consistent with the rest of the logic.
55-55: Verify the correctness of the updated JSON path.The
inputpath has been updated to$.products[*].variations[*].height. Ensure that the updated path is correct and consistent with the rest of the logic.src/engine.ts (2)
90-90: Ensure the newmappingsoption is correctly integrated.The
createFlatMappingsASTmethod now includes a newmappingsoption. Verify that this option is correctly integrated and does not introduce any issues.
95-96: Verify the correctness of the updatedparsemethod calls.The
parsemethod calls now include the newmappingsoption. Ensure that these calls are correct and consistent with the rest of the logic.src/types.ts (1)
110-110: Ensure the newmappingsproperty is correctly integrated.The
EngineOptionsinterface now includes a newmappingsproperty. Verify that this property is correctly integrated and does not introduce any issues.src/utils/converter.ts (1)
113-127: Add comments to explain the logic.The function
mergeObjectFilterPartscould benefit from additional comments explaining its logic for better readability and maintainability.+ // Merges object filter parts under specific conditions. function mergeObjectFilterParts(existParts: Expression[], newParts: Expression[]) { for (let index = 0; index < newParts.length; index++) { if ( newParts[index].type === SyntaxType.OBJECT_FILTER_EXPR && existParts[index].type === SyntaxType.OBJECT_FILTER_EXPR ) { if (newParts[index].options?.index) { existParts[index].options = { ...existParts[index].options, ...newParts[index].options }; } if (newParts[index].filter.type !== SyntaxType.ALL_FILTER_EXPR) { existParts[index].filter = newParts[index].filter; } } } }Tools
GitHub Check: codecov/patch
[warning] 120-121: src/utils/converter.ts#L120-L121
Added lines #L120 - L121 were not covered by testsGitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
[warning] 120-120: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 121-121: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 119-119: 🌿 Branch is not covered
Warning! Not covered branch
[warning] 119-121: 🌿 Branch is not covered
Warning! Not covered branch
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/engine.ts (1)
Line range hint
20-35: Avoid usingFunctionas a type.Using
Functionas a type is discouraged. Prefer explicitly defining the function shape to avoid potential bugs.- private readonly fn: Function; - private constructor(fn: Function) { + private readonly fn: (data: unknown, bindings: Record<string, unknown>) => unknown; + private constructor(fn: (data: unknown, bindings: Record<string, unknown>) => unknown) { - private static compileAsSync(template: TemplateInput, options?: EngineOptions): Function { + private static compileAsSync(template: TemplateInput, options?: EngineOptions): (data: unknown, bindings: Record<string, unknown>) => unknown { - private static compileAsAsync(templateOrExpr: TemplateInput, options?: EngineOptions): Function { + private static compileAsAsync(templateOrExpr: TemplateInput, options?: EngineOptions): (data: unknown, bindings: Record<string, unknown>) => unknown { - private static translateExpression(expr: Expression): string { + private static translateExpression(expr: Expression): string {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/engine.ts (2 hunks)
Additional context used
Biome
src/engine.ts
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 22-22: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 26-26: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 35-35: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
Additional comments not posted (2)
src/engine.ts (2)
135-135: LGTM!The change to parse the mapping paths if
expris an array improves the robustness of thereverseTranslatemethod.
90-90: Verify the impact of themappingsproperty.Ensure that adding the
mappingsproperty to theoptionsparameter does not cause unintended side effects in other parts of the codebase.Verification successful
Verify the impact of the
mappingsproperty.The addition of the
mappingsproperty in thecreateFlatMappingsASTmethod does not cause unintended side effects. The property is explicitly set totrueand is used in a conditional check insrc/parser.ts, ensuring it is being utilized as intended.
src/parser.ts
- Line:
!this.options?.mappings &&src/engine.ts
- Line:
const newOptions = { ...options, mappings: true };Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the `mappings` property in the `options` parameter. # Test: Search for the usage of the `createFlatMappingsAST` method. Expect: No unintended side effects. rg --type-add 'typescript' --type 'typescript' 'createFlatMappingsAST'Length of output: 126
Script:
#!/bin/bash # Description: Verify the impact of the `mappings` property in the `options` parameter. # Test: Search for the usage of the `createFlatMappingsAST` method. rg 'createFlatMappingsAST' # Test: Search for any references to the `options` object that include the `mappings` property. rg '\boptions\b.*\bmappings\b'Length of output: 339
Script:
#!/bin/bash # Description: Extract the relevant code surrounding the references to `options.mappings`. # Extract code from `src/parser.ts` around the reference to `options.mappings`. rg -C 5 '\boptions\b.*\bmappings\b' src/parser.ts # Extract code from `src/engine.ts` around the reference to `options.mappings`. rg -C 5 '\boptions\b.*\bmappings\b' src/engine.tsLength of output: 905
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/scenarios/mappings/filters.json (1 hunks)
Additional comments not posted (2)
test/scenarios/mappings/filters.json (2)
7-7: Verify the removal of the filter condition.The JSON path expression
$.products[*].namenow selects all product names. Ensure this change is intentional and aligns with the desired behavior of including all products regardless of category.
11-11: Verify the removal of the filter condition.The JSON path expression
$.products[*].categorynow selects all product categories. Ensure this change is intentional and aligns with the desired behavior of including all products regardless of category.
What are the changes introduced in this PR?
now users need to specify filters and indexes in one mappings and it will be automatically mapped to all applicable rules
What is the related Linear task?
Resolves INT-2240
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
New Features
parsemethod in the JSON Template Engine to support a newmappingsoption.mergeObjectFilterPartsfunction for better handling of object filter parts.Bug Fixes
Refactor
JsonTemplateParserclass to usethisinstead of direct class references, improving code maintainability.Tests