mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-12 01:45:14 +02:00
fix(MR-656): address Devin Review findings on /query and /change
Two issues raised by Devin Review on PR #110: 1. `POST /query` mutation-rejection error pointed at the deprecated `/change` endpoint instead of the canonical `/mutate`. Fixed in three places: the runtime error message in `server_query`, the utoipa 400-response description, and the handler doc comment. The `QueryRequest` schema docstrings in `api.rs` got the same update so the openapi.json bodies match. Server and openapi tests updated. 2. `execute_change_remote` serialized `ChangeRequest` directly, which emits the new canonical field names `query` / `name` on the wire. `#[serde(alias = "query_source")]` only affects deserialization, so a newer CLI talking to an older server would have its `/change` POST body fail with "missing field: query_source". Fixed by extracting a `legacy_change_request_body` helper that hand-rolls the JSON with the legacy keys (`query_source` / `query_name`), the same byte-stable contract `execute_read_remote` already uses against `/read`. Added two unit tests on the helper to lock the wire shape in. Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
This commit is contained in:
parent
a3e1b27a63
commit
0949f28794
6 changed files with 103 additions and 25 deletions
|
|
@ -18,7 +18,7 @@ use omnigraph_compiler::{
|
|||
};
|
||||
use omnigraph_server::api::{
|
||||
BranchCreateOutput, BranchCreateRequest, BranchDeleteOutput, BranchListOutput,
|
||||
BranchMergeOutput, BranchMergeRequest, ChangeOutput, ChangeRequest, CommitListOutput,
|
||||
BranchMergeOutput, BranchMergeRequest, ChangeOutput, CommitListOutput,
|
||||
CommitOutput, ErrorOutput, ExportRequest, IngestOutput, IngestRequest, ReadOutput, ReadRequest,
|
||||
SchemaApplyOutput, SchemaApplyRequest, SchemaOutput, SnapshotOutput, SnapshotTableOutput,
|
||||
commit_output, ingest_output, read_output, schema_apply_output, snapshot_payload,
|
||||
|
|
@ -1636,6 +1636,33 @@ async fn execute_change(
|
|||
})
|
||||
}
|
||||
|
||||
/// Build the JSON body for `POST /change` using the legacy wire shape.
|
||||
///
|
||||
/// `ChangeRequest`'s Rust field names are now `query` / `name` (the canonical
|
||||
/// wire shape going forward), but old `omnigraph-server` builds still require
|
||||
/// the legacy `query_source` / `query_name` keys on `/change`. Hand-rolling
|
||||
/// the JSON with the legacy names keeps a newer CLI talking to an older
|
||||
/// server intact -- the same byte-stability contract we apply to
|
||||
/// `execute_read_remote` against `/read`.
|
||||
fn legacy_change_request_body(
|
||||
query_source: &str,
|
||||
query_name: Option<&str>,
|
||||
branch: &str,
|
||||
params_json: Option<&Value>,
|
||||
) -> Value {
|
||||
let mut body = serde_json::json!({
|
||||
"query_source": query_source,
|
||||
"branch": branch,
|
||||
});
|
||||
if let Some(name) = query_name {
|
||||
body["query_name"] = Value::String(name.to_string());
|
||||
}
|
||||
if let Some(params) = params_json {
|
||||
body["params"] = params.clone();
|
||||
}
|
||||
body
|
||||
}
|
||||
|
||||
async fn execute_change_remote(
|
||||
client: &reqwest::Client,
|
||||
uri: &str,
|
||||
|
|
@ -1649,12 +1676,12 @@ async fn execute_change_remote(
|
|||
client,
|
||||
Method::POST,
|
||||
remote_url(uri, "/change"),
|
||||
Some(serde_json::to_value(ChangeRequest {
|
||||
query: query_source.to_string(),
|
||||
name: query_name.map(ToOwned::to_owned),
|
||||
params: params_json.cloned(),
|
||||
branch: Some(branch.to_string()),
|
||||
})?),
|
||||
Some(legacy_change_request_body(
|
||||
query_source,
|
||||
query_name,
|
||||
branch,
|
||||
params_json,
|
||||
)),
|
||||
bearer_token,
|
||||
)
|
||||
.await
|
||||
|
|
@ -2627,14 +2654,62 @@ mod tests {
|
|||
use std::fs;
|
||||
|
||||
use super::{
|
||||
DEFAULT_BEARER_TOKEN_ENV, apply_bearer_token, bearer_token_from_env_file, load_cli_config,
|
||||
load_env_file_into_process, normalize_bearer_token, parse_env_assignment,
|
||||
resolve_remote_bearer_token,
|
||||
DEFAULT_BEARER_TOKEN_ENV, apply_bearer_token, bearer_token_from_env_file,
|
||||
legacy_change_request_body, load_cli_config, load_env_file_into_process,
|
||||
normalize_bearer_token, parse_env_assignment, resolve_remote_bearer_token,
|
||||
};
|
||||
use omnigraph_server::load_config;
|
||||
use reqwest::header::AUTHORIZATION;
|
||||
use serde_json::json;
|
||||
use tempfile::tempdir;
|
||||
|
||||
#[test]
|
||||
fn legacy_change_request_body_uses_legacy_field_names() {
|
||||
// `execute_change_remote` hits `POST /change`, which old
|
||||
// `omnigraph-server` builds deserialize as `ChangeRequest` with
|
||||
// **required** `query_source` and optional `query_name` keys.
|
||||
// Newer servers accept both spellings via serde alias, but a
|
||||
// newer CLI must still emit the legacy keys on the wire so it
|
||||
// can talk to an old server during a rolling upgrade.
|
||||
let body = legacy_change_request_body(
|
||||
"query insert_person($n: String) { insert Person { name: $n } }",
|
||||
Some("insert_person"),
|
||||
"main",
|
||||
Some(&json!({ "n": "Alice" })),
|
||||
);
|
||||
assert_eq!(
|
||||
body["query_source"].as_str(),
|
||||
Some("query insert_person($n: String) { insert Person { name: $n } }"),
|
||||
);
|
||||
assert_eq!(body["query_name"].as_str(), Some("insert_person"));
|
||||
assert_eq!(body["branch"].as_str(), Some("main"));
|
||||
assert_eq!(body["params"]["n"].as_str(), Some("Alice"));
|
||||
// Crucially, the **new** field names must NOT appear -- old
|
||||
// servers would silently treat them as unknown fields and then
|
||||
// fail on missing required `query_source`.
|
||||
assert!(
|
||||
body.get("query").is_none(),
|
||||
"legacy /change body must not carry the renamed `query` key; got {body}"
|
||||
);
|
||||
assert!(
|
||||
body.get("name").is_none(),
|
||||
"legacy /change body must not carry the renamed `name` key; got {body}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn legacy_change_request_body_omits_optional_fields_when_unset() {
|
||||
let body = legacy_change_request_body(
|
||||
"query find() { match { $p: Person } return { $p.name } }",
|
||||
None,
|
||||
"main",
|
||||
None,
|
||||
);
|
||||
assert_eq!(body["branch"].as_str(), Some("main"));
|
||||
assert!(body.get("query_name").is_none());
|
||||
assert!(body.get("params").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apply_bearer_token_adds_header_when_configured() {
|
||||
let client = reqwest::Client::new();
|
||||
|
|
|
|||
|
|
@ -252,13 +252,15 @@ pub struct ReadRequest {
|
|||
///
|
||||
/// Friendlier-named alternative to [`ReadRequest`] for ad-hoc reads and
|
||||
/// AI-agent integration. Mutations are rejected with 400 — use `POST
|
||||
/// /change` for write queries. Field names are deliberately short
|
||||
/// (`query`, `name`) to match the GQ keyword and the CLI `-e` flag.
|
||||
/// /mutate` (or its deprecated alias `POST /change`) for write queries.
|
||||
/// Field names are deliberately short (`query`, `name`) to match the GQ
|
||||
/// keyword and the CLI `-e` flag.
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, ToSchema)]
|
||||
pub struct QueryRequest {
|
||||
/// GQ read-query source. May declare one or more named queries; pick one
|
||||
/// with `name` when more than one is declared. Mutations
|
||||
/// (`insert`/`update`/`delete`) get 400 — use `POST /change` instead.
|
||||
/// (`insert`/`update`/`delete`) get 400 — use `POST /mutate` (or its
|
||||
/// deprecated alias `POST /change`) instead.
|
||||
#[schema(example = "query get_person($name: String) {\n match {\n $p: Person { name: $name }\n }\n return { $p.name, $p.age }\n}")]
|
||||
pub query: String,
|
||||
/// Name of the query to run when `query` declares multiple. Optional when
|
||||
|
|
|
|||
|
|
@ -1026,7 +1026,7 @@ async fn server_read(
|
|||
request_body = QueryRequest,
|
||||
responses(
|
||||
(status = 200, description = "Query results", body = ReadOutput),
|
||||
(status = 400, description = "Bad request - also returned when the query body contains mutations; use POST /change for write queries", body = ErrorOutput),
|
||||
(status = 400, description = "Bad request - also returned when the query body contains mutations; use POST /mutate (or its deprecated alias POST /change) for write queries", body = ErrorOutput),
|
||||
(status = 401, description = "Unauthorized", body = ErrorOutput),
|
||||
(status = 403, description = "Forbidden", body = ErrorOutput),
|
||||
),
|
||||
|
|
@ -1037,9 +1037,10 @@ async fn server_read(
|
|||
/// Designed for ad-hoc exploration and AI-agent tool-use: short field
|
||||
/// names (`query`, `name`) match the CLI `-e` flag and the GQ `query`
|
||||
/// keyword. Mutations (`insert`/`update`/`delete`) are rejected with 400
|
||||
/// -- use `POST /change` for write queries. Otherwise behaves
|
||||
/// identically to `POST /read`: same target semantics (branch xor
|
||||
/// snapshot), same Cedar action (Read), same response shape.
|
||||
/// -- use `POST /mutate` (or its deprecated alias `POST /change`) for
|
||||
/// write queries. Otherwise behaves identically to `POST /read`: same
|
||||
/// target semantics (branch xor snapshot), same Cedar action (Read),
|
||||
/// same response shape.
|
||||
async fn server_query(
|
||||
State(state): State<AppState>,
|
||||
actor: Option<Extension<AuthenticatedActor>>,
|
||||
|
|
@ -1080,7 +1081,7 @@ async fn server_query(
|
|||
.map_err(|err| ApiError::bad_request(err.to_string()))?;
|
||||
if !query_decl.mutations.is_empty() {
|
||||
return Err(ApiError::bad_request(format!(
|
||||
"query '{}' contains mutations (insert/update/delete); use POST /change for write queries",
|
||||
"query '{}' contains mutations (insert/update/delete); use POST /mutate for write queries",
|
||||
query_decl.name
|
||||
)));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -485,7 +485,7 @@ fn query_endpoint_documents_mutation_400() {
|
|||
let four_hundred = &doc["paths"]["/query"]["post"]["responses"]["400"];
|
||||
let description = four_hundred["description"].as_str().unwrap_or_default();
|
||||
assert!(
|
||||
description.contains("mutations") || description.contains("POST /change"),
|
||||
description.contains("mutations") || description.contains("POST /mutate"),
|
||||
"expected /query 400 response to mention mutation rejection, got: {description}"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2060,8 +2060,8 @@ async fn query_endpoint_rejects_mutation_with_400() {
|
|||
assert_eq!(status, StatusCode::BAD_REQUEST);
|
||||
let err = body["error"].as_str().unwrap_or_default();
|
||||
assert!(
|
||||
err.contains("contains mutations") && err.contains("POST /change"),
|
||||
"expected mutation-rejection message, got: {err}"
|
||||
err.contains("contains mutations") && err.contains("POST /mutate"),
|
||||
"expected mutation-rejection message pointing at canonical /mutate, got: {err}"
|
||||
);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -778,7 +778,7 @@
|
|||
"queries"
|
||||
],
|
||||
"summary": "Execute an inline read query (friendlier-named alternative to `POST /read`).",
|
||||
"description": "Designed for ad-hoc exploration and AI-agent tool-use: short field\nnames (`query`, `name`) match the CLI `-e` flag and the GQ `query`\nkeyword. Mutations (`insert`/`update`/`delete`) are rejected with 400\n-- use `POST /change` for write queries. Otherwise behaves\nidentically to `POST /read`: same target semantics (branch xor\nsnapshot), same Cedar action (Read), same response shape.",
|
||||
"description": "Designed for ad-hoc exploration and AI-agent tool-use: short field\nnames (`query`, `name`) match the CLI `-e` flag and the GQ `query`\nkeyword. Mutations (`insert`/`update`/`delete`) are rejected with 400\n-- use `POST /mutate` (or its deprecated alias `POST /change`) for\nwrite queries. Otherwise behaves identically to `POST /read`: same\ntarget semantics (branch xor snapshot), same Cedar action (Read),\nsame response shape.",
|
||||
"operationId": "query",
|
||||
"requestBody": {
|
||||
"content": {
|
||||
|
|
@ -802,7 +802,7 @@
|
|||
}
|
||||
},
|
||||
"400": {
|
||||
"description": "Bad request - also returned when the query body contains mutations; use POST /change for write queries",
|
||||
"description": "Bad request - also returned when the query body contains mutations; use POST /mutate (or its deprecated alias POST /change) for write queries",
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": {
|
||||
|
|
@ -1611,7 +1611,7 @@
|
|||
},
|
||||
"QueryRequest": {
|
||||
"type": "object",
|
||||
"description": "Inline read-query request for `POST /query`.\n\nFriendlier-named alternative to [`ReadRequest`] for ad-hoc reads and\nAI-agent integration. Mutations are rejected with 400 — use `POST\n/change` for write queries. Field names are deliberately short\n(`query`, `name`) to match the GQ keyword and the CLI `-e` flag.",
|
||||
"description": "Inline read-query request for `POST /query`.\n\nFriendlier-named alternative to [`ReadRequest`] for ad-hoc reads and\nAI-agent integration. Mutations are rejected with 400 — use `POST\n/mutate` (or its deprecated alias `POST /change`) for write queries.\nField names are deliberately short (`query`, `name`) to match the GQ\nkeyword and the CLI `-e` flag.",
|
||||
"required": [
|
||||
"query"
|
||||
],
|
||||
|
|
@ -1635,7 +1635,7 @@
|
|||
},
|
||||
"query": {
|
||||
"type": "string",
|
||||
"description": "GQ read-query source. May declare one or more named queries; pick one\nwith `name` when more than one is declared. Mutations\n(`insert`/`update`/`delete`) get 400 — use `POST /change` instead.",
|
||||
"description": "GQ read-query source. May declare one or more named queries; pick one\nwith `name` when more than one is declared. Mutations\n(`insert`/`update`/`delete`) get 400 — use `POST /mutate` (or its\ndeprecated alias `POST /change`) instead.",
|
||||
"example": "query get_person($name: String) {\n match {\n $p: Person { name: $name }\n }\n return { $p.name, $p.age }\n}"
|
||||
},
|
||||
"snapshot": {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue