feat(new-form-builder): updated the regex for initial fields to not allow empty string #1881
Conversation
…llow empty string
sandeepdsvs
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
ItsSudip,
krishna2020,
1abhishekpandey,
RanjeetMishra,
saikumarrs and
a team
as code owners
WalkthroughThis pull request introduces a comprehensive validation mechanism for UI configuration files across various destinations. A new Python script Changes
Sequence DiagramsequenceDiagram
participant Script as validateRegex.py
participant ConfigFile as UI Config JSON
participant Validator as Validation Functions
Script->>ConfigFile: Read Configuration
Script->>Validator: Validate UI Config
Validator-->>Script: Validation Results
Script->>Script: Categorize Results
Script->>Script: Generate Summary
Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
![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
🧹 Nitpick comments (10)
scripts/validateRegex.py (4)
60-68: Consider validating additional field constraints.You already verify that
regexandregexErrorMessageexist, but further checks (e.g., confirmingregexErrorMessageisn't empty) could provide additional robustness.
70-76: Nice enforcement to disallow empty strings.This logic aligns with the PR goal of preventing empty strings. Documenting the rationale or linking to the requirement would help future maintainers.
101-101: Rename the unused loop variable “dirs”.As suggested by the static analysis hint, rename
dirsto “_” or “_dirs” to adhere to Python conventions for unused variables.-for root, dirs, files in os.walk(base_path): +for root, _dirs, files in os.walk(base_path):🧰 Tools
🪛 Ruff (0.8.2)
101-101: Loop control variable
dirsnot used within loop bodyRename unused
dirsto_dirs(B007)
137-139: Optional enhancement: parameterize the base path.Accepting
base_pathas a CLI argument or environment variable would make this script more flexible for different folder structures and usage contexts.src/configurations/destinations/rakuten/ui-config.json (1)
19-19: Regex adjustment successfully disallows empty “Merchant ID”.Changing
^(.{0,100})$to^(.{1,100})$ensures that the field cannot be empty. However, note that the error message says "Invalid Marketing ID" while the label is "Merchant ID." Consider updating the error message for consistency.-"regexErrorMessage": "Invalid Marketing ID" +"regexErrorMessage": "Invalid Merchant ID"src/configurations/destinations/cordial/ui-config.json (1)
35-35: Fix typo in error message.There's a typo in the error message: "URl" should be "URL".
- "regexErrorMessage": "Invalid API Base URl", + "regexErrorMessage": "Invalid API Base URL",src/configurations/destinations/bloomreach_catalog/ui-config.json (1)
26-27: LGTM! Consistent regex update with typo.The regex update aligns with other files, but there's the same typo in the error message.
- "regexErrorMessage": "Invalid API Base URl", + "regexErrorMessage": "Invalid API Base URL",src/configurations/destinations/bloomreach/ui-config.json (2)
26-27: LGTM! Consistent regex update with typo.The regex update aligns with other files, but there's the same typo in the error message.
- "regexErrorMessage": "Invalid API Base URl", + "regexErrorMessage": "Invalid API Base URL",
26-26: Consider extracting common regex patterns into a shared configuration.The regex patterns for URL validation and length constraints are duplicated across multiple files. Consider extracting these into a shared configuration to improve maintainability and ensure consistency.
You could create a common patterns file like:
// src/configurations/common/patterns.js export const patterns = { url: "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|(?!.*\\.ngrok\\.io)^(?:http(s)?:\\/\\/)?[\\w.-]+(?:\\.[\\w\\.-]+)+[\\w\\-\\._~:/?#[\\]@!\\$&'\\(\\)\\*\\+,;=.]+$", length100: "^(.{1,100})$" };Then combine patterns as needed in each config file.
src/configurations/destinations/optimizely_fullstack/ui-config.json (1)
26-26: LGTM! The regex change successfully prevents empty strings.The updated regex pattern now requires at least one character and limits the maximum length to 100 characters, which aligns with the PR objective.
Consider breaking down this complex regex into smaller, more maintainable parts and adding validation for specific URL patterns. Here's a suggested improvement:
- "regex": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|(?!.*\\.ngrok\\.io)^(?:http(s)?:\\/\\/)?[\\w.-]+(?:\\.[\\w\\.-]+)+[\\w\\-\\._~:/?#[\\]@!\\$&'\\(\\)\\*\\+,;=.]+$|^(.{1,100})$", + "regex": "(?x) # Enable comments mode (^\\{\\{.*\\|\\|(.*)\\}\\}$)| # Template variables (^env[.].+)| # Environment variables (?!.*\\.ngrok\\.io) # Exclude ngrok.io domains (?:^(?:https?:\\/\\/)? # Optional http(s) (?:[\\w-]+\\.)+ # Require at least one subdomain [a-z]{2,} # Require valid TLD [\\w\\-\\._~:/?#[\\]@!\\$&'\\(\\)\\*\\+,;=.]+$)| # Rest of URL ^(?!\\s*$).{1,100}$ # Non-empty string up to 100 chars",This improved version:
- Uses the
(?x)flag to enable comments for better maintainability- Requires valid TLD in URLs
- Prevents whitespace-only strings
- Maintains all existing functionality
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
scripts/validateRegex.py(1 hunks)src/configurations/destinations/active_campaign/schema.json(1 hunks)src/configurations/destinations/active_campaign/ui-config.json(1 hunks)src/configurations/destinations/bloomreach/schema.json(1 hunks)src/configurations/destinations/bloomreach/ui-config.json(1 hunks)src/configurations/destinations/bloomreach_catalog/schema.json(1 hunks)src/configurations/destinations/bloomreach_catalog/ui-config.json(1 hunks)src/configurations/destinations/cordial/schema.json(1 hunks)src/configurations/destinations/cordial/ui-config.json(1 hunks)src/configurations/destinations/facebook_conversions/schema.json(1 hunks)src/configurations/destinations/facebook_conversions/ui-config.json(1 hunks)src/configurations/destinations/fb_custom_audience/schema.json(2 hunks)src/configurations/destinations/fb_custom_audience/ui-config.json(2 hunks)src/configurations/destinations/fullstory/schema.json(2 hunks)src/configurations/destinations/fullstory/ui-config.json(2 hunks)src/configurations/destinations/ga4/schema.json(2 hunks)src/configurations/destinations/ga4/ui-config.json(2 hunks)src/configurations/destinations/google_adwords_remarketing_lists/schema.json(1 hunks)src/configurations/destinations/google_adwords_remarketing_lists/ui-config.json(3 hunks)src/configurations/destinations/http/schema.json(1 hunks)src/configurations/destinations/http/ui-config.json(1 hunks)src/configurations/destinations/intercom/ui-config.json(1 hunks)src/configurations/destinations/movable_ink/schema.json(1 hunks)src/configurations/destinations/movable_ink/ui-config.json(1 hunks)src/configurations/destinations/optimizely_fullstack/schema.json(1 hunks)src/configurations/destinations/optimizely_fullstack/ui-config.json(1 hunks)src/configurations/destinations/rakuten/schema.json(1 hunks)src/configurations/destinations/rakuten/ui-config.json(1 hunks)src/configurations/destinations/smartly/schema.json(1 hunks)src/configurations/destinations/smartly/ui-config.json(1 hunks)src/configurations/destinations/tune/schema.json(1 hunks)src/configurations/destinations/tune/ui-config.json(1 hunks)test/data/validation/destinations/ga4.json(1 hunks)test/data/validation/destinations/movable_ink.json(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
scripts/validateRegex.py
101-101: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (31)
scripts/validateRegex.py (1)
7-15: Great use of a try-except block for robust file I/O.The code gracefully handles file reading errors by catching exceptions and returning a structured response. This contributes to more resilient validation logic.
src/configurations/destinations/bloomreach_catalog/schema.json (1)
9-9: Regex update correctly prevents empty strings.Removing the allowance for empty strings aligns well with the increased validation requirements. This is consistent with the PR’s objective of enforcing non-empty inputs.
src/configurations/destinations/cordial/ui-config.json (1)
34-34: LGTM! Regex update prevents empty strings.The updated regex pattern now enforces a non-empty string requirement while maintaining URL validation, which aligns with the PR objectives.
src/configurations/destinations/movable_ink/ui-config.json (1)
20-20: LGTM! Consistent regex update.The regex update follows the same pattern as other files, ensuring non-empty strings and enforcing a maximum length of 100 characters.
src/configurations/destinations/active_campaign/ui-config.json (1)
28-29: LGTM! Simple regex for API Key validation.The regex pattern appropriately enforces a length constraint of 1-100 characters for the API Key.
The AI summary mentions removal of the
requiredattribute, but this change is not visible in the provided diff. Let's verify this:src/configurations/destinations/google_adwords_remarketing_lists/schema.json (1)
13-13: LGTM! Regex changes correctly enforce non-empty values.The updated regex patterns now require at least one character while preserving support for template variables and environment variables. This change aligns with the PR objective of not allowing empty strings.
Let's verify the regex pattern consistency across all files:
Also applies to: 17-17, 21-21
src/configurations/destinations/smartly/ui-config.json (1)
20-20: LGTM! Api Token validation updated consistently.The regex pattern change enforces non-empty values while maintaining support for template variables and environment variables.
test/data/validation/destinations/movable_ink.json (1)
18-18: LGTM! Test cases updated to reflect new validation rules.The error message has been updated to match the new regex pattern that enforces non-empty values.
src/configurations/destinations/google_adwords_remarketing_lists/ui-config.json (1)
20-20: LGTM! UI validation rules aligned with schema changes.The regex patterns for Customer ID, List ID, and Login Customer ID have been consistently updated to enforce non-empty values while maintaining support for template variables and environment variables.
Also applies to: 29-29, 52-52
src/configurations/destinations/fullstory/ui-config.json (1)
20-20: LGTM! Field-specific validation rules updated appropriately.The regex patterns for API Key and FS ORG have been updated to enforce non-empty values while maintaining their respective maximum lengths (200 and 100 characters).
Also applies to: 79-79
src/configurations/destinations/tune/ui-config.json (1)
21-22: LGTM! UI validation has been improved.The Subdomain field validation has been strengthened with:
- Updated regex to require at least one character
- Added clear error message for better user feedback
src/configurations/destinations/intercom/ui-config.json (1)
95-95: LGTM! App Id validation has been strengthened.The regex pattern has been updated to require at least one character while maintaining support for template variables and environment variables.
src/configurations/destinations/facebook_conversions/ui-config.json (1)
29-29: LGTM! Business Access Token validation has been strengthened.The regex pattern has been updated to require at least one character while maintaining support for template variables and environment variables.
test/data/validation/destinations/ga4.json (1)
80-80: LGTM! Test case has been updated to reflect new validation rules.The error message has been updated to remove the empty string pattern, correctly reflecting that measurementId must now contain at least one character.
src/configurations/destinations/ga4/ui-config.json (2)
20-20: Approve the regex change for API Secret.The updated regex pattern now requires at least one character for the API Secret field, which is a good security practice for sensitive credentials.
53-53: Approve the regex change for Measurement Id.The updated regex pattern now requires at least one character for the Measurement Id field, while still maintaining support for the GA4 format (
G-prefix) and environment variables.src/configurations/destinations/http/ui-config.json (1)
53-53: Approve the regex changes for authentication fields.The updated regex patterns now require at least one character for Username, Token, Key, and Value fields, which is a good security practice for authentication credentials.
src/configurations/destinations/fb_custom_audience/ui-config.json (2)
30-30: Approve the regex change for Ad Account id.The updated regex pattern now requires at least one character for the Ad Account id field, which is necessary for Facebook Marketing API integration.
47-48: Approve the regex change and error message for Audience Id.The changes improve validation by:
- Requiring at least one character for the Audience Id
- Adding a clear error message for invalid inputs
src/configurations/destinations/rakuten/schema.json (1)
9-9: Approve the regex change for mid property.The updated regex pattern aligns with the schema requirement where
midis a required field, now properly enforcing at least one character.src/configurations/destinations/cordial/schema.json (1)
13-13: Approve the regex change for apiBaseUrl property.The updated regex pattern removes the empty string option (
|^$), which aligns with the schema requirement whereapiBaseUrlis a required field. The URL validation pattern remains robust, ensuring proper URL format while preventing empty values.src/configurations/destinations/movable_ink/schema.json (1)
9-9: Verify impact of removing empty string support for endpoint.The regex pattern change removes support for empty strings (
|^$). This is a breaking change that could affect existing configurations.Run this script to identify any configurations that might be affected:
✅ Verification successful
Empty endpoint removal is safe to proceed
The verification shows no existing configurations use empty endpoints, and the UI validation still maintains flexibility with its pattern. All test cases use valid URLs, suggesting this is a safe change that aligns with actual usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for empty endpoint values in existing configurations # that might be affected by the removal of empty string support echo "Searching for configurations with empty endpoint values..." rg -l '"endpoint"\s*:\s*""' || echo "No empty endpoint values found."Length of output: 229
Script:
#!/bin/bash # Search for variations of empty endpoints echo "Checking for whitespace endpoints..." rg -l '"endpoint"\s*:\s*"\s+"' echo -e "\nChecking for endpoint fields with no value..." rg -l '"endpoint"\s*:\s*(?!["{])' echo -e "\nChecking deployment configurations..." fd -e json -e yaml -e yml . | xargs rg -l 'endpoint.*(?:""|\s*$)'Length of output: 1113
Script:
#!/bin/bash # Examine content of matched files focusing on endpoint configurations echo "=== Checking movable_ink related files ===" echo -e "\nschema.json:" rg -A 2 -B 2 '"endpoint"' src/configurations/destinations/movable_ink/schema.json echo -e "\nui-config.json:" rg -A 2 -B 2 '"endpoint"' src/configurations/destinations/movable_ink/ui-config.json echo -e "\ndb-config.json:" rg -A 2 -B 2 '"endpoint"' src/configurations/destinations/movable_ink/db-config.json echo -e "\ntest config:" rg -A 2 -B 2 '"endpoint"' test/data/validation/destinations/movable_ink.jsonLength of output: 3896
src/configurations/destinations/active_campaign/schema.json (1)
13-13: LGTM! Improved validation for apiKey.The change enforces a reasonable length constraint (1-100 characters) while maintaining support for template strings and environment variables.
src/configurations/destinations/bloomreach/schema.json (1)
9-9:⚠️ Potential issueFix the regex pattern for apiBaseUrl.
The removal of the trailing
$anchor from the regex pattern could allow unexpected trailing characters in URLs. This appears to be unintentional and should be fixed.Apply this diff to fix the pattern:
- "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|(?!.*\\.ngrok\\.io)^(?:http(s)?:\\/\\/)?[\\w.-]+(?:\\.[\\w\\.-]+)+[\\w\\-\\._~:/?#[\\]@!\\$&'\\(\\)\\*\\+,;=.]+$" + "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|(?!.*\\.ngrok\\.io)^(?:http(s)?:\\/\\/)?[\\w.-]+(?:\\.[\\w\\.-]+)+[\\w\\-\\._~:/?#[\\]@!\\$&'\\(\\)\\*\\+,;=.]+$"Likely invalid or redundant comment.
src/configurations/destinations/smartly/schema.json (1)
9-9: LGTM! Consistent validation for apiToken.The change aligns with other files by enforcing the same length constraint (1-100 characters) while maintaining support for template strings and environment variables.
src/configurations/destinations/tune/schema.json (1)
9-9: LGTM! Well-structured validation for subdomain.The change correctly enforces non-empty strings (1-100 characters) for the required subdomain field while maintaining {0,100} for optional fields like eventName. This is a good pattern that aligns validation with field requirements.
src/configurations/destinations/facebook_conversions/schema.json (1)
13-13: Improved validation for access token.The updated regex pattern now enforces a minimum length of 1 character for the access token, which is a security improvement.
src/configurations/destinations/fullstory/schema.json (1)
954-954: Enhanced validation for authentication fields.The updated regex patterns for both
apiKeyandfs_orgnow enforce non-empty values, which is a security improvement. These changes are correctly scoped within their respective connection mode conditions.Also applies to: 979-979
src/configurations/destinations/optimizely_fullstack/schema.json (1)
9-9:⚠️ Potential issueVerify the removal of ngrok.io URL restriction.
The removal of the negative lookahead for
.ngrok.iofrom the URL pattern could potentially allow development/testing URLs in production. Please confirm if this is intentional.src/configurations/destinations/ga4/schema.json (1)
9-9: Improved validation for GA4 credentials.The updated regex patterns enforce:
- Non-empty API secret
- Valid measurement ID format with G- prefix
Also applies to: 958-958
src/configurations/destinations/http/schema.json (1)
Line range hint
968-1100: Enhanced authentication field validation.The updated regex patterns improve security by:
- Enforcing non-empty values for all authentication fields
- Preventing whitespace-only values for API key names
- Maintaining appropriate length limits
| @@ -387,7 +387,7 @@ | |||
| "properties": { | |||
| "audienceId": { | |||
| "type": "string", | |||
| "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$" | |||
| "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,100})$" | |||
There was a problem hiding this comment.
💡 Codebase verification
Schema validation is not enforcing the minimum length requirement
The schema correctly defines the pattern ^(.{1,100})$ requiring at least one character, but validation tests reveal that empty strings are still being accepted for both audienceId and adAccountId. This indicates a potential issue with the schema validation implementation.
🔗 Analysis chain
LGTM! Schema validation has been strengthened.
The regex patterns for audienceId and adAccountId have been updated to require at least one character while maintaining support for template variables and environment variables.
Run the following script to verify the schema validation:
Also applies to: 415-415
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the schema validation correctly enforces non-empty strings
# Test both valid and invalid cases for audienceId and adAccountId
echo '{"connectionMode":{"cloud":"cloud"},"audienceId":""}' | python3 -c '
import sys, json
from jsonschema import validate
schema = json.load(open("src/configurations/destinations/fb_custom_audience/schema.json"))
try:
validate(json.loads(sys.stdin.read()), schema)
print("Validation should have failed for empty audienceId")
exit(1)
except Exception as e:
print("Validation failed as expected:", str(e))
'
echo '{"connectionMode":{"warehouse":"cloud"},"adAccountId":""}' | python3 -c '
import sys, json
from jsonschema import validate
schema = json.load(open("src/configurations/destinations/fb_custom_audience/schema.json"))
try:
validate(json.loads(sys.stdin.read()), schema)
print("Validation should have failed for empty adAccountId")
exit(1)
except Exception as e:
print("Validation failed as expected:", str(e))
'
Length of output: 960
Script:
#!/bin/bash
# Inspect the schema structure for audienceId and adAccountId
cat src/configurations/destinations/fb_custom_audience/schema.json | jq '.properties | {audienceId: .audienceId, adAccountId: .adAccountId}'
Length of output: 190
Script:
#!/bin/bash
# View the complete schema structure to locate audienceId and adAccountId
cat src/configurations/destinations/fb_custom_audience/schema.json | jq '.'
Length of output: 12784
Script:
#!/bin/bash
# Test with correct schema path and proper structure
echo '{"configSchema":{"connectionMode":{"cloud":"cloud"},"audienceId":""}}' | python3 -c '
import sys, json
from jsonschema import validate
schema = json.load(open("src/configurations/destinations/fb_custom_audience/schema.json"))
try:
validate(json.loads(sys.stdin.read()), schema)
print("Validation should have failed for empty audienceId")
exit(1)
except Exception as e:
print("Validation failed as expected:", str(e))
'
echo '{"configSchema":{"connectionMode":{"warehouse":"cloud"},"adAccountId":""}}' | python3 -c '
import sys, json
from jsonschema import validate
schema = json.load(open("src/configurations/destinations/fb_custom_audience/schema.json"))
try:
validate(json.loads(sys.stdin.read()), schema)
print("Validation should have failed for empty adAccountId")
exit(1)
except Exception as e:
print("Validation failed as expected:", str(e))
'
Length of output: 994
What are the changes introduced in this PR?
Updated the regex for initial fields to not allow empty string
What is the related Linear task?
Resolves INT-3172
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
Based on the comprehensive review of changes, here are the release notes:
Validation Improvements
Configuration Updates
New Tooling
validateRegex.py) for comprehensive UI configuration file validationDestinations Affected