feat: Integrate Presto for enhanced analytics and reporting #4682
Conversation
📝 WalkthroughWalkthroughThe changes update the database connection mechanism by replacing the use of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant AnalyticsController
participant AnalyticsUtil
participant Database
Client->>Router: GET /presto/user-stats (with query params)
Router->>AnalyticsController: getPrestoUserStats(req, res, next)
AnalyticsController->>AnalyticsUtil: getPrestoUserStats(parameters)
AnalyticsUtil->>Database: Execute SQL query via Presto client
Database-->>AnalyticsUtil: Return query results
AnalyticsUtil-->>AnalyticsController: Return statistics data
AnalyticsController-->>Router: Send response with stats
Suggested labels
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #4682 +/- ##
===========================================
- Coverage 11.39% 11.23% -0.16%
===========================================
Files 156 111 -45
Lines 19161 15748 -3413
Branches 584 0 -584
===========================================
- Hits 2183 1769 -414
+ Misses 16974 13979 -2995
+ Partials 4 0 -4 🚀 New features to boost your workflow:
|
|
Auth-service changes in this PR available for preview here |
![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
♻️ Duplicate comments (7)
src/auth-service/controllers/analytics.controller.js (3)
901-936: Duplicate concern regarding shared code structure.The repeated steps (error handling, tenant derivation, calling a utility, etc.) mirror previous comments. Consolidating these common steps could improve readability and maintainability.
937-972: Duplicate concern regarding date validation and refactoring.As highlighted previously, consider validating the date parameters explicitly and refactoring the repeated logic into a shared utility method.
973-1008: Duplicate concern regarding repetitive pattern.The code block again implements similar logic. Centralizing this pattern in a helper function can significantly reduce future maintenance overhead.
src/auth-service/utils/analytics.util.js (4)
2692-2736: Duplicate concern about unsanitized user inputs in dynamic query.As noted, constructing queries with user-provided parameters should be done safely. Apply the same security measures to prevent injection risks and validate the date values.
2738-2782: Duplicate concern about query construction.Reinforce input validation and avoid interpolating raw user input directly into SQL. This will help mitigate vulnerabilities.
2784-2828: Duplicate concern regarding dynamic query inputs.Similar to other functions in this file, ensure that
startDateandendDateare validated to prevent malicious or invalid input strings from triggering query issues.
2830-2874: Duplicate concern about raw user input in query.Please remember to sanitize or validate
startDateandendDate. Consistent input checks help maintain security and reliability across these endpoints.
🧹 Nitpick comments (6)
src/auth-service/config/environments/development.js (1)
44-50: Consider documenting the purpose of dual authentication parameters.There are two sets of credentials:
PRESTO_USER/PRESTO_PASSWORDandPRESTO_AUTH_USER/PRESTO_AUTH_PASSWORD. The distinction between these credential sets isn't immediately clear.Consider adding a comment explaining the purpose of each credential set:
PRESTO_HOST: process.env.DEV_PRESTO_HOST, PRESTO_PORT: process.env.DEV_PRESTO_PORT, +// Credentials for Presto client connections PRESTO_USER: process.env.DEV_PRESTO_USER, PRESTO_PASSWORD: process.env.DEV_PRESTO_PASSWORD, PRESTO_DATABASE: process.env.DEV_PRESTO_DATABASE, +// Credentials for authentication between services PRESTO_AUTH_USER: process.env.DEV_PRESTO_AUTH_USER, PRESTO_AUTH_PASSWORD: process.env.DEV_PRESTO_AUTH_PASSWORD,src/auth-service/config/database.js (3)
34-46: Add environment variable checks for Presto fallback.Defining PRESTO_CONFIG is comprehensive. Consider validating environment variables or throwing descriptive errors if they’re missing, especially for host/port, to avoid silent misconfigurations.
89-92: Initialize Presto client after MongoDB connections.It’s helpful to initialize the Presto client in the same flow, ensuring all database connections are activated consistently. Consider adding minimal retry logic if Presto is critical.
186-231: Complement large results with streaming or pagination.The
executeFederatedQueryfunction effectively collects data viapresto-client. However, for potentially large queries, consider a streaming approach or pagination to avoid high memory usage. Otherwise, this promise-based pattern is acceptable for moderate workloads.src/auth-service/controllers/analytics.controller.js (2)
830-864: Consider validating date inputs.It would be beneficial to add explicit checks to ensure that
startDateandendDateare valid and non-empty. This can help prevent potential errors or incorrect results when these parameters are missing or invalid.
865-900: Reduce repetitive patterns.This block follows the same logic pattern as
getPrestoUserStats. Consider extracting shared logic (e.g., tenant assignment, error extraction, response handling) into a reusable helper function to keep the code DRY and more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/auth-service/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
src/auth-service/bin/server.js(1 hunks)src/auth-service/config/database.js(6 hunks)src/auth-service/config/environments/development.js(1 hunks)src/auth-service/config/environments/production.js(1 hunks)src/auth-service/config/environments/staging.js(1 hunks)src/auth-service/controllers/analytics.controller.js(1 hunks)src/auth-service/package.json(1 hunks)src/auth-service/routes/v2/analytics.routes.js(1 hunks)src/auth-service/utils/analytics.util.js(1 hunks)src/auth-service/validators/analytics.validators.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/auth-service/config/database.js (2)
src/auth-service/bin/server.js (3)
require(11-11)require(18-24)require(43-43)src/auth-service/utils/analytics.util.js (8)
require(2-11)require(12-12)require(20-26)require(2662-2662)require(2708-2708)require(2754-2754)require(2800-2800)require(2846-2846)
src/auth-service/controllers/analytics.controller.js (4)
src/auth-service/utils/analytics.util.js (18)
httpStatus(27-27)request(1531-1531)request(1571-1571)request(1610-1610)request(1649-1649)request(1677-1677)request(1715-1715)request(1753-1753)request(1781-1781)request(1809-1813)request(1847-1847)request(1878-1878)request(1918-1918)request(1960-1966)request(2461-2461)request(2489-2489)request(2536-2536)constants(1-1)src/auth-service/config/database.js (1)
constants(6-6)src/auth-service/validators/analytics.validators.js (1)
constants(2-2)src/auth-service/bin/server.js (2)
constants(3-3)isEmpty(38-38)
src/auth-service/utils/analytics.util.js (4)
src/auth-service/controllers/analytics.controller.js (5)
constants(2-2)require(3-3)require(9-15)httpStatus(4-4)logger(6-8)src/auth-service/config/database.js (3)
constants(6-6)require(9-15)logger(8-8)src/auth-service/validators/analytics.validators.js (2)
constants(2-2)require(1-1)src/auth-service/bin/server.js (5)
constants(3-3)require(11-11)require(18-24)require(43-43)logger(39-41)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (14)
src/auth-service/package.json (1)
89-89:✅ Verification successful
New dependency looks good, but verify version.
The addition of the Presto client library is appropriate for the integration as specified in the PR objectives.
🏁 Script executed:
#!/bin/bash # Verify latest version of presto-client npm view presto-client versionLength of output: 38
Dependency Version Verified: The dependency "presto-client" is confirmed to be at version 1.1.0 in
src/auth-service/package.json(line 89) as validated by the npm query. The addition meets the PR objectives, and no further modifications are necessary.src/auth-service/validators/analytics.validators.js (1)
182-182: Good addition of validateDateRange to module exports.The export of this validation function makes it available for reuse in the new Presto analytics routes, ensuring consistent date range validation across the application.
src/auth-service/bin/server.js (1)
11-12:✅ Verification successful
Function renamed to support multiple database connections.
The change from
connectToMongoDBtoconnectToDatabasesaccurately reflects the expanded functionality to support both MongoDB and Presto connections.
🏁 Script executed:
#!/bin/bash # Check the implementation of connectToDatabases to ensure it properly handles errors cat src/auth-service/config/database.js | grep -A 30 "connectToDatabases"Length of output: 2135
Action: Approve function renaming and updated error handling
The updated
connectToDatabasesfunction now correctly initializes both MongoDB (command and query) and Presto connections, along with proper error and exception handling using try/catch and global event listeners. The changes accurately reflect the intended expanded functionality from the previousconnectToMongoDBimplementation.No additional updates are needed.
src/auth-service/config/environments/development.js (1)
44-50:✅ Verification successful
Presto configuration parameters are appropriately defined.
The Presto configuration parameters are correctly set up using environment variables, maintaining consistency with other configuration practices in the codebase.
🏁 Script executed:
#!/bin/bash # Check if similar configuration exists in other environment files echo "Checking staging environment:" grep -A 10 "PRESTO_" src/auth-service/config/environments/staging.js || echo "No Presto config in staging" echo "Checking production environment:" grep -A 10 "PRESTO_" src/auth-service/config/environments/production.js || echo "No Presto config in production"Length of output: 1075
Consistent Environment Configurations Verified
The Presto configuration in the development file is defined consistently with the staging and production environments. All three files use the appropriate environment-specific variable names (e.g., DEV_PRESTO_, STAGE_PRESTO_, PROD_PRESTO_*), confirming that the configuration parameters are correctly set up across the environments.
src/auth-service/config/environments/production.js (1)
44-51: Use environment variable validation for Presto credentials.These environment variables look correct for production, but consider validating them at startup (e.g., checking for null or undefined) to prevent runtime errors if any of them are missing. This also helps ensure deployment readiness.
src/auth-service/config/environments/staging.js (1)
45-52: Maintain consistency with production configuration.The added Presto-related environment variables align with the production environment approach. Ensure the staging secrets are securely managed and do not leak sensitive credentials in logs.
src/auth-service/routes/v2/analytics.routes.js (1)
132-161: Confirm authentication and throttling for new routes.These new
/prestoanalytics routes appear well-structured and consistent with existing API patterns. However, ensure that only authorized users can access these analytics, and consider rate-limiting if these endpoints can lead to heavy query loads on Presto.src/auth-service/config/database.js (7)
16-16: Ensurepresto-clientis installed and up to date.Adding
presto-clientis crucial for Presto integration. Verify that it's listed in your project dependencies and confirm its version meets your security and performance requirements.
64-64: Variable naming aligns with usage.Storing the Presto client instance as
prestoClientInstanceis clear and aligns with the naming convention used below. This helps readability.
80-82: Method renamed to support multiple databases smoothly.Renaming
connectToMongoDBtoconnectToDatabasesclarifies that both MongoDB and Presto connections are established. This is a good step toward better maintainability.
103-103: Return both MongoDB connections and Presto client.Returning a structured object with named properties simplifies usage throughout the app. Great job keeping it self-explanatory.
105-105: Log the error and rethrow.This is a solid error-handling pattern: log the error details and rethrow, preventing silent failures while still providing diagnostic information.
110-115: Early initialization of databases and Presto client on import.Deconstructing the returned connections upon module load ensures availability throughout the application. Verify that any modules depending on these connections do not inadvertently import too soon, especially for test setups.
253-260: Exporting new database utilities.Exporting these new Presto-related methods (and
connectToDatabases) helps keep the architecture modular and consistent with existing patterns.
| /** | ||
| * Execute a federated query across multiple tenant databases | ||
| * @param {Array} tenantIds - Array of tenant IDs to query across | ||
| * @param {string} query - SQL query template (with placeholders for tenant IDs) | ||
| * @returns {Promise<Array>} - Combined results | ||
| */ | ||
| async function queryAcrossTenants(tenantIds, queryTemplate) { | ||
| // Create a UNION ALL query across multiple tenant databases | ||
| const unionQueries = tenantIds.map((tenantId) => { | ||
| // Replace placeholders with actual tenant IDs | ||
| return queryTemplate.replace(/\${tenantId}/g, tenantId); | ||
| }); | ||
|
|
||
| const fullQuery = unionQueries.join(" UNION ALL "); | ||
| return executeFederatedQuery(fullQuery); | ||
| } | ||
|
|
There was a problem hiding this comment.
Protect against potential SQL injection in dynamic queries.
Replacing tenant placeholders using .replace() can be fine if tenant IDs are controlled, but always be mindful of injection risks when constructing queries dynamically. Adding a check or sanitization step ensures unexpected user input doesn’t break your query logic.
| getPrestoUserStats: async (request, next) => { | ||
| try { | ||
| const { tenant, startDate, endDate } = request.query; | ||
|
|
||
| // Construct the Presto query | ||
| const prestoQuery = ` | ||
| SELECT | ||
| COUNT(*) AS totalUsers, | ||
| COUNT(CASE WHEN isActive = true THEN 1 END) AS activeUsers, | ||
| AVG(loginCount) AS averageLoginCount | ||
| FROM | ||
| mongodb."${constants.DB_NAME}_${tenant.toLowerCase()}".users | ||
| WHERE | ||
| createdAt >= TIMESTAMP '${new Date(startDate).toISOString()}' | ||
| AND createdAt <= TIMESTAMP '${new Date(endDate).toISOString()}' | ||
| `; | ||
|
|
||
| // Execute the Presto query | ||
| const { executeFederatedQuery } = require("@config/database"); | ||
| const responseFromPresto = await executeFederatedQuery(prestoQuery); | ||
|
|
||
| if (responseFromPresto && responseFromPresto.length > 0) { | ||
| return { | ||
| success: true, | ||
| message: "Successfully retrieved user statistics from Presto", | ||
| data: responseFromPresto[0], | ||
| status: httpStatus.OK, | ||
| }; | ||
| } else { | ||
| return { | ||
| success: true, | ||
| message: "No user statistics found", | ||
| data: {}, | ||
| status: httpStatus.OK, | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Potential injection risk when constructing query strings.
Directly interpolating startDate and endDate into the query string without additional sanitization can pose a security threat. Consider validating and sanitizing these inputs, or using parameterized queries if supported, to mitigate potential SQL injection risks.
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (17)
src/auth-service/controllers/analytics.controller.js (8)
854-889: Avoid code repetition in Presto query functions.The query construction and execution logic here is nearly identical to other Presto-based functions. Consider extracting common logic (e.g., date handling and query execution) into a shared helper to reduce code duplication.
927-961: Consider consolidating the 'getRecentUsers' logic.There is a pattern of repeated code related to date handling and Presto query execution. Merging common steps into a utility or reusing an existing helper could simplify maintenance.
963-997: Refine approach for retrieving new users.This function closely resembles other newly introduced Presto-based methods. Ensure consistent parameter validation and consider reusing a common Presto execution helper to improve clarity.
1074-1111: Inconsistent or missing error handling for invalid dates.Similar to other Presto integration points, ensure the code handles incorrect date formats or absent date parameters. This helps prevent unexpected query behavior.
1113-1147: Promote code reusability for top devices retrieval.The lines constructing the query for top devices are essentially repeated in other functions with minor differences. Extraction of a common query builder could make the code more maintainable.
1224-1258: Encourage standardized date validations.This function for daily active users follows the same pattern of building a time-based query. A shared utility or middleware for date verification could streamline repeated checks.
1296-1333: Clarify usage time calculation.When averaging usage over a time window, confirm whether the difference in timestamps should account for edge cases (e.g. identical timestamps, missing lastActive). Minor clarifications could preempt inaccurate results.
1371-1405: Consistent pattern for fetching least used features.Same remarks as other Presto-based methods: unify date validation, handle missing parameters, and consider extracting repeated query logic for maintainability.
src/auth-service/routes/v2/analytics.routes.js (1)
132-233: Consolidate repetitive route definitions for Presto analytics.Many newly added routes share similar validations (
tenant,validateDateRange). Consider grouping them or creating a composite middleware to streamline definition and avoid redundancy.src/auth-service/utils/analytics.util.js (8)
2680-2715: Optimize repeated pattern for building Presto queries.The logic here closely mirrors that in other newly added functions (e.g., validating
tenant, building query strings). A utility approach could reduce duplication and enhance maintainability.
2727-2771: Same query structure with minimal differences.For active APIs, you could unify the repeated lines (tenant retrieval, date conversion, query building), especially if the only difference is the target collection and fields.
2958-3004: Ensure 'activity summary' aligns with aggregator logic.As with similar blocks, the code structure remains consistent, but re-usable helper methods could improve clarity and consistency across all these Presto queries.
3005-3056: Reassess duplication for 'top devices' retrieval.Major sections of this query logic parallel other functions. Consider extracting a general-purpose query executor that parameterizes the collection name, date range, and grouping fields.
3057-3107: Scrutinize location data queries for missing or nullable fields.It's possible that
users.countryis not always set. Provide fallback or ensure data is well-defined in Presto to avoid edge-case errors.
3143-3188: Daily active users logic is repetitive.Similar pattern notes: confirm date validation and consider a shared helper for repeated query building. This helps reduce risk of overshadowing minor differences.
3236-3281: Confirm unit correctness for usage time calculations.When averaging
(lastActive - createdAt)as a DOUBLE, confirm that resulting values are measured in a clear unit (e.g. hours, days) to present consistent, interpretable usage times.
3283-3333: Avoid duplication across 'most used' and 'least used' features.This block is structurally similar to the code for least-used features. Extracting a unified function that parameterizes the sort order could streamline your logic further.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/auth-service/controllers/analytics.controller.js(1 hunks)src/auth-service/routes/v2/analytics.routes.js(1 hunks)src/auth-service/utils/analytics.util.js(1 hunks)src/auth-service/validators/analytics.validators.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/auth-service/validators/analytics.validators.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/auth-service/controllers/analytics.controller.js
[error] 929-963: This property value named getRecentUsers is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named getRecentUsers
(lint/suspicious/noDuplicateObjectKeys)
src/auth-service/utils/analytics.util.js
[error] 2775-2819: This property value named getRecentUsers is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named getRecentUsers
(lint/suspicious/noDuplicateObjectKeys)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
src/auth-service/controllers/analytics.controller.js (6)
819-853: Ensure robust date validation for Presto queries.Kindly verify that
startDateandendDateare properly validated before constructing the query to avoid potential issues with invalid or missing dates.
891-925: Validate 'startDate' and 'endDate' to prevent invalid queries.As in other Presto-based methods, ensure these query parameters are present and valid. Otherwise, the resulting query might produce unexpected results or errors.
1035-1072: Check for possible injection if query parameters are not strictly validated.While this logic looks consistent for retrieving user engagement metrics from Presto, confirm that all input parameters (
tenant,startDate,endDate) are validated upstream to avoid injection vulnerabilities.
1149-1183: Potential missing fallback for date range.This newly introduced location-data function should gracefully handle missing or malformed
startDateandendDate. Otherwise, queries might fail or return unexpectedly broad results.
1260-1294: Check for null or undefined date parameters.Similarly, be sure to handle any run-time scenario where
startDateorendDatemight be missing, to avoid generating invalid Presto queries for monthly active users.
1335-1369: Refine limit usage for performance.When retrieving the most used features, double-check whether capping results at a default or user-specified
limitis enforced. Large queries could adversely impact performance if the limit is not carefully validated.src/auth-service/utils/analytics.util.js (4)
2633-2679: Validate query parameters before constructing Presto query.The code uses
new Date(startDate).toISOString()andnew Date(endDate).toISOString(). Consider verifying thatstartDateandendDateare valid dates to safeguard against malformed queries.
2910-2957: Check for potential injection in custom Presto logs queries.Although it references the logs collection, confirm that each incoming parameter (especially timestamps) is validated to prevent unexpected injection or erroneous queries.
3190-3234: Match monthly date range usage to daily approach.Check that the monthly active users function defends against absent or malformed date parameters, maintaining consistent logic with the daily active users approach.
3335-3385: Maintain consistent date validations for 'least used features'.As with other routes, ensure that the provided
startDateandendDateare valid. This fosters more reliable query execution.
| executeCustomQueryPresto: async (req, res, next) => { | ||
| try { | ||
| const errors = extractErrorsFromRequest(req); | ||
| if (errors) { | ||
| next( | ||
| new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const request = req; | ||
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | ||
| request.query.tenant = isEmpty(req.query.tenant) | ||
| ? defaultTenant | ||
| : req.query.tenant; | ||
|
|
||
| const result = await analyticsUtil.executeCustomQueryPresto( | ||
| request, | ||
| next | ||
| ); | ||
|
|
||
| if (isEmpty(result) || res.headersSent) { | ||
| return; | ||
| } | ||
|
|
||
| handleResponse({ result, key: "custom_query_result", res }); | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| return; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Review custom query input for malicious injection.
Executing random prestoQuery strings from the request body is high risk. Verify your custom query validator robustly sanitizes or restricts these queries to prevent injection.
| getRecentUsers: async (req, res, next) => { | ||
| try { | ||
| const errors = extractErrorsFromRequest(req); | ||
| if (errors) { | ||
| next( | ||
| new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const request = req; | ||
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | ||
| request.query.tenant = isEmpty(req.query.tenant) | ||
| ? defaultTenant | ||
| : req.query.tenant; | ||
|
|
||
| const result = await analyticsUtil.getRecentUsers(request, next); | ||
|
|
||
| if (isEmpty(result) || res.headersSent) { | ||
| return; | ||
| } | ||
|
|
||
| handleResponse({ result, key: "recent_users", res }); | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| return; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Duplicate key: 'getRecentUsers' overwrites the previous definition.
Defining another function named getRecentUsers here nullifies the earlier one at lines 927-961. Please rename or remove one of these functions to avoid confusion and ensure intended functionality.
- // Either rename this function:
- getRecentUsers: async (req, res, next) => {
+ getRecentlyActiveUsers: async (req, res, next) => {
...
}📝 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.
| getRecentUsers: async (req, res, next) => { | |
| try { | |
| const errors = extractErrorsFromRequest(req); | |
| if (errors) { | |
| next( | |
| new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) | |
| ); | |
| return; | |
| } | |
| const request = req; | |
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | |
| request.query.tenant = isEmpty(req.query.tenant) | |
| ? defaultTenant | |
| : req.query.tenant; | |
| const result = await analyticsUtil.getRecentUsers(request, next); | |
| if (isEmpty(result) || res.headersSent) { | |
| return; | |
| } | |
| handleResponse({ result, key: "recent_users", res }); | |
| } catch (error) { | |
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| return; | |
| } | |
| }, | |
| getRecentlyActiveUsers: async (req, res, next) => { | |
| try { | |
| const errors = extractErrorsFromRequest(req); | |
| if (errors) { | |
| next( | |
| new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) | |
| ); | |
| return; | |
| } | |
| const request = req; | |
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | |
| request.query.tenant = isEmpty(req.query.tenant) | |
| ? defaultTenant | |
| : req.query.tenant; | |
| const result = await analyticsUtil.getRecentUsers(request, next); | |
| if (isEmpty(result) || res.headersSent) { | |
| return; | |
| } | |
| handleResponse({ result, key: "recent_users", res }); | |
| } catch (error) { | |
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| return; | |
| } | |
| }, |
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, | ||
| // New function to get a list of recently active users using Presto | ||
| getRecentUsers: async (request, next) => { | ||
| try { | ||
| const { tenant, startDate, endDate } = request.query; | ||
|
|
||
| // Construct the Presto query | ||
| const prestoQuery = ` | ||
| SELECT | ||
| COUNT(*) AS recentUsers | ||
| FROM | ||
| mongodb."${constants.DB_NAME}_${tenant.toLowerCase()}".users | ||
| WHERE | ||
| lastLogin >= TIMESTAMP '${new Date(startDate).toISOString()}' | ||
| AND lastLogin <= TIMESTAMP '${new Date(endDate).toISOString()}' | ||
| `; | ||
|
|
||
| // Execute the Presto query | ||
| const { executeFederatedQuery } = require("@config/database"); | ||
| const responseFromPresto = await executeFederatedQuery(prestoQuery); | ||
|
|
||
| if (responseFromPresto && responseFromPresto.length > 0) { | ||
| return { | ||
| success: true, | ||
| message: "Successfully retrieved recent users from Presto", | ||
| data: responseFromPresto[0], | ||
| status: httpStatus.OK, | ||
| }; | ||
| } else { | ||
| return { | ||
| success: true, | ||
| message: "No recent users found", | ||
| data: {}, | ||
| status: httpStatus.OK, | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Confirm date validation for 'recent users'.
Like other new Presto-based methods, ensure startDate and endDate are verified to avoid a query with invalid timestamps. This function effectively overrides the earlier duplicate definition.
| // New function to execute a custom Presto query | ||
| executeCustomQueryPresto: async (request, next) => { | ||
| try { | ||
| const { tenant, prestoQuery } = request.body; | ||
|
|
||
| // Execute the Presto query | ||
| const { executeFederatedQuery } = require("@config/database"); | ||
| const responseFromPresto = await executeFederatedQuery(prestoQuery); | ||
|
|
||
| if (responseFromPresto && responseFromPresto.length > 0) { | ||
| return { | ||
| success: true, | ||
| message: "Successfully executed custom query from Presto", | ||
| data: responseFromPresto, | ||
| status: httpStatus.OK, | ||
| }; | ||
| } else { | ||
| return { | ||
| success: true, | ||
| message: "No results found for the custom query", | ||
| data: {}, | ||
| status: httpStatus.OK, | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Custom query execution: heed injection concerns.
Executing arbitrary queries from the request body is dangerous. Ensure your upstream validations are comprehensive, preventing malicious statements or system instability.
| getRecentUsers: async (request, next) => { | ||
| try { | ||
| const { tenant, startDate, endDate } = request.query; | ||
|
|
||
| // Construct the Presto query | ||
| const prestoQuery = ` | ||
| SELECT | ||
| COUNT(*) AS recentUsers | ||
| FROM | ||
| mongodb."${constants.DB_NAME}_${tenant.toLowerCase()}".users | ||
| WHERE | ||
| lastLogin >= TIMESTAMP '${new Date(startDate).toISOString()}' | ||
| AND lastLogin <= TIMESTAMP '${new Date(endDate).toISOString()}' | ||
| `; | ||
|
|
||
| // Execute the Presto query | ||
| const { executeFederatedQuery } = require("@config/database"); | ||
| const responseFromPresto = await executeFederatedQuery(prestoQuery); | ||
|
|
||
| if (responseFromPresto && responseFromPresto.length > 0) { | ||
| return { | ||
| success: true, | ||
| message: "Successfully retrieved recent users from Presto", | ||
| data: responseFromPresto[0], | ||
| status: httpStatus.OK, | ||
| }; | ||
| } else { | ||
| return { | ||
| success: true, | ||
| message: "No recent users found", | ||
| data: {}, | ||
| status: httpStatus.OK, | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, | ||
| // New function to get number of new users using Presto | ||
| getNewUsers: async (request, next) => { |
There was a problem hiding this comment.
Duplicate key: 'getRecentUsers' is later redeclared.
This function is declared again at lines 2854-2909, causing the first definition to be overwritten. Please remove or rename one of them to avoid confusion and unexpected behavior.
- getRecentUsers: async (request, next) => {
+ getRecentlyActiveUsers: async (request, next) => {
...
}📝 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.
| getRecentUsers: async (request, next) => { | |
| try { | |
| const { tenant, startDate, endDate } = request.query; | |
| // Construct the Presto query | |
| const prestoQuery = ` | |
| SELECT | |
| COUNT(*) AS recentUsers | |
| FROM | |
| mongodb."${constants.DB_NAME}_${tenant.toLowerCase()}".users | |
| WHERE | |
| lastLogin >= TIMESTAMP '${new Date(startDate).toISOString()}' | |
| AND lastLogin <= TIMESTAMP '${new Date(endDate).toISOString()}' | |
| `; | |
| // Execute the Presto query | |
| const { executeFederatedQuery } = require("@config/database"); | |
| const responseFromPresto = await executeFederatedQuery(prestoQuery); | |
| if (responseFromPresto && responseFromPresto.length > 0) { | |
| return { | |
| success: true, | |
| message: "Successfully retrieved recent users from Presto", | |
| data: responseFromPresto[0], | |
| status: httpStatus.OK, | |
| }; | |
| } else { | |
| return { | |
| success: true, | |
| message: "No recent users found", | |
| data: {}, | |
| status: httpStatus.OK, | |
| }; | |
| } | |
| } catch (error) { | |
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| } | |
| }, | |
| // New function to get number of new users using Presto | |
| getNewUsers: async (request, next) => { | |
| getRecentlyActiveUsers: async (request, next) => { | |
| try { | |
| const { tenant, startDate, endDate } = request.query; | |
| // Construct the Presto query | |
| const prestoQuery = ` | |
| SELECT | |
| COUNT(*) AS recentUsers | |
| FROM | |
| mongodb."${constants.DB_NAME}_${tenant.toLowerCase()}".users | |
| WHERE | |
| lastLogin >= TIMESTAMP '${new Date(startDate).toISOString()}' | |
| AND lastLogin <= TIMESTAMP '${new Date(endDate).toISOString()}' | |
| `; | |
| // Execute the Presto query | |
| const { executeFederatedQuery } = require("@config/database"); | |
| const responseFromPresto = await executeFederatedQuery(prestoQuery); | |
| if (responseFromPresto && responseFromPresto.length > 0) { | |
| return { | |
| success: true, | |
| message: "Successfully retrieved recent users from Presto", | |
| data: responseFromPresto[0], | |
| status: httpStatus.OK, | |
| }; | |
| } else { | |
| return { | |
| success: true, | |
| message: "No recent users found", | |
| data: {}, | |
| status: httpStatus.OK, | |
| }; | |
| } | |
| } catch (error) { | |
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| } | |
| }, | |
| // New function to get number of new users using Presto | |
| getNewUsers: async (request, next) => { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 2775-2819: This property value named getRecentUsers is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named getRecentUsers
(lint/suspicious/noDuplicateObjectKeys)
1 participant
Description
This PR introduces Presto integration to offload complex analytics queries from MongoDB, improving performance and scalability for user engagement and activity reporting.
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Summary by CodeRabbit