From 5382c20ac0795628eb69c2383e81d7650d37c915 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 21 Jul 2026 15:36:27 -0500 Subject: [PATCH] Test roles: single source of truth with quoting-requiring names Move the test-role names into one file, test/roles.sql, \i'd per test by test/deps.sql instead of hard-coding the names inline. Previously the names were spelled out in deps.sql and implied across the test files. Rename the roles from cat_tools_testing__use_role / __no_use_role to "cat_tools testing: USE" / "cat_tools testing: NO USE" -- names that REQUIRE identifier quoting. The point is defensive: if the code under test (or a test) ever emits an unquoted role reference, the space/mixed-case name turns that latent quoting bug into a hard test failure instead of passing silently on a conveniently quote-free name. Every reference is quoted accordingly (:"use_role" in identifier position). test/sql/permissions.sql already used the quoted form; this makes the names actually exercise it, and brings the remaining test files and deps.sql/setup.sql into line. Test-harness only: no extension SQL or versioned files change. Co-Authored-By: Claude Opus 4.8 --- test/deps.sql | 10 +++++----- test/install/load.sql | 19 ++++++++++--------- test/roles.sql | 9 +++++++++ test/setup.sql | 2 +- test/sql/attribute.sql | 4 ++-- test/sql/enum.sql | 4 ++-- test/sql/extension.sql | 2 +- test/sql/function.sql | 4 ++-- test/sql/name__check.sql | 4 ++-- test/sql/object_type.sql | 6 +++--- test/sql/pg_depends.sql | 4 ++-- test/sql/relation_type.sql | 6 +++--- test/sql/sequence.sql | 6 +++--- test/sql/trigger.sql | 4 ++-- 14 files changed, 47 insertions(+), 37 deletions(-) create mode 100644 test/roles.sql diff --git a/test/deps.sql b/test/deps.sql index 09a7356..497f8a7 100644 --- a/test/deps.sql +++ b/test/deps.sql @@ -9,9 +9,9 @@ SET client_min_messages = WARNING; * the test/sql/ files). psql variables are session-local, not committed DB * state, so they must still be (re)set per test file. * - * The role names are also set by test/install/load.sql (which creates the - * roles); keep the two in sync. Add any OTHER per-test dependency - * \set/statements here; committed dependencies belong in test/install/load.sql. + * The role names live in exactly one place, test/roles.sql, \i'd here and by + * test/install/load.sql (which creates the roles). Add any OTHER per-test + * dependency \set/statements here; committed dependencies belong in + * test/install/load.sql. */ -\set no_use_role cat_tools_testing__no_use_role -\set use_role cat_tools_testing__use_role +\i test/roles.sql diff --git a/test/install/load.sql b/test/install/load.sql index 38d3ccd..6df45cc 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -35,14 +35,15 @@ SET client_min_messages = WARNING; /* - * The test-role names, set here and mirrored per-test by test/deps.sql (psql - * variables are session-local, so both this committed installer and each - * rolled-back test file must set them). Each :var holds the identifier; the - * pg_temp helper functions below receive it as a text literal (:'use_role') and - * quote it via format(%I). + * The test-role names live in one place, test/roles.sql, \i'd here and mirrored + * per-test by test/deps.sql (psql variables are session-local, so both this + * committed installer and each rolled-back test file must set them). Each :var + * holds the identifier; the pg_temp helper functions below receive it as a text + * literal (:'use_role') and quote it via format(%I); the identifier-position + * uses below (GRANT ... TO :"use_role") quote it directly. The names require + * quoting, so an unquoted reference would surface as a test failure. */ -\set no_use_role cat_tools_testing__no_use_role -\set use_role cat_tools_testing__use_role +\i test/roles.sql /* * Mode selection. The Makefile always exports cat_tools.test_load_mode via @@ -186,12 +187,12 @@ $$; SELECT pg_temp.create_role(:'no_use_role'); SELECT pg_temp.create_role(:'use_role'); -GRANT cat_tools__usage TO :use_role; +GRANT cat_tools__usage TO :"use_role"; /* * PG15+ removed CREATE on the public schema from PUBLIC; grant it explicitly * for tests that create shadow names in public to check catalog-lookup * correctness. */ -GRANT CREATE ON SCHEMA public TO :use_role; +GRANT CREATE ON SCHEMA public TO :"use_role"; -- vi: expandtab ts=2 sw=2 diff --git a/test/roles.sql b/test/roles.sql new file mode 100644 index 0000000..29a29bf --- /dev/null +++ b/test/roles.sql @@ -0,0 +1,9 @@ +/* + * Single source of truth for the test role names. \i'd by test/install/load.sql + * (which creates the roles, committed-once) and per-test by test/deps.sql (psql + * variables are session-local, so each rolled-back test file must re-set them). + * The names intentionally require quoting, so a quoting bug in the code under + * test surfaces as a failure rather than passing silently on a quote-free name. + */ +\set no_use_role 'cat_tools testing: NO USE' +\set use_role 'cat_tools testing: USE' diff --git a/test/setup.sql b/test/setup.sql index e0f6476..deec122 100644 --- a/test/setup.sql +++ b/test/setup.sql @@ -1,7 +1,7 @@ -- Pulls in deps.sql \i test/pgxntool/setup.sql -GRANT USAGE ON SCHEMA tap TO :use_role, :no_use_role; +GRANT USAGE ON SCHEMA tap TO :"use_role", :"no_use_role"; CREATE FUNCTION pg_temp.exec( sql text diff --git a/test/sql/attribute.sql b/test/sql/attribute.sql index b9ef1a7..6fadecc 100644 --- a/test/sql/attribute.sql +++ b/test/sql/attribute.sql @@ -18,7 +18,7 @@ SELECT plan( + 4 -- pg_attribute__get() ); -SET LOCAL ROLE :no_use_role; +SET LOCAL ROLE :"no_use_role"; SELECT throws_ok( format( @@ -33,7 +33,7 @@ SELECT throws_ok( FROM func_calls ; -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; /* * pg_attribute__get() diff --git a/test/sql/enum.sql b/test/sql/enum.sql index c37efbc..8cc9c0c 100644 --- a/test/sql/enum.sql +++ b/test/sql/enum.sql @@ -17,7 +17,7 @@ SELECT lives_ok( , 'Create test enum' ); -SET LOCAL ROLE :no_use_role; +SET LOCAL ROLE :"no_use_role"; SELECT throws_ok( format( 'SELECT cat_tools.enum_range%s( %L )', suffix, :'test_enum' ) , '42501' @@ -27,7 +27,7 @@ SELECT throws_ok( FROM unnest( array['', '_srf'] ) AS suffix ; -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; -- This will test both functions SELECT results_eq( diff --git a/test/sql/extension.sql b/test/sql/extension.sql index c6032a5..904132e 100644 --- a/test/sql/extension.sql +++ b/test/sql/extension.sql @@ -4,7 +4,7 @@ -- test_role is set in test/deps.sql -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; SELECT plan( 0 diff --git a/test/sql/function.sql b/test/sql/function.sql index 198ae72..f620123 100644 --- a/test/sql/function.sql +++ b/test/sql/function.sql @@ -20,7 +20,7 @@ SELECT plan( + 2 -- regprocedure() ); -SET LOCAL ROLE :no_use_role; +SET LOCAL ROLE :"no_use_role"; SELECT throws_ok( format( @@ -35,7 +35,7 @@ SELECT throws_ok( FROM func_calls ; -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; SELECT is( :s.function__arg_types($$IN in_int int, INOUT inout_int_array int[], OUT out_char "char", anyelement, boolean DEFAULT false$$) diff --git a/test/sql/name__check.sql b/test/sql/name__check.sql index 8a8e165..e65db56 100644 --- a/test/sql/name__check.sql +++ b/test/sql/name__check.sql @@ -7,7 +7,7 @@ SELECT plan(5); -SET LOCAL ROLE :no_use_role; +SET LOCAL ROLE :"no_use_role"; SELECT throws_ok( format( @@ -20,7 +20,7 @@ SELECT throws_ok( , 'Verify public has no perms' ); -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; SELECT lives_ok( format( diff --git a/test/sql/object_type.sql b/test/sql/object_type.sql index e230e9e..bf3e413 100644 --- a/test/sql/object_type.sql +++ b/test/sql/object_type.sql @@ -4,7 +4,7 @@ -- test_role is set in test/deps.sql -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; CREATE FUNCTION pg_temp.extra_types() RETURNS text[] LANGUAGE sql IMMUTABLE AS $$ @@ -76,7 +76,7 @@ SELECT is( , 'sanity check size of pg_temp.obj_type' ); -SET LOCAL ROLE :no_use_role; +SET LOCAL ROLE :"no_use_role"; SELECT throws_ok( format( 'SELECT NULL::%I', typename ) , '42704' -- undefined_object; not exactly correct, but close enough @@ -100,7 +100,7 @@ SELECT throws_ok( ) v(argtype) ; -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; SELECT function_returns( 'cat_tools' diff --git a/test/sql/pg_depends.sql b/test/sql/pg_depends.sql index c0bfdfa..ff149f7 100644 --- a/test/sql/pg_depends.sql +++ b/test/sql/pg_depends.sql @@ -75,7 +75,7 @@ END; * END tests to run as owner */ -SET LOCAL ROLE :no_use_role; +SET LOCAL ROLE :"no_use_role"; SELECT CASE WHEN pg_temp.major() < 903 THEN @@ -96,7 +96,7 @@ SELECT CASE FROM views ; -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; \i test/pgxntool/finish.sql diff --git a/test/sql/relation_type.sql b/test/sql/relation_type.sql index 0629ab7..9906070 100644 --- a/test/sql/relation_type.sql +++ b/test/sql/relation_type.sql @@ -4,7 +4,7 @@ -- test_role is set in test/deps.sql -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; /* * Canonical (relkind, kind) pairs per pg_class.h, hard-coded so the assertions * below actually verify the mapping rather than re-encoding enum order. @@ -45,7 +45,7 @@ SELECT is( , 'Simple sanity check of relation__relkind()' ); -SET LOCAL ROLE :no_use_role; +SET LOCAL ROLE :"no_use_role"; SELECT throws_ok( format( 'SELECT NULL::%I', typename ) , '42704' -- undefined_object; not exactly correct, but close enough @@ -69,7 +69,7 @@ SELECT throws_ok( ) v(suffix, argtype) ; -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; SELECT is(cat_tools.relation__relkind(kind)::text, relkind, format('SELECT cat_tools.relation_relkind(%L)', kind)) FROM kinds diff --git a/test/sql/sequence.sql b/test/sql/sequence.sql index 61071e3..a408b32 100644 --- a/test/sql/sequence.sql +++ b/test/sql/sequence.sql @@ -4,7 +4,7 @@ -- test_role is set in test/deps.sql -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; CREATE TEMP TABLE seqtest(s serial); CREATE TEMP TABLE functions(fname name); @@ -29,7 +29,7 @@ SELECT plan(( )::int); GRANT SELECT ON functions TO public; -SET LOCAL ROLE :no_use_role; +SET LOCAL ROLE :"no_use_role"; SELECT throws_ok( format( $$SELECT cat_tools.%I('seqtest', 's')$$, fname ) , '42501' -- insufficient_privilege @@ -39,7 +39,7 @@ SELECT throws_ok( FROM functions ; -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; SELECT function_returns( sname, fname, 'bigint'::regtype::text ) FROM fv; SELECT volatility_is( sname, fname, 'volatile' ) FROM fv; diff --git a/test/sql/trigger.sql b/test/sql/trigger.sql index 26785f9..76a3f49 100644 --- a/test/sql/trigger.sql +++ b/test/sql/trigger.sql @@ -19,7 +19,7 @@ SELECT plan( + 1 -- verify trigger__args_as_array() ); -SET LOCAL ROLE :no_use_role; +SET LOCAL ROLE :"no_use_role"; SELECT throws_ok( format( @@ -34,7 +34,7 @@ SELECT throws_ok( FROM unnest(:function_array) f ; -SET LOCAL ROLE :use_role; +SET LOCAL ROLE :"use_role"; CREATE TEMP TABLE "test table"();