Skip to content

Commit f3d91fa

Browse files
author
t
committed
fix(node): make the SDL mutation-fence wiring load-bearing; admit deprecated
An adversarial review of the SDL switch found three gaps: - The fence's completeness source (SDL) and its hidden-mutation cross-check were not load-bearing: because the real MutationRoot has no visible=false field, reverting `fields` back to introspection and dropping the cross-check left the whole suite green (INV-21). Extract the sourcing + cross-check into a generic `assert_mutation_fence<Q,M,S>` that the real fence AND a synthetic hidden schema both drive. A new #[should_panic] test (fence_rejects_a_hidden_mutation) feeds a visible=false mutation that IS registered, so only the cross-check can fire; reverting the SDL source or deleting the cross-check now changes/removes that panic and turns the test RED (verified). - The introspection cross-check used bare `fields { name }` (GraphQL default includeDeprecated:false), so a `#[graphql(deprecation = "...")]` mutation -- executable and present in SDL -- would false-red as a hidden visible=false mutation. Use `fields(includeDeprecated: true)`; a new test (fence_admits_a_deprecated_mutation) pins it, going RED without the flag. - schema_lists_exactly_the_registered_mutations still sourced from introspection and its comment called introspection "the source of truth", contradicting the fix. Retargeted at the SDL set with a corrected comment. Also folds the two introspection navigations into one generic helper (introspection_mutation_fields) and points the version-safety test at it, so there is a single introspection query. cargo test -p gitlawb-node --bins green (497); clippy and fmt clean. Only api/mod.rs changed.
1 parent 11db151 commit f3d91fa

1 file changed

Lines changed: 156 additions & 71 deletions

File tree

  • crates/gitlawb-node/src/api

‎crates/gitlawb-node/src/api/mod.rs‎

Lines changed: 156 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -290,37 +290,20 @@ mod authz_guard {
290290
("fail_task", "failTask", "did_matches("),
291291
];
292292

293-
// Completeness source is the SDL (#219): it includes visible=false mutations,
294-
// which introspection omits. Cross-check against introspection first so a hidden
295-
// mutation fails with a specific, actionable message rather than a generic
296-
// "unclassified".
297-
let fields = graphql_mutation_fields_from_sdl();
298-
let introspected = graphql_mutation_fields_from_schema();
299-
for f in &fields {
300-
assert!(
301-
introspected.iter().any(|i| i == f),
302-
"GraphQL mutation `{f}` is in the schema but hidden from introspection \
303-
(a `#[graphql(visible = false)]` field): hidden mutations are forbidden \
304-
by this fence — make it visible so its classification is enforceable, or \
305-
remove it"
306-
);
307-
}
308-
309-
// Completeness both directions against the authoritative (SDL) field set.
310-
for f in &fields {
311-
assert!(
312-
registered.iter().any(|(_, camel, _)| camel == f),
313-
"unclassified GraphQL mutation `{f}`: add it to \
314-
every_graphql_mutation_has_its_gate with a deliberate gate \
315-
(`require_repo_owner(` for a repo-write, `did_matches(` for a task op)"
316-
);
317-
}
318-
for (_, camel, _) in registered {
319-
assert!(
320-
fields.iter().any(|f| f == camel),
321-
"registered GraphQL mutation `{camel}` is not in the schema \
322-
(renamed or removed); update the table"
323-
);
293+
// Completeness + hidden-mutation cross-check against the REAL schema (#219),
294+
// delegated to the generic `assert_mutation_fence` so the SDL sourcing and the
295+
// cross-check are ALSO exercised by a synthetic hidden schema in
296+
// `fence_rejects_a_hidden_mutation` (INV-21). Because the real MutationRoot has no
297+
// hidden field today, that synthetic test — not a hypothetical production canary —
298+
// is what turns reverting the SDL source or deleting the cross-check RED.
299+
{
300+
use crate::graphql::mutation::MutationRoot;
301+
use crate::graphql::query::QueryRoot;
302+
use crate::graphql::subscription::SubscriptionRoot;
303+
let schema =
304+
async_graphql::Schema::build(QueryRoot, MutationRoot, SubscriptionRoot).finish();
305+
let registered_camel: Vec<&str> = registered.iter().map(|(_, c, _)| *c).collect();
306+
assert_mutation_fence(&schema, &registered_camel);
324307
}
325308

326309
// Marker present (source scrape; the vacuity limit, backstopped by the
@@ -344,7 +327,7 @@ mod authz_guard {
344327
/// RED — the assumption is load-bearing, not trusted.
345328
#[test]
346329
fn sdl_source_sees_hidden_mutations_that_introspection_omits() {
347-
use async_graphql::{EmptySubscription, Object, Schema, Value};
330+
use async_graphql::{EmptySubscription, Object, Schema};
348331
struct Q;
349332
#[Object]
350333
impl Q {
@@ -374,27 +357,9 @@ mod authz_guard {
374357
completeness guarantee is broken: {sdl_fields:?}"
375358
);
376359

377-
// Introspection (the OLD source): omits the hidden field. This is the exact gap
378-
// the fix closes — an introspection-sourced fence never sees `hiddenMut`.
379-
let resp = tokio::runtime::Runtime::new()
380-
.expect("tokio runtime")
381-
.block_on(schema.execute("{ __schema { mutationType { fields { name } } } }"));
382-
let mut introspected = Vec::new();
383-
if let Value::Object(root) = &resp.data {
384-
if let Some(Value::Object(sc)) = root.get("__schema") {
385-
if let Some(Value::Object(mt)) = sc.get("mutationType") {
386-
if let Some(Value::List(fl)) = mt.get("fields") {
387-
for fo in fl {
388-
if let Value::Object(o) = fo {
389-
if let Some(Value::String(n)) = o.get("name") {
390-
introspected.push(n.clone());
391-
}
392-
}
393-
}
394-
}
395-
}
396-
}
397-
}
360+
// Introspection (the fence's own helper): OMITS the hidden field. This is the
361+
// exact gap the fix closes — an introspection-sourced fence never sees `hiddenMut`.
362+
let introspected = introspection_mutation_fields(&schema);
398363
assert!(
399364
!introspected.iter().any(|f| f == "hiddenMut"),
400365
"introspection is expected to OMIT the visible=false mutation (that is the \
@@ -413,23 +378,141 @@ mod authz_guard {
413378
);
414379
}
415380

416-
/// The mutation field names (camelCase, as async-graphql exposes them) via the
417-
/// engine's own introspection. STRUCTURED: the names arrive as data, so nothing in
418-
/// the resolver source or the serialized SDL (a brace in a doc description, a
419-
/// comment, a string) can hide a field. The schema builds with no data and
420-
/// introspection runs no resolver, so this stays a plain DB-free unit test. A
421-
/// navigation miss yields an empty list, which fails loud via the set-equality
422-
/// above (a registered name reads as "not in the schema"), never vacuously.
423-
fn graphql_mutation_fields_from_schema() -> Vec<String> {
424-
use crate::graphql::mutation::MutationRoot;
425-
use crate::graphql::query::QueryRoot;
426-
use crate::graphql::subscription::SubscriptionRoot;
381+
/// INV-21 wiring guard (#219): drives the generic `assert_mutation_fence` — the SAME
382+
/// helper the real fence calls — against a synthetic schema whose mutation is
383+
/// `visible = false` and IS registered, so completeness passes and ONLY the
384+
/// hidden-from-introspection cross-check can fail. This is what makes the fence's SDL
385+
/// sourcing load-bearing without a production canary: reverting the source to
386+
/// introspection (the hidden field then absent from the SDL set) or deleting the
387+
/// cross-check removes this panic (or changes its message), turning the
388+
/// `should_panic` test RED. The real MutationRoot has no hidden field, so nothing on
389+
/// the real schema could catch that regression.
390+
#[test]
391+
#[should_panic(expected = "hidden from introspection")]
392+
fn fence_rejects_a_hidden_mutation() {
393+
use async_graphql::{EmptySubscription, Object, Schema};
394+
struct Q;
395+
#[Object]
396+
impl Q {
397+
async fn ping(&self) -> i32 {
398+
0
399+
}
400+
}
401+
struct M;
402+
#[Object]
403+
impl M {
404+
async fn visible_mut(&self) -> i32 {
405+
1
406+
}
407+
#[graphql(visible = false)]
408+
async fn hidden_mut(&self) -> i32 {
409+
2
410+
}
411+
}
412+
let schema = Schema::build(Q, M, EmptySubscription).finish();
413+
// Both fields registered, so completeness passes and only the cross-check fires.
414+
assert_mutation_fence(&schema, &["visibleMut", "hiddenMut"]);
415+
}
416+
417+
/// Deprecated-mutation guard (#219): a `#[graphql(deprecation = "...")]` mutation is
418+
/// executable and present in SDL, but GraphQL introspection omits it by default. The
419+
/// fence's introspection helper uses `includeDeprecated: true`, so a deprecated (but
420+
/// visible) mutation is NOT mistaken for a hidden one. With it registered, the fence
421+
/// must NOT panic. Remove `includeDeprecated: true` and this goes RED — the deprecated
422+
/// field would look hidden and false-red as a forbidden visible=false mutation.
423+
#[test]
424+
fn fence_admits_a_deprecated_mutation() {
425+
use async_graphql::{EmptySubscription, Object, Schema};
426+
struct Q;
427+
#[Object]
428+
impl Q {
429+
async fn ping(&self) -> i32 {
430+
0
431+
}
432+
}
433+
struct M;
434+
#[Object]
435+
impl M {
436+
async fn visible_mut(&self) -> i32 {
437+
1
438+
}
439+
#[graphql(deprecation = "use visibleMut")]
440+
async fn deprecated_mut(&self) -> i32 {
441+
2
442+
}
443+
}
444+
let schema = Schema::build(Q, M, EmptySubscription).finish();
445+
// Must not panic: the deprecated field is visible (present in introspection with
446+
// includeDeprecated) and registered, so it is neither hidden nor unclassified.
447+
assert_mutation_fence(&schema, &["visibleMut", "deprecatedMut"]);
448+
}
449+
450+
/// Completeness + hidden-mutation cross-check for a BUILT schema (#219). Generic over
451+
/// the schema type so the real fence AND the synthetic `fence_rejects_a_hidden_mutation`
452+
/// / `fence_admits_a_deprecated_mutation` tests drive the SAME sourcing and cross-check.
453+
/// Completeness is sourced from SDL (which includes `visible = false` fields); the
454+
/// cross-check fails any SDL field hidden from introspection. This genericity is the
455+
/// INV-21 fix: because the real MutationRoot has no hidden field, only a synthetic
456+
/// hidden schema through THIS helper makes reverting the SDL source or removing the
457+
/// cross-check RED, rather than leaving the class latent behind a production canary.
458+
fn assert_mutation_fence<Q, M, S>(
459+
schema: &async_graphql::Schema<Q, M, S>,
460+
registered_camel: &[&str],
461+
) where
462+
Q: async_graphql::ObjectType + 'static,
463+
M: async_graphql::ObjectType + 'static,
464+
S: async_graphql::SubscriptionType + 'static,
465+
{
466+
let sdl_fields = mutation_field_names_from_sdl(&schema.sdl());
467+
let introspected = introspection_mutation_fields(schema);
468+
// No SDL field may be hidden from introspection (a visible=false mutation).
469+
for f in &sdl_fields {
470+
assert!(
471+
introspected.iter().any(|i| i == f),
472+
"GraphQL mutation `{f}` is in the schema but hidden from introspection \
473+
(a `#[graphql(visible = false)]` field): hidden mutations are forbidden \
474+
by this fence — make it visible so its classification is enforceable, or \
475+
remove it"
476+
);
477+
}
478+
// Completeness both directions against the registered (camelCase) set.
479+
for f in &sdl_fields {
480+
assert!(
481+
registered_camel.contains(&f.as_str()),
482+
"unclassified GraphQL mutation `{f}`: add it to \
483+
every_graphql_mutation_has_its_gate with a deliberate gate \
484+
(`require_repo_owner(` for a repo-write, `did_matches(` for a task op)"
485+
);
486+
}
487+
for c in registered_camel {
488+
assert!(
489+
sdl_fields.iter().any(|f| f == c),
490+
"registered GraphQL mutation `{c}` is not in the schema \
491+
(renamed or removed); update the table"
492+
);
493+
}
494+
}
495+
496+
/// The mutation field names via the engine's introspection, run against a passed
497+
/// schema. STRUCTURED (names arrive as data). Uses `fields(includeDeprecated: true)`:
498+
/// GraphQL's default omits `@deprecated` fields, but a `#[graphql(deprecation = "...")]`
499+
/// mutation is still executable and present in SDL, so without this the cross-check
500+
/// above would false-red a deprecated mutation as a hidden (visible=false) one. Only a
501+
/// real visibility filter should trip that arm; deprecation is a lifecycle marker.
502+
fn introspection_mutation_fields<Q, M, S>(
503+
schema: &async_graphql::Schema<Q, M, S>,
504+
) -> Vec<String>
505+
where
506+
Q: async_graphql::ObjectType + 'static,
507+
M: async_graphql::ObjectType + 'static,
508+
S: async_graphql::SubscriptionType + 'static,
509+
{
427510
use async_graphql::Value;
428-
let schema =
429-
async_graphql::Schema::build(QueryRoot, MutationRoot, SubscriptionRoot).finish();
430511
let resp = tokio::runtime::Runtime::new()
431512
.expect("tokio runtime")
432-
.block_on(schema.execute("{ __schema { mutationType { fields { name } } } }"));
513+
.block_on(schema.execute(
514+
"{ __schema { mutationType { fields(includeDeprecated: true) { name } } } }",
515+
));
433516
assert!(
434517
resp.errors.is_empty(),
435518
"schema introspection failed: {:?}",
@@ -661,17 +744,19 @@ mod authz_guard {
661744
out
662745
}
663746

664-
/// The introspection field set (the completeness source of truth) is exactly the
747+
/// The SDL field set (the completeness source of truth, #219) is exactly the
665748
/// registered mutations. If async-graphql adds or renames a field, this reddens
666-
/// and forces the `every_graphql_mutation_has_its_gate` table to be updated.
749+
/// and forces the `every_graphql_mutation_has_its_gate` table to be updated. Uses
750+
/// SDL, not introspection: introspection omits `visible = false` fields, so an
751+
/// introspection-based pin here would go green while a hidden mutation existed.
667752
#[test]
668753
fn schema_lists_exactly_the_registered_mutations() {
669-
let mut fields = graphql_mutation_fields_from_schema();
754+
let mut fields = graphql_mutation_fields_from_sdl();
670755
fields.sort();
671756
assert_eq!(
672757
fields,
673758
vec!["claimTask", "completeTask", "createTask", "failTask"],
674-
"the introspected mutation field set is the authoritative discovered set"
759+
"the SDL mutation field set is the authoritative discovered set"
675760
);
676761
}
677762

0 commit comments

Comments
 (0)