feat: refactor mapping validators #122
Conversation
WalkthroughThe pull request introduces several enhancements to the Changes
Possibly related PRs
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 e8bd423 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
===========================================
- Coverage 100.00% 91.63% -8.37%
===========================================
Files 14 18 +4
Lines 4565 5138 +573
Branches 1082 1130 +48
===========================================
+ Hits 4565 4708 +143
- Misses 0 420 +420
- Partials 0 10 +10 ☔ 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: 5
🧹 Outside diff range and nitpick comments (5)
test/scenarios/mappings/non_path_output.json (2)
18-21: LGTM: Complex mapping with nested structure.This mapping provides an excellent test case for handling complex output structures, combining property access with quoted strings. It's valuable for ensuring the validator can process nested elements correctly.
Consider adding one more test case with an array access in the output, e.g.,
"output": "headers[0].'Content-Type'", to cover an even wider range of possible output structures.
1-22: Excellent set of test scenarios for non-path output mappings.This file provides a comprehensive set of test scenarios for non-path output mappings, covering various input types (strings, numbers) and output formats (simple strings, header-like strings, quoted paths, and nested structures). These scenarios align well with the PR objectives of refactoring mapping validators and implementing automatic conversion of output mappings to JSON paths.
The variety of cases will help ensure robust validation and conversion processes. Great job on creating these test scenarios!
To make the test suite even more comprehensive, consider adding a few more edge cases:
- An empty string input or output
- A very long string input or output
- An input or output containing special characters (e.g., emojis, non-ASCII characters)
These additions would help test the validators' behavior in more extreme scenarios.src/utils/converter.test.ts (1)
1-14: Good start, but consider expanding test coverage.This test file provides a solid foundation for testing the
convertToObjectMappingfunction. However, to ensure comprehensive coverage:
Consider adding more test cases to cover different scenarios, such as:
- Valid mappings
- Edge cases (e.g., empty array, null input)
- Different combinations of
SyntaxTypeforinputExprandoutputExprIf
convertToObjectMappinghas multiple responsibilities or branches, each should be tested separately.To improve the overall test architecture:
Consider using a test data factory or object mother pattern to generate test inputs. This can make tests more readable and easier to maintain.
If there are many test cases, consider using Jest's
test.eachfor parameterized tests.Example:
const testCases = [ { input: [...], expectedOutput: ... }, { input: [...], expectedOutput: ... }, // more test cases... ]; test.each(testCases)('convertToObjectMapping($input)', ({ input, expectedOutput }) => { expect(convertToObjectMapping(input)).toEqual(expectedOutput); });This approach can make it easier to add and manage multiple test cases.
src/engine.test.ts (1)
70-90: LGTM: New test suite for isValidMapping is well-structured, with suggestions for improvement.The new test suite for
isValidMappingis a good addition, covering important scenarios:
- Conversion to JSON paths when options are provided.
- Handling of invalid output paths without engine options.
However, consider the following suggestions to enhance test coverage and clarity:
Add more test cases to cover additional scenarios, such as:
- Both input and output being invalid.
- Valid input and output without needing conversion.
For the error handling test, consider checking for a specific error being thrown rather than just a falsy return value.
Make the test descriptions more specific. For example:
- "should successfully validate when output is converted to JSON path using options"
- "should return false when output is not a valid JSON path and no conversion options are provided"
Would you like me to provide example code for these improvements?
test/scenarios/mappings/data.ts (1)
292-301: LGTM! Consider adding corresponding input for completeness.The new scenario effectively tests the engine's ability to handle non-standard JSON paths in the output, including keys with dots and hyphens, nested structures, and different content types. This addition enhances the test coverage by including edge cases for output mapping.
To make the scenario more comprehensive, consider adding a corresponding
inputfield. This would allow testing both the input parsing and output generation for these non-standard cases, ensuring full coverage of the mapping process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- src/engine.test.ts (2 hunks)
- src/engine.ts (2 hunks)
- src/utils/converter.test.ts (1 hunks)
- test/scenarios/mappings/data.ts (1 hunks)
- test/scenarios/mappings/invalid_output_mapping.json (0 hunks)
- test/scenarios/mappings/non_path_output.json (1 hunks)
💤 Files with no reviewable changes (1)
- test/scenarios/mappings/invalid_output_mapping.json
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/engine.ts
[warning] 79-81: src/engine.ts#L79-L81
Added lines #L79 - L81 were not covered by tests
🔇 Additional comments (13)
test/scenarios/mappings/non_path_output.json (5)
1-22: LGTM: Well-structured test scenarios for non-path output mappings.The JSON file is well-structured and contains a variety of test scenarios for non-path output mappings. This aligns well with the PR objective of refactoring mapping validators and implementing automatic conversion of output mappings to JSON paths. The file name accurately reflects its content.
2-5: LGTM: Valid mapping for Content-Type header.This mapping correctly associates the MIME type 'application/json' with the Content-Type header. It's a good test case for handling quoted string inputs and header name outputs.
6-9: LGTM: Simple numeric to string mapping.This mapping provides a good test case for converting a numeric input to a simple string output. It helps ensure the validator can handle different data types correctly.
10-13: LGTM: Numeric to custom header-like string mapping.This mapping extends the previous test case by using an output that resembles a custom HTTP header. It's a valuable addition to ensure the validator can handle various output formats.
14-17: LGTM: Crucial test case for quoted path-like output.This mapping is particularly important as it tests the handling of outputs that resemble quoted paths. It ensures the validator can distinguish between actual JSON paths and string literals containing dots, which is crucial for the PR's objective of automatic conversion to JSON paths.
src/utils/converter.test.ts (2)
1-2: LGTM: Imports are appropriate for the test file.The imports are correctly bringing in the necessary dependencies:
SyntaxTypefrom '../types' is used in the test case.convertToObjectMappingfrom './converter' is the function being tested.
4-14: LGTM: Well-structured test suite.The test suite follows Jest's best practices:
- Uses
describefor grouping related tests.- Nests
describeblocks to provide clear context.- Uses
itfor individual test cases with clear descriptions.This structure enhances readability and maintainability of the tests.
src/engine.test.ts (1)
2-2: LGTM: Import statement for PathType is correctly added.The new import statement for
PathTypefrom './types' is correctly placed and necessary for the new test cases. It follows TypeScript best practices for importing types.test/scenarios/mappings/data.ts (1)
292-301: Good addition to test coverage, aligns with PR objectives.The new scenario effectively enhances the test suite by covering non-standard JSON path outputs. This addition aligns well with the PR objectives of improving mapping validators and implementing automatic conversion of output mappings to JSON paths. The change is non-breaking and focused, as it only adds a new test case without modifying existing ones.
src/engine.ts (4)
83-88: Ensure default behavior remains consistent inconvertToJSONPathmethodThe addition of the
optionsparameter toconvertToJSONPathadds flexibility. Please verify that whenoptionsis undefined oroptions.defaultPathTypeis not specified, the method behaves as expected.
101-105: LGTMThe addition of the
optionsparameter toprepareMappingsaligns with the updates toprepareMapping.
109-109: LGTMUpdating
validateMappingsto passoptionstoprepareMappingsensures consistent behavior.
135-135: LGTMPassing
optionstoprepareMappingsincreateFlatMappingsASTensures mappings are processed correctly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/engine.ts (1)
62-62: Add type annotation for theinputparameterTo improve type safety and code clarity, consider adding a type annotation for the
inputparameter in thetoJsonPathmethod.Apply this diff to add the type annotation:
-private static toJsonPath(input): string { +private static toJsonPath(input: string): string {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/engine.ts (2 hunks)
- src/parser.ts (1 hunks)
- src/reverse_translator.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/engine.ts
[warning] 79-81: src/engine.ts#L79-L81
Added lines #L79 - L81 were not covered by tests
🔇 Additional comments (9)
src/engine.ts (3)
83-88: LGTM: Conditional JSON path conversionThe
convertToJSONPathmethod correctly implements the conditional conversion of paths to JSON paths based on the provided options. The logic is clear and concise.
101-105: LGTM: Improved mapping preparationThe modifications to the
prepareMappingsmethod, including the addition of theoptionsparameter and the use of the newprepareMappingmethod, enhance the consistency and flexibility of mapping preparation.
109-109: LGTM: Consistent use of options in validationThe update to pass the
optionsparameter toprepareMappingsin thevalidateMappingsmethod ensures consistency with the recent changes and improves the flexibility of the validation process.src/reverse_translator.ts (1)
41-41: Excellent use ofreadonlyfor immutability!The addition of the
readonlymodifier to theoptionsproperty is a positive change. It enforces immutability, preventing accidental modifications to theoptionsafter initialization. This aligns with TypeScript best practices and enhances code safety without affecting the existing functionality.src/parser.ts (5)
60-60: Improved immutability for lexer property.The
lexerproperty has been changed fromprivatetoprivate readonly. This is a good practice as it ensures that the lexer cannot be reassigned after initialization, potentially preventing bugs and improving code clarity.
62-62: Improved immutability for options property.The
optionsproperty has been changed fromprivatetoprivate readonly. This change is beneficial as it guarantees that the options object cannot be modified after it's set in the constructor, maintaining the integrity of the parser's configuration throughout its lifecycle.
64-64: Improved immutability for pathTypesStack property.The
pathTypesStackproperty has been changed fromprivatetoprivate readonly. This modification ensures that the array reference itself cannot be reassigned, although the contents of the array can still be modified. This change adds an extra layer of safety to the code.
60-64: Overall improvement in class design.The changes made to the
JsonTemplateParserclass, specifically the modification of property declarations toprivate readonly, represent a positive step towards improved immutability and code safety. These changes help prevent accidental modifications to critical properties after initialization, potentially reducing bugs and improving the overall robustness of the parser.The rest of the class implementation remains unchanged, which is appropriate given the nature of the modifications. The constructor and methods continue to work as expected with these new immutable properties.
Line range hint
1-1161: Focused improvements to JsonTemplateParser class.The changes made to
src/parser.tsare concentrated on improving the immutability of theJsonTemplateParserclass. By changing key properties toprivate readonly, the code now better enforces the principle of least privilege and reduces the risk of unintended modifications to these properties.These changes are beneficial and align well with best practices in TypeScript/JavaScript development. They improve the robustness of the parser without introducing new functionality or breaking existing behavior.
No further changes are necessary based on this review.
|
What are the changes introduced in this PR?
Add more validators and also auto convert output mappings to JSON paths.
What is the related Linear task?
Resolves INT-2780
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
JsonTemplateEngine.convertToObjectMappingfunction.Bug Fixes
convertToObjectMappingfunction.Documentation