feat(http): added pathParams field and renamed the tab ro URL params #1831
Conversation
sandeepdsvs
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
ruchiramoitra,
ItsSudip,
shrouti1507,
krishna2020 and
1abhishekpandey
as code owners
WalkthroughThe changes introduce a new configuration entry, Changes
Possibly related PRs
Suggested reviewers
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1831 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ 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: 1
🧹 Outside diff range and nitpick comments (3)
test/data/validation/destinations/http.json (1)
582-627: LGTM! Basic test case for pathParamsThe test case appropriately demonstrates the basic usage of the new pathParams field.
Consider adding additional test cases to validate:
- Empty path parameter
- Invalid JSON path syntax
- Multiple path parameters
- Path parameter with constant value
src/configurations/destinations/http/ui-config.json (1)
496-510: Consider enhancing the placeholder textThe configuration is well-structured with appropriate validation, but the placeholder could be more descriptive.
Consider updating the placeholder to better illustrate the expected format:
- "placeholder": "$.userId or users", + "placeholder": "e.g: $.properties.userId or constant_value",src/configurations/destinations/http/schema.json (1)
46-57: Consider adding validation rules for path parametersThe schema would benefit from additional validation rules for path parameters in the
allOfsection, similar to other conditional validations present in the schema.Consider adding validation rules to ensure path parameters are properly handled when specific HTTP methods are used:
"allOf": [ + { + "if": { + "properties": { + "method": { + "enum": ["GET", "DELETE"] + } + } + }, + "then": { + "properties": { + "pathParams": { + "type": "array", + "maxItems": 10 + } + } + } + }, { "if": { "properties": {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/configurations/destinations/http/db-config.json(1 hunks)src/configurations/destinations/http/schema.json(1 hunks)src/configurations/destinations/http/ui-config.json(1 hunks)test/data/validation/destinations/http.json(1 hunks)
🔇 Additional comments (3)
src/configurations/destinations/http/db-config.json (1)
48-48: LGTM! Addition of pathParams configuration
The placement of pathParams in the defaultConfig array is appropriate, maintaining a logical grouping with other HTTP request parameters.
src/configurations/destinations/http/ui-config.json (1)
493-493: LGTM! Improved tab name clarity
Renaming to "URL Parameters" better represents the section's purpose, encompassing both path and query parameters.
src/configurations/destinations/http/schema.json (1)
46-57: LGTM! The pathParams implementation is functionally correct
The implementation correctly defines the structure for path parameters with appropriate pattern matching for both dynamic and static values. This addition enhances the HTTP destination's capability to handle URL path parameters.
Let's verify the integration with other configuration files:
✅ Verification successful
The pathParams implementation is properly integrated across the codebase
The verification confirms that the pathParams implementation is correctly integrated:
- The schema.json defines the correct structure and validation pattern for pathParams
- The ui-config.json includes pathParams in the URL Parameters tab under Request Body configuration
- The db-config.json lists pathParams in the defaultConfig array, making it available across all supported source types
- Test cases in test/data/validation/destinations/http.json include validation for pathParams functionality
The implementation is consistent across all configuration files and properly tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of pathParams implementation across related files
# Test 1: Check if ui-config.json has been updated with the new field
rg -l "pathParams" --type json
# Test 2: Check for any existing usages of path parameters in test files
rg "pathParams" --type json -g "test/**"
Length of output: 414
Script:
#!/bin/bash
# Let's examine the implementation details in the related files
# Check ui-config.json for pathParams configuration
echo "=== ui-config.json content ==="
cat src/configurations/destinations/http/ui-config.json
# Check db-config.json for pathParams configuration
echo -e "\n=== db-config.json content ==="
cat src/configurations/destinations/http/db-config.json
# Check test case implementation
echo -e "\n=== test case content ==="
cat test/data/validation/destinations/http.json
Length of output: 41736
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/configurations/destinations/http/schema.json (1)
46-54: 🛠️ Refactor suggestionConsider aligning pathParams structure with existing parameter fields
The implementation of
pathParamsdiffers from similar fields likequeryParamsandheaders. While functionally correct, this inconsistency might affect parameter mapping and maintenance.Apply this diff to align with existing parameter fields:
"pathParams": { "type": "array", "items": { "type": "object", "properties": { - "path": { + "to": { + "type": "string", + "pattern": "^(?!\\$)[A-Za-z0-9!#$%&'*+.^_`|~-]{1,100}$" + }, + "from": { "type": "string", "pattern": "^\\$(?:\\.|(\\.(\\w+|\\*)|\\[\\d+\\]|\\[('\\w+'|\"\\w+\")\\]|\\[\\*\\]|\\.\\w+\\(\\))*)$|^(?!\\$)[A-Za-z0-9!#$%&'*+.^_`|~-]{1,100}$" } } } },
🧹 Nitpick comments (1)
src/configurations/destinations/http/schema.json (1)
46-54: Add validation constraints for pathParamsConsider adding the following validations to prevent potential issues:
- Minimum/maximum items constraint for the array
- Required properties within items
- Additional pattern validation for path parameter names
Apply these additional schema validations:
"pathParams": { "type": "array", + "minItems": 1, + "maxItems": 10, "items": { "type": "object", + "required": ["to", "from"], "properties": { "to": { "type": "string", + "minLength": 1, "pattern": "^(?!\\$)[A-Za-z0-9!#$%&'*+.^_`|~-]{1,100}$" }, "from": { "type": "string", "pattern": "^\\$(?:\\.|(\\.(\\w+|\\*)|\\[\\d+\\]|\\[('\\w+'|\"\\w+\")\\]|\\[\\*\\]|\\.\\w+\\(\\))*)$|^(?!\\$)[A-Za-z0-9!#$%&'*+.^_`|~-]{1,100}$" } } } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/configurations/destinations/http/schema.json(3 hunks)src/configurations/destinations/http/ui-config.json(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/configurations/destinations/http/ui-config.json
🔇 Additional comments (1)
src/configurations/destinations/http/schema.json (1)
41-41: LGTM: Pattern updates are consistent and well-structured
The pattern modifications consistently enforce that values cannot start with '$' while maintaining the existing validation rules.
Also applies to: 65-65, 69-69, 81-81, 85-85
What are the changes introduced in this PR?
Added pathParams field and renamed the tab ro URL params
What is the related Linear task?
Resolves INT-2908
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 checks got introduced or modified in test suites. Please explain the changes.
N/A
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?
I have executed schemaGenerator tests and updated schema if needed
Are sensitive fields marked as secret in definition config?
My test cases and placeholders use only masked/sample values for sensitive fields
Is the PR limited to 10 file changes & one task?
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
pathParamsin HTTP settings, enhancing request flexibility.Bug Fixes
Tests
pathParams, ensuring proper validation and functionality.