test(oracle): add E2E tests for Flyway-managed Oracle schema - #576
Conversation
…r management scripts - Added support for Flyway-managed Oracle schema configuration, including a new YAML schema config file. - Implemented creation and cleanup scripts for an Oracle Flyway user in the CI workflow. - Updated the Router and Gateway classes to handle the new schema configuration. - Enhanced the Python bindings to include schema configuration as an argument. - Added tests for state management with the new oracle-custom backend. This change improves the flexibility of schema management in Oracle environments. Signed-off-by: key4ng <rukeyang@gmail.com>
📝 WalkthroughWalkthroughThis PR introduces schema configuration support across the codebase to enable Oracle Flyway-managed table and column remapping. It adds CLI arguments, configuration loading infrastructure, and new Flyway SQL migration scripts that define conversation and response storage tables for Oracle backends. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Config as Config Loader
participant Router as Router/Gateway
participant Backend as Backend Config<br/>(Oracle/Postgres/Redis)
User->>Config: Provide schema_config path & other args
Config->>Config: Load YAML schema from path
Config->>Config: Parse into SchemaConfig object
Config->>Router: Initialize Router with SchemaConfig
Router->>Router: Call to_router_config()
Router->>Backend: Pass SchemaConfig to Oracle builder
Router->>Backend: Pass SchemaConfig to Postgres builder
Router->>Backend: Pass SchemaConfig to Redis builder
Backend->>Backend: Augment config with schema<br/>(table/column remapping)
Router->>Router: Construct RouterConfig with<br/>schema-aware backends
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
github-actions
Bot
added
python-bindings
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 significantly enhances the flexibility and management of Oracle database schemas within the system. By introducing Flyway-managed schema support, it allows for dynamic configuration of table and column mappings through a YAML file, which is crucial for environments requiring custom schema layouts. The changes also include robust CI tooling for managing dedicated Flyway users and comprehensive test coverage for the new 'oracle-custom' backend, ensuring reliable operation and easier integration with diverse Oracle setups. Highlights
Changelog
Ignored Files
Activity
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}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Flyway-managed Oracle schemas, allowing for more flexible schema management. The changes include updates to the Router and Gateway to handle schema configuration from a YAML file, new CI scripts for managing a Flyway user in Oracle, and corresponding updates to Python bindings and tests.
My review focuses on improving code structure and maintainability. I've identified some code duplication for loading schema configurations and an opportunity to make the schema application logic more efficient by avoiding unnecessary cloning. These suggestions aim to make the codebase cleaner and more robust.
| let schema = if let Some(ref path) = self.schema_config { | ||
| let content = std::fs::read_to_string(path).map_err(|e| { | ||
| config::ConfigError::ValidationFailed { | ||
| reason: format!("Failed to read schema config file '{}': {}", path, e), | ||
| } | ||
| })?; | ||
| let schema: config::SchemaConfig = | ||
| serde_yaml::from_str(&content).map_err(|e| { | ||
| config::ConfigError::ValidationFailed { | ||
| reason: format!("Failed to parse schema config file '{}': {}", path, e), | ||
| } | ||
| })?; | ||
| Some(schema) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
This schema loading logic is duplicated in model_gateway/src/main.rs (inside load_schema_config). To improve maintainability and avoid code duplication, consider extracting this logic into a shared function. A good place for this would be within the smg::config module, for example, as pub fn load_schema_from_path(path: &str) -> ConfigResult<SchemaConfig>.
References
- Extract duplicated logic into a shared helper function to improve maintainability and reduce redundancy.
There was a problem hiding this comment.
Appreciate the suggestion, but these two callers live in separate entry-point crates (CLI binary vs Python bindings). The logic is ~6 lines of straightforward read-file-then-parse glue — extracting it would add public API surface to the config module for something only used at initialization. Keeping it inline in each caller is fine here.
| let oracle = if history_backend == HistoryBackend::Oracle { | ||
| Some(self.build_oracle_config()?) | ||
| Some(self.build_oracle_config(schema.clone())?) | ||
| } else { | ||
| None | ||
| }; | ||
| let postgres = if history_backend == HistoryBackend::Postgres { | ||
| Some(self.build_postgres_config()?) | ||
| Some(self.build_postgres_config(schema.clone())?) | ||
| } else { | ||
| None | ||
| }; | ||
| let redis = if history_backend == HistoryBackend::Redis { | ||
| Some(self.build_redis_config()?) | ||
| Some(self.build_redis_config(schema)?) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
The current logic for applying the schema configuration involves cloning the schema for both Oracle and Postgres backends. Since only one history backend can be active at a time, these clones are unnecessary. You can refactor this to avoid cloning by using a match statement to construct a tuple of the backend configs, moving the schema into the correct one. This will make the code more efficient and cleaner.
let (oracle, postgres, redis) = match history_backend {
HistoryBackend::Oracle => (Some(self.build_oracle_config(schema)?), None, None),
HistoryBackend::Postgres => (None, Some(self.build_postgres_config(schema)?), None),
HistoryBackend::Redis => (None, None, Some(self.build_redis_config(schema)?)),
_ => (None, None, None),
};References
- Refactor
matchstatements to avoid duplication. When arms have common logic, use thematchto return the differing value and perform the common logic once.
There was a problem hiding this comment.
Good catch — since only one history backend is active at a time, the clones are unnecessary. Refactored to a single match statement. 👍
key4ng
changed the title
key4ng
changed the title
- Modified the test suite to focus on OpenAI only for the base class. - Introduced a new test class for xAI cloud API, inheriting from the OpenAI tests. - Updated documentation strings to reflect the changes in backend focus. This refactor enhances clarity and organization of the state management tests for different cloud backends. Signed-off-by: key4ng <rukeyang@gmail.com>
…g file handling - Updated error messages in the Router and CLI argument handling to use more concise formatting with Rust's string interpolation. - Changed schema assignment in Oracle and Postgres configurations to use `clone_from` for better performance. These changes enhance code readability and maintainability while improving error reporting for schema configuration issues. Signed-off-by: key4ng <rukeyang@gmail.com>
- Refactored the state management test classes to introduce a base class for shared functionality. - Updated the OpenAI and xAI test classes to inherit from the new base class, improving code reuse. - Enhanced documentation strings to clarify the purpose of each test class. This restructuring improves the organization and maintainability of the test suite for cloud backends. Signed-off-by: key4ng <rukeyang@gmail.com>
…reading - Simplified error handling in the Router and CLI argument loading functions by reducing nested structures. - Improved readability of error messages related to schema configuration file parsing. These changes enhance code clarity and maintainability in schema configuration handling. Signed-off-by: key4ng <rukeyang@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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/python/Cargo.toml (1)
30-35:⚠️ Potential issue | 🟡 MinorProfile section has no effect in non-root package.
The pipeline warning indicates this
[profile.ci]section will be ignored. Cargo only reads profile configurations from the workspace rootCargo.toml. Consider moving this to the workspace root or removing it from this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/python/Cargo.toml` around lines 30 - 35, The [profile.ci] section in this Cargo.toml has no effect because Cargo only reads profile settings from the workspace root; either remove the [profile.ci] block here or move its exact contents (inherits = "release", opt-level = 2, lto = "thin", codegen-units = 16, strip = true) into the workspace root Cargo.toml so Cargo will apply the profile; update or delete the local [profile.ci] block accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/infra/gateway.py`:
- Around line 307-313: The code appends a relative schema-config path
("scripts/oracle_flyway/schema-config.yaml") to mode_args when history_backend
== "oracle-custom", which makes startup CWD-dependent; change this to compute
and pass an absolute path (e.g. resolve the repo-root-relative path using the
current module's location or a known repo root) before extending mode_args so
schema-config is always an absolute filesystem path; update the branch handling
in gateway.py where history_backend and mode_args are used to perform the
resolution and append the absolute path for the "schema-config" argument.
In `@scripts/ci_agentic_svc_deps.sh`:
- Around line 211-213: Multiple consecutive appends to "$GITHUB_ENV" (the three
echo lines writing ATP_FLYWAY_USER, ATP_FLYWAY_PASSWORD, ATP_FLYWAY_DSN) should
be grouped into a single append to avoid repeated redirects; replace the three
echo statements that write to "$GITHUB_ENV" with a single multiline append that
writes all three environment entries at once (e.g., a here-doc or combined
printf) so the writes for ATP_FLYWAY_USER, ATP_FLYWAY_PASSWORD and
ATP_FLYWAY_DSN are performed in one redirected block.
---
Outside diff comments:
In `@bindings/python/Cargo.toml`:
- Around line 30-35: The [profile.ci] section in this Cargo.toml has no effect
because Cargo only reads profile settings from the workspace root; either remove
the [profile.ci] block here or move its exact contents (inherits = "release",
opt-level = 2, lto = "thin", codegen-units = 16, strip = true) into the
workspace root Cargo.toml so Cargo will apply the profile; update or delete the
local [profile.ci] block accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/pr-test-rust.ymlbindings/python/Cargo.tomlbindings/python/src/lib.rsbindings/python/src/smg/router_args.pye2e_test/fixtures/hooks.pye2e_test/infra/gateway.pye2e_test/responses/test_state_management.pymodel_gateway/src/config/types.rsmodel_gateway/src/main.rsscripts/ci_agentic_svc_deps.shscripts/oracle_flyway/schema-config.yamlscripts/oracle_flyway/sql/V1__Create_responses_table.sqlscripts/oracle_flyway/sql/V2__Create_v2_conversations_and_alter_responses.sql
…feedback (#591) Signed-off-by: Keyang Ru <rukeyang@gmail.com>
2 participants
Description
Problem
SMG supports
--schema-configfor working with externally-managed Oracle tables (e.g. Flyway), but there are no automated tests verifying this works end-to-end.Solution
Add CI infrastructure to create an Oracle user with Flyway-like DDL tables, and run the existing state management tests against it using
--schema-config.Changes
scripts/ci_agentic_svc_deps.sh: Addcreate-oracle-flyway-userandcleanup-oracle-flyway-usercommands that create a second Oracle test user, execute V1+V2 DDL viaoracledbPython, and exportATP_FLYWAY_*env varsscripts/oracle_flyway/: Add CI-only Flyway SQL files (tables + indexes, no sweep procedures) andschema-config.yamle2e_test/infra/gateway.py: Handleoracle-customhistory backend — maps to--history-backend oraclewithATP_FLYWAY_*credentials and--schema-confige2e_test/fixtures/hooks.py: Registeroracle-customin storage marker help texte2e_test/responses/test_state_management.py: AddTestStateManagementOracleCustominheriting fromTestStateManagementCloud(OpenAI-only).github/workflows/pr-test-rust.yml: Add Flyway user setup/cleanup to Oracle stepsbindings/python/src/lib.rs: Addschema_configfield to Router, load YAML and apply to storage backend configsbindings/python/src/smg/router_args.py: Add--schema-configCLI argumentbindings/python/Cargo.toml: Addserde_yamldependency for schema config parsingSummary by CodeRabbit
Release Notes
New Features
Tests
Chores