refactor: consolidate staging and commit helpers - #1929

Optional, but I'd want it in this PR — there's a second behaviour change here that the body doesn't call out.

Folding registers_new_graph into the shared is_noop doesn't just move that guard — it adds it to the three tx.rs commit paths (execute_transaction, transact_with_trig_meta, transact_with_named_graphs) that never had it. Their old inline rule was !view.has_staged() && (Update|Upsert || sync_graph.is_some()), with no registration clause at all; only the three tx_builder.rs copies carried one.

The effect: on Fluree::transact(), a zero-flake update whose graph_delta names a graph the registry doesn't know now writes an empty registration commit and advances t, where before it returned without one. I verified rather than inferred it — a temporary probe against a ledger seeded at t=1:

let update = json!({
    "@context": {"ex": "http://example.org/"},
    "where":  {"@id": "?s", "ex:absent": "?o"},          // matches nothing
    "insert": {"@id": "?s", "@graph": "http://example.org/graphs/brandnew", "ex:q": "y"}
});
fluree.update_with_opts(ledger, &update, TxnOpts::default(), Default::default(), &index_cfg).await
// HEAD:                              receipt.t == 2, flake_count == 0   ← empty registration commit
// `.has_unregistered(std::iter::empty())`:  receipt.t == 1, flake_count == 0   ← the base's rule on this path

I actually think this is the right direction — it's what the builder already did, and a transact() that disagrees with stage(&handle) about whether a graph got registered is exactly the drift this PR exists to kill. But the rule is broader than the intent the old comment stated ("CREATE GRAPH: zero staged flakes, but a graph_delta IRI the registry doesn't know yet"), and a no-match update that merely mentions a graph in an INSERT template isn't a CREATE GRAPH.

So two things would settle it: a line in the "Behavior" section saying this path changed too, and a test — nothing in the suite distinguishes HEAD from base here today, which is why I had to write one. If instead you'd rather the rule stay narrow, the distinguishing signal is presumably txn.graph_mgmt / the SPARQL CREATE GRAPH lowering rather than "any unregistered IRI in the delta".

Good catch, thanks. I kept the broad rule: the point of this PR is that transact() and the builder stop disagreeing, and the builder's rule was the one already in use. Narrowing it to CREATE GRAPH raises a wider question: should a graph a template names but writes nothing to be registered at all? Commits with other flakes do that today on every path, so that belongs in its own decision, not the no-op case alone.

Documented in the Behavior section and pinned in 8321b52 by no_match_update_naming_a_new_graph_commits_its_registration, which is your probe: t advances, zero flakes, graph registered. It fails with has_unregistered neutralised.

Optional — this test now passes whichever behaviour is in effect, so it isn't a net for the change any more.

Under the mutation that reverts the no-op rule (TxnType::Update | TxnType::Upsert → TxnType::Upsert), the other three tests this PR touches go red and this one stays green — the Err(_) => {} arm absorbs the old 500.

I get why it was relaxed: what the test cares about is "nothing was written", and asserting after.t() == before_t says that far more directly than asserting an error string did. So this isn't wrong, it's just no longer load-bearing. Since tracked_update_matching_nothing_is_a_noop covers the no-op rule directly, maybe the cleanest thing is to drop the Err(_) arm and let this test assert the outcome it now expects:

// The profile cannot see a Person, so the WHERE matches nothing: the
// update is a no-op that writes nothing and does not advance the ledger.
let before_t = ledger.t();
let after = try_txn(&fluree, ledger, TxnType::Update, &update_person, &ctx)
    .await
    .expect("a view-filtered no-match update is a no-op, not an error");
assert_eq!(
    after.t(),
    before_t,
    "Lead write profile must NOT edit a Person"
);

Minor and non-blocking — but if you agree it's right, I'd rather see it folded in now than lost in the backlog.

Done in 8321b52, using your version: it expects the no-op and asserts t did not advance. With the no-op rule reverted it now goes red (it stayed green before).

More of a question than a suggestion — sync_graph: None is hardcoded here while every sibling StageResult derives it from txn.sync_graph.

It's accurate today — this entry point parses through parse_transaction, and only parse_sync_transaction ever sets sync_graph — but it was inert before this PR and isn't any more: is_noop reads that field, so if a sync ever reaches this door it silently loses the no-op and lands back on the 500 this PR just fixed.

Given #1932 is adding stage_sync_transaction_tracked immediately on top of this branch, it seems worth either deriving it (let sync_graph = txn.sync_graph.clone(); alongside txn_meta, one line up) or leaving a comment saying why None is structurally guaranteed here.

Derived it in 8321b52: let sync_graph = txn.sync_graph.clone(); alongside txn_meta, so a sync reaching this entry point keeps its no-op.

Praise — reading the routing from flake_graph_ids instead of re-deriving it is the best decision in the PR, and it deletes a silent failure mode.

The old flake_graph_id's unwrap_or(0) meant a named-graph flake whose IRI didn't resolve through staged_ns.lookup_sid_for_iri got validated and uniqueness-checked against the default graph, with nothing to announce it. StagedOverlay::from_flakes is strict about the same question, so reading the answer from there can't fail quietly.

A coverage note while we're on this routing, because it's more useful to you than to me. I mutation-checked it — collapsed staged_flakes_by_graph() to g_id = 0, which is the old unwrap_or(0) fallback firing on every flake — and it is pinned: five tests go red, in it_config_graph (shacl_rejects_violation_in_any_named_graph_{sparql,jsonld} and shacl_per_graph_mode_applies_to_graph_variable_targets) and it_constraints_inline (both inline_unique_property_enforced_in_second_named_graph_*). So SHACL and uniqueness are both genuinely covered. What surprised me is where it isn't: that same mutation left all 86 tests green across it_sync_graph, it_branch_shacl, it_policy_named_graphs, it_enforce_unique*, it_named_graphs and it_named_graph_isolation — i.e. every suite whose name suggests it would catch a graph-routing regression. Nothing for this PR to fix, but worth knowing that the net under this code is narrower, and sits somewhere other than where you'd reach for it.

The one thing I'd flag for a future reader: zip truncates to the shorter side, so if flake_graph_ids ever fell out of lock-step with store.flakes the consequence is silently dropped flakes from SHACL and uniqueness, not a panic. It's correct today — both are built in the one pass at staged.rs:83-97 and neither is mutated after — and honestly a debug_assert_eq! on the two lengths is probably over-engineering for a private field with one writer. Raising it only because the failure shape is "validation quietly stops seeing some of your data", which is the worst one to discover late.

Praise — stage_and_check keeping txn_context and the error mapping at the caller is the right seam.

Those two really are the only things that legitimately differ per entry point, and the tracked path at tx.rs:3178 deliberately not going through stage_options (because it attaches its tracker unconditionally rather than gating on is_enabled()) is the kind of detail a consolidation like this usually flattens by accident. Nice.