Relations filters #173
Relations filters #173
Conversation
WalkthroughThis pull request introduces several modifications across multiple files, focusing on error handling, logging, and schema management. The changes primarily affect the Airtable datasource, application controllers, and schema-related utilities. Key modifications include removing verbose error logging, enhancing field-level permissions, updating schema retrieval mechanisms, and adding a new type for string column metadata. The modifications aim to improve data access control, streamline error handling, and provide more flexible schema definitions. Changes
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }, | ||
| auth.user_identifier, | ||
| queryFields, | ||
| const total = body.length |
Check failure
Code scanning / CodeQL
Type confusion through parameter tampering Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 11 days ago
To fix the problem, we need to ensure that the body parameter is of the expected type before processing it. Specifically, we should check that body is either an object or an array of objects. If it is not, we should return a 400 Bad Request response. This will prevent type confusion attacks by ensuring that the body parameter is always in a predictable format.
| @@ -82,2 +82,6 @@ | ||
|
|
||
| if (typeof body !== 'object' || body === null || (Array.isArray(body) && !body.every(item => typeof item === 'object' && item !== null))) { | ||
| return res.status(400).send(this.response.text('Invalid request body')) | ||
| } | ||
|
|
||
| if (!(body instanceof Array)) { |
| errored++ | ||
| errors.push({ | ||
| item: Array.isArray(body) ? body.findIndex(i => i === item) : -1, | ||
| message: insertResult.message, | ||
| item: body.indexOf(item), |
Check failure
Code scanning / CodeQL
Type confusion through parameter tampering Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 11 days ago
To fix the problem, we need to ensure that the body parameter is of the expected type before using it. Specifically, we should check that body is either an object or an array of objects. If it is not, we should return a 400 Bad Request response. This can be done by adding a type check at the beginning of the create method.
| @@ -40,2 +40,5 @@ | ||
| ): Promise<FindOneResponseObject | CreateManyResponseObject> { | ||
| if (typeof body !== 'object' || (Array.isArray(body) && !body.every(item => typeof item === 'object'))) { | ||
| return res.status(400).send(this.response.text('Invalid request body')); | ||
| } | ||
| const x_request_id = headers['x-request-id'] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/app.controller.post.ts (1)
94-166: Consider parallelization for large batch inserts.
Creating records, publishing to websockets, and running webhooks in a loop is correct. However, for large batches, consider parallel or batched processing to improve throughput—only if concurrency and data consistency permit.🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 114-114: Type confusion through parameter tampering
Potential type confusion as this HTTP request parameter may be either an array or a string.src/helpers/Schema.ts (1)
60-98: Database-agnostic schema retrieval with fallback.
The switch statement is well-structured, though consider logging an explicit warning or returning if an unsupported database type is reached.src/app.controller.get.test.spec.ts (2)
26-26: Remove unused import if not required.It appears that
statis unused within this test file. If it's not needed, remove it to keep dependencies minimal and the code clean.
207-215: Reinstate status code checks to ensure test correctness.By commenting out
.expect(200)and replacing it with a console log, there is no explicit assertion on the HTTP status code. Reinstating an explicit check for200(or the expected status code) will help ensure correctness and provide clearer test signals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
demo/databases/airtable.ts(0 hunks)src/app.controller.get.test.spec.ts(3 hunks)src/app.controller.get.ts(6 hunks)src/app.controller.post.ts(1 hunks)src/app.service.bootup.ts(3 hunks)src/datasources/airtable.datasource.ts(0 hunks)src/helpers/Schema.ts(1 hunks)src/types/datasource.types.ts(2 hunks)
💤 Files with no reviewable changes (2)
- src/datasources/airtable.datasource.ts
- demo/databases/airtable.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app.controller.post.ts
[error] 83-83: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.
(lint/suspicious/useIsArray)
🪛 GitHub Check: CodeQL
src/app.controller.post.ts
[failure] 88-88: Type confusion through parameter tampering
Potential type confusion as this HTTP request parameter may be either an array or a string.
[failure] 114-114: Type confusion through parameter tampering
Potential type confusion as this HTTP request parameter may be either an array or a string.
🔇 Additional comments (22)
src/app.controller.post.ts (4)
76-78: Ensure consistent logging on unauthorized access.
When both public_auth and auth fail, the method immediately returns a 401. This is valid, but consider logging or exposing additional context for diagnosis when unauthorized requests occur.
81-81: Informative naming.
The singular variable clarifies the request body shape well, improving readability for single-item handling.
88-93: Check for empty request body edge case.
You handle single vs. multiple record creation. Consider validating non-empty arrays if the logic requires at least one record.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 88-88: Type confusion through parameter tampering
Potential type confusion as this HTTP request parameter may be either an array or a string.
168-178: Conditional response design is clean.
Returning a single record for singular mode or a summary object for multiple records is clear and intuitive.
src/app.controller.get.ts (5)
125-128: Schema retrieval with field filtering.
Fetching the schema with selected fields is efficient and aligned with the new fields parameter usage.
233-237: Handle missing schemas gracefully.
Catching and returning 404 for non-existent schemas is consistent with other controller patterns.
288-320: Robust relation-based field filtering.
Filtering relation columns based on public and role-based auth ensures minimal unauthorized data exposure. Well done.
461-465: 404 on missing schemas.
Consistent usage of 404 status code for missing schemas is a good practice.
489-521: Relation-level allowed fields checks.
Ensuring relations also respect public_auth and role-based fields helps avoid data leakage in nested structures.
src/types/datasource.types.ts (2)
103-106: New string metadata interface.
The ColumnExtraString interface is a valuable addition for specifying string-field sizes, improving schema clarity.
117-117: Expanded extra property coverage.
Integrating ColumnExtraString in extra? ensures string-length constraints can be enforced in the schema.
src/helpers/Schema.ts (7)
50-50: New fields parameter in getSchema.
Adding the optional fields parameter is a neat way to fetch partial columns, aiding in permission-based column selection.
56-59: Synchronized cache keys.
Using schema:${options.table}:${options.fields?.join(',')} ensures each field subset is cached distinctly.
100-102: Check for missing schema.
Throwing an error if result?.table is missing helps catch misconfiguration early.
104-109: Leverage cache for repeated schema queries.
Storing the freshly retrieved schema in the cache reduces subsequent overhead. Mind eviction policies if schemas can be dynamically modified.
110-111: Uniform error handling.
Re-throwing a new error with context is consistent, ensuring no raw error messages leak externally.
115-119: Filtering columns by provided fields.
Including the primary key while trimming columns is a straightforward pattern for partial schema usage.
121-123: Appending request identifier for traceability.
Adding _x_request_id in the final schema is beneficial for consistent logging and debugging.
src/app.service.bootup.ts (3)
25-25: Proper import of ColumnExtraString.
Ensuring typed support for string-based extra constraints.
132-134: Length constraint for allowed_fields.
The ColumnExtraString usage ensures maximum length validation for allowed_fields. This helps prevent overly long data.
266-268: Consistent string metadata usage.
Applying the ColumnExtraString with a defined length: 1020 ensures uniform constraints across _llana_public_tables and _llana_roles.
src/app.controller.get.test.spec.ts (1)
420-462: Overall logic is sound, but consider reducing logging noise.
This comprehensive test verifies the proper filtering of fields and relationships based on read permissions. The logic appears correct, though consider removing or reducing the verbosity of console.log(result.body) to keep test outputs concise unless these logs are strictly necessary for debugging.
| if (!(body instanceof Array)) { | ||
| body = [body] | ||
| singular = true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Array.isArray() instead of instanceof Array.
Static analysis suggests replacing body instanceof Array with Array.isArray(body) to consistently detect arrays from different contexts.
- if (!(body instanceof Array)) {
+ if (!Array.isArray(body)) {📝 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.
| if (!(body instanceof Array)) { | |
| body = [body] | |
| singular = true | |
| } | |
| if (!Array.isArray(body)) { | |
| body = [body] | |
| singular = true | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.
(lint/suspicious/useIsArray)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docker/docker-compose.dev.yml (3)
41-46: Health check configuration looks good, minor formatting fixes needed.The health check configuration is well-structured with appropriate timing parameters. However, there are some minor formatting issues to fix:
- healthcheck: - test: ["CMD", "mysqladmin" ,"ping", "-h", "localhost"] + healthcheck: + test: ["CMD", "mysqladmin", "ping", "-h", "localhost"]🧰 Tools
🪛 yamllint (1.35.1)
[error] 41-41: trailing spaces
(trailing-spaces)
[warning] 42-42: too many spaces before comma
(commas)
[warning] 42-42: too few spaces after comma
(commas)
Line range hint
25-31: Consider improving security configuration.The current MySQL configuration has several security considerations:
- Root password and user credentials are hardcoded
MYSQL_ROOT_HOST: '%'allows remote root connectionsConsider:
- Using environment variables for sensitive data
- Restricting root access to localhost
environment: - MYSQL_ROOT_PASSWORD: pass - MYSQL_USER: user - MYSQL_PASSWORD: pass + MYSQL_ROOT_PASSWORD: ${MYSQL_ROOT_PASSWORD} + MYSQL_USER: ${MYSQL_USER} + MYSQL_PASSWORD: ${MYSQL_PASSWORD} MYSQL_DATABASE: llana - MYSQL_ROOT_HOST: '%' + MYSQL_ROOT_HOST: 'localhost'🧰 Tools
🪛 yamllint (1.35.1)
[error] 41-41: trailing spaces
(trailing-spaces)
[warning] 42-42: too many spaces before comma
(commas)
[warning] 42-42: too few spaces after comma
(commas)
Line range hint
48-63: Consider adding health checks for Postgres and MongoDB services.The MySQL and MSSQL services have health checks, but Postgres and MongoDB services don't. For consistency and better orchestration, consider adding similar health checks.
For Postgres:
networks: - llana-network + healthcheck: + test: ["CMD-SHELL", "pg_isready -U user -d llana"] + interval: 10s + timeout: 3s + retries: 10 + start_period: 10sFor MongoDB:
networks: - llana-network + healthcheck: + test: ["CMD", "mongosh", "--eval", "db.adminCommand('ping')"] + interval: 10s + timeout: 3s + retries: 10 + start_period: 10sAlso applies to: 65-80
🧰 Tools
🪛 yamllint (1.35.1)
[error] 41-41: trailing spaces
(trailing-spaces)
[warning] 42-42: too many spaces before comma
(commas)
[warning] 42-42: too few spaces after comma
(commas)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker/docker-compose.dev.yml(1 hunks)src/app.controller.get.test.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.controller.get.test.spec.ts
🧰 Additional context used
🪛 yamllint (1.35.1)
docker/docker-compose.dev.yml
[error] 41-41: trailing spaces
(trailing-spaces)
[warning] 42-42: too many spaces before comma
(commas)
[warning] 42-42: too few spaces after comma
(commas)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/testing/relations.testing.service.ts (3)
11-14: Consider adding input validation or error handling for dependencies.
While injectingqueryandschema, it's generally good practice to validate or at least document any assumptions about these dependencies—such as whether they can be null or undefined.
16-30: Consider factoring out repeated schema retrieval logic.
Both methods retrieve the same schema fromawait this.schema.getSchema(...). You could enhance maintainability by extracting this repeated logic into a helper method, especially if further expansions or error handling become necessary.
31-33: Remove extra blank lines to satisfy ESlint/prettier rules.
This change is purely stylistic but maintains consistency throughout the project.} -} +}🧰 Tools
🪛 eslint
[error] 31-33: Delete
⏎⏎(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app.controller.get.test.spec.ts(2 hunks)src/testing/relations.testing.service.ts(1 hunks)src/types/datasource.types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app.controller.get.test.spec.ts
- src/types/datasource.types.ts
🧰 Additional context used
🪛 eslint
src/testing/relations.testing.service.ts
[error] 31-33: Delete ⏎⏎
(prettier/prettier)
🔇 Additional comments (1)
src/testing/relations.testing.service.ts (1)
16-17: Handle potential schema retrieval issues.
If the schema service fails or returns an invalid schema, the method will still proceed to the Query call and may cause runtime errors. Consider a guard or try/catch to handle unexpected responses from getSchema().
| async deleteRelationsRecord(data: any): Promise<void> { | ||
| const schema = await this.schema.getSchema({ table: LLANA_RELATION_TABLE, x_request_id: 'test' }) | ||
| await this.query.perform(QueryPerform.DELETE, { | ||
| id: data[schema.primary_key], | ||
| schema, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restrict data parameter type in deleteRelationsRecord.
Using any weakens type safety and can introduce unexpected bugs if the id property is missing or invalid. Consider introducing a stricter contract or type for this parameter.
-async deleteRelationsRecord(data: any): Promise<void> {
+async deleteRelationsRecord(data: { [key: string]: any }): Promise<void> {
const schema = await this.schema.getSchema({ table: LLANA_RELATION_TABLE, x_request_id: 'test' })
await this.query.perform(QueryPerform.DELETE, {
id: data[schema.primary_key],
schema,
})
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
Performance
Security
New Features