Conversation
…umn names Add a YAML-driven schema configuration layer to the data_connector crate, allowing users to customize table names, column names, and schema owner prefixes without forking the repository. What changed: - data_connector/src/schema.rs: new file with SchemaConfig, TableConfig types, validation logic (identifiers must match [a-zA-Z0-9_]+), col() for column name remapping, qualified_table() for owner-prefixed table names, Default impl producing current hardcoded names, serde support, and 15 unit tests - data_connector/src/config.rs: added schema: Option<SchemaConfig> field to OracleConfig, PostgresConfig, RedisConfig - data_connector/src/lib.rs: added pub mod schema and re-exports - data_connector/src/oracle.rs: refactored all SQL strings to use schema.col() and qualified_table(), changed SchemaInitFn signature to accept &SchemaConfig, stored Arc<SchemaConfig> on OracleStore - data_connector/src/postgres.rs: refactored all SQL strings, replaced SELECT * with explicit column lists, stored Arc<SchemaConfig> on PostgresStore - data_connector/src/redis.rs: refactored key patterns to use owner prefix and hash field names to use col(), stored Arc<SchemaConfig> on RedisStore - data_connector/src/common.rs: extracted shared RESPONSE_COLUMNS const and build_response_select_base() from oracle/postgres backends - data_connector/src/core.rs: made get_response_chain() a default trait method on ResponseStorage, eliminating tripled implementation - data_connector/README.md: documented schema configuration with YAML examples, type reference, and key behaviors - bindings/python/src/lib.rs: added schema: None to Python config conversions - model_gateway/src/main.rs: added schema: None to backend config construction - model_gateway/tests/routing/test_openai_routing.rs: added schema: None to test config Why: Internal teams were forced to fork the repo (~800 lines of changes) just to use different column names for their database schema. This change enables runtime schema customization via configuration, eliminating the need to fork. When no schema config is provided, all backends produce identical SQL/keys to the previous hardcoded behavior. How: - SchemaConfig stored as Arc on each backend Store struct, shared across all three storage trait implementations per backend - col() returns remapped column name or passes through the logical name unchanged (zero-cost for default config) - qualified_table() returns OWNER."TABLE" for Oracle or just table name - Validation runs at construction time, rejecting invalid identifiers before any queries execute - Redis: only owner (key prefix) and columns (hash field names) affect behavior; table config is ignored for Redis key patterns - RESPONSE_COLUMNS and build_response_select_base extracted to common.rs as single source of truth for Oracle and Postgres - get_response_chain default trait method eliminates 75 lines of identical code across 3 backends Refs: Phase 1 of schema configuration initiative Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
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 YAML-driven SchemaConfig/TableConfig and threads schema metadata through Postgres, Oracle, and Redis stores; replaces hard-coded table/column/key names with schema-driven generation; exposes schema types from crate root; adds shared response utilities and a default get_response_chain implementation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant RespTrait as ResponseStorage
participant Store as BackendStore
participant DB as Database
Client->>RespTrait: get_response_chain(response_id, max_depth)
RespTrait->>Store: get_response(current_id)
Store->>DB: SELECT ... FROM <qualified_table> WHERE id = $1
DB-->>Store: row / None
Store-->>RespTrait: Response (includes previous_response_id)
RespTrait->>RespTrait: push response, detect cycles, check depth
alt has previous_response_id and within depth
RespTrait->>Store: get_response(previous_response_id)
Store->>DB: SELECT ... (repeat)
end
RespTrait-->>Client: Ok(ResponseChain) (oldest-first)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
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 introduces a significant enhancement to the data connector by providing a flexible schema configuration layer. This change empowers users to adapt the system to their existing database schemas without modifying the core codebase, addressing a common pain point that previously required repository forking. The implementation ensures backward compatibility, with zero behavioral changes for existing users who do not provide a custom schema configuration. This foundational work sets the stage for further schema customization initiatives. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for customizing database schemas at runtime, reducing the need for forking. A comprehensive security review found no vulnerabilities. However, a critical backward compatibility issue was identified for Oracle database schemas: the new quoting mechanism and lowercase default names will cause queries to fail against existing uppercase tables. There is also a medium-severity suggestion to improve documentation clarity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
data_connector/src/config.rs (1)
101-132: 🧹 Nitpick | 🔵 TrivialConsider validating
SchemaConfiginsidePostgresConfig::validate()(andRedisConfig::validate()).Currently, schema validation only happens in the store constructors (e.g.,
OracleStore::new). If a user callsconfig.validate()and it passes, they might reasonably expect the entire config is valid — but an invalid schema would only surface later at store creation time. Propagating schema validation into the config-levelvalidate()methods would provide earlier, more predictable error reporting.Example for PostgresConfig
pub fn validate(&self) -> Result<(), String> { // ... existing URL and pool checks ... + if let Some(ref schema) = self.schema { + schema.validate().map_err(|e| format!("schema: {e}"))?; + } + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/config.rs` around lines 101 - 132, Add schema validation to the top-level PostgresConfig::validate() and RedisConfig::validate() by invoking the SchemaConfig validation (e.g., call schema.validate() or the appropriate method on the SchemaConfig instance found in PostgresConfig/RedisConfig) and propagate any returned error as part of the config validation result; update PostgresConfig::validate and RedisConfig::validate to return Err with the schema validation message when SchemaConfig validation fails so config.validate() fails early if the embedded schema is invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/python/src/lib.rs`:
- Line 286: The Python bindings currently hardcode schema: None which prevents
schema customization; add a PySchemaConfig wrapper type and make the schema
parameter optional on each Python-facing config (the Py*Config constructors used
in bindings/lib.rs), propagate the optional schema through the conversion to the
underlying Rust config (set schema = Some(schema.into()) when provided,
otherwise None), and update the binding code paths that currently set schema:
None to use the new optional field so Python callers can pass a schema; ensure
constructors, conversion impls, and any pyclass definitions accept
Option<PySchemaConfig> and map it to the Rust config field.
In `@data_connector/README.md`:
- Around line 152-154: Update the README comment for the owner field to remove
ambiguity: explicitly state whether owner is ignored for Postgres today or note
intended future behavior (e.g., whether Postgres schema qualification via
qualified_table or SET search_path will be supported later). Refer to the owner
field and the term qualified_table in your edit and either replace "Postgres:
not used in qualified_table today" with "Postgres: owner is ignored today (no
schema qualification); future support for schema.table or SET search_path may be
considered" or with a clear statement that owner is intentionally ignored —
whichever matches the project's intent.
In `@data_connector/src/core.rs`:
- Around line 468-505: get_response_chain currently can loop forever on cyclic
previous_response_id links; modify the function to detect cycles by tracking
visited ResponseId values (e.g. with a HashSet<ResponseId>) while walking the
chain in get_response_chain, and if you encounter an id already in the set
(including immediate self-reference) return
Err(ResponseStorageError::InvalidChain) (or a clear ResponseStorageError
variant) instead of looping; keep existing max_depth logic and still reverse
chain.responses before returning.
In `@data_connector/src/schema.rs`:
- Around line 83-91: Add a doc comment to the qualified_table method clarifying
that its current output uses Oracle-style quoting (owner unquoted, table
double-quoted like OWNER."TABLE") and is backend-specific; mention that Postgres
typically uses "schema"."table" or schema.table and that callers should adapt or
update qualified_table when supporting Postgres owners. Update the doc for the
function named qualified_table(&self, owner: Option<&str>) to make this behavior
explicit so future maintainers know to change quoting logic if they switch
backends.
- Around line 128-139: validate_identifier currently allows names that start
with a digit and imposes no length limit; update validate_identifier to also
reject identifiers that begin with an ASCII digit and to enforce a maximum
length (e.g., 63 characters by default, or make the limit configurable) so
callers get early, clear errors instead of DB rejections. Modify the function
validate_identifier(name: &str) to first check name.chars().next() for
is_ascii_digit() and return an Err with a clear message like "identifier must
not start with a digit", and then check name.len() against the chosen
MAX_IDENTIFIER_LEN (use a constant MAX_IDENTIFIER_LEN) and return an Err if
exceeded; keep existing ASCII and empty checks and include the function name
validate_identifier and the new constant symbol when editing.
- Around line 34-44: TableConfig::default() yields an empty table name which
leads to confusing validation errors when users supply a partial TableConfig in
YAML; change the deserialization/default strategy so a missing table is not
silently set to "". Either implement a custom Default for TableConfig that sets
table to a clear sentinel (e.g. "<unspecified_table>") or add a serde
deserialize_with for the table field to return an explicit error when
absent/empty during deserialization; update references to TableConfig,
TableConfig::default(), and the table field (and keep columns behavior
unchanged) so validation messages become actionable.
---
Outside diff comments:
In `@data_connector/src/config.rs`:
- Around line 101-132: Add schema validation to the top-level
PostgresConfig::validate() and RedisConfig::validate() by invoking the
SchemaConfig validation (e.g., call schema.validate() or the appropriate method
on the SchemaConfig instance found in PostgresConfig/RedisConfig) and propagate
any returned error as part of the config validation result; update
PostgresConfig::validate and RedisConfig::validate to return Err with the schema
validation message when SchemaConfig validation fails so config.validate() fails
early if the embedded schema is invalid.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
bindings/python/src/lib.rsdata_connector/README.mddata_connector/src/common.rsdata_connector/src/config.rsdata_connector/src/core.rsdata_connector/src/lib.rsdata_connector/src/oracle.rsdata_connector/src/postgres.rsdata_connector/src/redis.rsdata_connector/src/schema.rsmodel_gateway/src/main.rsmodel_gateway/tests/routing/test_openai_routing.rs
What changed: - data_connector/src/schema.rs: added uppercase_table_names() method on SchemaConfig for Oracle backward compatibility, updated qualified_table() doc comment to clarify Oracle-specific quoting behavior, added 2 unit tests for the new method - data_connector/src/oracle.rs: call uppercase_table_names() in OracleStore::new() before validation so quoted table names match Oracle's uppercase catalog entries, removed redundant .to_uppercase() calls from all init_schema and migration functions since table names are now pre-uppercased - data_connector/src/core.rs: added HashSet-based cycle detection to the default get_response_chain() trait method to prevent infinite loops on self-referencing or cyclic response chains - data_connector/README.md: clarified that owner is ignored for Postgres (use search_path instead) Why: PR review identified that Oracle folds unquoted identifiers to uppercase, so existing tables are stored as CONVERSATIONS etc. The qualified_table() method quotes identifiers, making them case-sensitive. Without uppercasing the configured names first, queries would target "conversations" but the actual table is CONVERSATIONS — a backward compatibility break. Additionally, the default get_response_chain() had no cycle detection, so a self-referencing chain with max_depth=None would loop forever. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
data_connector/src/core.rs (1)
468-506:⚠️ Potential issue | 🟡 MinorReturn an InvalidChain error when a cycle is detected.
The cycle check currently just breaks and returns a partial chain, which hides corrupt links and skips the warning path in callers. Consider returningResponseStorageError::InvalidChainso invalid chains are surfaced while callers can still degrade gracefully.🛠️ Proposed fix
- // Cycle detection: stop if we've already visited this ID. - if !seen.insert(lookup_id.clone()) { - break; - } + // Cycle detection: stop if we've already visited this ID. + if !seen.insert(lookup_id.clone()) { + return Err(ResponseStorageError::InvalidChain(format!( + "cycle detected at {}", + lookup_id.0.clone() + ))); + }Based on learnings: In model_gateway/src/routers/openai/router.rs, load_input_history logs a warning but proceeds when get_response_chain fails to preserve UX.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/core.rs` around lines 468 - 506, In get_response_chain, instead of silently breaking when a cycle is detected, return an error indicating an invalid chain: on the cycle-detection branch (where seen.insert(...) returns false) return Err(ResponseStorageError::InvalidChain { response_id: lookup_id.clone(), details: "cycle detected in previous_response_id chain".into() }.into()) so callers of get_response_chain (which returns ResponseResult<ResponseChain>) receive a clear InvalidChain error; update the function's cycle branch and ensure ResponseId, get_response_chain, and ResponseStorageError::InvalidChain are referenced as the identifying symbols.data_connector/src/schema.rs (2)
146-156:⚠️ Potential issue | 🟡 MinorTighten identifier validation for leading digits and length.
Identifiers starting with digits or exceeding backend limits pass validation but will fail at DDL/query time. Adding explicit checks gives earlier, clearer errors.🛠️ Proposed validation tightening
+const MAX_IDENTIFIER_LEN: usize = 63; + /// Reject identifiers that are empty or contain characters outside `[a-zA-Z0-9_]`. fn validate_identifier(name: &str) -> Result<(), String> { if name.is_empty() { return Err("identifier must not be empty".to_string()); } + if name.chars().next().map_or(false, |c| c.is_ascii_digit()) { + return Err("identifier must not start with a digit".to_string()); + } + if name.len() > MAX_IDENTIFIER_LEN { + return Err(format!( + "identifier must not exceed {MAX_IDENTIFIER_LEN} characters" + )); + } if !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { return Err(format!( "invalid identifier '{name}' — only ASCII alphanumeric and underscores allowed" ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/schema.rs` around lines 146 - 156, The validate_identifier function currently allows names that start with digits or exceed backend limits; update validate_identifier to (1) reject identifiers whose first character is an ASCII digit with an explicit error like "identifier must not start with a digit", and (2) enforce a maximum length constant (e.g., MAX_IDENTIFIER_LEN set to the backend limit) and return a clear error like "identifier must be at most N characters"; keep the existing empty and non-ASCII/alphanumeric/underscore checks and make sure to reference the same function name validate_identifier when implementing these additional checks and error messages.
33-44: 🧹 Nitpick | 🔵 Trivial
TableConfig::default()yields an empty table name; make missing-table errors clearer.
If users provide a partial table config (e.g., onlycolumns), serde fillstablewith""and validation errors are confusing. Consider a customDefault/deserializer that emits an explicit “table is required” error when the field is omitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/schema.rs` around lines 33 - 44, TableConfig currently uses #[serde(default)] so a missing "table" field deserializes to an empty string, causing unclear validation errors; change TableConfig deserialization to fail when "table" is omitted or empty by implementing a custom Deserialize (or a custom deserialize_with for the table field) that returns a serde::de::Error (e.g., Error::missing_field or a custom message) if "table" is not present or is an empty string, and keep the columns handling via its existing default; target the TableConfig struct and its table field in your change so deserializing a partial object produces a clear “table is required” error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@data_connector/src/core.rs`:
- Around line 468-506: In get_response_chain, instead of silently breaking when
a cycle is detected, return an error indicating an invalid chain: on the
cycle-detection branch (where seen.insert(...) returns false) return
Err(ResponseStorageError::InvalidChain { response_id: lookup_id.clone(),
details: "cycle detected in previous_response_id chain".into() }.into()) so
callers of get_response_chain (which returns ResponseResult<ResponseChain>)
receive a clear InvalidChain error; update the function's cycle branch and
ensure ResponseId, get_response_chain, and ResponseStorageError::InvalidChain
are referenced as the identifying symbols.
In `@data_connector/src/schema.rs`:
- Around line 146-156: The validate_identifier function currently allows names
that start with digits or exceed backend limits; update validate_identifier to
(1) reject identifiers whose first character is an ASCII digit with an explicit
error like "identifier must not start with a digit", and (2) enforce a maximum
length constant (e.g., MAX_IDENTIFIER_LEN set to the backend limit) and return a
clear error like "identifier must be at most N characters"; keep the existing
empty and non-ASCII/alphanumeric/underscore checks and make sure to reference
the same function name validate_identifier when implementing these additional
checks and error messages.
- Around line 33-44: TableConfig currently uses #[serde(default)] so a missing
"table" field deserializes to an empty string, causing unclear validation
errors; change TableConfig deserialization to fail when "table" is omitted or
empty by implementing a custom Deserialize (or a custom deserialize_with for the
table field) that returns a serde::de::Error (e.g., Error::missing_field or a
custom message) if "table" is not present or is an empty string, and keep the
columns handling via its existing default; target the TableConfig struct and its
table field in your change so deserializing a partial object produces a clear
“table is required” error.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
data_connector/README.mddata_connector/src/core.rsdata_connector/src/oracle.rsdata_connector/src/schema.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
data_connector/src/schema.rs (1)
156-170:⚠️ Potential issue | 🟠 MajorReject identifiers that start with a digit to avoid invalid unquoted SQL.
validate_identifiercurrently accepts names like1table. In most SQL dialects, unquoted identifiers cannot start with a digit, and the backends appear to embed identifiers without quoting in several places. This passes validation but fails at runtime with invalid SQL. Add a start-character check (or alternatively ensure all identifiers are consistently quoted across backends).🛠️ Suggested fix
fn validate_identifier(name: &str) -> Result<(), String> { if name.is_empty() { return Err("identifier must not be empty".to_string()); } if name.len() > MAX_IDENTIFIER_LEN { return Err(format!( "identifier '{name}' exceeds maximum length of {MAX_IDENTIFIER_LEN} characters" )); } + if name.chars().next().unwrap().is_ascii_digit() { + return Err("identifier must not start with a digit".to_string()); + } if !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { return Err(format!( "invalid identifier '{name}' — only ASCII alphanumeric and underscores allowed" )); } Ok(()) }PostgreSQL identifier rules: can unquoted identifiers start with a digit? Oracle identifier rules: can unquoted identifiers start with a digit?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/schema.rs` around lines 156 - 170, The validator validate_identifier currently allows identifiers that start with a digit; add a check in validate_identifier (before the per-character check) that the first character satisfies is_ascii_alphabetic() || c == '_' and return an Err with a clear message (e.g. "identifier '{name}' must not start with a digit; unquoted identifiers must begin with a letter or underscore") if it doesn't; keep the existing MAX_IDENTIFIER_LEN and subsequent all-char ASCII alnum/underscore checks intact so only the start-character rule is enforced in addition to the existing validations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@data_connector/src/schema.rs`:
- Around line 156-170: The validator validate_identifier currently allows
identifiers that start with a digit; add a check in validate_identifier (before
the per-character check) that the first character satisfies
is_ascii_alphabetic() || c == '_' and return an Err with a clear message (e.g.
"identifier '{name}' must not start with a digit; unquoted identifiers must
begin with a letter or underscore") if it doesn't; keep the existing
MAX_IDENTIFIER_LEN and subsequent all-char ASCII alnum/underscore checks intact
so only the start-character rule is enforced in addition to the existing
validations.
… limit
What changed:
- data_connector/src/schema.rs: added 128-char max length check on
identifiers (covers Oracle 30/128 and Postgres 63 limits), improved
error message when table name is empty from a partial YAML config
(now says "table name is required" with a hint about the missing key),
added tests for both new behaviors
Why:
PR review noted that an empty table name from a partial YAML config
(e.g. `conversations: { columns: { id: "MY_ID" } }`) produces a
confusing "identifier must not be empty" error. Also, overly long
identifiers would only fail at query time — catching them at startup
gives a clearer error.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
data_connector/src/oracle.rs (3)
326-366: 🧹 Nitpick | 🔵 Trivial
create_conversation— dynamic SQL construction is clean.The column list, placeholders, and params are built in sync. The pattern of
let columns = [col_id, col_created, col_meta]alongsidelet params: Vec<&dyn ToSql> = vec![...]is clear.Minor fragility: the
columnsarray andparamsvector are manually kept in parallel. A paired declaration (like thelogical_fieldspattern used instore_responseat line 1224) would be more robust. Consider aligning the style here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/oracle.rs` around lines 326 - 366, In create_conversation, avoid manual parallel arrays by pairing each column with its value like the logical_fields pattern used in store_response: build a Vec of (column, &dyn ToSql) tuples using schema.conversations (e.g., col("id"), col("created_at"), col("metadata")), then derive the columns list, placeholders, and params by iterating that Vec before calling conn.execute inside self.store.execute; this keeps columns, placeholders and params synchronized and replaces the current separate let columns = [...] and let params = vec![...] usage.
550-575:⚠️ Potential issue | 🟠 MajorHardcoded constraint and index names will collide when table names are customized.
pk_conv_item_link(line 559) andconv_item_links_conv_idx(line 570) are Oracle-schema-scoped names. If two deployments share the same Oracle schema but use differentconversation_item_linkstable names, the secondCREATE TABLEfails because the constraint name already exists (and this error is not suppressed — unlike the ORA-00955 handling increate_index_if_missing). The index creation would also silently skip due to ORA-00955.Derive these names from the actual table name to avoid clashes:
Proposed fix
- format!("CONSTRAINT pk_conv_item_link PRIMARY KEY ({col_cid}, {col_iid})"), + format!("CONSTRAINT pk_{} PRIMARY KEY ({col_cid}, {col_iid})", sl.table.to_lowercase()), ]; conn.execute( &format!("CREATE TABLE {sl_table} ({})", col_defs.join(", ")), &[], ) .map_err(map_oracle_error)?; conn.execute( - &format!( - "CREATE INDEX conv_item_links_conv_idx ON {sl_table} ({col_cid}, {col_added})" - ), + &format!( + "CREATE INDEX idx_{}_conv ON {sl_table} ({col_cid}, {col_added})", + sl.table.to_lowercase() + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/oracle.rs` around lines 550 - 575, The hardcoded constraint name pk_conv_item_link and index name conv_item_links_conv_idx will collide across customized table names; update the block that runs when exists_links == 0 to derive unique names from the table name (use the existing sl_table variable or sl.col/table-id helpers) for the PRIMARY KEY constraint and the index (e.g., include sl_table in the constraint/index identifier), and apply the same ORA-00955-safe behavior used by create_index_if_missing for index creation (and similarly ignore duplicate-constraint errors or check for existing constraint before CREATE) so table creation succeeds when multiple table names share a schema.
284-314: 🧹 Nitpick | 🔵 TrivialSchema-driven DDL for conversations table looks correct.
The existence check uses
s.table(uppercased) againstuser_tables, and the CREATE TABLE uses the fully qualified name. Column definitions are derived from schema vias.col(). The pattern is consistent.One minor observation: lines 291–294 interpolate
s.tabledirectly into the SQL string rather than using a bind parameter. This is safe becausevalidate_identifierrestricts to[a-zA-Z0-9_], but using a bind param (:1) would be more defensive and consistent with howindex_nameis bound increate_index_if_missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/oracle.rs` around lines 284 - 314, The existence check in init_schema currently interpolates s.table directly into the SQL string; change the conn.query_row_as call in init_schema to use a bind parameter (e.g. "SELECT COUNT(*) FROM user_tables WHERE table_name = :1") and pass s.table as the bound value to avoid direct interpolation, following the same binding pattern used in create_index_if_missing; keep using the validated/uppercased s.table and map_oracle_error unchanged.
🤖 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 736-830: The long 7-element tuple type for rows makes the closure
hard to read; replace the Vec<(String, Option<String>, String, Option<String>,
Option<String>, Option<String>, DateTime<Utc>)> annotation by either (A)
defining a small local struct (e.g., ConversationRow { id, response_id,
item_type, role, content_raw, status, created_at }) and push instances of that
struct, or (B) skip the intermediate tuple entirely and build the final response
objects directly inside the closure (similar to a build_response_from_row
helper) after extracting fields (refer to si_col_id, si_col_resp, si_col_type,
si_col_role, si_col_content, si_col_status, si_col_created and the closure
passed to self.store.execute); update types returned by the closure and the
outer let rows variable accordingly to remove the verbose tuple annotation.
- Around line 1029-1043: The index name strings for the responses table are
hardcoded ("RESPONSES_PREV_IDX", "RESPONSES_USER_IDX") which can collide across
schemas; update the calls to create_index_if_missing so the index name is
derived from the configured table (s.table) similar to other fixes: build unique
index names (e.g., format!("{}_prev_idx", s.table) and format!("{}_user_idx",
s.table) or otherwise include sanitized s.table) and pass those generated names
as the second argument while keeping the CREATE INDEX SQL using the same table
and column variables (prev and safety) to avoid collisions; adjust any
sanitization if present to ensure valid identifier names.
In `@data_connector/src/schema.rs`:
- Around line 101-172: uppercase_table_names() only uppercases table names but
not column override values in TableConfig.columns, which can cause mismatches
for Oracle; add a companion method (e.g., uppercase_column_names()) on
SchemaConfig that iterates each TableConfig.columns and calls
make_ascii_uppercase() on the physical column names (the HashMap values), then
call this method alongside uppercase_table_names() where OracleStore::new()
initializes the schema; update references to SchemaConfig::uppercase_table_names
and TableConfig.columns in your changes.
---
Outside diff comments:
In `@data_connector/src/oracle.rs`:
- Around line 326-366: In create_conversation, avoid manual parallel arrays by
pairing each column with its value like the logical_fields pattern used in
store_response: build a Vec of (column, &dyn ToSql) tuples using
schema.conversations (e.g., col("id"), col("created_at"), col("metadata")), then
derive the columns list, placeholders, and params by iterating that Vec before
calling conn.execute inside self.store.execute; this keeps columns, placeholders
and params synchronized and replaces the current separate let columns = [...]
and let params = vec![...] usage.
- Around line 550-575: The hardcoded constraint name pk_conv_item_link and index
name conv_item_links_conv_idx will collide across customized table names; update
the block that runs when exists_links == 0 to derive unique names from the table
name (use the existing sl_table variable or sl.col/table-id helpers) for the
PRIMARY KEY constraint and the index (e.g., include sl_table in the
constraint/index identifier), and apply the same ORA-00955-safe behavior used by
create_index_if_missing for index creation (and similarly ignore
duplicate-constraint errors or check for existing constraint before CREATE) so
table creation succeeds when multiple table names share a schema.
- Around line 284-314: The existence check in init_schema currently interpolates
s.table directly into the SQL string; change the conn.query_row_as call in
init_schema to use a bind parameter (e.g. "SELECT COUNT(*) FROM user_tables
WHERE table_name = :1") and pass s.table as the bound value to avoid direct
interpolation, following the same binding pattern used in
create_index_if_missing; keep using the validated/uppercased s.table and
map_oracle_error unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
data_connector/src/oracle.rs (1)
1053-1096: 🧹 Nitpick | 🔵 Trivial
col_upperon line 1060 is redundant afteruppercase_for_oracle().Since
uppercase_for_oracle()already uppercases all column override values,col_safetyis already uppercase when an override is configured. When no override exists,col("safety_identifier")returns the lowercase literal"safety_identifier", so.to_uppercase()is actually needed for the catalog lookup. This is correct — the code handles both paths.However, the comment
// Table and column names are already uppercased by OracleStore::new().on line 1059 is misleading: only override values are uppercased, not the default logical names returned bycol()when no override is present. Consider clarifying the comment.📝 Suggested comment clarification
- // Table and column names are already uppercased by OracleStore::new(). + // Column override values are uppercased by OracleStore::new(), but + // default logical names (returned by col() when no override is set) + // are lowercase — use .to_uppercase() for catalog lookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/oracle.rs` around lines 1053 - 1096, Update the misleading comment in alter_safety_identifier_column to clarify that only override values are uppercased by OracleStore::new() (via uppercase_for_oracle()), whereas col("safety_identifier") returns the lowercase default when no override exists so col_safety.to_uppercase() (col_upper) is necessary for the catalog lookup; reference col_safety, col_upper and the function name in the comment and explain both code paths briefly.
♻️ Duplicate comments (3)
data_connector/src/oracle.rs (2)
739-833: The 7-element tuple inlist_itemsis verbose but functional.The tuple type annotation spanning lines 740–748 is a readability concern as noted in the prior review. However, refactoring to a local struct is a purely cosmetic improvement and not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/oracle.rs` around lines 739 - 833, The seven-element tuple type for rows in list_items is hard to read; replace it with a small local struct or type alias (e.g. define a struct like ListRow { id: String, response_id: Option<String>, item_type: String, role: Option<String>, content: Option<String>, status: Option<String>, created_at: DateTime<Utc> } ) and change the variable rows from Vec<(...)> to Vec<ListRow>, update the out.push to push ListRow { ... } (using the same field names) and adjust any later uses that destructure or reference tuple elements to use the struct fields; locate this in the closure passed to self.store.execute and the variables id/resp_id/item_type/role/content_raw/status/created_at used when building out.
1032-1048: Index names for responses table are now derived from config — addresses the prior review concern.
{s.table}_PREV_IDXand{s.table}_USER_IDXreplace the previously hardcodedRESPONSES_PREV_IDX/RESPONSES_USER_IDX, preventing collisions with custom table names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/oracle.rs` around lines 1032 - 1048, The SQL strings passed to create_index_if_missing use format!("CREATE INDEX {prev_idx} ON {table}({prev})") and format!("CREATE INDEX {user_idx} ON {table}({safety})") but do not supply the named arguments, so the interpolations are wrong; update the two format! calls in the block that builds prev_idx and user_idx (variables prev_idx, user_idx, prev, safety, s.table and the create_index_if_missing calls) to provide the values—either use positional placeholders with the variables (e.g. "CREATE INDEX {} ON {}({})", prev_idx, s.table, prev) or named parameters (prev_idx=..., table=..., prev=...), and do the same for the user_idx SQL string so the generated index names and column names are correctly embedded.data_connector/src/schema.rs (1)
34-44:TableConfig::default()still yields an invalid emptytable— butSchemaConfig::default()andvalidate()mitigate this.The
#[derive(Default)]onTableConfigproducestable: "", which will fail validation. SinceSchemaConfig::default()fills in proper names andvalidate()catches the empty case with a helpful message (line 141–142), this is safe in practice. However, a user providing a partialTableConfigin YAML (e.g.,conversations: { columns: { id: "MY_ID" } }) will hit this path — the error message is now clear thanks to the contextual formatting, so the current state is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data_connector/src/schema.rs` around lines 34 - 44, TableConfig currently derives Default producing an invalid empty table name; remove #[derive(Default)] from TableConfig and add a manual impl Default for TableConfig that returns a clear sentinel (e.g. table = "__UNSET__" and empty columns) so it's obvious when a TableConfig is uninitialized, then update SchemaConfig::default() (where it currently fills names) to replace that sentinel with the proper generated name, and ensure validate() on SchemaConfig still treats "__UNSET__" as invalid; reference TableConfig, the new impl Default for TableConfig, SchemaConfig::default(), and validate().
🤖 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/schema.rs`:
- Around line 89-94: The qualified_table() function currently only quotes the
table name when owner is present, causing differing case-sensitivity behavior;
update qualified_table (or decide and document) to always quote the table name
for consistent behavior by changing the None branch to return the table wrapped
in quotes (i.e., use a format that produces "\"{self.table}\""); also ensure any
internal double quotes in self.table are escaped before quoting so generated
identifiers remain valid across Postgres/Oracle.
---
Outside diff comments:
In `@data_connector/src/oracle.rs`:
- Around line 1053-1096: Update the misleading comment in
alter_safety_identifier_column to clarify that only override values are
uppercased by OracleStore::new() (via uppercase_for_oracle()), whereas
col("safety_identifier") returns the lowercase default when no override exists
so col_safety.to_uppercase() (col_upper) is necessary for the catalog lookup;
reference col_safety, col_upper and the function name in the comment and explain
both code paths briefly.
---
Duplicate comments:
In `@data_connector/src/oracle.rs`:
- Around line 739-833: The seven-element tuple type for rows in list_items is
hard to read; replace it with a small local struct or type alias (e.g. define a
struct like ListRow { id: String, response_id: Option<String>, item_type:
String, role: Option<String>, content: Option<String>, status: Option<String>,
created_at: DateTime<Utc> } ) and change the variable rows from Vec<(...)> to
Vec<ListRow>, update the out.push to push ListRow { ... } (using the same field
names) and adjust any later uses that destructure or reference tuple elements to
use the struct fields; locate this in the closure passed to self.store.execute
and the variables id/resp_id/item_type/role/content_raw/status/created_at used
when building out.
- Around line 1032-1048: The SQL strings passed to create_index_if_missing use
format!("CREATE INDEX {prev_idx} ON {table}({prev})") and format!("CREATE INDEX
{user_idx} ON {table}({safety})") but do not supply the named arguments, so the
interpolations are wrong; update the two format! calls in the block that builds
prev_idx and user_idx (variables prev_idx, user_idx, prev, safety, s.table and
the create_index_if_missing calls) to provide the values—either use positional
placeholders with the variables (e.g. "CREATE INDEX {} ON {}({})", prev_idx,
s.table, prev) or named parameters (prev_idx=..., table=..., prev=...), and do
the same for the user_idx SQL string so the generated index names and column
names are correctly embedded.
In `@data_connector/src/schema.rs`:
- Around line 34-44: TableConfig currently derives Default producing an invalid
empty table name; remove #[derive(Default)] from TableConfig and add a manual
impl Default for TableConfig that returns a clear sentinel (e.g. table =
"__UNSET__" and empty columns) so it's obvious when a TableConfig is
uninitialized, then update SchemaConfig::default() (where it currently fills
names) to replace that sentinel with the proper generated name, and ensure
validate() on SchemaConfig still treats "__UNSET__" as invalid; reference
TableConfig, the new impl Default for TableConfig, SchemaConfig::default(), and
validate().
… columns What changed: - data_connector/src/schema.rs: renamed uppercase_table_names() to uppercase_for_oracle() and extended it to also uppercase column override values in the columns HashMap, so catalog lookups like user_tab_columns match Oracle's uppercase entries - data_connector/src/oracle.rs: derive index names from configured table name (e.g. RESPONSES_PREV_IDX, RESPONSES_USER_IDX, CONVERSATION_ITEM_LINKS_CONV_IDX) instead of hardcoding, preventing collisions when multiple schemas coexist; also derive PK constraint name (PK_CONVERSATION_ITEM_LINKS) from table name Why: PR review identified that hardcoded index names like responses_prev_idx would collide if two schemas use different table names in the same Oracle database. Also, column override values were not uppercased, creating a mismatch when Oracle catalog queries compare column names. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…ms closure Replace the 7-element tuple (String, Option<String>, String, ...) with direct ConversationItem construction inside the Oracle list_items closure. This eliminates the verbose type annotation and the separate post-processing .map() iterator, making the code easier to read. The JSON content parsing now happens inside the closure alongside the other row field extractions, matching the pattern used by build_response_from_row. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…f silent break Change get_response_chain cycle detection from silently breaking the loop to returning ResponseStorageError::InvalidChain with the offending response ID. This makes data corruption (self-referencing or cyclic previous_response_id links) visible to callers rather than returning a truncated chain that looks valid. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
added a commit
that referenced
this pull request
Feb 25, 2026
Add StorageHook trait, task-local context bridge, and hooked storage wrappers that intercept all storage operations with before/after hooks. What changed: - data_connector/src/hooks.rs: new StorageHook trait with before()/after() methods, StorageOperation enum (16 variants), BeforeHookResult (Continue/Reject), ExtraColumns type alias, HookError - data_connector/src/context.rs: RequestContext (per-request key-value bag) and ExtraColumns task-local bridge via tokio::task_local!; exposes with_request_context/current_request_context and with_extra_columns/current_extra_columns - data_connector/src/hooked.rs: HookedConversationStorage, HookedResponseStorage, HookedConversationItemStorage decorator wrappers; write operations scope ExtraColumns via task-local around inner backend calls; hook errors are non-fatal (logged, operation continues) unless explicitly Rejected - data_connector/src/factory.rs: StorageFactoryConfig gains hook: Option<Arc<dyn StorageHook>>; create_storage() wraps backends in Hooked*Storage when hook is provided - data_connector/src/lib.rs: module declarations and re-exports for context, hooks, hooked modules - model_gateway/src/app_context.rs: pass hook: None to StorageFactoryConfig Why: Storage hooks allow injecting custom logic (audit logging, multi-tenancy, PII redaction, validation) before and after storage operations without forking the codebase. The task-local bridge avoids changing any public storage trait signatures while still passing hook-provided data to backends. How: Decorator pattern wraps inner backends. Task-local storage bridges ExtraColumns from hooks to backends without trait signature changes. Hook errors use warn!+continue semantics to prevent hooks from accidentally breaking storage operations (explicit Reject required). Refs: #526 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
added a commit
that referenced
this pull request
Feb 25, 2026
…ll backends Add ColumnDef, extra_columns, skip_columns to SchemaConfig and wire them through DDL, INSERT, and SELECT paths in Postgres, Oracle, and Redis. What changed: - data_connector/src/schema.rs: add ColumnDef (sql_type + default_value), extra_columns: HashMap<String, ColumnDef> and skip_columns: HashSet on TableConfig; add is_skipped() method; validate_sql_type() with allowlisted chars to prevent SQL injection; uppercase_for_oracle() handles extra/skip column names; 18 new tests - data_connector/src/common.rs: add extra_column_defs(), sorted_extra_column_names(), value_to_sql_string(), resolve_extra_column_values() helpers; build_response_select_base() respects skip_columns; 10 new tests - data_connector/src/postgres.rs: DDL conditionally omits skipped columns and appends extra column defs for all 4 tables (conversations, items, links, responses); INSERT dynamically builds column/param lists respecting skip_columns and appending extra columns from current_extra_columns(); SELECT and row parsing use is_skipped() guards with sensible defaults - data_connector/src/oracle.rs: same DDL/INSERT/SELECT changes as Postgres; captures current_extra_columns() before spawn_blocking since task-locals don't propagate to blocking threads - data_connector/src/redis.rs: write paths skip hset for skipped columns and append extra columns from hooks; read paths (build_item_from_map, build_response_from_map, get_conversation) use defaults for skipped fields; HashMap import consolidated Why: extra_columns enables hooks to persist custom fields (tenant ID, audit trail) alongside core data without schema changes. skip_columns enables adapting to existing schemas that lack certain standard columns. Together they make the storage layer flexible enough to fit into enterprise environments without forking. How: DDL: filter out skipped core columns, append extra column defs. INSERT: build column/param lists dynamically, append extra column values resolved via hook > default > NULL priority chain. SELECT: omit skipped columns from queries, use defaults in row parsing. Oracle: capture task-local extra columns before spawn_blocking boundary. Redis: skip hset/hget for skipped fields, append extra as hash fields. Refs: #526 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
added a commit
that referenced
this pull request
Feb 25, 2026
Add WIT interface, wasmtime bindings, and WasmStorageHook bridge for running storage hooks as sandboxed WASM components. Includes a working guest example and updated data_connector README. What changed: - wasm/src/interface/storage/storage-hooks.wit: WIT interface defining storage-hook-types (ContextEntry, Operation enum, ExtraColumn, BeforeResult variant), storage-hook-before and storage-hook-after export interfaces, and storage-hook world - wasm/src/storage_spec.rs: wasmtime component::bindgen! for the storage-hook world with async imports/exports - wasm/src/storage_hook.rs: WasmStorageHook struct implementing StorageHook trait; compiles WASM component once at construction, instantiates per call with 10MB memory limit; type conversions between Rust StorageOperation/ExtraColumns and WIT types; 5 unit tests - wasm/Cargo.toml: add storage-hooks feature flag gating smg-data-connector, async-trait, serde_json as optional deps - wasm/src/lib.rs: feature-gated module declarations and re-export of WasmStorageHook - examples/wasm/wasm-guest-storage-hook/: complete guest example demonstrating multi-tenancy (tenant_id from context) and audit trail (created_by) extra columns; includes Cargo.toml, src/lib.rs, build.sh, README.md - data_connector/README.md: document StorageHook trait, wiring, request context, extra columns (with YAML config example and resolution order), skip columns, and WASM storage hooks Why: WASM storage hooks enable sandboxed, language-agnostic hook implementations for teams that cannot or prefer not to write Rust. The Component Model provides strong isolation with memory limits. The example guest demonstrates the primary use cases (multi-tenancy, audit trail). How: Separate WIT world (storage-hook) avoids polluting the existing smg middleware world. Feature-gated behind storage-hooks to keep the WASM crate lean by default. Each before()/after() call creates a fresh WASM store instance for isolation. Type conversions bridge between Rust domain types and WIT flat types. Refs: #526 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
This was referenced Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a YAML-driven schema configuration layer to the
data_connectorcrate so users can customize table names, column names, and schema owner prefixes at runtime — without forking the repository. When noSchemaConfigis provided, all backends produce identical SQL/keys to the previous hardcoded behavior. Zero behavioral change for existing users.Phase 1 of the schema configuration initiative.
What changed
New file:
data_connector/src/schema.rs—SchemaConfigandTableConfigtypes with validation, column remapping (col()), owner-prefixed table names (qualified_table()), serde support, and 15 unit testsBackend refactors:
data_connector/src/oracle.rs— All SQL strings useschema.col()andqualified_table().SchemaInitFnsignature changed to accept&SchemaConfig.Arc<SchemaConfig>stored onOracleStoredata_connector/src/postgres.rs— All SQL strings refactored.SELECT *replaced with explicit column lists.Arc<SchemaConfig>stored onPostgresStoredata_connector/src/redis.rs— Key patterns use owner prefix, hash field names usecol().Arc<SchemaConfig>stored onRedisStoreShared code extraction:
data_connector/src/common.rs— ExtractedRESPONSE_COLUMNSconst andbuild_response_select_base()from Oracle/Postgres backends (single source of truth)data_connector/src/core.rs—get_response_chain()promoted to default trait method onResponseStorage, eliminating 75 lines of identical code across 3 backendsConfig and plumbing:
data_connector/src/config.rs— Addedschema: Option<SchemaConfig>toOracleConfig,PostgresConfig,RedisConfigdata_connector/src/lib.rs— Addedpub mod schemaand re-exportsbindings/python/src/lib.rs— Addedschema: Noneto Python config conversionsmodel_gateway/src/main.rs— Addedschema: Noneto backend config constructionmodel_gateway/tests/routing/test_openai_routing.rs— Addedschema: Noneto test configDocumentation:
data_connector/README.md— Documented schema configuration with YAML examples, type reference table, and key behaviorsWhy
Internal teams were forced to fork the repo (~800 lines of changes) just to use different column names for their database schema. This enables runtime schema customization via YAML/JSON config, eliminating the need to fork.
How
SchemaConfigstored asArcon each backendStorestruct, shared across all three storage trait implementations per backendcol()returns the remapped column name or passes through the logical name unchanged (zero-cost for default config)qualified_table()returnsOWNER."TABLE"for Oracle or just the table name otherwise[a-zA-Z0-9_]+, rejected before any queries executeowner(key prefix) andcolumns(hash field names) affect behavior; thetableconfig field is ignored for Redis key patterns (keys always use hardcoded entity names)Test plan
cargo build -p data-connector— compiles cleanlycargo test -p data-connector— all 121 tests pass (existing + 15 new schema tests)cargo clippy -p data-connector— zero warningsSchemaConfigproduces identical SQL strings to previous hardcoded valuesSummary by CodeRabbit
New Features
Behavior
Documentation
Tests