feat: added path param tab http #1813
Conversation
aashishmalik
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
WalkthroughThe pull request introduces changes to the HTTP configuration files in the project. A new property, 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 #1813 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/configurations/destinations/http/db-config.json (1)
48-48: Consider documenting path parameter security guidelines.While path parameters are added as a configuration option, it's important to ensure they are properly validated to prevent path traversal attacks.
Consider:
- Adding validation rules in the schema to restrict path parameter formats
- Documenting best practices for path parameter usage in the integration guide
- Implementing server-side sanitization of path parameters
src/configurations/destinations/http/ui-config.json (1)
600-618: Add validation and improve documentation for path parametersThe implementation follows the existing pattern well, but could benefit from these improvements:
- Add JSONPath validation like other mapping fields:
{ "type": "mapping", "label": "Enter your path parameters in sequence", + "jsonPathValidation": true, "columns": [
- Add a note explaining the relationship with API URL:
{ "type": "mapping", "label": "Enter your path parameters in sequence", + "note": [ + "Path parameters will be applied in sequence to replace variables in your API URL. For example, if your URL is 'http://api.example.com/users/{{param1}}/posts/{{param2}}', the first path parameter will replace {{param1}} and the second will replace {{param2}}.", + { + "text": "Learn more", + "link": "https://www.rudderstack.com/docs/destinations/http/setup-guide/#path-parameters" + } + ], "columns": [
- Consider adding an order column to make the sequence more explicit:
"columns": [ + { + "key": "order", + "type": "textInput", + "label": "Order", + "placeholder": "1", + "disabled": true + }, { "key": "path",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
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)
🔇 Additional comments (1)
src/configurations/destinations/http/schema.json (1)
6-6: LGTM! Consider documenting the schema change.
The addition of "additionalProperties": true is necessary to support the new path parameters feature. While this relaxes the schema validation to allow undefined properties, it maintains the required property validations and existing constraints.
Let's verify the integration with related changes:
✅ Verification successful
Schema change is properly integrated across configurations
The verification confirms that pathParams is properly integrated:
- Present in UI config as a configurable field with appropriate label and placeholder
- Listed in DB config as one of the supported configuration properties
- Schema change with
additionalProperties: trueenables this new property
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of schema changes with UI and DB configs
# Expected: Find corresponding path parameter configurations in UI and DB config files
# Check for path parameter configurations in UI config
echo "Checking UI config for path parameter settings..."
rg -l "pathParams" "src/configurations/destinations/http/ui-config.json"
# Check for path parameter configurations in DB config
echo "Checking DB config for path parameter settings..."
rg -l "pathParams" "src/configurations/destinations/http/db-config.json"
Length of output: 463
Script:
#!/bin/bash
# Let's examine the actual implementation of pathParams in both files
echo "UI Config pathParams implementation:"
rg -A 5 -B 5 "pathParams" "src/configurations/destinations/http/ui-config.json"
echo -e "\nDB Config pathParams implementation:"
rg -A 5 -B 5 "pathParams" "src/configurations/destinations/http/db-config.json"
Length of output: 853
| @@ -45,6 +45,7 @@ | |||
| "propertiesMapping", | |||
| "queryParams", | |||
| "headers", | |||
| "pathParams", | |||
There was a problem hiding this comment.
💡 Codebase verification
Schema validation missing for pathParams
The schema.json file lacks validation rules for the pathParams configuration, while the UI configuration (ui-config.json) correctly includes the Path Parameters tab. This could lead to validation issues.
- Add schema validation for
pathParamsin schema.json similar to the existingqueryParamsandheadersvalidation:
"pathParams": {
"type": "array",
"items": {
"type": "object",
"properties": {
"path": {
"type": "string",
"pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$"
}
}
}
}🔗 Analysis chain
LGTM! The placement of pathParams is logical and non-breaking.
The addition of "pathParams" is well-positioned among other HTTP request configuration options like queryParams and headers.
Let's verify the integration with related schema and UI files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that schema.json and ui-config.json have corresponding changes for pathParams
# Check if schema.json has been updated to allow pathParams
echo "Checking schema.json for pathParams support..."
rg -A 5 '"pathParams"' "src/configurations/destinations/http/schema.json"
# Check if ui-config.json has UI configuration for pathParams
echo "Checking ui-config.json for pathParams UI configuration..."
rg -A 5 '"pathParams"' "src/configurations/destinations/http/ui-config.json"
Length of output: 482
Script:
#!/bin/bash
# Let's verify schema.json changes more thoroughly by checking its content
echo "Checking complete schema.json content..."
cat "src/configurations/destinations/http/schema.json"
# Also check if there are any validation rules or type definitions for pathParams
echo -e "\nChecking for pathParams validation rules..."
rg -C 10 'pathParams|path.*param' "src/configurations/destinations/http/schema.json"
# Check the complete UI config to understand the context better
echo -e "\nChecking complete ui-config.json content..."
cat "src/configurations/destinations/http/ui-config.json"
Length of output: 51198
| }, | ||
| { | ||
| "name": "Path Parameters", | ||
| "fields": [ | ||
| { | ||
| "type": "mapping", | ||
| "label": "Enter your path parameters in sequence", | ||
| "columns": [ | ||
| { | ||
| "key": "path", | ||
| "type": "textInput", | ||
| "label": "Path", | ||
| "placeholder": "$.userId" | ||
| } | ||
| ], | ||
| "default": [], | ||
| "configKey": "pathParams" | ||
| } | ||
| ] |
There was a problem hiding this comment.
💡 Codebase verification
Documentation needs update to reference path parameters feature
The API URL field's documentation currently mentions dynamic path variables using JSON template syntax (e.g., {{$.traits.userId}}), but doesn't reference the new dedicated path parameters feature. The documentation should be updated to:
- Mention the new Path Parameters section
- Clarify the relationship between URL templating and path parameters
- Guide users on when to use each approach
🔗 Analysis chain
Verify documentation for path parameter usage
The new path parameters feature needs to be properly referenced in the API URL field's documentation.
Let's check if the API URL field's documentation mentions path parameters:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API URL field documentation in the file
rg -U "API URL.*note.*path" src/configurations/destinations/http/ui-config.json
Length of output: 81
Script:
#!/bin/bash
# Let's search for the API URL field configuration to check its documentation
rg -U "API URL" -A 10 src/configurations/destinations/http/ui-config.json
Length of output: 1300
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/configurations/destinations/http/ui-config.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/configurations/destinations/http/ui-config.json
[error] 616-616: Expected a property but instead found '}'.
Expected a property here.
(parse)
🔇 Additional comments (1)
src/configurations/destinations/http/ui-config.json (1)
600-601: Update API URL field documentation to reference path parameters
The API URL field's documentation needs to be updated to mention the new Path Parameters section and clarify when to use path parameters versus template syntax.
| "regexErrorMessage": "Please enter a valid JSON path or a constant", | ||
| } |
There was a problem hiding this comment.
Fix JSON syntax error: Remove trailing comma
There's a trailing comma after the regexErrorMessage field which needs to be removed to ensure valid JSON syntax.
Apply this fix:
"regex": "^\\$(\\.(\\w+|\\*)|\\[\\d+\\]|\\[('\\w+'|\"\\w+\")\\]|\\[\\*\\]|\\.\\w+\\(\\))*$|^(?!\\$).{1,100}$",
- "regexErrorMessage": "Please enter a valid JSON path or a constant",
+ "regexErrorMessage": "Please enter a valid JSON path or a constant"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "regexErrorMessage": "Please enter a valid JSON path or a constant", | |
| } | |
| "regex": "^\\$(\\.(\\w+|\\*)|\\[\\d+\\]|\\[('\\w+'|\"\\w+\")\\]|\\[\\*\\]|\\.\\w+\\(\\))*$|^(?!\\$).{1,100}$", | |
| "regexErrorMessage": "Please enter a valid JSON path or a constant" | |
| } |
| }, | ||
| { | ||
| "name": "Path Parameters", | ||
| "fields": [ | ||
| { | ||
| "type": "mapping", | ||
| "label": "Enter your path parameters in sequence", | ||
| "columns": [ | ||
| { | ||
| "key": "path", | ||
| "type": "textInput", | ||
| "label": "Path", | ||
| "placeholder": "$.userId or users", | ||
| "regex": "^\\$(\\.(\\w+|\\*)|\\[\\d+\\]|\\[('\\w+'|\"\\w+\")\\]|\\[\\*\\]|\\.\\w+\\(\\))*$|^(?!\\$).{1,100}$", | ||
| "regexErrorMessage": "Please enter a valid JSON path or a constant", | ||
| } | ||
| ], | ||
| "default": [], | ||
| "configKey": "pathParams" | ||
| } | ||
| ] |
There was a problem hiding this comment.
💡 Codebase verification
Documentation needs to be added for path parameters feature
The codebase search reveals:
- Path parameters UI configuration exists but lacks comprehensive documentation
- Example URLs with path parameters are only present in test files
- The API URL field's documentation briefly mentions dynamic path variables but doesn't explain the new path parameters feature
Required documentation updates:
- Add path parameters documentation in README.md or create a new documentation file
- Include examples demonstrating the relationship between path parameters and API URL
- Document the syntax and usage of the new path parameters feature
🔗 Analysis chain
Verify path parameter documentation and examples
Let's ensure the documentation and examples are consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for path parameter documentation and examples
echo "Searching for path parameter documentation..."
rg -l "path param(eter)?" docs/
rg -l "path param(eter)?" README.md
echo "Searching for example URLs with path parameters..."
rg -l "http.*\{\{.*\}\}" docs/
Length of output: 408
Script:
#!/bin/bash
# Let's try a broader search for documentation and examples
echo "Searching for path parameter documentation in all files..."
rg -i "path param(eter)?"
echo -e "\nSearching for example URLs with path parameters..."
rg -i "https?://[^\"'\s]*\{[^}]+\}"
echo -e "\nSearching for any markdown or documentation files..."
fd -e md
fd -e mdx
fd -e txt
Length of output: 2693
🧰 Tools
🪛 Biome (1.9.4)
[error] 616-616: Expected a property but instead found '}'.
Expected a property here.
(parse)
| "name": "Path Parameters", | ||
| "fields": [ | ||
| { | ||
| "type": "mapping", | ||
| "label": "Enter your path parameters in sequence", | ||
| "columns": [ | ||
| { | ||
| "key": "path", | ||
| "type": "textInput", | ||
| "label": "Path", | ||
| "placeholder": "$.userId or users", | ||
| "regex": "^\\$(\\.(\\w+|\\*)|\\[\\d+\\]|\\[('\\w+'|\"\\w+\")\\]|\\[\\*\\]|\\.\\w+\\(\\))*$|^(?!\\$).{1,100}$", | ||
| "regexErrorMessage": "Please enter a valid JSON path or a constant", | ||
| } | ||
| ], | ||
| "default": [], | ||
| "configKey": "pathParams" | ||
| } | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing path parameter validation and documentation
The implementation needs additional safeguards and clarity:
- Add validation to prevent duplicate path parameters which could cause runtime issues
- Clarify the sequence importance in the label or add a note
- Consider adding validation for the total number of path parameters to match URL placeholders
Apply these changes to enhance validation and clarity:
{
"name": "Path Parameters",
"fields": [
{
"type": "mapping",
- "label": "Enter your path parameters in sequence",
+ "label": "Enter your path parameters in sequence (order matters)",
+ "note": "Parameters will be substituted in order into URL placeholders. Use either JSON path (e.g., $.userId) for dynamic values or constants.",
"columns": [
{
"key": "path",
"type": "textInput",
"label": "Path",
"placeholder": "$.userId or users",
"regex": "^\\$(\\.(\\w+|\\*)|\\[\\d+\\]|\\[('\\w+'|\"\\w+\")\\]|\\[\\*\\]|\\.\\w+\\(\\))*$|^(?!\\$).{1,100}$",
"regexErrorMessage": "Please enter a valid JSON path or a constant",
+ "validation": {
+ "unique": true,
+ "errorMessage": "Duplicate path parameters are not allowed"
+ }
}
],
"default": [],
"configKey": "pathParams"
}
]
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 616-616: Expected a property but instead found '}'.
Expected a property here.
(parse)
|
This PR is considered to be stale. It has been open for 20 days with no further activity thus it is going to be closed in 7 days. To avoid such a case please consider removing the stale label manually or add a comment to the PR. |
What are the changes introduced in this PR?
Write a brief explainer on your code changes.
What is the related Linear task?
Resolves INT-XXX
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
Bug Fixes
Documentation