Conversation
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a schema versioning and migration framework for Postgres and Oracle, new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Factory as StorageFactory
participant Store as PostgresStore/OracleStore
participant Versioning as Versioning Module
participant DB as Database
App->>Factory: create_storage()
Factory->>Store: new() / init_schema()
Store->>DB: Create tables (responses, items, ...)
DB-->>Store: Tables created
Factory->>Store: run_migrations()
Store->>Versioning: run_postgres_migrations / run_oracle_migrations(schema, migrations, version, auto_migrate)
Versioning->>DB: Ensure _schema_versions table exists
DB-->>Versioning: Table ready
Versioning->>DB: Read current version(s)
DB-->>Versioning: Current version
alt auto_migrate true AND pending migrations
Versioning->>Versioning: Determine pending migrations
loop For each pending migration
Versioning->>DB: Execute migration SQL/PLSQL
DB-->>Versioning: Migration executed
Versioning->>DB: Record applied version in _schema_versions
DB-->>Versioning: Recorded
end
Versioning-->>Store: Return applied versions
else auto_migrate false AND pending migrations
Versioning-->>Store: Return error listing pending migrations and SQL
end
Store-->>Factory: migrations result
Factory-->>App: storage ready / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the data connector's database schema management by introducing a robust, versioned migration system. It moves away from untracked, ad-hoc schema changes to a structured approach that ensures consistency, provides explicit control over database modifications, and improves operational safety. The new system allows for clear tracking of schema evolution, offers a safe-by-default mechanism for applying changes, and incorporates concurrency safeguards for multi-instance deployments. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f85d491398
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data_connector/src/oracle_migrations.rs`:
- Around line 37-44: The current oracle_v2_up function unconditionally issues a
DROP (USER_ID) which can remove the active safety_identifier column when the
schema maps that logical field to USER_ID; change oracle_v2_up to first inspect
the responses mapping (the s variable from schema.responses) and skip emitting
the DROP when s.safety_identifier (or the responses alias for the safety
identifier) resolves to "USER_ID" (use a case-insensitive comparison), returning
an empty Vec<String> in that case; otherwise keep emitting the existing PL/SQL
DROP statement. Ensure you reference and check the SchemaConfig.responses ->
safety_identifier mapping before constructing the SQL.
In `@data_connector/src/postgres_migrations.rs`:
- Around line 35-39: pg_v2_up currently always issues "ALTER TABLE ... DROP
COLUMN IF EXISTS user_id", which can accidentally remove a configured safety
identifier column; change pg_v2_up (and reference SchemaConfig,
schema.responses, s.qualified_table, and responses.columns.safety_identifier) to
inspect s.columns.safety_identifier (as_deref()) and only emit the DROP COLUMN
statement when the configured safety_identifier is absent or equals "user_id"
(i.e., the default); otherwise return an empty Vec so no column is dropped.
In `@data_connector/src/versioning.rs`:
- Around line 234-240: The code interpolates schema.owner directly into SQL (see
schema.owner usage in building check_sql and other queries) which can break
queries or enable injection; add validation/sanitization for owner (e.g.,
implement a validate_owner(owner: &str) function that enforces non-empty and
only ASCII letters, digits, and underscores, and call it when constructing the
qualified table name or during schema initialization) or switch to parameterized
queries where supported; update all places that format owner into SQL (the
check_sql construction and the other occurrences referenced in the review) to
use the validated value (or a safely escaped/quoted form) before formatting the
SQL string.
- Around line 388-393: The code casts Migration::version (u32) to i32 when
inserting into the VERSIONS_TABLE which creates a silent type mismatch; change
the Migration struct's version field from u32 to i32 (update the Migration
definition and all usages/tests that construct or compare migrations) so types
align and you can remove the cast in the tx.execute call, or if you prefer to
keep u32, add a clear doc comment on Migration::version stating the Postgres
INTEGER limit and validate/convert to i32 at insertion time (e.g., check for
overflow and return an error) so the contract is explicit; update references to
VERSIONS_TABLE, Migration::version and the tx.execute insertion site
accordingly.
- Around line 233-240: The existence check uppercases the versions table name
and owner when schema.owner is Some, which breaks matching for quoted/lowercase
identifiers created by oracle_create_versions_table; update the logic so that
when schema.owner.is_some() you use the raw VERSIONS_TABLE (not
VERSIONS_TABLE.to_ascii_uppercase()) and use the owner string as provided (not
owner.to_ascii_uppercase()) in the all_tables query (i.e., change how check_sql
is built for the Some(owner) branch), leaving the uppercase behavior only for
the user_tables branch when owner is None.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
data_connector/README.mddata_connector/src/factory.rsdata_connector/src/lib.rsdata_connector/src/oracle.rsdata_connector/src/oracle_migrations.rsdata_connector/src/postgres.rsdata_connector/src/postgres_migrations.rsdata_connector/src/schema.rsdata_connector/src/versioning.rs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that introduces a robust, versioned schema migration system, replacing the previous ad-hoc ALTER TABLE calls. The new system is well-designed, with safety as a default, clear separation of concerns for different database backends, and thoughtful handling of concurrency and idempotency. The actionable error messages for pending migrations are a great touch for operator experience.
I've found one high-severity issue related to panic safety in the Postgres migration runner that could lead to deadlocks, and one minor point of improvement in the Oracle helper. Please see the detailed comments.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
data_connector/src/postgres_migrations.rs (1)
35-39:⚠️ Potential issue | 🔴 CriticalGuard
user_iddrop when it is the configured safety identifier.Line 38 unconditionally drops
user_id. Ifresponses.columns.safety_identifieris mapped touser_id, this migration deletes the active identifier column.💡 Proposed fix
fn pg_v2_up(schema: &SchemaConfig) -> Vec<String> { let s = &schema.responses; + if s.is_skipped("safety_identifier") + || s.col("safety_identifier").eq_ignore_ascii_case("user_id") + { + return vec![]; + } let table = s.qualified_table(schema.owner.as_deref()); vec![format!("ALTER TABLE {table} DROP COLUMN IF EXISTS user_id")] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/postgres_migrations.rs` around lines 35 - 39, The migration unconditionally drops user_id in pg_v2_up; change pg_v2_up to check schema.responses.columns.safety_identifier and only include the "ALTER TABLE ... DROP COLUMN IF EXISTS user_id" statement when the configured safety identifier is not "user_id" (i.e., if responses.columns.safety_identifier.as_deref() != Some("user_id")). Update the vec! construction in pg_v2_up to conditionally push the DROP statement based on that check so the active safety identifier column is never removed.data_connector/src/oracle_migrations.rs (1)
37-44:⚠️ Potential issue | 🔴 CriticalDo not drop
USER_IDwhen it backssafety_identifier.Line 42 always drops
USER_ID; this can delete the activesafety_identifiercolumn when the schema maps that logical field toUSER_ID.💡 Proposed fix
fn oracle_v2_up(schema: &SchemaConfig) -> Vec<String> { let s = &schema.responses; + if s.is_skipped("safety_identifier") + || s.col("safety_identifier").eq_ignore_ascii_case("USER_ID") + { + return vec![]; + } let table = s.qualified_table(schema.owner.as_deref()); // PL/SQL block: ORA-00904 = "invalid identifier" (column doesn't exist) vec![format!( "BEGIN EXECUTE IMMEDIATE 'ALTER TABLE {table} DROP (USER_ID)'; \ EXCEPTION WHEN OTHERS THEN IF SQLCODE != -904 THEN RAISE; END IF; END;" )] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/oracle_migrations.rs` around lines 37 - 44, The current oracle_v2_up always drops the literal USER_ID column which can remove the active safety_identifier column when the schema maps that logical field to USER_ID; update oracle_v2_up to first resolve the physical column name that backs the logical field "safety_identifier" from schema.responses (using the existing SchemaConfig/Responses API you have), then only generate a DROP for that resolved column if it is not equal to "USER_ID" (or skip the DROP entirely when the resolved column equals "USER_ID"); keep using table = s.qualified_table(...) and ensure the DROP statement references the resolved column name instead of hardcoding "USER_ID".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data_connector/src/versioning.rs`:
- Around line 260-265: The code currently reads MAX(version) into row:
Option<i64> and blindly casts row.unwrap_or(0) as u32 which can panic or produce
incorrect results for negative/overflow values; update the function that
contains conn/query_row_as_named (use the local variables conn, versions_table,
row) to: unwrap the Option into an i64 defaulting to 0, then validate that the
i64 is >= 0 and <= u32::MAX, and return a descriptive Err (map_err or custom
error) if it is out of range; only after validation convert to u32 and return
Ok(value).
---
Duplicate comments:
In `@data_connector/src/oracle_migrations.rs`:
- Around line 37-44: The current oracle_v2_up always drops the literal USER_ID
column which can remove the active safety_identifier column when the schema maps
that logical field to USER_ID; update oracle_v2_up to first resolve the physical
column name that backs the logical field "safety_identifier" from
schema.responses (using the existing SchemaConfig/Responses API you have), then
only generate a DROP for that resolved column if it is not equal to "USER_ID"
(or skip the DROP entirely when the resolved column equals "USER_ID"); keep
using table = s.qualified_table(...) and ensure the DROP statement references
the resolved column name instead of hardcoding "USER_ID".
In `@data_connector/src/postgres_migrations.rs`:
- Around line 35-39: The migration unconditionally drops user_id in pg_v2_up;
change pg_v2_up to check schema.responses.columns.safety_identifier and only
include the "ALTER TABLE ... DROP COLUMN IF EXISTS user_id" statement when the
configured safety identifier is not "user_id" (i.e., if
responses.columns.safety_identifier.as_deref() != Some("user_id")). Update the
vec! construction in pg_v2_up to conditionally push the DROP statement based on
that check so the active safety identifier column is never removed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
data_connector/README.mddata_connector/src/factory.rsdata_connector/src/lib.rsdata_connector/src/oracle.rsdata_connector/src/oracle_migrations.rsdata_connector/src/postgres.rsdata_connector/src/postgres_migrations.rsdata_connector/src/schema.rsdata_connector/src/versioning.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e11eca54b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27a89f4507
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
data_connector/src/versioning.rs (1)
262-273:⚠️ Potential issue | 🟡 MinorAvoid unchecked signed/unsigned casts for schema versions.
Lines 272 and 435 cast signed DB values to
u32without range checks, and Line 398 castsu32toi32for insert. A negative/corrupt row or oversized migration version can silently wrap and miscompute pending migrations.🛡️ Proposed defensive conversion
fn oracle_current_version(conn: &oracle::Connection, schema: &SchemaConfig) -> Result<u32, String> { @@ - Ok(row.unwrap_or(0) as u32) + let raw = row.unwrap_or(0); + u32::try_from(raw) + .map_err(|_| format!("invalid schema version value in {versions_table}: {raw}")) } @@ - tx.execute( + let version_i32 = i32::try_from(migration.version).map_err(|_| { + format!( + "migration version {} exceeds PostgreSQL INTEGER range", + migration.version + ) + })?; + tx.execute( &format!("INSERT INTO {VERSIONS_TABLE} (version, description) VALUES ($1, $2)"), - &[&(migration.version as i32), &migration.description], + &[&version_i32, &migration.description], ) @@ let version: i32 = row.get(0); - Ok(version as u32) + u32::try_from(version) + .map_err(|_| format!("invalid schema version value in {VERSIONS_TABLE}: {version}")) }#!/bin/bash set -euo pipefail # Find unchecked numeric casts in versioning flow. rg -n --type=rust 'as u32|as i32|MAX\(version\)|COALESCE\(MAX\(version\), 0\)' data_connector/src/versioning.rsAlso applies to: 398-399, 425-436
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/versioning.rs` around lines 262 - 273, The code currently performs unchecked casts between signed DB types and u32 (e.g., in oracle_current_version where an Option<i64> from MAX(version) is cast to u32, and where a u32 version is cast to i32 for insertion), which can silently wrap on negative or out-of-range values; update these conversion sites (referencing oracle_current_version, the MAX(version) query handling, and the code path that casts u32 -> i32 for inserts) to perform defensive checks: treat None as zero explicitly, verify the i64 is >= 0 and <= u32::MAX before converting (use i64::try_into or TryFrom/TryInto with explicit bounds check), and validate any u32 -> i32 conversion by ensuring the u32 value <= i32::MAX before casting (return an Err with a clear message if checks fail) so corrupted or oversized values are rejected rather than wrapped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data_connector/src/versioning.rs`:
- Around line 67-75: The versions table is currently unqualified, causing
cross-schema migration state leaks; update postgres_create_versions_table to
take &SchemaConfig, add a helper (e.g., qualified_table(owner: Option<&str>) ->
String or reuse qualified_table(schema.owner.as_deref())) that returns the
schema-qualified name for VERSIONS_TABLE, and replace all hardcoded
{VERSIONS_TABLE} usages in CREATE TABLE, INSERT, and SELECT queries (including
the functions that call or compose those SQL strings) to use the qualified name
so the table is created and queried under the correct SchemaConfig.owner.
---
Duplicate comments:
In `@data_connector/src/versioning.rs`:
- Around line 262-273: The code currently performs unchecked casts between
signed DB types and u32 (e.g., in oracle_current_version where an Option<i64>
from MAX(version) is cast to u32, and where a u32 version is cast to i32 for
insertion), which can silently wrap on negative or out-of-range values; update
these conversion sites (referencing oracle_current_version, the MAX(version)
query handling, and the code path that casts u32 -> i32 for inserts) to perform
defensive checks: treat None as zero explicitly, verify the i64 is >= 0 and <=
u32::MAX before converting (use i64::try_into or TryFrom/TryInto with explicit
bounds check), and validate any u32 -> i32 conversion by ensuring the u32 value
<= i32::MAX before casting (return an Err with a clear message if checks fail)
so corrupted or oversized values are rejected rather than wrapped.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
data_connector/src/oracle.rsdata_connector/src/oracle_migrations.rsdata_connector/src/postgres_migrations.rsdata_connector/src/versioning.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4820ebe4e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
data_connector/src/versioning.rs (1)
67-75:⚠️ Potential issue | 🟠 MajorPostgres
_schema_versionsis not schema-scoped, so migration state can leak across owners.Line 68 creates the table unqualified, and Lines 397/428 write/read it unqualified. With different
SchemaConfig.ownervalues in one database, one schema can incorrectly inherit another schema’s migration state and skip required migrations.💡 Proposed fix
+fn postgres_versions_table(schema: &SchemaConfig) -> String { + match &schema.owner { + Some(owner) => format!("{owner}.{VERSIONS_TABLE}"), + None => VERSIONS_TABLE.to_string(), + } +} + -pub fn postgres_create_versions_table() -> String { +pub fn postgres_create_versions_table(schema: &SchemaConfig) -> String { + let versions_table = postgres_versions_table(schema); format!( - "CREATE TABLE IF NOT EXISTS {VERSIONS_TABLE} (\ + "CREATE TABLE IF NOT EXISTS {versions_table} (\ version INTEGER NOT NULL PRIMARY KEY, \ description VARCHAR(512), \ applied_at TIMESTAMPTZ NOT NULL DEFAULT NOW())" ) } - client.batch_execute(&postgres_create_versions_table()).await?; + client + .batch_execute(&postgres_create_versions_table(schema)) + .await?; - &format!("INSERT INTO {VERSIONS_TABLE} (version, description) VALUES ($1, $2)"), + &format!( + "INSERT INTO {} (version, description) VALUES ($1, $2)", + postgres_versions_table(schema) + ), -async fn postgres_current_version(client: &tokio_postgres::Client) -> Result<u32, String> { +async fn postgres_current_version( + client: &tokio_postgres::Client, + schema: &SchemaConfig, +) -> Result<u32, String> { + let versions_table = postgres_versions_table(schema); let row = client .query_one( - &format!("SELECT COALESCE(MAX(version), 0) FROM {VERSIONS_TABLE}"), + &format!("SELECT COALESCE(MAX(version), 0) FROM {versions_table}"), &[], )#!/bin/bash set -euo pipefail # Show all Postgres versions-table references and whether they are schema-qualified. rg -n --type=rust 'postgres_create_versions_table|INSERT INTO \{VERSIONS_TABLE\}|FROM \{VERSIONS_TABLE\}|COALESCE\(MAX\(version\), 0\)' data_connector/src/versioning.rsAlso applies to: 289-292, 397-399, 425-429
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/versioning.rs` around lines 67 - 75, The versions table is created and referenced without schema-qualification which lets migration state leak across different SchemaConfig.owner values; update postgres_create_versions_table to create the table in the owner schema (use the SchemaConfig.owner value to prefix the table name), and update every SQL template that uses {VERSIONS_TABLE} (the INSERT INTO {VERSIONS_TABLE}, FROM {VERSIONS_TABLE}, COALESCE(MAX(version), 0) references) to include the same schema-qualified identifier; ensure you properly quote/escape the schema and table identifiers (use double-quoting or a safe identifier-quoting helper) when formatting strings so owners with special characters are handled safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data_connector/src/oracle.rs`:
- Around line 85-92: The migration can add columns required for indexes, but
init_schema's earlier attempt to create those indexes may fail (ORA-00904) and
be suppressed, leaving indexes uncreated until restart; fix by re-invoking the
schema initialization after running migrations: after
crate::versioning::run_oracle_migrations(...) returns successfully, call the
same init_schemas/init_schema path used earlier (the function that creates
RESPONSES_USER_IDX and other indexes) with the current connection and schema to
retry index creation in the same startup, and propagate or log any errors from
that retry so failures are visible.
In `@data_connector/src/versioning.rs`:
- Around line 239-257: The TOCTOU occurs where you check for the versions table
(using schema.owner and VERSIONS_TABLE with conn.query_row_as) then call
oracle_create_versions_table and conn.execute to create it; modify the execute
error handling to detect Oracle's ORA-00955 "name is already used by an existing
object" and treat it as success (silently proceed) instead of returning an
error, mirroring the existing migration conflict handling and the
create_index_if_missing pattern; specifically wrap the conn.execute error
mapping to check the error string/code for ORA-00955 and only return an error
for other cases, then still call conn.commit() on success.
---
Duplicate comments:
In `@data_connector/src/versioning.rs`:
- Around line 67-75: The versions table is created and referenced without
schema-qualification which lets migration state leak across different
SchemaConfig.owner values; update postgres_create_versions_table to create the
table in the owner schema (use the SchemaConfig.owner value to prefix the table
name), and update every SQL template that uses {VERSIONS_TABLE} (the INSERT INTO
{VERSIONS_TABLE}, FROM {VERSIONS_TABLE}, COALESCE(MAX(version), 0) references)
to include the same schema-qualified identifier; ensure you properly
quote/escape the schema and table identifiers (use double-quoting or a safe
identifier-quoting helper) when formatting strings so owners with special
characters are handled safely.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
data_connector/src/oracle.rsdata_connector/src/oracle_migrations.rsdata_connector/src/postgres_migrations.rsdata_connector/src/versioning.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 287e8ed001
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cd7f2344e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b31b5615e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let current = postgres_current_version(client).await?; | ||
| let skip_up_to = config_version.unwrap_or(0); | ||
| let effective_start = current.max(skip_up_to); |
There was a problem hiding this comment.
This runner derives effective_start solely from _schema_versions, so on a brand-new deployment (where init_schema already created tables with the latest shape) current is still 0 and all migrations are treated as pending. With the default auto_migrate: false, startup then fails on clean installs until operators run no-op DDL plus manual version inserts, which is a regression for default bootstrapping. Consider stamping fresh schemas to the latest version (or otherwise detecting a bootstrap state) before applying the pending-migration gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8338219a2e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // When auto_migrate is off, fail with actionable info | ||
| if !auto_migrate { | ||
| return Err(pending_migrations_error( |
There was a problem hiding this comment.
This auto_migrate gate fails startup whenever _schema_versions is empty, even though the same startup path has already created tables with the latest shape (for example, responses is created with safety_identifier and no user_id before migration checks). On a brand-new Oracle/Postgres deployment with default auto_migrate: false, this produces a hard failure and forces operators to run redundant no-op migration SQL plus manual version inserts just to boot. The migration runner should baseline freshly initialized schemas (or otherwise detect no-op pending migrations) instead of treating them as a required manual migration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 742c14a038
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !auto_migrate { | ||
| return Err(pending_migrations_error( | ||
| "postgres", | ||
| effective_start, | ||
| &pending, |
There was a problem hiding this comment.
On a brand-new Postgres/Oracle database, table initialization already creates the latest responses shape (including safety_identifier and without user_id), but _schema_versions starts empty, so current is 0 and this branch raises a migration-required error when auto_migrate is left at its default false. That makes first boot fail even though there is no real schema delta to apply, forcing operators to run no-op historical migrations or set skip flags; this should treat freshly bootstrapped schemas as up-to-date by recording a baseline version after initial DDL.
Useful? React with 👍 / 👎.
| let version: i32 = row.get(0); | ||
| Ok(version as u32) |
There was a problem hiding this comment.
The versions table allows any signed integer, but this cast converts negative values to huge u32 numbers (-1 becomes 4294967295). If an operator manually inserts a bad version row (which this feature explicitly supports via manual SQL guidance), migration checks will silently think the schema is far ahead and skip required migrations; reject negative values (or add a DB check constraint) instead of using an unchecked as u32 conversion.
Useful? React with 👍 / 👎.
github-actions
Bot
added
ci
|
Hi @slin1237, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93b6bd92f3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !applied.is_empty() { | ||
| store.ensure_response_indexes().await?; | ||
| } |
There was a problem hiding this comment.
PostgresResponseStorage::new now downgrades responses_safety_idx creation failures to a debug log when the migrated column is not present yet, and this function retries index creation only when run_migrations() reports locally applied versions. In concurrent startup, another instance can apply the migrations first (so applied is empty here), which skips this retry path even though this process already deferred index creation earlier; that leaves safety_identifier queries running without the intended index until a later restart.
Useful? React with 👍 / 👎.
a20c81b to
99d0a29
Compare
…ations Add a tracked, per-backend migration system that replaces ad-hoc ALTER TABLE calls with versioned migrations recorded in a _schema_versions table. What changed: - data_connector/src/versioning.rs: new migration infrastructure with Migration struct, version tracking table DDL, Oracle runner (sync with ORA-00001 concurrent safety), Postgres runner (async with pg_advisory_lock serialization), and pending_migrations_error for actionable failure messages - data_connector/src/oracle_migrations.rs: Oracle migration definitions (v1: add safety_identifier, v2: remove user_id) using PL/SQL idempotency - data_connector/src/postgres_migrations.rs: Postgres migration definitions (v1: add safety_identifier IF NOT EXISTS, v2: drop user_id IF EXISTS) - data_connector/src/schema.rs: add version (Option<u32>) and auto_migrate (bool, default false) fields to SchemaConfig with serde support - data_connector/src/oracle.rs: remove ad-hoc alter_safety_identifier_column and remove_user_id_column_if_exists (91 lines), wire ORACLE_MIGRATIONS into OracleStore::new after table creation - data_connector/src/postgres.rs: add PostgresStore::run_migrations method, wire POSTGRES_MIGRATIONS via factory - data_connector/src/factory.rs: call store.run_migrations() after Postgres table creation, clone store to keep it alive - data_connector/src/lib.rs: add versioning, oracle_migrations, and postgres_migrations modules - data_connector/README.md: document schema versioning section with safe-by-default behavior, migration table, config fields, concurrency safety, and YAML examples Why: Ad-hoc ALTER TABLE calls had no version tracking, re-ran on every startup relying solely on idempotency, and had race conditions between concurrent instances. A versioned migration system provides: - Version tracking via _schema_versions table (run-once guarantee) - Safe-by-default: auto_migrate defaults to false; when pending migrations exist, startup fails with the exact SQL statements needed so operators can review and apply manually - Opt-in auto-migration for development/convenience (auto_migrate: true) - Concurrency safety: pg_advisory_lock for Postgres, ORA-00001 catch for Oracle concurrent instances - Migration definitions respect SchemaConfig (custom table/column names, skip_columns) How: Migration struct holds version number, description, and an up function that takes &SchemaConfig and returns Vec<String> of DDL statements. Per-backend migration lists are static arrays in dedicated files. Runners check current version from _schema_versions, compare against pending migrations, and either apply (auto_migrate=true) or fail with actionable error (auto_migrate=false). Oracle uses PL/SQL exception handling for DDL idempotency since DDL auto- commits. Postgres uses transactions per migration since DDL is transactional. Oracle existence check uses all_tables with owner filter for cross-schema deployments. Signed-off-by: Si Lin <silin@umich.edu> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Three bugs identified by automated PR review, all fixed with tests: 1. Oracle _schema_versions existence check used wrong case for quoted identifiers. When schema.owner is set, the DDL creates a quoted table (OWNER."_schema_versions" = lowercase in catalog), but the check looked for _SCHEMA_VERSIONS (uppercase). Now uses lowercase for owner-qualified checks and uppercase for unquoted (current user) checks. - versioning.rs: ensure_oracle_versions_table uses case-aware SQL 2. Oracle init_schema creates safety_identifier index before migrations add the column on legacy schemas. create_index_if_missing now tolerates ORA-00904 (invalid identifier) so the index creation is skipped when the column doesn't exist yet. Next startup after migration creates it. - oracle.rs: create_index_if_missing handles ORA-00904 3. v2 migration (drop USER_ID) could destroy a column that SchemaConfig maps to USER_ID. Both Oracle and Postgres v2 now check if any column mapping targets USER_ID before dropping. - oracle_migrations.rs: oracle_v2_up checks column mappings - postgres_migrations.rs: pg_v2_up checks column mappings - Added tests for both skip cases Signed-off-by: Si Lin <silin@umich.edu> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Oracle does not allow identifiers starting with `_` unless they are
double-quoted. The CREATE TABLE, INSERT, and SELECT statements for the
_schema_versions table were using the name unquoted when no schema
owner was configured, producing ORA-00911 ("invalid character").
What changed:
- versioning.rs: extract `oracle_versions_table()` helper that always
returns a quoted identifier (`"_schema_versions"` or
`OWNER."_schema_versions"`)
- Replace 3 inline match arms in `oracle_create_versions_table`,
`run_oracle_migrations`, and `oracle_current_version` with the helper
- Fix `ensure_oracle_versions_table` existence check for the no-owner
case: look for lowercase name in user_tables since the quoted
identifier preserves case
- Add test for the helper covering both owner and no-owner cases
- Update existing DDL test to assert quoting
Why:
E2E Oracle tests fail with ORA-00911 because `CREATE TABLE
_schema_versions (...)` is invalid Oracle SQL.
Signed-off-by: simolin <simolin@users.noreply.github.com>
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
What changed: - versioning.rs: catch ORA-00955 (table already exists) in ensure_oracle_versions_table to handle concurrent startup race - versioning.rs: include INSERT INTO _schema_versions SQL in the pending-migration error message so operators recording the version after manual DDL application - oracle.rs: re-run init_schemas after migrations so that indexes on newly-added columns (e.g. safety_identifier from v1) are created in the same startup cycle instead of requiring a restart - postgres.rs: separate CREATE INDEX from CREATE TABLE in PostgresResponseStorage::new() so legacy tables missing migrated columns don't block startup; add ensure_response_indexes() to retry index creation after migrations - factory.rs: call ensure_response_indexes() after Postgres migrations when any migrations were applied Why: PR review bots identified several real issues: Oracle TOCTOU race on _schema_versions creation, missing version-tracking INSERT in manual migration guidance, and index creation on migrated columns failing on legacy schemas (blocking upgrades for both Oracle and Postgres). Signed-off-by: simolin <simolin@users.noreply.github.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
What changed: - oracle_migrations.rs: v2 migration also checks extra_columns keys for USER_ID before dropping the column - postgres_migrations.rs: same guard for Postgres v2 migration - Added tests for extra_columns guard in both backends Why: user_id is not a core column, so SchemaConfig validation allows it as an extra_column. Without this check, v2 migration would drop a legitimately configured extra column and its data. Signed-off-by: simolin <simolin@users.noreply.github.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
What changed: - model_gateway/src/main.rs: add --db-auto-migrate CLI flag (env: DB_AUTO_MIGRATE) that wires into SchemaConfig.auto_migrate for both Oracle and Postgres backends - model_gateway/src/config/types.rs: re-export SchemaConfig from data-connector crate - .github/workflows/pr-test-rust.yml: set DB_AUTO_MIGRATE=true in agentic-apis E2E test env so ephemeral CI databases auto-migrate Why: The safe-by-default auto_migrate=false design causes E2E tests to fail on fresh CI Oracle databases since there is no _schema_versions table and pending migrations block startup. CI environments need auto_migrate enabled, while production keeps the safe default. Signed-off-by: simolin <simolin@users.noreply.github.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…n control" This reverts commit 63132e9. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…tions
default_auto_migrate() now checks the DB_AUTO_MIGRATE environment
variable ("true" or "1") before falling back to false. This allows
CI and ephemeral environments to opt into automatic migrations
without requiring CLI changes or config file modifications.
SchemaConfig::default() delegates to default_auto_migrate() instead
of hardcoding false, so both serde deserialization and programmatic
construction respect the env var.
The CI Oracle setup script (ci_agentic_svc_deps.sh) now exports
DB_AUTO_MIGRATE=true alongside the ATP_* credentials, fixing the
agentic-apis E2E test failure on fresh Oracle databases where
pending migrations caused startup to abort.
Signed-off-by: Simolin <simolin@users.noreply.github.com>
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
1 participant
Summary
_schema_versionstableauto_migratedefaults tofalse— startup fails with the exact SQL statements needed when pending migrations exist, so operators can review and apply manuallyauto_migrate: truefor development/convenienceWhat changed
New files
versioning.rs: Migration infrastructure —Migrationstruct, version tracking table DDL, Oracle runner (sync, ORA-00001 concurrent safety), Postgres runner (async,pg_advisory_lockserialization),pending_migrations_errorfor actionable failure messagesoracle_migrations.rs: Oracle migration definitions (v1: addsafety_identifiervia PL/SQL, v2: removeuser_idvia PL/SQL) with idempotency via exception handlingpostgres_migrations.rs: Postgres migration definitions (v1: addsafety_identifierIF NOT EXISTS, v2: dropuser_idIF EXISTS)Modified files
schema.rs: Addversion: Option<u32>andauto_migrate: bool(defaultfalse) toSchemaConfigwith serde support and testsoracle.rs: Remove ad-hocalter_safety_identifier_columnandremove_user_id_column_if_exists(~91 lines), wireORACLE_MIGRATIONSintoOracleStore::new()after table creation. Fixensure_oracle_versions_tableto useall_tableswith owner filter for cross-schema deploymentspostgres.rs: AddPostgresStore::run_migrations()method wiringPOSTGRES_MIGRATIONSfactory.rs: Callstore.run_migrations()after Postgres table creationlib.rs: Addversioning,oracle_migrations,postgres_migrationsmodulesREADME.md: Document schema versioning section with safe-by-default behavior, migration table, config fields, concurrency safety, and YAML examplesWhy
Ad-hoc ALTER TABLE calls had no version tracking, re-ran on every startup relying solely on idempotency, and had race conditions between concurrent instances. This provides:
_schema_versionstracking tablepg_advisory_lockfor Postgres, ORA-00001 duplicate detection for Oracleskip_columnsHow
Migrationstruct holds version, description, and anupfunction (fn(&SchemaConfig) -> Vec<String>). Per-backend migration lists are static arrays in dedicated files. Runners check current version from_schema_versions, compare against pending migrations, and either apply (auto_migrate: true) or fail with actionable error (auto_migrate: false). Oracle uses PL/SQL exception handling for DDL idempotency. Postgres uses transactions per migration.Test plan
cargo test -p data-connector— 208 tests passcargo clippy --all-targets --all-features -- -D warnings— cleanpending_migrations_erroroutput tests (SQL included, version hints, auto_migrate hint)versionandauto_migratefieldsSummary by CodeRabbit
New Features
Documentation
Tests