fix: gaec schema #1787
fix: gaec schema #1787
Conversation
WalkthroughThe pull request modifies the JSON schema for Google AdWords enhanced conversions by changing the 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 (
|
aashishmalik
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
AchuthaSourabhC,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/data/validation/destinations/google_adwords_enhanced_conversions.json (1)
226-236: Review handling of sensitive data in test fixtures.The
configDatafield contains customer IDs which could be sensitive information. Consider:
- Using mock/anonymized data in test fixtures
- Adding a comment indicating that these are test values
Apply this diff to improve the test data:
"authStatus": "active", - "configData": "{\"customerId\": \"1234567890\", \"loginCustomerId\": \"\"}" + "configData": "{\"customerId\": \"SAMPLE_ID\", \"loginCustomerId\": \"\"}" // Test values onlysrc/configurations/destinations/google_adwords_enhanced_conversions/schema.json (1)
Line range hint
1-249: LGTM with suggestions for improvement.The schema structure is well-defined with appropriate validation patterns for existing properties. The change to allow additional properties is backwards-compatible and aligns with the validation file changes.
Consider adding:
- A schema description field to document the purpose and usage
- Examples of valid configurations in the schema
- Documentation for the regular expression pattern
(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$used throughout{ "configSchema": { "$schema": "http://json-schema.org/draft-07/schema#", + "description": "Configuration schema for Google AdWords Enhanced Conversions integration", + "examples": [ + { + "configData": "{\"customerId\": \"123\"}", + "requireHash": true + } + ], "type": "object",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/configurations/destinations/google_adwords_enhanced_conversions/schema.json(1 hunks)test/data/validation/destinations/google_adwords_enhanced_conversions.json(1 hunks)
🔇 Additional comments (2)
test/data/validation/destinations/google_adwords_enhanced_conversions.json (1)
223-238: Verify schema validation for the new configuration object.
The new test case validates the structure of Google AdWords enhanced conversions configuration. Let's ensure the schema properly validates all fields.
✅ Verification successful
Let me analyze the schema validation rules for the new configuration object by searching for the schema definition.
Schema validation for the new configuration object is correct
The test case is valid according to the schema definition:
configDatais required and properly formatted as a JSON string containing customer IDslistOfConversionsmatches the array schema with valid conversion strings under 100 characters- Additional properties like
rudderAccountIdandauthStatusare allowed due to"additionalProperties": true
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify schema validation rules for new fields
# Test: Search for schema validation rules for new fields
rg -A 5 '"rudderAccountId"|"listOfConversions"|"authStatus"|"configData"' --type json
# Test: Search for any existing usage of these fields
ast-grep --pattern 'rudderAccountId|listOfConversions|authStatus|configData'
Length of output: 47195
Script:
#!/bin/bash
# Search for schema definition for Google AdWords Enhanced Conversions
cat src/configurations/destinations/google_adwords_enhanced_conversions/schema.json
Length of output: 8790
src/configurations/destinations/google_adwords_enhanced_conversions/schema.json (1)
6-6: Verify the necessity of allowing additional properties.
Changing additionalProperties to true relaxes the schema validation to allow undefined properties. While this aligns with the new configuration fields mentioned in the validation file (rudderAccountId, authStatus), it's important to ensure this flexibility is intentionally required.
Consider:
- Explicitly defining all known properties in the schema if possible
- Adding documentation about which additional properties are expected
- Adding property patterns if the additional properties follow specific formats
✅ Verification successful
The previous search was too broad. Let's specifically look for the validation test file and documentation to understand what additional properties are expected in the configuration.
Setting additionalProperties to true is justified and necessary
The validation test file confirms that additional top-level properties are indeed required and actively used in the configuration:
rudderAccountIdlistOfConversionsauthStatusconfigData
These properties are being validated in the test cases and are essential for the destination's functionality. The schema change to allow additional properties is correct and intentional to support these configuration fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any new properties being used in validation tests
# that necessitate this schema change
echo "Checking validation tests for new properties..."
rg -l "google_adwords_enhanced_conversions" test/data/validation/destinations/
rg -A 10 "google_adwords_enhanced_conversions" test/data/validation/destinations/*.json
Length of output: 11807
Script:
#!/bin/bash
# Let's check for the validation test file specific to google_adwords_enhanced_conversions
fd "google_adwords_enhanced_conversions.json" test/data/validation/destinations/ --exec cat {}
# Also check for any documentation about this destination
fd -e md -e txt . --exec grep -l "google_adwords_enhanced_conversions" {}
Length of output: 5859
aashishmalik
force-pushed
the
fix.gaec
branch
from
11ad344
to
d3129ea
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1787 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/data/validation/destinations/google_adwords_enhanced_conversions.json (1)
227-234: Enhance validation for listOfConversions array.The current test case doesn't validate:
- Empty array scenario
- Duplicate conversion types
- Invalid conversion types
Consider adding validation test cases for these scenarios:
{ "config": { "listOfConversions": [] // empty array }, "result": false, "err": ["listOfConversions must not be empty"] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/configurations/destinations/google_adwords_enhanced_conversions/schema.json(1 hunks)src/configurations/destinations/http/ui-config.json(2 hunks)test/data/validation/destinations/google_adwords_enhanced_conversions.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/configurations/destinations/http/ui-config.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/configurations/destinations/google_adwords_enhanced_conversions/schema.json
🔇 Additional comments (1)
test/data/validation/destinations/google_adwords_enhanced_conversions.json (1)
236-236:
Review handling of sensitive data in test cases.
The configData field contains what appears to be a customer ID. Consider using placeholder or mock values in test cases to avoid exposing potentially sensitive information.
test/data/validation/destinations/google_adwords_enhanced_conversions.json
Show resolved
Hide resolved
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
rudderAccountId,listOfConversions,authStatus, andconfigData.Bug Fixes