chore: add mock source config api to test device mode sdk loading #1856
Conversation
koladilip
requested review from
ItsSudip,
sandeepdsvs,
krishna2020,
RanjeetMishra,
lvrach,
saikumarrs and
a team
as code owners
WalkthroughThe changes involve modifications to three JavaScript files in the scripts directory. A new constant Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Server as Source Config Server
participant FileSystem as Destination Configs
Client->>Server: HTTP Request
Server->>Server: Extract Write Key
Server->>FileSystem: Read Destination Configs
FileSystem-->>Server: Return Config Files
Server->>Server: Filter Destinations
Server-->>Client: Return Source Configuration JSON
Poem
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: 2
🧹 Nitpick comments (4)
scripts/common.js (2)
1-6: Consider a more descriptive identifier forlanguageMap.
WhilelanguageMapis adequately descriptive, consider selecting a name that conveys its purpose more clearly—e.g.,platformLanguageMap. This makes the code more readable for new developers.
9-13: Ensure consistent handling whensupportedConnectionModesis absent.
Currently, a warning is logged andfalseis returned. While this is acceptable, consider recording or tracking this missing configuration in a more robust way (for example, incrementing a metrics counter) to facilitate troubleshooting in larger codebases.scripts/generateConstants.js (1)
4-4: Adopt a consistent import style to address ESLint warnings.
Your ESLint error suggests that you may need ES6 imports rather thanrequire(). If your environment supports ES modules, consider usingimport { filterLanguages } from './common.js';and update your configuration accordingly.🧰 Tools
🪛 eslint
[error] 4-4: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
scripts/dummySourceConfigApiToTestSdKs.js (1)
10-35: Consider caching when readingdb-config.jsonfiles repeatedly.
Repeatedly reading and parsing the same JSON files could be inefficient if you expect a high request volume. Caching or reading once on startup can improve performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/common.js(1 hunks)scripts/dummySourceConfigApiToTestSdKs.js(1 hunks)scripts/generateConstants.js(1 hunks)
🧰 Additional context used
🪛 eslint
scripts/generateConstants.js
[error] 4-4: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
scripts/dummySourceConfigApiToTestSdKs.js
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 4-4: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 5-5: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 6-6: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (1)
scripts/common.js (1)
17-23: Validate unknown platforms before checkinglanguageMap[platform].
The optional chaining prevents runtime errors; however, you might consider logging unrecognized platforms to detect invalid configurations.Here is a possible snippet to verify if any platform is missing from
languageMap:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1856 +/- ##
=========================================
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: 2
🧹 Nitpick comments (6)
scripts/dummySourceConfigApiToTestSdKs.js (6)
37-37: Consider lazy loading of destinations.The destinations are loaded and cached at startup. For a test server, this might be fine, but consider implementing lazy loading or periodic refresh if the configurations need to be updated during runtime.
39-62: Document the mock configuration structure.Consider adding JSDoc comments to document the expected structure and purpose of the mock configuration. This will help other developers understand what this test configuration represents.
Example:
+/** + * Generates a mock source configuration for testing device mode SDK loading. + * @param {string} writeKey - The write key to include in the configuration + * @returns {Object} A mock source configuration object matching the production schema + */ const getSourceConfig = (writeKey) => ({
103-104: Improve auth info handling.The current implementation mixes auth sources without clear precedence rules.
Consider this improvement:
- const authInfo = getAuthInfo(authHeader); - const writeKey = authInfo?.username || query.writeKey; + const authInfo = getAuthInfo(authHeader); + if (authInfo.error) { + console.warn(`Auth validation failed: ${authInfo.error}`); + } + // Prefer auth header over query param + const writeKey = authInfo.username || query.writeKey;
79-83: Document CORS configuration decisions.The current CORS configuration allows all origins, which is fine for testing but should be documented as a test-only configuration.
Add a comment explaining the permissive CORS setup:
function setCorsHeaders(res) { + // Note: Permissive CORS configuration for testing purposes only + // Do not use this configuration in production res.setHeader('Access-Control-Allow-Origin', '*');
86-111: Add request timeout and error handling.The server implementation could benefit from additional error handling and timeouts.
Consider these improvements:
const server = http.createServer((req, res) => { + // Set a timeout for all requests + req.setTimeout(5000, () => { + res.writeHead(408); + res.end('Request timeout'); + }); + + // Handle request errors + req.on('error', (error) => { + console.error('Request error:', error); + res.writeHead(500); + res.end('Internal server error'); + }); // Rest of the implementation... });
113-117: Add graceful shutdown handling.The server should handle shutdown signals to close existing connections gracefully.
Add this after the server creation:
process.on('SIGTERM', () => { console.log('Received SIGTERM. Performing graceful shutdown...'); server.close(() => { console.log('Server closed'); process.exit(0); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/dummySourceConfigApiToTestSdKs.js(1 hunks)
🧰 Additional context used
🪛 eslint
scripts/dummySourceConfigApiToTestSdKs.js
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 4-4: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 5-5: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 6-6: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (1)
scripts/dummySourceConfigApiToTestSdKs.js (1)
1-9: LGTM! Module imports and path handling look good.The code correctly uses
path.joinfor cross-platform path handling and properly imports required modules.🧰 Tools
🪛 eslint
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 4-4: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 5-5: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 6-6: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
What are the changes introduced in this PR?
This creates a mock source API with device mode destinations to test loading of SDKs
What is the related Linear task?
Resolves INT-3081
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
Refactor
Chores