mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-09 01:35:18 +02:00
Resolve graph config by identity, not server mode
Which policy/queries block applies for a graph was decided three different,
mode-dependent ways: single-mode boot used top-level even for a named graph;
multi-mode used per-graph (and silently ignored a top-level queries block); the
CLI used per-graph for a named target. So `queries validate --target prod`
could check a different registry than the single-mode server loaded, and a
named graph's per-graph policy/queries were silently shadowed.
Make config a function of graph IDENTITY: a graph served by NAME
(--target/server.graph, a graphs: entry) uses its own graphs.<name>.{policy,
queries}; a bare URI is anonymous and uses top-level. One rule, applied by
single-mode boot, multi-mode boot, and the CLI — so they can't diverge and the
CLI predicts the server exactly.
No silent ignore: serving a named graph while a top-level policy/queries block
is populated now refuses boot, naming the block (the multi-mode top-level-policy
bail, extended to queries and to single-mode-named). The CLI's `queries
validate` derives the schema URI and the registry from ONE selection, and a
positional URI forces anonymous (ignoring cli.graph) so the two can't come from
different graphs.
BREAKING (released behavior): single mode by name (--target/server.graph) with
top-level policy/queries previously used top-level; it now uses the per-graph
block and refuses boot if top-level is also populated. Bare-URI single mode is
unchanged. Loud, with migration text pointing at graphs.<name>.
- config: resolve_policy_file_for (policy sibling of query_entries_for, no
top-level fallback) + populated_top_level_blocks for the coherence check.
- characterization tests (single-mode named -> per-graph; named + top-level ->
bail; multi-mode top-level queries -> bail; CLI positional-URI -> top-level).
- docs: policy.md, server.md, cli-reference.md.
This commit is contained in:
parent
ad2fc27849
commit
cd570ce59c
8 changed files with 282 additions and 33 deletions
|
|
@ -1679,19 +1679,36 @@ struct QueriesListOutput {
|
|||
queries: Vec<QueriesListItem>,
|
||||
}
|
||||
|
||||
/// Resolve the stored-query registry entries for the selected graph via
|
||||
/// the same `query_entries_for` the server boot uses, so the CLI
|
||||
/// validates exactly the block the server would load. A `--target`
|
||||
/// overrides the configured default graph.
|
||||
fn registry_entries<'a>(
|
||||
config: &'a OmnigraphConfig,
|
||||
target: Option<&str>,
|
||||
) -> &'a std::collections::BTreeMap<String, omnigraph_server::config::QueryEntry> {
|
||||
config.query_entries_for(target.or_else(|| config.cli_graph_name()))
|
||||
/// Resolve the selected graph to `(local URI, registry selection)` from one
|
||||
/// precedence, so a command's schema and its stored-query registry can never
|
||||
/// come from different graphs. A **positional URI is anonymous** (top-level
|
||||
/// registry, ignoring the configured default graph); otherwise `--target`
|
||||
/// or the configured `cli.graph` names the graph (its per-graph block).
|
||||
/// Mirrors the server's single-mode identity rule.
|
||||
fn resolve_selected_graph(
|
||||
config: &OmnigraphConfig,
|
||||
cli_uri: Option<String>,
|
||||
cli_target: Option<&str>,
|
||||
operation: &str,
|
||||
) -> Result<(String, Option<String>)> {
|
||||
let selected = if cli_uri.is_some() {
|
||||
None
|
||||
} else {
|
||||
cli_target
|
||||
.map(str::to_string)
|
||||
.or_else(|| config.cli_graph_name().map(str::to_string))
|
||||
};
|
||||
let uri = resolve_local_uri(config, cli_uri, cli_target, operation)?;
|
||||
Ok((uri, selected))
|
||||
}
|
||||
|
||||
fn load_registry_or_report(config: &OmnigraphConfig, target: Option<&str>) -> Result<QueryRegistry> {
|
||||
QueryRegistry::load(config, registry_entries(config, target)).map_err(|errors| {
|
||||
/// Load the stored-query registry for an already-resolved graph selection
|
||||
/// (`None` = anonymous → top-level; `Some(name)` = that graph's block).
|
||||
fn load_registry_or_report(
|
||||
config: &OmnigraphConfig,
|
||||
selected: Option<&str>,
|
||||
) -> Result<QueryRegistry> {
|
||||
QueryRegistry::load(config, config.query_entries_for(selected)).map_err(|errors| {
|
||||
color_eyre::eyre::eyre!(
|
||||
"stored-query registry failed to load:\n {}",
|
||||
errors
|
||||
|
|
@ -1710,8 +1727,11 @@ async fn execute_queries_validate(
|
|||
json: bool,
|
||||
) -> Result<()> {
|
||||
let config = load_cli_config(config_path)?;
|
||||
let registry = load_registry_or_report(&config, target.as_deref())?;
|
||||
let uri = resolve_local_uri(&config, uri, target.as_deref(), "queries validate")?;
|
||||
// One selection drives both the schema URI and the registry, so a
|
||||
// positional URI and a `--target` can't validate different graphs.
|
||||
let (uri, selected) =
|
||||
resolve_selected_graph(&config, uri, target.as_deref(), "queries validate")?;
|
||||
let registry = load_registry_or_report(&config, selected.as_deref())?;
|
||||
let db = Omnigraph::open(&uri).await?;
|
||||
let report = check(®istry, &db.catalog());
|
||||
|
||||
|
|
@ -1766,7 +1786,10 @@ fn execute_queries_list(
|
|||
json: bool,
|
||||
) -> Result<()> {
|
||||
let config = load_cli_config(config_path)?;
|
||||
let registry = load_registry_or_report(&config, target.as_deref())?;
|
||||
// `list` takes no URI, so the selection is the target or the configured
|
||||
// default graph (named → its per-graph block; else top-level).
|
||||
let selected = target.as_deref().or_else(|| config.cli_graph_name());
|
||||
let registry = load_registry_or_report(&config, selected)?;
|
||||
|
||||
let output = QueriesListOutput {
|
||||
queries: registry
|
||||
|
|
|
|||
|
|
@ -2493,3 +2493,49 @@ fn queries_validate_exits_nonzero_on_duplicate_tool_name() {
|
|||
"duplicate tool name should be reported naming both queries; stderr:\n{stderr}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn queries_validate_positional_uri_ignores_default_graph() {
|
||||
// A positional URI is anonymous → the schema AND the registry both come
|
||||
// from top-level, even when `cli.graph` names a graph whose per-graph
|
||||
// queries would fail. Pins that the URI and registry can't diverge.
|
||||
let graph = SystemGraph::loaded();
|
||||
graph.write_query(
|
||||
"clean.gq",
|
||||
"query clean($name: String) { match { $p: Person { name: $name } } return { $p.age } }",
|
||||
);
|
||||
// `Widget` is not in the fixture schema — the default graph's per-graph
|
||||
// query would break validate if it were (wrongly) selected.
|
||||
graph.write_query("broken.gq", "query broken() { match { $w: Widget } return { $w.name } }");
|
||||
let config = graph.write_config(
|
||||
"omnigraph.yaml",
|
||||
concat!(
|
||||
"cli:\n graph: prod\n",
|
||||
"graphs:\n",
|
||||
" prod:\n",
|
||||
" uri: /nonexistent-prod.omni\n",
|
||||
" queries:\n",
|
||||
" broken:\n",
|
||||
" file: ./broken.gq\n",
|
||||
"queries:\n",
|
||||
" clean:\n",
|
||||
" file: ./clean.gq\n",
|
||||
"policy: {}\n",
|
||||
),
|
||||
);
|
||||
// Positional URI = the real loaded graph; selection is anonymous, so the
|
||||
// CLEAN top-level registry validates (not prod's broken one).
|
||||
let output = output_success(
|
||||
cli()
|
||||
.arg("queries")
|
||||
.arg("validate")
|
||||
.arg(graph.path())
|
||||
.arg("--config")
|
||||
.arg(&config),
|
||||
);
|
||||
let stdout = stdout_string(&output);
|
||||
assert!(
|
||||
stdout.contains("OK"),
|
||||
"positional URI must validate the top-level registry, not the cli.graph default; stdout:\n{stdout}"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -331,6 +331,36 @@ impl OmnigraphConfig {
|
|||
}
|
||||
}
|
||||
|
||||
/// The policy file that applies for a graph selection — the policy
|
||||
/// sibling of [`OmnigraphConfig::query_entries_for`], so policy and
|
||||
/// queries resolve by the same identity rule. A named graph in
|
||||
/// `graphs:` uses its per-graph `policy.file` with **no** top-level
|
||||
/// fallback (a named graph with no per-graph policy has no policy —
|
||||
/// that keeps the boot-time coherence check meaningful); anything else
|
||||
/// (no selection, or a bare URI) uses the top-level `policy.file`.
|
||||
pub fn resolve_policy_file_for(&self, graph: Option<&str>) -> Option<PathBuf> {
|
||||
match graph {
|
||||
Some(name) if self.graphs.contains_key(name) => self.resolve_target_policy_file(name),
|
||||
_ => self.resolve_policy_file(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Names of any top-level config blocks (`policy.file`, `queries:`)
|
||||
/// that are populated. Used by the boot-time coherence check: when a
|
||||
/// **named** graph is served (single-mode by name, or multi-mode),
|
||||
/// the top-level blocks are not honored, so a populated one is a
|
||||
/// configuration error rather than a silent no-op.
|
||||
pub fn populated_top_level_blocks(&self) -> Vec<&'static str> {
|
||||
let mut blocks = Vec::new();
|
||||
if self.policy.file.is_some() {
|
||||
blocks.push("policy.file");
|
||||
}
|
||||
if !self.queries.is_empty() {
|
||||
blocks.push("queries");
|
||||
}
|
||||
blocks
|
||||
}
|
||||
|
||||
/// Resolve a stored-query `.gq` file path (from a registry entry),
|
||||
/// relative to the config's `base_dir`. Mirrors policy-file
|
||||
/// resolution; the registry loader calls this to turn each entry's
|
||||
|
|
@ -640,6 +670,42 @@ queries:
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolve_policy_file_for_follows_identity() {
|
||||
let temp = tempdir().unwrap();
|
||||
fs::write(
|
||||
temp.path().join("omnigraph.yaml"),
|
||||
"policy:\n file: ./top.yaml\ngraphs:\n prod:\n uri: s3://b/prod\n \
|
||||
policy:\n file: ./prod.yaml\n bare:\n uri: s3://b/bare\n",
|
||||
)
|
||||
.unwrap();
|
||||
let config = load_config_in(temp.path(), None).unwrap();
|
||||
|
||||
// Named graph with its own policy → per-graph (not top-level).
|
||||
assert!(
|
||||
config
|
||||
.resolve_policy_file_for(Some("prod"))
|
||||
.unwrap()
|
||||
.ends_with("prod.yaml")
|
||||
);
|
||||
// Named graph with NO per-graph policy → None (no top-level fallback;
|
||||
// load-bearing for the boot coherence check).
|
||||
assert!(config.resolve_policy_file_for(Some("bare")).is_none());
|
||||
// Anonymous (bare URI) or an unknown name → top-level.
|
||||
assert!(
|
||||
config
|
||||
.resolve_policy_file_for(None)
|
||||
.unwrap()
|
||||
.ends_with("top.yaml")
|
||||
);
|
||||
assert!(
|
||||
config
|
||||
.resolve_policy_file_for(Some("nope"))
|
||||
.unwrap()
|
||||
.ends_with("top.yaml")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn queries_block_absent_yields_empty_registry() {
|
||||
let temp = tempdir().unwrap();
|
||||
|
|
|
|||
|
|
@ -927,11 +927,34 @@ pub fn load_server_settings(
|
|||
let uri = normalize_root_uri(&raw_uri).wrap_err_with(|| {
|
||||
format!("normalize single-graph URI '{raw_uri}' from server settings")
|
||||
})?;
|
||||
let policy_file = config.resolve_policy_file();
|
||||
// Single mode uses the top-level `queries:` (mirrors top-level
|
||||
// `policy.file`). Load + identity-check now (no engine needed);
|
||||
// the schema type-check happens when the engine opens.
|
||||
let queries = QueryRegistry::load(&config, config.query_entries_for(None))
|
||||
// Config follows graph IDENTITY, not mode: a bare URI is anonymous
|
||||
// (top-level config); a graph chosen by name uses its per-graph
|
||||
// `graphs.<name>.{policy,queries}`. `resolve_target_uri` already
|
||||
// errored on an unknown name, so a `Some(name)` here is a known graph.
|
||||
let selected: Option<&str> = if has_cli_uri {
|
||||
None
|
||||
} else {
|
||||
cli_target.as_deref().or_else(|| config.server_graph_name())
|
||||
};
|
||||
// A named selection must not leave a populated top-level block
|
||||
// silently unused — refuse boot and point at the per-graph block.
|
||||
if let Some(name) = selected {
|
||||
let unhonored = config.populated_top_level_blocks();
|
||||
if !unhonored.is_empty() {
|
||||
bail!(
|
||||
"serving named graph '{name}', but top-level {} {} set — a named graph \
|
||||
uses its own `graphs.{name}.…` block, so the top-level value is ignored. \
|
||||
Move it to `graphs.{name}` (e.g. `graphs.{name}.policy.file`, \
|
||||
`graphs.{name}.queries`).",
|
||||
unhonored.join(" and "),
|
||||
if unhonored.len() == 1 { "is" } else { "are" },
|
||||
);
|
||||
}
|
||||
}
|
||||
// Load + identity-check now (no engine needed); the schema
|
||||
// type-check happens when the engine opens.
|
||||
let policy_file = config.resolve_policy_file_for(selected);
|
||||
let queries = QueryRegistry::load(&config, config.query_entries_for(selected))
|
||||
.map_err(|errs| color_eyre::eyre::eyre!(format_registry_load_errors(&uri, &errs)))?;
|
||||
ServerConfigMode::Single {
|
||||
uri,
|
||||
|
|
@ -939,12 +962,16 @@ pub fn load_server_settings(
|
|||
queries,
|
||||
}
|
||||
} else if has_explicit_config && has_graphs_map {
|
||||
if config.resolve_policy_file().is_some() {
|
||||
// Multi mode: every graph uses its per-graph block; top-level
|
||||
// policy/queries are never honored, so a populated one is an error.
|
||||
let unhonored = config.populated_top_level_blocks();
|
||||
if !unhonored.is_empty() {
|
||||
bail!(
|
||||
"top-level `policy.file` is single-graph/CLI-local policy only; \
|
||||
in multi-graph mode move per-graph rules to \
|
||||
`graphs.<graph_id>.policy.file` and move `graph_list` rules to \
|
||||
`server.policy.file`."
|
||||
"multi-graph mode: top-level {} {} not honored — each graph uses its own \
|
||||
`graphs.<graph_id>.…` block. Move per-graph rules there (and any \
|
||||
`graph_list` policy to `server.policy.file`).",
|
||||
unhonored.join(" and "),
|
||||
if unhonored.len() == 1 { "is" } else { "are" },
|
||||
);
|
||||
}
|
||||
// Rule 4 → Multi mode. Build a startup config per graph.
|
||||
|
|
|
|||
|
|
@ -5506,11 +5506,11 @@ graphs:
|
|||
let err = load_server_settings(Some(&config_path), None, None, None, true).unwrap_err();
|
||||
let msg = err.to_string();
|
||||
assert!(
|
||||
msg.contains("top-level `policy.file` is single-graph/CLI-local policy only"),
|
||||
"expected single-graph policy guidance, got: {msg}"
|
||||
msg.contains("top-level") && msg.contains("policy.file") && msg.contains("not honored"),
|
||||
"expected top-level-not-honored guidance, got: {msg}"
|
||||
);
|
||||
assert!(
|
||||
msg.contains("graphs.<graph_id>.policy.file"),
|
||||
msg.contains("graphs.<graph_id>"),
|
||||
"expected per-graph migration guidance, got: {msg}"
|
||||
);
|
||||
assert!(
|
||||
|
|
@ -5519,6 +5519,86 @@ graphs:
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mode_inference_multi_rejects_top_level_queries() {
|
||||
// Symmetric to the policy guard: a top-level `queries:` block in
|
||||
// multi-graph mode is not honored (each graph uses its own), so it
|
||||
// is a loud error rather than a silent no-op.
|
||||
let temp = tempfile::tempdir().unwrap();
|
||||
let config_path = temp.path().join("omnigraph.yaml");
|
||||
fs::write(
|
||||
&config_path,
|
||||
"queries:\n q:\n file: ./q.gq\ngraphs:\n alpha:\n uri: /tmp/alpha.omni\n",
|
||||
)
|
||||
.unwrap();
|
||||
let err = load_server_settings(Some(&config_path), None, None, None, true).unwrap_err();
|
||||
let msg = err.to_string();
|
||||
assert!(
|
||||
msg.contains("queries") && msg.contains("not honored"),
|
||||
"top-level queries must be rejected in multi-graph mode: {msg}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn single_mode_named_graph_rejects_top_level_blocks() {
|
||||
// Serving a graph by name (`--target`/`server.graph`) uses its
|
||||
// per-graph block; a populated top-level block would be silently
|
||||
// shadowed, so boot refuses and names the per-graph location.
|
||||
let temp = tempfile::tempdir().unwrap();
|
||||
let config_path = temp.path().join("omnigraph.yaml");
|
||||
fs::write(
|
||||
&config_path,
|
||||
"policy:\n file: ./top.yaml\ngraphs:\n prod:\n uri: /tmp/prod.omni\n",
|
||||
)
|
||||
.unwrap();
|
||||
let err =
|
||||
load_server_settings(Some(&config_path), None, Some("prod".to_string()), None, true)
|
||||
.unwrap_err();
|
||||
let msg = err.to_string();
|
||||
assert!(
|
||||
msg.contains("prod") && msg.contains("policy.file") && msg.contains("graphs.prod"),
|
||||
"named single-mode + top-level policy must refuse, naming the graph: {msg}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn single_mode_named_graph_uses_per_graph_policy_and_queries() {
|
||||
// The identity rule: `--target prod` attaches `graphs.prod`'s own
|
||||
// policy + queries, not the top-level ones (which are absent here).
|
||||
let temp = tempfile::tempdir().unwrap();
|
||||
fs::write(
|
||||
temp.path().join("prod.gq"),
|
||||
"query pq() { match { $u: User } return { $u.name } }",
|
||||
)
|
||||
.unwrap();
|
||||
let config_path = temp.path().join("omnigraph.yaml");
|
||||
fs::write(
|
||||
&config_path,
|
||||
"graphs:\n prod:\n uri: /tmp/prod.omni\n policy:\n file: ./prod-policy.yaml\n \
|
||||
queries:\n pq:\n file: ./prod.gq\n",
|
||||
)
|
||||
.unwrap();
|
||||
let settings =
|
||||
load_server_settings(Some(&config_path), None, Some("prod".to_string()), None, true)
|
||||
.unwrap();
|
||||
match settings.mode {
|
||||
ServerConfigMode::Single {
|
||||
policy_file,
|
||||
queries,
|
||||
..
|
||||
} => {
|
||||
assert!(
|
||||
policy_file
|
||||
.as_ref()
|
||||
.is_some_and(|p| p.ends_with("prod-policy.yaml")),
|
||||
"per-graph policy attached: {policy_file:?}"
|
||||
);
|
||||
assert!(queries.lookup("pq").is_some(), "per-graph query attached");
|
||||
}
|
||||
other => panic!("expected Single mode, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mode_inference_normalizes_multi_graph_uris() {
|
||||
let temp = tempfile::tempdir().unwrap();
|
||||
|
|
|
|||
|
|
@ -67,7 +67,7 @@ aliases:
|
|||
graph: <name>
|
||||
branch: <name>
|
||||
format: <output-format>
|
||||
queries: # top-level stored-query registry (single-graph mode); mirrors top-level `policy`
|
||||
queries: # top-level registry — applies only to a bare-URI (anonymous) graph; a graph served by name uses its `graphs.<id>.queries`. Mirrors top-level `policy`.
|
||||
<query-name>: { file: <path-to-.gq> } # mcp.expose defaults to true
|
||||
policy:
|
||||
file: ./policy.yaml
|
||||
|
|
|
|||
|
|
@ -47,10 +47,15 @@ graphs:
|
|||
# no per-graph policy → no engine-layer Cedar enforcement on beta
|
||||
```
|
||||
|
||||
Top-level `policy.file` is single-graph / CLI-local policy only. Multi-graph
|
||||
server startup rejects it because applying one graph policy to every configured
|
||||
graph is ambiguous. Move per-graph rules to `graphs.<graph_id>.policy.file` and
|
||||
move `graph_list` rules to `server.policy.file`.
|
||||
**Config follows graph identity, not server mode.** A graph served by **name**
|
||||
(`--target <name>` or `server.graph`) uses its own `graphs.<name>.policy.file`,
|
||||
exactly as in multi-graph mode. Top-level `policy.file` applies only to an
|
||||
**anonymous** graph — one served by a bare `<URI>` with no `graphs:` entry.
|
||||
Serving a **named** graph (single- or multi-graph mode) while top-level
|
||||
`policy.file` (or `queries:`) is populated **refuses boot**, naming the block,
|
||||
since the top-level value would otherwise be silently shadowed by the per-graph
|
||||
block. Move per-graph rules to `graphs.<graph_id>.policy.file` and `graph_list`
|
||||
rules to `server.policy.file`.
|
||||
|
||||
Each graph's HTTP request flows through its own per-graph policy. The management endpoint (`GET /graphs`) flows through the server-level policy. When `server.policy.file` is unset, `GET /graphs` is denied in every runtime state, including `--unauthenticated`; with bearer tokens configured, it returns 403 after admission control because `graph_list` is not a `read`-equivalent action. The operator must explicitly authorize via `server-policy.yaml` to expose `/graphs`.
|
||||
|
||||
|
|
|
|||
|
|
@ -6,7 +6,9 @@ Axum 0.8 + tokio + utoipa-generated OpenAPI. **Two modes** (v0.6.0+): single-gra
|
|||
|
||||
### Single-graph mode (legacy)
|
||||
|
||||
`omnigraph-server <URI>` or `omnigraph-server --target <name> --config omnigraph.yaml`. Routes are flat — `/snapshot`, `/read`, `/branches`, etc. Behavior unchanged from v0.6.0.
|
||||
`omnigraph-server <URI>` or `omnigraph-server --target <name> --config omnigraph.yaml`. Routes are flat — `/snapshot`, `/read`, `/branches`, etc.
|
||||
|
||||
**Config follows graph identity.** A bare `<URI>` is an *anonymous* graph and uses the **top-level** `policy.file` / `queries:`. A graph chosen by **name** (`--target` / `server.graph`) uses its own `graphs.<name>.{policy.file, queries}` — the same block multi-graph mode uses. ⚠️ *Changed from v0.6.0, which always used top-level config in single mode: a named-graph config that puts `policy`/`queries` at top-level now **refuses boot** and points you at `graphs.<name>.…` (move the block there). Bare-`<URI>` single mode is unchanged.*
|
||||
|
||||
### Multi-graph mode (v0.6.0+)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue