Conversation
…th filtering and sorting
There was a problem hiding this comment.
Pull Request Overview
This PR implements table data fetching and filtering functionality for database deployments. It adds a new API endpoint to query table data with support for filtering, sorting, pagination, and search capabilities.
- Adds a new
fetchTablesDatafunction with query construction utilities for WHERE and ORDER BY clauses - Creates a new POST endpoint
/api/deployment/table/datafor retrieving filtered table data - Implements SQL injection protection through basic string escaping
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| api/sql.ts | Adds core table data fetching logic with query construction functions |
| api/routes.ts | Implements new API endpoint with input validation and schema verification |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const safeValue = value.replace(/'/g, "''") | ||
| whereClauses.push(`${key} ${comparator} '${safeValue}'`) |
There was a problem hiding this comment.
This basic string escaping approach is insufficient for SQL injection protection. Consider using parameterized queries or a proper SQL escaping library instead of manual quote replacement.
| } | ||
| if (params.search) { | ||
| const searchClauses = Array.from(columnsMap.values()).map((col) => { | ||
| return `${col.name} LIKE '%${params.search.replace(/'/g, "''")}%'` |
There was a problem hiding this comment.
The search functionality uses the same vulnerable string escaping approach. This could allow SQL injection through the search parameter.
api/routes.ts
Outdated
| 'POST/api/deployment/table/data': route({ | ||
| fn: (_, { deployment, table, ...input }) => { | ||
| const depData = DeploymentsCollection.get(deployment) | ||
| if (!deployment) { |
There was a problem hiding this comment.
The condition checks !deployment but should check !depData since deployment is the input parameter and depData is the result from the collection lookup.
| if (!deployment) { | |
| if (!depData) { |
api/sql.ts
Outdated
| limitOffsetClause += `LIMIT ${params.limit} ` | ||
|
|
||
| if (params.offset && parseInt(params.offset) >= 0) { | ||
| limitOffsetClause += `OFFSET ${params.offset} ` |
There was a problem hiding this comment.
Direct string interpolation of limit and offset values without validation could allow SQL injection. Although parseInt is used for validation, the original string values are interpolated into the query.
| limitOffsetClause += `LIMIT ${params.limit} ` | |
| if (params.offset && parseInt(params.offset) >= 0) { | |
| limitOffsetClause += `OFFSET ${params.offset} ` | |
| const limit = parseInt(params.limit, 10) | |
| limitOffsetClause += `LIMIT ${limit} ` | |
| if (params.offset && parseInt(params.offset) >= 0) { | |
| const offset = parseInt(params.offset, 10) | |
| limitOffsetClause += `OFFSET ${offset} ` |
99f9f43 to
51805fb
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
api/sql.ts
Outdated
| const query = | ||
| `SELECT * FROM ${params.table} ${whereClause} ${orderByClause} ${limitOffsetClause}` | ||
| .trim() |
There was a problem hiding this comment.
The table name is directly interpolated into the SQL query without validation or escaping. This could allow SQL injection if the table parameter is not properly validated upstream.
| limit: STR('The maximum number of rows to return'), | ||
| offset: STR('The number of rows to skip'), |
There was a problem hiding this comment.
The limit and offset parameters should be defined as numbers (NUM) rather than strings (STR) since they represent numeric values and are parsed as integers in the implementation.
| limit: STR('The maximum number of rows to return'), | |
| offset: STR('The number of rows to skip'), | |
| limit: NUM('The maximum number of rows to return'), | |
| offset: NUM('The number of rows to skip'), |
api/routes.ts
Outdated
| const columnsMap = new Map( | ||
| tableDef.columns.map((col) => [col.name, col]), | ||
| ) | ||
|
|
There was a problem hiding this comment.
// could prepare the data once to only do this in the route:
const columnsMap = formattedSchema[deployement]?.[table]
api/sql.ts
Outdated
| const { key, comparator, value } = filter | ||
| const column = columnsMap.get(key) | ||
| if (!column) { | ||
| throw new Error(`Invalid filter column: ${key}`) |
api/sql.ts
Outdated
| } | ||
| } | ||
| if (params.search) { | ||
| const searchClauses = Array.from(columnsMap.values()).map((col) => { |
There was a problem hiding this comment.
instead of Array.from(columnsMap.values()).map(...)
do columnsMap.values().map(...).toArray()
api/sql.ts
Outdated
| const orderByClause = constructOrderByClause(params, columnsMap) | ||
|
|
||
| let limitOffsetClause = '' | ||
| const limit = parseInt(params.limit, 10) |
There was a problem hiding this comment.
use Math.floor(Number(params.limit))
51805fb to
3b86820
Compare
… of github.com:01-edu/devtools into 66-implementation-of-table-data-fetching-and-filtering
3b86820 to
1dc16e2
Compare
…chTablesData to use it
* feat(routes): change endpoint from GET to POST for deployment table data retrieval feat(Filtre): export parseFilters and parseSort functions for external use feat(DeploymentPage): integrate filter and sort functionality for deployment table data * feat(DeploymentPage): refactor data fetching and improve table display logic * feat(DeploymentPage): refactor layout and improve component structure * fix(DeploymentPage): correct parameter name from tq to qt for search functionality
No description provided.