Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/claude-autofix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,12 @@ jobs:
| select(.starter.nodes[0] | trusted)
| select(.comments.nodes[-1]
| (.author.__typename == "Bot" and (.body | startswith("**Claude autofix ("))) | not)
# A thread a maintainer left for the release quality pass (the last reply from a
# writer ends with the quality-sweep marker) waits for that pass, not for autofix. A
# later reply from a bot or a non-writer doesn't bring it back; a writer's does.
| select(([.comments.nodes[] | select(.author.__typename != "Bot" and (.author.login as $l | $writers | index($l)))]
| last | (.body // ""))
| test("<!-- quality-sweep -->\\s*$") | not)
| { id, path, line: (.line // .originalLine), outdated: .isOutdated,
comments: (.comments.totalCount as $n | .starter.nodes[0] as $s
| [ $s, (.comments.nodes[] | select(.id != $s.id)) ]
Expand Down
5 changes: 4 additions & 1 deletion apps/rustgis-python/python/rustgis/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,10 @@ def add_tiles(self, url: str, name: Optional[str] = None, **params: Any) -> Any:
**params: More ``map.addTiles`` parameters (``kind``, ``attribution``, ...).

Returns:
The tile layer's position, kind and name.
The tile layer's position, kind and name. An archive (PMTiles or MBTiles)
added without an ``id`` also reports ``zoomed``, whether its bounds framed
the map by the time the call returned (``False`` while its metadata is
still loading, which can frame the map later).
"""
if name is not None:
params["name"] = name
Expand Down
4 changes: 2 additions & 2 deletions crates/analysis/src/control.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ pub fn install_progress(control: Option<Arc<Control>>) -> rustgis_data::progress
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct Cancelled;

/// What [`crate::run_with`] says of a run that was cancelled (it names the tool and how far it
/// came).
/// The error text the `From<Cancelled>` conversions carry. [`crate::run_with`] replaces it with
/// its own "… was cancelled" message, which names the tool and how far it came.
pub(crate) const CANCELLED: &str = "cancelled";

impl From<Cancelled> for String {
Expand Down
76 changes: 69 additions & 7 deletions crates/engine/src/cmd/workflows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub fn commands() -> Vec<C> {
.params(r#"{"name"?: model (default: the last), "graph"?: run this graph instead (as model.new)}"#),
C::new("model.validate", "Validate Model", "Analyze › Geoprocessing", |s, p| {
let (_, g, _) = model_graph(s, p)?;
note_passed_graph(s, p);
let problems = crate::model::validate(s, &g);
Ok(json!({"ok": problems.is_empty(), "problems": problems}))
}).roles(&[crate::ParamRole { key: "graph", role: crate::Role::ModelGraph }]).effects(crate::policy::Effects::NONE)
Expand Down Expand Up @@ -443,9 +444,60 @@ pub fn gallery_list(kind: &str) -> CmdResult {
Ok(Value::Array(out))
}

/// Tells the user what a graph passed to a command lost or changed when it was read: one notice
/// per kind, each with a count.
fn notify_graph_dropped(s: &mut Session, dropped: crate::model::GraphDropped) {
if dropped.nodes > 0 {
s.notify(format!("model graph: {} node(s) left out: unreadable, without an id, or with the id of another", dropped.nodes), false);
}
if dropped.edges > 0 {
s.notify(
format!(
"model graph: {} connection(s) left out: unreadable, to a missing node or itself, without an input port, or repeating another",
dropped.edges
),
false,
);
}
if dropped.ids > 0 {
s.notify(
format!(
"model graph: {} node id(s) changed: trimmed of spaces or cut to {} characters (connections naming the old id follow it)",
dropped.ids,
crate::model::MAX_NODE_ID
),
false,
);
}
if dropped.positions > 0 {
s.notify(
format!("model graph: {} node position(s) changed: not a number (moved to 0, 0) or out of range (clamped)", dropped.positions),
false,
);
}
if dropped.params > 0 {
s.notify(format!("model graph: {} node(s) had parameters that were not an object; they were left out", dropped.params), false);
}
}

/// Reads the `graph` a command was passed, if any, with what reading it left out or changed. The
/// command reports that once it succeeds, so a command that fails says nothing about it.
fn read_graph(p: &Value) -> Result<(Option<crate::project::ModelGraph>, crate::model::GraphDropped), String> {
let Some(g) = p.get("graph").filter(|g| !g.is_null()) else { return Ok((None, Default::default())) };
let (graph, dropped) = crate::model::graph_from_json_counted(g)?;
Ok((Some(graph), dropped))
}

/// Says what the graph a command was passed (rather than a saved model's) lost when read.
fn note_passed_graph(s: &mut Session, p: &Value) {
if let Some(Ok((_, dropped))) = p.get("graph").filter(|g| !g.is_null()).map(crate::model::graph_from_json_counted) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (performance / clarity): every command that takes a graph now parses it twice. read_graph (or model_graph) reads it, and note_passed_graph runs graph_from_json_counted again just to get the counts, including the layout pass. It is harmless at the 2000-node cap, but model_graph could return the GraphDropped it already computes, which would remove the second parse in model.run, model.validate and model.export.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left for the release quality pass: the second parse is bounded by the node and connection caps, and returning the counts from model_graph is a refactor.

notify_graph_dropped(s, dropped);
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

fn new_model(s: &mut Session, p: &Value) -> CmdResult {
let base = ps(p, "name").map(str::trim).filter(|n| !n.is_empty()).unwrap_or("Model").chars().take(200).collect::<String>();
let graph = p.get("graph").filter(|g| !g.is_null()).map(crate::model::graph_from_json).transpose()?;
let (graph, dropped) = read_graph(p)?;
let steps: Vec<ModelStep> = match p.get("steps") {
_ if graph.is_some() => graph.as_ref().map(crate::model::graph_to_steps).unwrap_or_default(),
Some(v) => serde_json::from_value(v.clone()).map_err(|e| format!("steps: {e}"))?,
Expand Down Expand Up @@ -506,6 +558,7 @@ fn new_model(s: &mut Session, p: &Value) -> CmdResult {
}
let n = steps.len();
s.project.models.push(Model { name: name.clone(), steps, graph });
notify_graph_dropped(s, dropped);
Ok(json!({"model": name, "steps": n}))
}

Expand Down Expand Up @@ -637,11 +690,14 @@ fn run_model_here(s: &mut Session, p: &Value) -> CmdResult {
}
let (name, _, trusted) = model_graph(s, p)?;
let op = s.authorize_model(p)?;
if trusted || p.get("graph").is_some_and(|g| !g.is_null()) {
crate::model::run(s, op)
let out = if trusted || p.get("graph").is_some_and(|g| !g.is_null()) {
crate::model::run(s, op)?
} else {
s.narrowed(crate::policy::Narrow::Item { kind: "model", name }, |s| crate::model::run(s, op))?
}
s.narrowed(crate::policy::Narrow::Item { kind: "model", name }, |s| crate::model::run(s, op))??
};
// Said once the run went ahead, not for one that was refused.
note_passed_graph(s, p);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Ok(out)
}

/// The stored source and destination names used by graph saving and its provenance check.
Expand All @@ -654,7 +710,8 @@ pub(crate) fn model_save_names(p: &Value) -> Result<(String, String), String> {
/// Saves a graph as a model, replacing the one of that name (or adding it).
fn save_model(s: &mut Session, p: &Value) -> CmdResult {
let (name, new_name) = model_save_names(p)?;
let g = crate::model::graph_from_json(p.get("graph").ok_or("graph is required")?)?;
let (g, dropped) = read_graph(p)?;
let g = g.ok_or("graph is required")?;
Comment thread
giswqs marked this conversation as resolved.
Resolved
let i = s.project.models.iter().position(|m| m.name.eq_ignore_ascii_case(&name));
if !new_name.eq_ignore_ascii_case(&name) && s.project.models.iter().any(|m| m.name.eq_ignore_ascii_case(&new_name)) {
return Err(format!("a model named '{new_name}' already exists"));
Expand All @@ -667,6 +724,7 @@ fn save_model(s: &mut Session, p: &Value) -> CmdResult {
None => s.project.models.push(model),
}
s.trusted_models.remove(&name.to_ascii_lowercase());
notify_graph_dropped(s, dropped);
Ok(json!({"model": new_name}))
}

Expand All @@ -680,9 +738,13 @@ fn export_model(s: &mut Session, p: &Value) -> CmdResult {
match ps(p, "path") {
Some(path) => {
rustgis_data::io::write_atomic(std::path::Path::new(path), text.as_bytes()).map_err(|e| format!("{path}: {e}"))?;
note_passed_graph(s, p);
Ok(json!({"model": name, "path": path}))
}
None => Ok(json!({"model": name, "fileName": format!("{file}.model.json"), "text": text})),
None => {
note_passed_graph(s, p);
Ok(json!({"model": name, "fileName": format!("{file}.model.json"), "text": text}))
}
}
}

Expand Down
78 changes: 71 additions & 7 deletions crates/engine/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::project::{ModelEdge, ModelGraph, ModelNode, ModelNodeKind, ModelStep}
pub const MAX_NODES: usize = 2000;
pub const MAX_EDGES: usize = 4000;
/// Identifier length and canvas coordinate limits used by graph commands and file checks.
const MAX_NODE_ID: usize = 100;
pub(crate) const MAX_NODE_ID: usize = 100;
const MAX_NODE_POSITION: f32 = 1e6;
/// The output port of input and tool nodes.
pub const OUT: &str = "out";
Expand Down Expand Up @@ -477,6 +477,28 @@ pub fn graph_to_steps(g: &ModelGraph) -> Vec<ModelStep> {
/// edges to missing nodes or to themselves. Bounded in size.
/// Repeated connections into many-valued input ports retain their multiplicity.
pub fn graph_from_json(v: &Value) -> Result<ModelGraph, String> {
graph_from_json_counted(v).map(|(g, _)| g)
}

/// What [`graph_from_json_counted`] left out of a graph.
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
pub struct GraphDropped {
/// Nodes that could not be read, had no id, or repeated another's id.
pub nodes: usize,
/// Connections that could not be read, joined a missing node or itself, had no input port,
/// or repeated another.
pub edges: usize,
/// Kept nodes whose id was trimmed of spaces or cut to its longest length.
pub ids: usize,
/// Kept nodes whose position was not a number (moved to 0, 0) or out of range (clamped).
pub positions: usize,
/// Kept nodes whose parameters were not an object, so they were left out.
pub params: usize,
}

/// Reads a graph as [`graph_from_json`] does, and counts the nodes and connections it left out
/// and the nodes it changed, so a command can say so. Errors when `nodes` is missing or either list is too long.
pub fn graph_from_json_counted(v: &Value) -> Result<(ModelGraph, GraphDropped), String> {
let nodes = v.get("nodes").and_then(Value::as_array).ok_or("a model graph needs \"nodes\"")?;
if nodes.len() > MAX_NODES {
return Err(format!("a model can have at most {MAX_NODES} nodes"));
Expand All @@ -486,37 +508,79 @@ pub fn graph_from_json(v: &Value) -> Result<ModelGraph, String> {
return Err(format!("a model can have at most {MAX_EDGES} connections"));
}
let mut g = ModelGraph::default();
let mut dropped = GraphDropped::default();
Comment thread
giswqs marked this conversation as resolved.
Resolved
// Ids trimmed or cut, from what the file gave to what is kept: connections naming a node by
// the id it was given still reach it.
let mut renamed: Vec<(String, String)> = Vec::new();
// The ids given to the nodes kept, and to those left out for repeating a kept one's id once
// trimmed or cut: a connection naming only a left-out node is left out with it, rather than
// moved onto the node that kept the id.
let mut given_kept: Vec<String> = Vec::new();
let mut given_dropped: Vec<String> = Vec::new();
for n in nodes {
let Ok(mut node) = serde_json::from_value::<ModelNode>(n.clone()) else { continue };
node.id = node.id.trim().chars().take(MAX_NODE_ID).collect();
if node.id.is_empty() || g.nodes.iter().any(|m| m.id == node.id) {
let Ok(mut node) = serde_json::from_value::<ModelNode>(n.clone()) else {
dropped.nodes += 1;
continue;
};
let id: String = node.id.trim().chars().take(MAX_NODE_ID).collect();
if id.is_empty() || g.nodes.iter().any(|m| m.id == id) {
Comment thread
giswqs marked this conversation as resolved.
Resolved
dropped.nodes += 1;
if !id.is_empty() {
given_dropped.push(node.id);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
continue;
}
given_kept.push(node.id.clone());
if id != node.id {
dropped.ids += 1;
renamed.push((std::mem::take(&mut node.id), id.clone()));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
node.id = id;
}
Comment thread
giswqs marked this conversation as resolved.
Resolved
let (x, y) = (node.x, node.y);
if !node.x.is_finite() || !node.y.is_finite() {
node.x = 0.0;
node.y = 0.0;
}
node.x = node.x.clamp(-MAX_NODE_POSITION, MAX_NODE_POSITION);
node.y = node.y.clamp(-MAX_NODE_POSITION, MAX_NODE_POSITION);
if !node.params.is_object() {
if node.x.to_bits() != x.to_bits() || node.y.to_bits() != y.to_bits() {
dropped.positions += 1;
}
Comment thread
giswqs marked this conversation as resolved.
Resolved
if !node.params.is_object() && !node.params.is_null() {
dropped.params += 1;
node.params = Value::Null;
}
g.nodes.push(node);
}
for e in edges {
let Ok(e) = serde_json::from_value::<ModelEdge>(e.clone()) else { continue };
let Ok(mut e) = serde_json::from_value::<ModelEdge>(e.clone()) else {
dropped.edges += 1;
continue;
};
let left_out = |end: &String| given_dropped.contains(end) && !given_kept.contains(end);
if left_out(&e.from) || left_out(&e.to) {
dropped.edges += 1;
continue;
}
for end in [&mut e.from, &mut e.to] {
if let Some((_, id)) = renamed.iter().find(|(given, _)| given == end) {
*end = id.clone();
}
}
let has = |id: &str| g.nodes.iter().any(|n| n.id == id);
let repeated_input =
g.nodes.iter().find(|n| n.id == e.to).is_some_and(|n| input_ports(n).iter().any(|p| p.id == e.to_port && p.many));
if e.from != e.to && has(&e.from) && has(&e.to) && !e.to_port.is_empty() && (repeated_input || !g.edges.contains(&e)) {
g.edges.push(e);
} else {
dropped.edges += 1;
}
}
// A graph with no positions (written by hand or by an agent) is laid out.
if g.nodes.iter().all(|n| n.x == 0.0 && n.y == 0.0) {
layout(&mut g);
}
Ok(g)
Ok((g, dropped))
}

/// How one node fared in a run.
Expand Down
69 changes: 69 additions & 0 deletions crates/engine/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25721,6 +25721,75 @@ fn dem_tiles_mosaic_clip_by_polygon_share_a_stretch_and_report_overviews() {
assert!(e.to_string().contains("overview"), "{e}");
}

/// A graph passed to a model command says what reading it left out (one notice per kind, with a
/// count); a clean graph says nothing.
#[test]
fn model_graph_drops_are_noticed() {
let mut s = world();
let clean = json!({
"nodes": [
{"id": "i1", "kind": "input", "x": 0, "y": 0, "layer": "Cities"},
{"id": "o1", "kind": "output", "x": 240, "y": 0, "name": "Kept"}
],
"edges": [{"from": "i1", "fromPort": "out", "to": "o1", "toPort": "in"}]
});
let said = |s: &Session, mark: u64, what: &str| s.notices_since(mark).iter().filter(|n| n.text.contains(what)).count();
let mark = s.notice_mark();
s.run("model.new", &json!({"name": "Clean", "graph": clean})).unwrap();
s.run("model.save", &json!({"name": "Clean", "graph": clean})).unwrap();
s.run("model.validate", &json!({"graph": clean})).unwrap();
assert_eq!(said(&s, mark, "model graph"), 0, "{:?}", s.notices_since(mark));

let lossy = json!({
"nodes": [
{"id": "i1", "kind": "input", "x": 0, "y": 0, "layer": "Cities"},
{"id": "i1", "kind": "input", "x": 10, "y": 0, "layer": "Countries"},
{"id": " ", "kind": "input", "x": 20, "y": 0, "layer": "Cities"},
{"kind": 7},
{"id": "o1", "kind": "output", "x": 240, "y": 0, "name": "Kept"},
{"id": " i2 ", "kind": "input", "x": 5e7, "y": 0, "layer": "Cities", "params": "not an object"},
// Repeats " i2 " once trimmed: left out, and so is the connection naming it.
{"id": "i2", "kind": "input", "x": 30, "y": 0, "layer": "Countries"}
],
"edges": [
{"from": "i1", "fromPort": "out", "to": "o1", "toPort": "in"},
{"from": "i1", "fromPort": "out", "to": "missing", "toPort": "in"},
{"from": "o1", "fromPort": "out", "to": "o1", "toPort": "in"},
{"from": "i1", "fromPort": "out", "to": "o1", "toPort": ""},
// Names the node by the id it was given: it follows the trimmed id, so it is kept.
{"from": " i2 ", "fromPort": "out", "to": "o1", "toPort": "in"},
{"from": "i2", "fromPort": "out", "to": "o1", "toPort": "in"},
"not an edge"
]
});
for (cmd, params) in [
("model.new", json!({"name": "Lossy", "graph": lossy})),
("model.save", json!({"name": "Lossy", "graph": lossy})),
("model.validate", json!({"graph": lossy})),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
("model.export", json!({"graph": lossy})),
] {
let mark = s.notice_mark();
s.run(cmd, &params).unwrap();
assert_eq!(said(&s, mark, "4 node(s) left out"), 1, "{cmd}: {:?}", s.notices_since(mark));
assert_eq!(said(&s, mark, "5 connection(s) left out"), 1, "{cmd}: {:?}", s.notices_since(mark));
assert_eq!(said(&s, mark, "1 node id(s) changed"), 1, "{cmd}: {:?}", s.notices_since(mark));
assert_eq!(said(&s, mark, "1 node position(s) changed"), 1, "{cmd}: {:?}", s.notices_since(mark));
assert_eq!(said(&s, mark, "1 node(s) had parameters"), 1, "{cmd}: {:?}", s.notices_since(mark));
}
// A command that fails says nothing about the graph it was passed.
let mark = s.notice_mark();
assert!(s.run("model.save", &json!({"name": "Lossy", "rename": "Clean", "graph": lossy})).is_err());
// A path under a regular file can't be written.
let blocker = std::env::temp_dir().join(format!("rustgis-model-export-{}", std::process::id()));
std::fs::write(&blocker, b"not a folder").unwrap();
let unwritable = blocker.join("lossy.model.json");
assert!(s.run("model.export", &json!({"graph": lossy, "path": unwritable.to_string_lossy()})).is_err());
let _ = std::fs::remove_file(&blocker);
assert_eq!(said(&s, mark, "model graph"), 0, "{:?}", s.notices_since(mark));
let saved = s.project.models.iter().find(|m| m.name == "Lossy").and_then(|m| m.graph.as_ref()).map(|g| g.edges.len());
assert_eq!(saved, Some(2), "the connection naming \" i2 \" follows its trimmed id");
}

/// ModelBuilder graphs: validated, run in flow order, intermediate results dropped, outputs named.
fn model_graphs(s: &mut Session) {
let graph = json!({
Expand Down
11 changes: 10 additions & 1 deletion crates/geom/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,13 @@ pub fn format_area(m2: f64) -> String {
if !m2.is_finite() {
return "—".to_string();
}
if m2.abs().round() < 1.0e6 { format!("{m2:.0} m²") } else { format!("{:.1} km²", m2 / 1.0e6) }
if m2.abs().round() < 1.0e6 {
// A tiny negative area rounds to `-0`; it reads `0 m²`.
let whole = format!("{m2:.0}");
format!("{} m²", if whole == "-0" { "0" } else { &whole })
} else {
format!("{:.1} km²", m2 / 1.0e6)
}
}

/// Formats a lon/lat pair in decimal degrees with hemispheres: `12.345° E 45.678° N`.
Expand Down Expand Up @@ -1234,6 +1240,9 @@ mod tests {
assert_eq!(format_distance(-12_500.0), "-12.5 km");
assert_eq!(format_distance(-99_949.0), "-99.9 km");
assert_eq!(format_distance(-99_960.0), "-100 km");
assert_eq!(format_area(-0.3), "0 m²");
assert_eq!(format_area(-0.0), "0 m²");
assert_eq!(format_area(-0.6), "-1 m²");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
assert_eq!(format_area(-999_999.4), "-999999 m²");
assert_eq!(format_area(-999_999.6), "-1.0 km²");
assert_eq!(format_area(-1.0e6), "-1.0 km²");
Expand Down
2 changes: 1 addition & 1 deletion crates/tiles/src/style.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1150,7 +1150,7 @@ fn parse_sprites(entries: &[Json]) -> (Vec<(String, String)>, Vec<String>) {
}
if past > 0 {
left_out.push(format!(
"{past} of {total} sprites left out, so their icons and patterns aren't drawn: a style's first {MAX_SPRITES} are read"
"{past} of {total} sprites left out, so their icons and patterns aren't drawn: only the first {MAX_SPRITES} with a usable id of their own and an address are used"
));
}
(sprites, left_out)
Expand Down
Loading
Loading