refactor(data-connector): remove redundant fields from StoredResponse - #574
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRemoved dedicated response fields ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant Storage
participant DB
Client->>Gateway: send response (includes output/instructions/etc.)
Gateway->>Storage: build StoredResponse with raw_response = {"output": [...], ...}
Storage->>DB: INSERT/UPDATE (no dedicated output/metadata columns)
DB-->>Storage: OK
Storage-->>Gateway: persisted id
Gateway-->>Client: ack
Client->>Gateway: request conversation history
Gateway->>Storage: fetch StoredResponse
Storage->>DB: SELECT columns (raw_response included)
DB-->>Storage: row with raw_response JSON
Storage-->>Gateway: StoredResponse(raw_response)
Gateway->>Client: reconstruct history from raw_response["output"]
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)
Comment |
github-actions
Bot
added
grpc
key4ng
changed the title
Contributor
Summary of ChangesHello, 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 streamlines the internal representation of stored responses by centralizing various response-related data into a single, flexible 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 refactors the StoredResponse struct by removing dedicated fields for instructions, output, tool_calls, and metadata. These previously separate fields are now consolidated into the raw_response field, which stores the full raw JSON response. The changes include updating the StoredResponse definition, modifying data parsing and storage logic across Oracle, PostgreSQL, and Redis connectors, and adjusting related tests to access these values via raw_response["output"], raw_response["metadata"], etc. Database migrations (version 3) have been added for both Oracle and PostgreSQL to drop the redundant columns from their respective schemas, with safeguards to prevent dropping columns that are still in use via schema mappings or as extra columns. Review comments highlight an Insecure Direct Object Reference (IDOR) vulnerability in load_input_history, load_previous_messages, and load_conversation_history functions, recommending a dedicated pull request to address this cross-cutting security concern. Additionally, a suggestion was made to optimize the PostgreSQL migration by combining multiple DROP COLUMN statements into a single ALTER TABLE command for efficiency, which would also require updating the corresponding test case.
…redundant columns and updating raw_response handling This commit removes the `output`, `instructions`, `tool_calls`, and `metadata` fields from the `StoredResponse` struct and related parsing functions, consolidating response data into a single `raw_response` field. It also updates database migration scripts to drop the now-redundant columns from the schema. Tests and related code have been adjusted to reflect these changes, ensuring that output is accessed through `raw_response` instead. Signed-off-by: key4ng <rukeyang@gmail.com>
key4ng
force-pushed
the
keyang/db-redundant-field
branch
from
March 2, 2026 21:08
4963526 to
2dbf927
Compare
key4ng
marked this pull request as ready for review
March 2, 2026 21:08
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
…tatement for redundant columns This commit modifies the `pg_v3_up` function to consolidate the dropping of redundant columns into a single SQL statement. The function now checks for columns to drop and returns an empty vector if none are found. Additionally, the related test has been updated to reflect this change, ensuring it verifies the correct generation of the drop statement. Signed-off-by: key4ng <rukeyang@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 182-196: The test oracle_v3_up_skips_column_mapped_to_output
currently guards the content check with an if, allowing a silent pass when
oracle_v3_up returns no statements; change this to explicitly assert that the
returned stmts is non-empty (e.g., assert!(!stmts.is_empty(), "...")) and then
assert that stmts[0] does not contain "OUTPUT", so SchemaConfig setup and
oracle_v3_up behavior are validated instead of silently skipped.
- Around line 87-92: The current batch DROP generator uses a single ALTER TABLE
... DROP (cols) PL/SQL block (see the vector built from format! with
cols_to_drop) which causes ORA-00904 on a missing column to skip the entire
batch; change the generator to emit one PL/SQL drop block per column (iterate
cols_to_drop and produce a separate "BEGIN EXECUTE IMMEDIATE 'ALTER TABLE
{table} DROP (col)'; EXCEPTION WHEN OTHERS THEN IF SQLCODE != -904 THEN RAISE;
END IF; END;" for each col) so each missing column is ignored individually and
existing columns are still dropped, preserving idempotency for partial schema
states.
In `@data_connector/src/postgres_migrations.rs`:
- Around line 64-75: In pg_v3_up, the redundant field list ("output",
"metadata", "instructions", "tool_calls") is being treated as physical column
names; instead resolve each redundant field to its mapped physical name via
s.col(field) and then run the collision guards against that resolved name (check
s.columns.values() and s.extra_columns.keys() using the resolved column) and use
the resolved column in the ALTER TABLE DROP COLUMN IF EXISTS string; apply the
same change to the analogous block used later (the pg_v3_up redundant-columns
removal and the similar block at the other site) so we drop the actual mapped
physical columns rather than the logical field names.
In `@model_gateway/tests/routing/test_openai_routing.rs`:
- Around line 563-567: The test contains duplicated assertions for
stored.raw_response["metadata"]["topic"] == "unicorns" and
stored.raw_response["instructions"] == "Be kind"; remove the earlier duplicate
block (the assertions around stored.raw_response["metadata"]["topic"] and
stored.raw_response["instructions"] at lines 563–567) so only the later checks
remain, leaving a single assertion pair validating
stored.raw_response["metadata"]["topic"] and stored.raw_response["instructions"]
in the test containing the variable stored.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
data_connector/src/common.rsdata_connector/src/core.rsdata_connector/src/hooked.rsdata_connector/src/memory.rsdata_connector/src/oracle.rsdata_connector/src/oracle_migrations.rsdata_connector/src/postgres.rsdata_connector/src/postgres_migrations.rsdata_connector/src/redis.rsdata_connector/src/schema.rsmodel_gateway/src/routers/grpc/harmony/responses/common.rsmodel_gateway/src/routers/grpc/regular/responses/common.rsmodel_gateway/src/routers/openai/router.rsmodel_gateway/src/routers/persistence_utils.rsmodel_gateway/tests/api/api_endpoints_test.rsmodel_gateway/tests/routing/test_openai_routing.rs
💤 Files with no reviewable changes (1)
- data_connector/src/schema.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
data_connector/src/postgres_migrations.rs (1)
60-82:⚠️ Potential issue | 🟠 Major
pg_v3_upstill doesn't resolve field names to physical column names.The issue flagged in the prior review remains: for custom mappings like
columns["output"] = "resp_output", this code attempts to drop the literaloutputcolumn instead of the actual physical columnresp_output. Uses.col(field)to resolve each redundant field to its physical name before applying guards and generating the DROP statement.🤖 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 60 - 82, pg_v3_up is dropping logical field names (e.g., "output") instead of their physical column names when column mappings exist; update the logic to call s.col(field) for each entry in the redundant list so guards and DROP statements use the resolved physical name. Specifically, for each field in redundant (the array in pg_v3_up) call s.col(field) to get the actual column name, skip the drop if that resolved name is present in s.columns or s.extra_columns (using the same eq_ignore_ascii_case checks), and generate the DROP COLUMN IF EXISTS using the resolved physical name when building cols_to_drop and the final ALTER TABLE statement.
🤖 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/postgres_migrations.rs`:
- Around line 167-182: Rename the test pg_v3_up_skips_column_mapped_to_output to
pg_v3_up_skips_when_output_is_used_by_another_field to accurately reflect that
it covers the case where another field maps to the physical column "output";
then add a new test that sets
schema.responses.columns.insert("output".to_string(), "resp_output".to_string())
and calls pg_v3_up(&schema) asserting that the generated statement drops/handles
the mapped physical column ("resp_output") appropriately (e.g., contains
expected DROP/EXISTS logic for resp_output and not the logical "output"),
referencing SchemaConfig, responses.columns, and pg_v3_up to locate and
implement the new test.
---
Duplicate comments:
In `@data_connector/src/postgres_migrations.rs`:
- Around line 60-82: pg_v3_up is dropping logical field names (e.g., "output")
instead of their physical column names when column mappings exist; update the
logic to call s.col(field) for each entry in the redundant list so guards and
DROP statements use the resolved physical name. Specifically, for each field in
redundant (the array in pg_v3_up) call s.col(field) to get the actual column
name, skip the drop if that resolved name is present in s.columns or
s.extra_columns (using the same eq_ignore_ascii_case checks), and generate the
DROP COLUMN IF EXISTS using the resolved physical name when building
cols_to_drop and the final ALTER TABLE statement.
…dually and update tests This commit refines the `oracle_v3_up` function to drop redundant columns one at a time, ensuring that the absence of a column does not prevent others from being dropped. The filtering logic has been improved to check for column mappings and extra columns more effectively. Corresponding tests have been updated to reflect the new behavior, verifying that each redundant column is handled correctly. Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 79-84: The skip logic incorrectly treats any other mapped field
(mapped_by_other_field) or extra column (used_as_extra) as a blocker even if
that other field/extra is also being dropped in the same migration; update the
condition so when computing mapped_by_other_field over s.columns and
used_as_extra over s.extra_columns you ignore keys that are in the current
migration's removal set (e.g., fields_being_removed / redundant_fields) — i.e.,
only count a mapping/extra as a blocker if the other key is not also scheduled
for removal; adjust the function signature or capture the existing removal list
and use it when evaluating mapped_by_other_field and used_as_extra.
In `@data_connector/src/postgres_migrations.rs`:
- Around line 68-83: The filter currently prevents dropping a physical column if
any other logical field maps to it—even if that other field is itself
redundant—causing shared redundant fields to block each other; update the
mapping check inside cols_to_drop so mapped_by_other_field only considers
non-redundant fields (i.e., ignore keys that are in the redundant set) when
testing s.columns for a value equal to s.col(field), while keeping the
used_as_extra check on s.extra_columns unchanged; locate the logic around
cols_to_drop, redundant, s.col, s.columns, and s.extra_columns and modify the
any(...) predicate to exclude keys present in redundant (and still
case-insensitively compare v to col).
In `@model_gateway/tests/routing/test_openai_routing.rs`:
- Line 490: The test seeds previous.raw_response["output"] as a string but the
suite expects the canonical array-of-output-items shape; update the seeded value
so previous.raw_response contains "output" as an array matching the persisted
response shape used elsewhere in tests (e.g., an array of output items/objects
rather than a bare string) so history/streaming fixtures mirror production
payloads and exercise load_input_history's array-handling.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
data_connector/src/oracle_migrations.rsdata_connector/src/postgres_migrations.rsmodel_gateway/tests/routing/test_openai_routing.rs
…d pg_v3_up functions This commit improves the column filtering logic in both the `oracle_v3_up` and `pg_v3_up` functions to exclude redundant fields when determining which columns to drop. The updated logic ensures that only non-redundant columns are considered for dropping, enhancing the accuracy of the migration scripts. Corresponding tests have been adjusted to validate these changes. Signed-off-by: key4ng <rukeyang@gmail.com>
slin1237
approved these changes
Mar 2, 2026
3 tasks done
This was referenced Mar 26, 2026
12 tasks done
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
2 participants
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.
Description
Problem
StoredResponsestores four fields that are redundant withraw_response:output(identical toraw_response["output"]),metadataandinstructions(write-only, never read back), andtool_calls(dead code, never populated).Solution
Remove all four fields from the struct, schema, storage backends, and read/write paths. Add v3 database migrations to drop the columns. Read paths
now access output via
raw_response.get("output").Changes
data_connector/src/core.rs: Remove 4 fields fromStoredResponse, updatebuild_context()to read fromraw_responsedata_connector/src/common.rs: Remove columns fromRESPONSE_COLUMNS, deleteparse_tool_calls()andparse_metadata()data_connector/src/schema.rs: Remove columns fromcore_columns_for("responses")data_connector/src/postgres.rs: Remove from DDL,build_response_from_row,store_responsedata_connector/src/oracle.rs: Samedata_connector/src/redis.rs: Samedata_connector/src/postgres_migrations.rs: Add v3 migration to drop 4 columnsdata_connector/src/oracle_migrations.rs: Add v3 migration to drop 4 columnsmodel_gateway/src/routers/persistence_utils.rs: Remove redundant field assignments in write pathmodel_gateway/src/routers/openai/router.rs: Read output fromraw_responseinstead ofstored.outputmodel_gateway/src/routers/grpc/{regular,harmony}/responses/common.rs: SameTest Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Refactor
Chores