Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a patch file for the mock-firebolt server that introduces bidirectional event handling improvements and a new raw payload testing API. The patch modifies event validation logic, updates OpenRPC method lookup to support SDK-specific queries, and adds a raw API endpoint for testing purposes.
Key changes:
- Enhanced event validation for bidirectional mode with support for wrapped values and x-notifier-for tags
- Added raw payload API endpoint for testing websocket communications
- Renamed
createBidirectionalPayloadtocreateBidirectionalEventPayloadand removed auto-incrementing ID
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| + * | ||
| + * Unless required by applicable law or agreed to in writing, software | ||
| + * distributed under the License is distributed on an "AS IS" BASIS, | ||
| + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. |
There was a problem hiding this comment.
Spelling error in comment: "paload" should be "payload".
| + } | ||
| + const resultErrors = fireboltOpenRpc.validateMethodResult(resultToValidate, method); | ||
| if ( resultErrors && resultErrors.length > 0 ) { | ||
| + logger.error(`${method} validation error for ${method} with ${JSON.stringify(resultToValidate)}, err: ${JSON.stringify(resultErrors)}`); |
There was a problem hiding this comment.
The error message duplicates the method name: "${method} validation error for ${method}". This should likely say something like "Event validation error for ${method}" or just "Validation error for ${method}".
| + logger.error(`${method} validation error for ${method} with ${JSON.stringify(resultToValidate)}, err: ${JSON.stringify(resultErrors)}`); | |
| + logger.error(`Validation error for ${method} with ${JSON.stringify(resultToValidate)}, err: ${JSON.stringify(resultErrors)}`); |
| +// POST /api/v1/raw | ||
| +// Expected body: Any | ||
| +function rawPayload(req, res) { | ||
| + try { | ||
| + const userId = getUserIdFromReq(req); | ||
| + const ws = userManagement.getWsForUser(userId) | ||
| + | ||
| + const payload = { | ||
| + jsonrpc: "2.0", | ||
| + ...req.body |
There was a problem hiding this comment.
Missing error handling: If 'ws' is null or undefined (when getWsForUser returns no connection), calling ws.send() will throw an error. Add validation to check if ws exists before attempting to send the payload.
| + try { | ||
| + const userId = getUserIdFromReq(req); | ||
| + const ws = userManagement.getWsForUser(userId) | ||
| + |
There was a problem hiding this comment.
Security concern: The raw API endpoint accepts arbitrary JSON payloads without any validation and sends them directly to websocket connections. This could allow malicious or malformed payloads to be sent to clients. Consider adding at least basic validation to ensure the payload structure is safe and expected fields are present.
| + | |
| + | |
| + // Basic validation: payload must be a non-null object, not an array | |
| + if ( | |
| + typeof req.body !== 'object' || | |
| + req.body === null || | |
| + Array.isArray(req.body) | |
| + ) { | |
| + return res.status(400).send({ | |
| + status: "Invalid payload: must be a JSON object" | |
| + }); | |
| + } | |
| + | |
| + // Optionally, check for required fields (e.g., method) | |
| + // if (!('method' in req.body)) { | |
| + // return res.status(400).send({ | |
| + // status: "Invalid payload: missing 'method' field" | |
| + // }); | |
| + // } | |
| + |
| + | ||
| +// --- Route Handlers --- | ||
| + | ||
| +// POST /api/v1/raw |
There was a problem hiding this comment.
Missing semicolon at the end of the statement. While JavaScript allows this, the codebase appears to use semicolons consistently (as seen throughout the patch file).
| + * limitations under the License. | ||
| + */ | ||
| + | ||
| +// HTTP-based API routes: Raw, without any validation, just "jsonrpc": "2.0" field is being added to the paload |
There was a problem hiding this comment.
Missing semicolon at the end of the statement. While JavaScript allows this, the codebase appears to use semicolons consistently (as seen throughout the patch file).
| + let oResult = undefined; | ||
| + if (config.dotConfig.bidirectional && methodName.includes('.on')) { | ||
| + let oMethod = getMethod(methodName, "coreToApp"); | ||
| + if (oMethod && oMethod.tags) { |
There was a problem hiding this comment.
Potential null/undefined error: If 'oMethod' is undefined or null (when getMethod returns undefined), accessing 'oMethod.tags' will throw a TypeError. The code should verify 'oMethod' exists before accessing its properties.
| + if (oMethod && oMethod.tags) { | |
| + if (oMethod && Array.isArray(oMethod.tags)) { |
|
|
||
| try { | ||
| - const oMethod = getMethod(methodName); | ||
| - const oResult = oMethod.result; |
There was a problem hiding this comment.
Variable shadowing issue: A new variable 'oMethod' is declared with 'let' inside the if block, which shadows the outer 'oMethod' variable declared on line 119. This prevents the outer variable from being assigned and causes the condition check on line 134 to always be true. Remove the 'let' keyword on this line to assign to the outer variable instead.
| + const payload = { | ||
| + jsonrpc: "2.0", | ||
| + ...req.body | ||
| + }; | ||
| + | ||
| + console.log(`Sending raw payload: ${JSON.stringify(payload)}`); | ||
| + | ||
| + ws.send(JSON.stringify(payload)); | ||
| + |
There was a problem hiding this comment.
Use logger instead of console.log/console.error for consistency. Other parts of the codebase use the logger module for logging (as seen in the fireboltOpenRpc.mjs and events.mjs changes), but this new route uses console methods. Import and use the logger module for consistent logging practices.
|
🎉 This PR is included in version 0.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
No description provided.