Conversation
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/db/src/sql/pg.ts`:
- Around line 1135-1168: The addColumn implementation interpolates identifiers
and string defaults directly into SQL, risking SQL injection and syntax errors
(e.g., "O'Brien"); fix by validating/quoting identifiers and using parameterized
values for defaults: validate table and column.name (and optionally column.type)
with a strict identifier regex (e.g., /^[A-Za-z_][A-Za-z0-9_]*$/) in addColumn,
reject or sanitize invalid identifiers, and build the ALTER TABLE statement
using proper identifier quoting (double quotes with doubled internal quotes) or
a helper quoteIdentifier function; for default values, avoid direct string
interpolation—use a parameter placeholder in this.db.query and pass the default
as a parameter (or if NULL, use the literal NULL) and ensure string defaults are
escaped if you must inline them. Ensure error logs reference addColumn, table,
column and the final query/params for debugging.
- Around line 1173-1198: The ensureColumns implementation can race when two
processes try to add the same missing column; update the code so concurrent
ALTER TABLE ADD COLUMN is idempotent by either making addColumn execute "ADD
COLUMN IF NOT EXISTS" (Postgres >=9.6) or by catching and swallowing the
duplicate-column error inside addColumn (check Postgres error code '42701') so
ensureColumns' reduce chain ignores that specific error; modify the
addColumn(table: T, col: ColumnDefinition) method (or the promise returned by
the reduce in ensureColumns) to detect error.code === '42701' and return
normally without rethrowing, while still logging other errors.
In `@packages/db/src/sql/sqlite.ts`:
- Around line 1165-1204: The addColumn implementation constructs DDL unsafely;
validate identifiers and types and escape string defaults: in addColumn (and
likewise in getTableColumns, _createTables.ts and the PostgreSQL implementation)
enforce an identifier whitelist (e.g., /^[a-z_][a-z0-9_]*$/i) for table and
column.name, validate/whitelist column.type against an allowed set of SQL types
(e.g., TEXT, VARCHAR, INTEGER, BIGINT, JSONB, etc.) instead of interpolating
arbitrary text, and when building DEFAULT for string values escape single quotes
by replacing ' with ''; fail fast with a clear logger.error when validation
fails and only interpolate validated/whitelisted values into the ALTER
TABLE/PRAGMA/CREATE statements.
- Around line 1207-1235: The ensureColumns flow races when multiple instances
try to add the same missing column; modify addColumn (and/or the call site in
ensureColumns) to catch SQLite "duplicate column" errors and ignore them instead
of propagating—specifically intercept the error from addColumn/ALTER TABLE that
contains SQLite's duplicate-column indicator (e.g., "duplicate column name" /
SQLITE_ERROR for duplicate column) and return success; leave all other errors
thrown. Update the addColumn function (referenced by ensureColumns ->
this.addColumn) to perform this error check so concurrent ALTER TABLE calls
become idempotent.
🧹 Nitpick comments (1)
.compose/examples/cs-bridge.yml (1)
318-318: Consider renaming the volume for consistency.The volume
common-settings-datastill uses the old naming convention while the services have been renamed totom-twp-cs-bridge*. Consider renaming to something liketom-twp-cs-bridge-datafor consistency.Note: If this is intentional to preserve backward compatibility with existing deployments, you may disregard this suggestion.
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
c1bbb57 to
423838b
Compare
|
View your CI Pipeline Execution ↗ for commit 423838b
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.compose/examples/cs-bridge.yml:
- Around line 160-162: The new named volume tom-twp-cs-bridge-data will orphan
existing data from the previous common-settings-data name; update the volumes
block so the new name aliases the existing volume (e.g., declare
tom-twp-cs-bridge-data as external and point it to the existing
common-settings-data external name or list both names to the same underlying
volume) or add a documented migration step to copy data from
common-settings-data into tom-twp-cs-bridge-data; apply the same change where
the other volumes (lines referenced around the other occurrence) are renamed so
upgrades don’t create an empty volume.
🧹 Nitpick comments (1)
packages/db/src/sql/pg.ts (1)
1098-1133: Schema filter in column lookup is a defensive improvement.The
information_schema.columnsquery currently lacks a schema filter. While this codebase appears to use the defaultpublicschema (tables are created without explicit schema qualification), addingAND table_schema = current_schema()would be a defensive measure for robustness and future-proofing.🔧 Suggested refinement
const query = ` SELECT column_name, data_type, column_default FROM information_schema.columns WHERE table_name = $1 + AND table_schema = current_schema() `
…rt (#323) Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
…rt (#323) Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
Summary by CodeRabbit
New Features
Chores