Skip to content

test(node): GraphQL opaque-error scrape is blind to a bare ?; make the bypass a compile error #293

Description

@beardthelion

every_graphql_map_err_uses_opaque_helpers (added in #255, crates/gitlawb-node/src/graphql/mod.rs:163) scans query.rs and mutation.rs for .map_err( and asserts each one routes through graphql_db_err / graphql_app_err or discards with |_|. It is keyed on the shape of the fix, so it cannot see a sink that carries no marker at all.

async-graphql ships a blanket conversion (async-graphql-7.2.1/src/error.rs:320):

impl<T: Display + Send + Sync + 'static> From<T> for Error {
    fn from(e: T) -> Self { Self { message: e.to_string(), .. } }
}

So db.foo().await? in a resolver hands the raw error text straight to the client, with no .map_err( on the line for the scan to find. That is #250 again.

There is no leak today. All 13 .map_err sites route through the helpers, the direct Error::new calls take string literals, and subscription.rs and types.rs have no error paths. This is about the next resolver, not the current ones.

What I measured, replacing .map_err(graphql_db_err)? with a bare ?:

  • in repos: compiles, the scrape still passes, and repos_query_db_error_message_is_opaque fails with column "is_public" does not exist reaching the client.
  • in tasks: compiles, and the entire graphql suite stays green at 21 passed.

The difference is that only two of the seven resolvers have a test that drives a real DB fault through them, repos (query.rs:438) and create_task (mutation.rs:246). For the other five there is nothing between a bare ? and a silent reintroduction.

Suggested fix

async-graphql has a custom-error-conversion feature whose only effect is to drop that blanket impl and replace it with From<&'static str> and From<String>. Enabling it turns the bypass into a compile error rather than something a text scan has to chase:

async-graphql = { version = "7", features = ["chrono", "uuid", "tracing", "custom-error-conversion"] }

I built this both ways at fe0cd35b. With the feature on, cargo check -p gitlawb-node --all-targets finishes clean in 4m13s with no source changes. With the feature on and a bare ? reintroduced, it fails:

error[E0277]: `?` couldn't convert the error to `async_graphql::Error`
  --> crates/gitlawb-node/src/graphql/query.rs:14:54
   the trait `From<anyhow::Error>` is not implemented for `async_graphql::Error`

Blast radius is contained: gitlawb-node is the only crate depending on async-graphql, and no async_graphql::Error or async_graphql::Result appears outside src/graphql/.

The alternative, if the feature turns out to be unwanted for another reason, is a second rule in the same test loop rejecting a bare .await? in those two files. Neither file contains one today, so it would not fire on current source. It is strictly weaker, since it only covers the two files the scrape happens to read.

Worth doing either way while touching this: subscription.rs is mounted in the schema (mod.rs:15) but is not in the scrape's file list, and the loop has no floor on how many sites it checked, so "scraped nothing" and "scraped everything, all compliant" are the same green.

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:nodegitlawb-node — the serving node and REST APIkind:testTest coverage or harnesssev:mediumDegraded but workaround existssubsystem:apiNode REST API request/response surface

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions