feat: reformat obs sql #2763
feat: reformat obs sql #2763
Conversation
WalkthroughThe pull request introduces a comprehensive refactoring of SQL query construction across multiple repository provider classes in the observatory services. The changes focus on replacing traditional string-based SQL query generation with a more robust and secure approach using tagged template literals from a new SQL utility library. The modifications enhance query readability, improve parameter handling, and reduce the risk of SQL injection by leveraging Changes
Possibly related PRs
Suggested Labels
Poem
✨ Finishing Touches
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
♻️ Duplicate comments (1)
api/src/pdc/services/observatory/providers/DistributionRepositoryProvider.ts (1)
78-87:⚠️ Potential issueReiterate caution on
raw(tableName)usageSimilar to the previous method, ensure that
tableNameused withraw()in the SQL query is securely handled. Verifying the safety ofgetTableName()remains crucial to prevent potential SQL injection vulnerabilities.
🧹 Nitpick comments (8)
api/src/pdc/services/observatory/providers/InfraRepositoryProvider.ts (1)
Line range hint
35-50: Well-structured query with clear formatting.The template literal approach with explicit column selection improves readability and maintainability.
Consider using TypeScript type definitions for the selected columns to ensure type safety:
interface AiresCovoiturageColumns { id_lieu: string; nom_lieu: string; // ... other columns } const columns: (keyof AiresCovoiturageColumns)[] = [ 'id_lieu', 'nom_lieu', // ... other columns ]; const query = sql` SELECT ${join(columns, ', ')} FROM ${raw(this.table)} WHERE ${join(filters, ' AND ')} `;api/src/pdc/services/observatory/providers/KeyfiguresRepositoryProvider.ts (3)
33-52: Consider reducing code duplication in join conditions.The
joinOnBandjoinOnCarrays contain identical conditions with only the table alias different. Consider extracting these into a function to reduce duplication.+ private createJoinConditions(alias: string) { + return [ + sql`(a.territory_1 = ${alias}.code OR a.territory_2 = ${alias}.code)`, + sql`a.type = ${alias}.type`, + sql`a.year = ${alias}.year`, + ]; + } - const joinOnB = [ - sql`(a.territory_1 = b.code OR a.territory_2 = b.code)`, - sql`a.type = b.type`, - sql`a.year = b.year`, - ]; - const joinOnC = [ - sql`(a.territory_1 = c.code OR a.territory_2 = c.code)`, - sql`a.type = c.type`, - sql`a.year = c.year`, - ]; + const joinOnB = this.createJoinConditions('b'); + const joinOnC = this.createJoinConditions('c');
59-80: Refactor time period conditions to reduce repetition.The code for handling different time periods (month, trimester, semester) follows the same pattern. Consider extracting this logic into a helper method to improve maintainability.
+ private addTimePeriodConditions( + period: 'month' | 'trimester' | 'semester', + value: number, + joinOnB: Array<any>, + joinOnC: Array<any>, + intraJourneysFilters: Array<any>, + filters: Array<any>, + hasDirection: boolean + ): void { + joinOnB.push(sql`a.${raw(period)} = b.${raw(period)}`); + joinOnC.push(sql`a.${raw(period)} = c.${raw(period)}`); + if (hasDirection) { + intraJourneysFilters.push(sql`${raw(period)} = ${value}`); + filters.push(sql`a.${raw(period)} = ${value}`); + } + } - if (params.month) { - joinOnB.push(sql`a.month = b.month`); - joinOnC.push(sql`a.month = c.month`); - if (params.direction) { - intraJourneysFilters.push(sql`month = ${params.month}`); - filters.push(sql`a.month = ${params.month}`); - } - } + if (params.month) { + this.addTimePeriodConditions('month', params.month, joinOnB, joinOnC, intraJourneysFilters, filters, !!params.direction); + }
Line range hint
83-105: Consider extracting type cast definitions.The query includes multiple type casts (::int, ::float) that could be centralized for better maintainability. Consider creating constants for these type definitions.
+ private readonly TYPE_CASTS = { + INTEGER: raw('::int'), + FLOAT: raw('::float'), + }; const query = sql` SELECT b.code,b.libelle,b.direction, - sum(a.passengers)::int AS passengers, - sum(a.distance)::int AS distance, - sum(a.duration)::int AS duration, - b.journeys::int, + sum(a.passengers)${this.TYPE_CASTS.INTEGER} AS passengers, + sum(a.distance)${this.TYPE_CASTS.INTEGER} AS distance, + sum(a.duration)${this.TYPE_CASTS.INTEGER} AS duration, + b.journeys${this.TYPE_CASTS.INTEGER}, (${intraJourneysQuery})::int as intra_journeys, - b.occupation_rate::float, - c.new_drivers::int, - c.new_passengers::int + b.occupation_rate${this.TYPE_CASTS.FLOAT}, + c.new_drivers${this.TYPE_CASTS.INTEGER}, + c.new_passengers${this.TYPE_CASTS.INTEGER}api/src/pdc/services/observatory/providers/LocationRepositoryProvider.ts (1)
54-66: Consider extracting common query parts.While the query construction is secure and readable, consider reducing duplication by extracting the common parts:
- const query = sql` - SELECT - start_lat as lat, - start_lon as lon - FROM ${raw(this.table)} - WHERE ${join(filters, " AND ")} - UNION ALL - SELECT - end_lat as lat, - end_lon as lon - FROM ${raw(this.table)} - WHERE ${join(filters, " AND ")} - `; + const commonWhere = sql`WHERE ${join(filters, " AND ")}`; + const query = sql` + SELECT start_lat as lat, start_lon as lon + FROM ${raw(this.table)} + ${commonWhere} + UNION ALL + SELECT end_lat as lat, end_lon as lon + FROM ${raw(this.table)} + ${commonWhere} + `;api/src/pdc/services/observatory/providers/IncentiveCampaignsRepositoryProvider.ts (1)
32-33: Consider improving date handling.The current implementation uses string operations (
right) and string-to-date conversions for date comparisons. Consider using PostgreSQL's date functions for more robust date handling.Here's a suggested improvement:
- filters.push(sql`right(a.date_fin,4) = ${params.year}::varchar`); - filters.push(sql`to_date(a.date_fin,'DD/MM/YYYY') < now()`); + filters.push(sql`EXTRACT(YEAR FROM TO_DATE(a.date_fin, 'DD/MM/YYYY')) = ${params.year}::integer`); + filters.push(sql`TO_DATE(a.date_fin, 'DD/MM/YYYY') < CURRENT_DATE`);Also applies to: 36-36, 39-39
api/src/pdc/services/observatory/providers/OccupationRepositoryProvider.ts (1)
75-81: Consider using prepared statements for performance optimization.While the current implementation is secure and maintainable, frequently executed queries could benefit from prepared statements to reduce parsing overhead.
Example implementation:
private async prepareQueries() { const occupationQuery = await this.pg.getClient().prepare( 'occupation_query', 'SELECT $1:raw FROM $2:name WHERE $3:raw', ['columns', 'table', 'conditions'] ); // ... prepare other queries }Also applies to: 123-131, 166-174
api/src/pdc/services/observatory/providers/FluxRepositoryProvider.ts (1)
91-92: Simplify Parameter Handling ingetEvolFluxWrapping
typeParamandlimitwithsql\${...}`is unnecessary since they are parameters within the query. Removing the extrasql` wrapping improves code clarity and reduces redundancy.Apply this diff to simplify the code:
- const typeParam = sql`${checkTerritoryParam(params.type)}`; - const limit = sql`${params.past ? Number(params.past) * 12 + 1 : 25}`; + const typeParam = checkTerritoryParam(params.type); + const limit = params.past ? Number(params.past) * 12 + 1 : 25;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/src/pdc/services/observatory/providers/DistributionRepositoryProvider.ts(3 hunks)api/src/pdc/services/observatory/providers/FluxRepositoryProvider.ts(4 hunks)api/src/pdc/services/observatory/providers/IncentiveCampaignsRepositoryProvider.ts(3 hunks)api/src/pdc/services/observatory/providers/IncentiveRepositoryProvider.ts(3 hunks)api/src/pdc/services/observatory/providers/InfraRepositoryProvider.ts(3 hunks)api/src/pdc/services/observatory/providers/KeyfiguresRepositoryProvider.ts(4 hunks)api/src/pdc/services/observatory/providers/LocationRepositoryProvider.ts(2 hunks)api/src/pdc/services/observatory/providers/OccupationRepositoryProvider.ts(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
api/src/pdc/services/observatory/providers/LocationRepositoryProvider.ts
[error] 71-71: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (24)
api/src/pdc/services/observatory/providers/InfraRepositoryProvider.ts (3)
3-3: LGTM! Good use of SQL utility functions.The introduction of
sql,join, andrawfunctions from the SQL utility library enhances query safety and maintainability.
23-26: Great pattern for filter composition!The array-based approach to building filters improves code maintainability and makes it easier to add or modify conditions.
28-33: Verify table name safety.While the query parameters are properly escaped using
sqltemplate literals, the table name is passed throughraw. Please ensure thatthis.perim_tableis properly validated or comes from a trusted source.api/src/pdc/services/observatory/providers/KeyfiguresRepositoryProvider.ts (1)
3-3: LGTM! Good use of SQL utilities.The introduction of typed SQL utilities will help prevent SQL injection and improve type safety.
Also applies to: 16-16
api/src/pdc/services/observatory/providers/LocationRepositoryProvider.ts (3)
4-4: LGTM! Good addition of SQL utilities.The import of
sql,join, andrawutilities aligns with the goal of improving SQL query construction safety and readability.
37-51: Well-structured filter construction with proper parameter binding.Good implementation of:
- Safe parameter binding using tagged templates
- Clear conditional filter addition
- Well-formatted complex CASE expression
27-34: Verify territory parameter validation.The SQL query construction looks good with proper use of tagged templates. The use of
raw()fortypeParamis safe as it's validated bycheckTerritoryParam, but let's verify the validation logic.✅ Verification successful
Territory parameter validation is properly implemented
The
checkTerritoryParamfunction implements a whitelist-based validation against predefined territory types with a safe fallback to "com", ensuring that theraw()usage in SQL queries is protected against injection.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation of checkTerritoryParam ast-grep --pattern 'function checkTerritoryParam($_) { $$$ }'Length of output: 65
Script:
#!/bin/bash # Try different patterns to find territory parameter validation ast-grep --pattern 'export function checkTerritory$_($_) { $$$ }' ast-grep --pattern 'const checkTerritory$_ = $_: $_ => { $$$ }' # Fallback to text search rg -i "checkterritory|validateterritory|territoryvalidation" -A 5 # Search for the specific parameter validation rg "typeParam.*validation|validate.*typeParam" -A 5Length of output: 11768
api/src/pdc/services/observatory/providers/IncentiveCampaignsRepositoryProvider.ts (4)
3-3: LGTM! Good choice of SQL utilities.The addition of
sql,join, andrawimports from the SQL utility library aligns well with the goal of improving SQL query construction.
24-26: Well-structured query construction with proper validation.Good practices observed:
- Parameter validation using
checkTerritoryParam- Clear initialization of filters array
- Use of SQL template literals for safe query construction
44-50: Well-structured query with proper SQL injection prevention.Good practices observed:
- Use of
rawfor table identifiers- Proper parameter binding through SQL template literals
- Clean separation of the SELECT clause, JOIN conditions, and WHERE clause
48-48: Verify the existence of geo.get_latest_millesime function.The query relies on the
geo.get_latest_millesime()function. Let's ensure it exists in the database.api/src/pdc/services/observatory/providers/OccupationRepositoryProvider.ts (4)
3-4: LGTM! Clean import organization.The addition of SQL utility imports and reorganization of parameter checking imports improves code organization and sets up the foundation for safer SQL query construction.
40-46: LGTM! Safe column selection.The use of
sqltemplate literals for column selection provides proper escaping and type safety.
58-72: LGTM! Robust parameter handling.Excellent implementation of:
- Parameterized filters preventing SQL injection
- Consistent parameter validation
- Clean conditional filter addition
Also applies to: 103-106, 151-164
48-55: Verify table name sanitization.While using
rawfor the table name is common, ensure thatthis.perim_tableis properly sanitized or validated to prevent SQL injection. Consider creating a whitelist of allowed table names.✅ Verification successful
Table name is properly secured against SQL injection.
The
perim_tableis implemented as a private readonly constant with a hardcoded value, making it impossible for malicious input to modify the table name. No additional sanitization is needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if perim_table is hardcoded across the codebase rg -l "perim_table\s*=\s*['\"]geo\.perimeters['\"]" api/src/pdc/services/observatory/Length of output: 378
Script:
#!/bin/bash # Check perim_table declaration and initialization ast-grep --pattern 'perim_table = $value' # Also check for readonly/private declarations ast-grep --pattern 'readonly perim_table' ast-grep --pattern 'private perim_table' # Check for any base repository class fd -e ts -e js repository api/src/pdc/services/observatory/ --exec grep -l "class.*Repository"Length of output: 1499
Script:
#!/bin/bash # Check interface definitions for perim_table rg "perim_table" api/src/pdc/services/observatory/interfaces/ -A 2 -B 2 # Check constructor implementations and class definitions ast-grep --pattern 'class $_ { constructor($_) { $$$ } }'Length of output: 813
Script:
#!/bin/bash # Check the provider implementations for perim_table rg "perim_table" api/src/pdc/services/observatory/providers/ -A 5 -B 5 # Look for any SQL sanitization or validation utilities fd -e ts -e js sql api/src/pdc/services/ --exec grep -l "sanitize\|validate\|escape"Length of output: 14247
api/src/pdc/services/observatory/providers/DistributionRepositoryProvider.ts (3)
32-35: Good use ofsqltagged templates for constructing filtersThe implementation correctly utilizes
sqltagged templates to build thefiltersarray. This approach enhances security by ensuring parameters are safely escaped and improves readability.Also applies to: 38-38, 41-41, 44-44
63-67: Consistent application of secure query construction ingetJourneysByDistancesThe
filtersarray ingetJourneysByDistancesis also constructed usingsqltagged templates, maintaining consistency and enhancing security across methods.Also applies to: 70-70, 73-73, 76-76
46-55:⚠️ Potential issueEnsure
tableNameis securely handled when usingraw()Using
raw(tableName)directly in the SQL query can pose a SQL injection risk iftableNameis derived from untrusted input. Please verify that thegetTableName()method constructstableNamesafely, with proper validation and sanitization to prevent any injection vulnerabilities.Run the following script to check the usage of
getTableName()across the codebase and ensure it does not introduce security risks:✅ Verification successful
Table name construction is secure
The
raw(tableName)usage is safe as the table name is constructed entirely from hardcoded strings ingetTableNamehelper, with no user input involved in the table name construction. The method only appends fixed suffixes ("_by_month", "_by_trimester", "_by_semester", "_by_year") based on the presence of strongly-typed parameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `getTableName()` generates safe table names. # Search for all implementations of `getTableName` ast-grep --pattern $'function getTableName($_, $_, $_) { $$$ }' --lang typescript # Look for any direct user inputs being passed to `getTableName()` rg --type ts 'getTableName\((.*(req\.body|req\.params|req\.query).*)\)'Length of output: 158
Script:
#!/bin/bash # Search for tableName assignments and usage in the specific file cat api/src/pdc/services/observatory/providers/DistributionRepositoryProvider.ts # Find any table name related patterns in the codebase rg --type ts "tableName.*=.*" -A 2 -B 2 # Look for class methods or properties containing 'table' ast-grep --pattern 'class $_ { $$$ table$_ $$$ }'Length of output: 8046
Script:
#!/bin/bash # Check the implementation of getTableName helper cat api/src/pdc/services/observatory/helpers/tableName.ts # Look for any direct usage of the params in table name construction rg --type ts -g "!*.test.ts" -g "!*.spec.ts" "observatoire_stats.*distribution"Length of output: 799
api/src/pdc/services/observatory/providers/IncentiveRepositoryProvider.ts (2)
27-56: Refactored SQL query enhances security and readabilityRefactoring the SQL query construction to use
sql,join, andrawfunctions improves readability and reduces the risk of SQL injection by properly handling parameter binding and avoiding string concatenation.
53-54:⚠️ Potential issueEnsure safety when using raw table names
Using
raw(tableName)can introduce SQL injection risks iftableNameis influenced by untrusted input. Please ensure thatgetTableNamereturns a safe, validated table name that cannot be manipulated by external inputs.Run the following script to verify that
getTableNameconstructs table names safely:api/src/pdc/services/observatory/providers/FluxRepositoryProvider.ts (4)
49-55: Validate Parameters Used in Query FiltersEnsure that all parameters used in the
filtersarray, such asobserveParamandparams.year, are properly validated and sanitized. This is crucial to prevent SQL injection and ensure the correctness of the query.
136-144: EnsurecheckTerritoryParamAdequately Sanitizes User InputIn the
getBestFluxmethod, theperimTableQueryagain usesrawinterpolation withtypeParam. Please verify thatcheckTerritoryParamstrictly validates and sanitizesparams.typeto prevent potential SQL injection vulnerabilities.
39-47:⚠️ Potential issueEnsure
checkTerritoryParamAdequately Sanitizes User InputIn the construction of
perimTableQuery, parametersobserveParamandtypeParamare interpolated usingraw. Please verify thatcheckTerritoryParamstrictly validates and sanitizesparams.observeandparams.typeto prevent potential SQL injection vulnerabilities.
95-96:⚠️ Potential issueEnsure
indicParamIs Safely Used in SQL QueriesThe
indicParamis interpolated usingrawwithin the SQL query. Please verify thatcheckIndicParamstrictly validatesparams.indicagainst the allowed list of indicators to prevent SQL injection vulnerabilities.
api/src/pdc/services/observatory/providers/LocationRepositoryProvider.ts
Show resolved
Hide resolved
api/src/pdc/services/observatory/providers/FluxRepositoryProvider.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/src/pdc/services/observatory/providers/FluxRepositoryProvider.ts (2)
91-95: Consider enhancing type safety for SQL parameters.While the implementation is good, consider adding type assertions or validation for numeric parameters to prevent potential runtime errors.
- const limit = sql`${params.past ? Number(params.past) * 12 + 1 : 25}`; + const limit = sql`${params.past ? Math.max(0, Number(params.past)) * 12 + 1 : 25}`;
157-162: Consider adding an index hint for the ORDER BY clause.Since this query is used for dashboard top 10 results, consider adding an index hint to ensure the database uses the most efficient execution plan for the ORDER BY clause.
const query = sql` SELECT territory_1, l_territory_1, territory_2, l_territory_2, journeys FROM ${raw(tableName)} WHERE ${join(filters, " AND ")} - ORDER BY journeys DESC + ORDER BY journeys DESC /* @index(journeys_idx) */ LIMIT ${params.limit}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/pdc/services/observatory/providers/FluxRepositoryProvider.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (4)
api/src/pdc/services/observatory/providers/FluxRepositoryProvider.ts (4)
3-3: LGTM! Good addition of SQL utility functions.The import of
sql,empty,join, andrawfunctions from the SQL utility library aligns well with the PR's objective of improving SQL query construction.
49-54: Well-structured query conditions using sql template literals!Good use of parameterized queries with the
sqltemplate literal for safe parameter handling. The filters array makes the conditions clear and maintainable.
Line range hint
1-166: Excellent SQL query reformatting implementation!The changes successfully achieve the PR's objective of improving SQL query construction. The consistent use of:
- SQL template literals for safe parameter handling
- Raw SQL for identifiers
- Join function for combining conditions
- Structured filter arrays
makes the code more maintainable and secure.
39-46: Verify the safety of raw SQL identifiers.While using
raw()for table names is sometimes necessary, ensure thatobserveParamandtypeParamare properly sanitized to prevent SQL injection. These parameters come fromcheckTerritoryParam(), so let's verify its implementation.✅ Verification successful
The raw SQL identifiers are properly secured
The
checkTerritoryParamfunction ensures that only valid territory types from a predefined enum can be used as column identifiers, with a fallback to "com". This effectively prevents SQL injection through these parameters.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation of checkTerritoryParam ast-grep --pattern 'function checkTerritoryParam($_) { $$$ }'Length of output: 65
Script:
#!/bin/bash # Try to find any files containing checkTerritory rg -l "checkTerritory" # Look for the construction of observeParam and typeParam rg -B 5 -A 5 "observeParam|typeParam" "api/src/pdc/services/observatory/providers/FluxRepositoryProvider.ts"Length of output: 2764
Script:
#!/bin/bash # Check the implementation in the helper file cat "api/src/pdc/services/observatory/helpers/checkParams.ts"Length of output: 618
#fixes (issues)
Description
Checklist
/api/specsMerge
Je squash la PR et vérifie que le message de commit utilise la convention d'Angular :
Voir la convention de message de commit...
Types de commit
Le scope (optionnel) précise le module ou le composant impacté par le commit.
Summary by CodeRabbit
Based on the comprehensive summary, here's a concise release note:
Improvements
Technical Updates
The changes represent a significant refactoring of database interaction methods, focusing on code quality and security across the observatory services.