From be520f31f445795b1e0adb0e01ae7c2917c98b14 Mon Sep 17 00:00:00 2001 From: andrew Date: Sat, 18 Apr 2026 00:30:46 +0300 Subject: [PATCH] Polish schema endpoint: rename show, align field name, add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #23, applied on top of the original commit: - Rename the CLI subcommand from `schema get` to `schema show` to match the existing `run show` / `commit show` convention. A `#[command(alias = "get")]` preserves muscle memory for anyone who already typed `get`. - Rename `SchemaGetOutput` → `SchemaOutput` and its field `source` → `schema_source`, so the get response and the apply request use the same field name for the same concept. - Use `println!` instead of `print!` in the CLI so the shell prompt doesn't land on the last line of schema output. - Add three integration tests on `/schema`: happy path (no auth), 401 when bearer is required but missing, 403 when the policy grants the actor branch_create but not read. Follow-ups left for a separate PR: include `schema_ir_hash` and `schema_identity_version` in the response payload so clients can do drift detection and the server can set an ETag; and a fast-path local read that skips `Omnigraph::open()` when only the schema source is needed. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/omnigraph-cli/src/main.rs | 17 ++--- crates/omnigraph-server/src/api.rs | 4 +- crates/omnigraph-server/src/lib.rs | 10 +-- crates/omnigraph-server/tests/server.rs | 89 ++++++++++++++++++++++++- 4 files changed, 104 insertions(+), 16 deletions(-) diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index 8c68759..c29cac6 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -18,7 +18,7 @@ use omnigraph_server::api::{ BranchCreateOutput, BranchCreateRequest, BranchDeleteOutput, BranchListOutput, BranchMergeOutput, BranchMergeRequest, ChangeOutput, ChangeRequest, CommitListOutput, CommitOutput, ErrorOutput, ExportRequest, IngestOutput, IngestRequest, ReadOutput, ReadRequest, - RunListOutput, RunOutput, SchemaApplyOutput, SchemaApplyRequest, SchemaGetOutput, SnapshotOutput, + RunListOutput, RunOutput, SchemaApplyOutput, SchemaApplyRequest, SchemaOutput, SnapshotOutput, SnapshotTableOutput, commit_output, ingest_output, read_output, run_output, schema_apply_output, snapshot_payload, }; @@ -303,8 +303,9 @@ enum SchemaCommand { #[arg(long)] json: bool, }, - /// Get the current accepted schema source - Get { + /// Show the current accepted schema source + #[command(alias = "get")] + Show { /// Repo URI uri: Option, #[arg(long)] @@ -2014,7 +2015,7 @@ async fn main() -> Result<()> { print_schema_apply_human(&output); } } - SchemaCommand::Get { + SchemaCommand::Show { uri, target, config, @@ -2025,7 +2026,7 @@ async fn main() -> Result<()> { resolve_remote_bearer_token(&config, uri.as_deref(), target.as_deref())?; let uri = resolve_uri(&config, uri, target.as_deref())?; let output = if is_remote_uri(&uri) { - remote_json::( + remote_json::( &http_client, Method::GET, remote_url(&uri, "/schema"), @@ -2035,14 +2036,14 @@ async fn main() -> Result<()> { .await? } else { let db = Omnigraph::open(&uri).await?; - SchemaGetOutput { - source: db.schema_source().to_string(), + SchemaOutput { + schema_source: db.schema_source().to_string(), } }; if json { print_json(&output)?; } else { - print!("{}", output.source); + println!("{}", output.schema_source); } } }, diff --git a/crates/omnigraph-server/src/api.rs b/crates/omnigraph-server/src/api.rs index ac5cd82..61770df 100644 --- a/crates/omnigraph-server/src/api.rs +++ b/crates/omnigraph-server/src/api.rs @@ -281,8 +281,8 @@ pub struct SchemaApplyOutput { } #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] -pub struct SchemaGetOutput { - pub source: String, +pub struct SchemaOutput { + pub schema_source: String, } #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index adef1c4..52d2718 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -14,7 +14,7 @@ use api::{ BranchMergeOutput, BranchMergeRequest, ChangeOutput, ChangeRequest, CommitListOutput, CommitListQuery, ErrorCode, ErrorOutput, ExportRequest, HealthOutput, IngestOutput, IngestRequest, ReadOutput, ReadRequest, RunListOutput, SchemaApplyOutput, SchemaApplyRequest, - SchemaGetOutput, SnapshotQuery, ingest_output, schema_apply_output, snapshot_payload, + SchemaOutput, SnapshotQuery, ingest_output, schema_apply_output, snapshot_payload, }; use axum::body::{Body, Bytes}; use axum::extract::DefaultBodyLimit; @@ -803,7 +803,7 @@ async fn server_change( path = "/schema", tag = "schema", responses( - (status = 200, description = "Current schema source", body = SchemaGetOutput), + (status = 200, description = "Current schema source", body = SchemaOutput), (status = 401, description = "Unauthorized", body = ErrorOutput), (status = 403, description = "Forbidden", body = ErrorOutput), ), @@ -812,7 +812,7 @@ async fn server_change( async fn server_schema_get( State(state): State, actor: Option>, -) -> std::result::Result, ApiError> { +) -> std::result::Result, ApiError> { authorize_request( &state, actor.as_ref().map(|Extension(actor)| actor), @@ -826,11 +826,11 @@ async fn server_schema_get( target_branch: None, }, )?; - let source = { + let schema_source = { let db = Arc::clone(&state.db).read_owned().await; db.schema_source().to_string() }; - Ok(Json(SchemaGetOutput { source })) + Ok(Json(SchemaOutput { schema_source })) } #[utoipa::path( diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index feebdc6..7a00c51 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -10,7 +10,7 @@ use omnigraph::db::{Omnigraph, ReadTarget}; use omnigraph::loader::{LoadMode, load_jsonl}; use omnigraph_server::api::{ BranchCreateRequest, BranchMergeRequest, ChangeRequest, ErrorOutput, ExportRequest, - IngestRequest, ReadRequest, SchemaApplyRequest, + IngestRequest, ReadRequest, SchemaApplyRequest, SchemaOutput, }; use omnigraph_server::{AppState, build_app}; use serde_json::{Value, json}; @@ -1042,6 +1042,93 @@ async fn snapshot_route_returns_manifest_dataset_version() { assert!(snapshot_body["tables"].is_array()); } +#[tokio::test(flavor = "multi_thread")] +async fn schema_route_returns_current_source() { + let (_temp, app) = app_for_loaded_repo().await; + let (status, body) = json_response( + &app, + Request::builder() + .uri("/schema") + .method(Method::GET) + .body(Body::empty()) + .unwrap(), + ) + .await; + + assert_eq!(status, StatusCode::OK); + let output: SchemaOutput = serde_json::from_value(body).unwrap(); + assert!(output.schema_source.contains("node Person")); +} + +#[tokio::test(flavor = "multi_thread")] +async fn schema_route_requires_bearer_token_when_auth_configured() { + let (_temp, app) = app_for_loaded_repo_with_auth("demo-token").await; + + let (missing_status, missing_body) = json_response( + &app, + Request::builder() + .uri("/schema") + .method(Method::GET) + .body(Body::empty()) + .unwrap(), + ) + .await; + let missing_error: ErrorOutput = serde_json::from_value(missing_body).unwrap(); + assert_eq!(missing_status, StatusCode::UNAUTHORIZED); + assert_eq!( + missing_error.code, + Some(omnigraph_server::api::ErrorCode::Unauthorized) + ); + + let (ok_status, ok_body) = json_response( + &app, + Request::builder() + .uri("/schema") + .method(Method::GET) + .header("authorization", "Bearer demo-token") + .body(Body::empty()) + .unwrap(), + ) + .await; + assert_eq!(ok_status, StatusCode::OK); + let output: SchemaOutput = serde_json::from_value(ok_body).unwrap(); + assert!(!output.schema_source.is_empty()); +} + +#[tokio::test(flavor = "multi_thread")] +async fn schema_route_denied_when_actor_lacks_read_permission() { + let temp = init_loaded_repo().await; + let repo = repo_path(temp.path()); + let policy_path = temp.path().join("policy.yaml"); + // Policy grants branch_create only — no read action for act-bruno. + fs::write(&policy_path, INGEST_CREATE_ONLY_POLICY_YAML).unwrap(); + let state = AppState::open_with_bearer_tokens_and_policy( + repo.to_string_lossy().to_string(), + vec![("act-bruno".to_string(), "team-token".to_string())], + Some(&policy_path), + ) + .await + .unwrap(); + let app = build_app(state); + + let (status, body) = json_response( + &app, + Request::builder() + .uri("/schema") + .method(Method::GET) + .header("authorization", "Bearer team-token") + .body(Body::empty()) + .unwrap(), + ) + .await; + let error: ErrorOutput = serde_json::from_value(body).unwrap(); + assert_eq!(status, StatusCode::FORBIDDEN); + assert_eq!( + error.code, + Some(omnigraph_server::api::ErrorCode::Forbidden) + ); +} + #[tokio::test(flavor = "multi_thread")] async fn policy_blocks_change_on_protected_main_but_allows_unprotected_branch() { let temp = init_loaded_repo().await;