feat: add runDiscourse extraction function #395
Conversation
WalkthroughThe changes introduced in this pull request involve the addition of a new environment variable, Changes
Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (5)
src/services/discourse/core.service.ts (1)
Line range hint
22-42: Address content type mismatch and improve error handling.There are a few issues in the implementation of
runDiscourseExtraction:
Content type mismatch: The
Content-Typeheader is set to'application/json', but the data is sent usingURLSearchParams. This is inconsistent and may cause issues.Error handling: The current implementation logs the error but doesn't provide specific error information in the thrown
ApiError.Please consider the following improvements:
- Fix the content type mismatch:
- body: new URLSearchParams(data), - headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(data), + headers: { 'Content-Type': 'application/json' },
- Improve error handling:
} catch (error) { - logger.error(error, 'Failed to run discourse extraction discourse'); - throw new ApiError(590, 'Failed to run discourse extraction discourse'); + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + logger.error({ error }, `Failed to run discourse extraction: ${errorMessage}`); + throw new ApiError(590, `Failed to run discourse extraction: ${errorMessage}`); }These changes will ensure consistency in the request format and provide more informative error messages.
src/controllers/platform.controller.ts (4)
Line range hint
44-44: Security concern: Avoid setting 'userId' from untrusted sourcesSetting
req.session.userId = req.query.userId;directly from a query parameter can lead to security vulnerabilities like session fixation or impersonation attacks. It's safer to retrieve theuserIdfrom the authenticated session or a secure token rather than user-controlled input.To fix this issue, remove the assignment from the query parameter and ensure
userIdis obtained from a trusted source. Here's the suggested change:- req.session.userId = req.query.userId;
Line range hint
78-80: Ensure 'userId' is obtained from a secure sourceIn the
connectGoogleCallbackfunction,userIdis retrieved fromreq.session.userId, which was previously set from a query parameter. Since query parameters can be manipulated, this may lead to security issues.Ensure that
userIdin the session is set from a trusted source. Remove any assignments from query parameters toreq.session.userId.
Line range hint
131-131: Security concern: Missing state parameter validation in 'connectGithubCallback'The
connectGithubCallbackfunction does not validate astateparameter to protect against Cross-Site Request Forgery (CSRF) attacks. Including and verifying astateparameter is a standard security practice in OAuth flows to ensure the integrity of the request.Consider modifying the
connectGithubCallbackfunction to includestateparameter generation and validation similar to other platform callbacks.
Line range hint
159-159: Typo: Duplicate 'await' in function callThere's a duplicated
awaitin the following line:const installation = await await githubService.coreService.getInstallationDetails(appAccessToken, installationId);This may cause unintended behavior or a syntax error.
Apply this diff to fix the typo:
- const installation = await await githubService.coreService.getInstallationDetails(appAccessToken, installationId); + const installation = await githubService.coreService.getInstallationDetails(appAccessToken, installationId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/config/index.ts (2 hunks)
- src/controllers/platform.controller.ts (1 hunks)
- src/services/discourse/core.service.ts (3 hunks)
- src/services/platform.service.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (8)
src/services/discourse/core.service.ts (2)
48-48: LGTM: Export of new function.The addition of
runDiscourseExtractionto the default export is correct and consistent with the function renaming.
5-6: Verify the usage of theTypesimport.The
Typesimport from 'mongoose' has been added, but it's not used in the visible parts of the code. Please ensure it's necessary for other parts of the file or remove it if unused.✅ Verification successful
Remove the unused
Typesimport.The
Typesimport from 'mongoose' is not used insrc/services/discourse/core.service.tsand can be safely removed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Types from mongoose is used in the file. rg --type typescript -w "Types" src/services/discourse/core.service.tsLength of output: 105
Script:
#!/bin/bash # Description: Check if Types from mongoose is used in the file. rg --type ts -w "Types" src/services/discourse/core.service.tsLength of output: 96
src/config/index.ts (3)
58-58: LGTM: New environment variable added correctly.The new
DISCOURSE_EXTRACTION_URLenvironment variable has been added to theenvVarsSchemafollowing the existing pattern. It's correctly defined as a required string with a clear description.
158-160: LGTM: New configuration property added correctly.The new
discourseproperty has been added to the exported configuration object following the existing structure. TheextractionURLfield correctly maps to the new environment variable, and the formatting is consistent with the rest of the file.
58-58: Summary: Discourse extraction URL configuration added successfully.The changes to this file are minimal and focused, adding only the necessary new configuration for the Discourse extraction URL. The additions are consistent with the existing code style and structure, and no potential issues or side effects are apparent from these changes.
To ensure the new configuration is used correctly:
Also applies to: 158-160
✅ Verification successful
Summary: Discourse extraction URL configuration verified successfully.
The
DISCOURSE_EXTRACTION_URLhas been correctly added tosrc/config/index.tsand is properly utilized insrc/services/discourse/core.service.ts. No issues were found with the implementation or usage of this configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of the new DISCOURSE_EXTRACTION_URL configuration # Check if the new environment variable is used in the codebase echo "Checking usage of DISCOURSE_EXTRACTION_URL:" rg "DISCOURSE_EXTRACTION_URL" --type ts # Check if the new configuration property is accessed in the codebase echo "\nChecking usage of config.discourse.extractionURL:" rg "config\.discourse\.extractionURL" --type tsLength of output: 593
src/services/platform.service.ts (1)
268-268: LGTM: Proper export of the new functionThe
callExtractionAppfunction is correctly added to the default export object, making it accessible to other modules that import this service. This integration is clean and follows the existing pattern in the file.src/controllers/platform.controller.ts (2)
Line range hint
44-54: Validate scopes and handle empty or invalid scopes arrayWhen aggregating requested scopes for Google OAuth:
let requestedScopes = req.query.scopes as string[]; let aggregatedScopes: string[] = []; requestedScopes.forEach((scope) => { if (google.scopes[scope]) { aggregatedScopes = [...aggregatedScopes, ...google.scopes[scope]]; } });Ensure that
requestedScopesis properly validated, and handle cases where it might be empty or contain invalid scope values to prevent errors during the OAuth process.To check if all
scopevalues are valid and handle potential issues, consider adding validation and default scopes if necessary.
Line range hint
180-184: Verify the implementation of 'getPropertyHandler' for DiscourseThe
getPropertiesfunction now includes handling forPlatformNames.Discourse. Ensure thatdiscourseService.coreService.getPropertyHandler(req);is properly implemented and that it correctly handles the request.To verify that
getPropertyHandlerexists and is correctly implemented indiscourseService:✅ Verification successful
Verification Successful: The
getPropertyHandlermethod is properly implemented indiscourseService.coreServiceand correctly handles the request.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of 'getPropertyHandler' in 'discourseService.coreService' # Test: Search for the 'getPropertyHandler' method in 'discourseService.coreService.ts' ast-grep --lang typescript --pattern 'getPropertyHandler($_)' src/services/discourse/core.service.tsLength of output: 102
Script:
#!/bin/bash # Description: Search for 'getPropertyHandler' across the entire TypeScript codebase to locate its implementation rg 'getPropertyHandler' --type typescriptLength of output: 76
Script:
#!/bin/bash # Description: Search for 'getPropertyHandler' across all TypeScript files in the codebase rg 'getPropertyHandler' --type tsLength of output: 570
Script:
#!/bin/bash # Description: Extract the implementation of 'getPropertyHandler' from 'src/services/discourse/core.service.ts' ast-grep --lang typescript --pattern 'async function getPropertyHandler($_) { $$$ }' src/services/discourse/core.service.tsLength of output: 753
| * run discourse extraction | ||
| * @param {Strin} platformId | ||
| * @returns {Promise<IDiscordUser>} | ||
| */ |
There was a problem hiding this comment.
Update the return type in the function comment.
The function comment incorrectly states that the return type is Promise<IDiscordUser>. This should be updated to match the actual return type of the function, which is Promise<void>.
Please update the comment as follows:
/**
* run discourse extraction
- * @param {Strin} platformId
- * @returns {Promise<IDiscordUser>}
+ * @param {string} platformId
+ * @returns {Promise<void>}
*/📝 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.
| * run discourse extraction | |
| * @param {Strin} platformId | |
| * @returns {Promise<IDiscordUser>} | |
| */ | |
| /** | |
| * run discourse extraction | |
| * @param {string} platformId | |
| * @returns {Promise<void>} | |
| */ |
| /** | ||
| * Call extraction app for the given platform | ||
| * @param {HydratedDocument<IPlatform>} platform | ||
| * @returns {Promise<Void>} | ||
| */ | ||
| const callExtractionApp = async (platform: HydratedDocument<IPlatform>): Promise<void> => { | ||
| switch (platform.name) { | ||
| case PlatformNames.Discourse: { | ||
| await discourseService.coreService.runDiscourseExtraction(platform.id as string); | ||
| return; | ||
| } | ||
| default: { | ||
| return; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling, logging, and type safety in callExtractionApp
The new callExtractionApp function looks good overall, but consider the following improvements:
- Error handling: Wrap the
discourseService.coreService.runDiscourseExtractioncall in a try-catch block to handle potential errors. - Logging: Add logging statements to track the function's execution and any errors that occur.
- Type safety: Use a type guard to ensure
platform.idis a string before passing it torunDiscourseExtraction.
Here's a suggested implementation incorporating these improvements:
import { logger } from '../utils/logger'; // Assume a logger utility exists
const callExtractionApp = async (platform: HydratedDocument<IPlatform>): Promise<void> => {
logger.info(`Calling extraction app for platform: ${platform.name}`);
switch (platform.name) {
case PlatformNames.Discourse: {
if (typeof platform.id !== 'string') {
throw new Error(`Invalid platform id type for Discourse: ${typeof platform.id}`);
}
try {
await discourseService.coreService.runDiscourseExtraction(platform.id);
logger.info(`Discourse extraction completed for platform id: ${platform.id}`);
} catch (error) {
logger.error(`Error during Discourse extraction: ${error}`);
throw error; // Re-throw or handle as appropriate
}
return;
}
default: {
logger.warn(`No extraction process defined for platform: ${platform.name}`);
return;
}
}
};This implementation adds error handling, logging, and ensures type safety for the platform id.
| @@ -25,7 +25,7 @@ const logger = parentLogger.child({ module: 'PlatformController' }); | |||
| const createPlatform = catchAsync(async function (req: IAuthRequest, res: Response) { | |||
| const community = req.community; | |||
| const platform = await platformService.managePlatformConnection(community?.id, req.body); | |||
| // await airflowService.triggerDag(platform); | |||
| await platformService.callExtractionApp(platform); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure all dependencies are updated after changing the extraction call
The call to platformService.callExtractionApp(platform); replaces the previous call to airflowService.triggerDag(platform);. Make sure that any dependencies on airflowService are properly updated or removed if they are no longer needed to prevent orphaned code and potential confusion.
Summary by CodeRabbit
New Features
Improvements
Refactor