From 5532bdcde9880fa0eced2c47dfbf3fadedee32be Mon Sep 17 00:00:00 2001 From: Pavlo Golub Date: Fri, 9 Oct 2026 19:44:54 +0200 Subject: [PATCH] [*] fix error hint in `timetable.cron_split_to_arrays` The hint was built with `+` instead of `||`, so an unrecognized cron value raised 42725 (operator is not unique) instead of the intended 'Value ("...") not recognized' error. Migration 00850 replaces the function on existing installs, and dbapi is bumped to 00850. TestMigrations checks the migrated function and TestCronSplitToArraysUnknownValue checks a fresh install. --- cmd/pg_timetable/main.go | 2 +- internal/pgengine/migration.go | 6 ++ internal/pgengine/migration_test.go | 25 ++++++++ internal/pgengine/sql/cron.sql | 6 +- internal/pgengine/sql/init.sql | 3 +- internal/pgengine/sql/migrations/00850.sql | 75 ++++++++++++++++++++++ 6 files changed, 112 insertions(+), 5 deletions(-) create mode 100644 internal/pgengine/sql/migrations/00850.sql diff --git a/cmd/pg_timetable/main.go b/cmd/pg_timetable/main.go index 83138303..5e22b2c0 100644 --- a/cmd/pg_timetable/main.go +++ b/cmd/pg_timetable/main.go @@ -94,7 +94,7 @@ var ( commit = "000000" version = "master" date = "unknown" - dbapi = "00820" + dbapi = "00850" ) func printVersion() { diff --git a/internal/pgengine/migration.go b/internal/pgengine/migration.go index c21eaacd..56033590 100644 --- a/internal/pgengine/migration.go +++ b/internal/pgengine/migration.go @@ -174,6 +174,12 @@ var Migrations func() migrator.Option = func() migrator.Option { return ExecuteMigrationScript(ctx, tx, "00820.sql") }, }, + &migrator.Migration{ + Name: "00850 Fix error hint in timetable.cron_split_to_arrays", + Func: func(ctx context.Context, tx pgx.Tx) error { + return ExecuteMigrationScript(ctx, tx, "00850.sql") + }, + }, // &migrator.Migration{ // Name: "000XX Short description of a migration", diff --git a/internal/pgengine/migration_test.go b/internal/pgengine/migration_test.go index 4bd6a0c1..b49fa7e4 100644 --- a/internal/pgengine/migration_test.go +++ b/internal/pgengine/migration_test.go @@ -3,12 +3,15 @@ package pgengine_test import ( "context" _ "embed" + "errors" "testing" "github.com/cybertec-postgresql/pg_timetable/internal/pgengine" "github.com/cybertec-postgresql/pg_timetable/internal/testutils" migrator "github.com/cybertec-postgresql/pgx-migrator" + "github.com/jackc/pgx/v5/pgconn" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) //go:embed sql/migrations/00000.sql @@ -37,6 +40,28 @@ func TestMigrations(t *testing.T) { JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace WHERE n.nspname='timetable' AND c.relname='secret')`).Scan(&hasSecret)) assert.True(t, hasSecret, "00820 must create timetable.secret") + + // 00850 replaces timetable.cron_split_to_arrays on existing installs. + assertCronSplitRejectsValue(t, pge) +} + +// TestCronSplitToArraysUnknownValue checks the function created by cron.sql on a fresh install. +func TestCronSplitToArraysUnknownValue(t *testing.T) { + container, cleanup := testutils.SetupPostgresContainer(t) + defer cleanup() + assertCronSplitRejectsValue(t, container.Engine) +} + +// assertCronSplitRejectsValue expects the "not recognized" exception, not the +// 42725 error the old `text + text` hint raised. +func assertCronSplitRejectsValue(t *testing.T, pge *pgengine.PgEngine) { + t.Helper() + _, err := pge.ConfigDb.Exec(context.Background(), "SELECT timetable.cron_split_to_arrays('foo * * * *')") + var pgErr *pgconn.PgError + require.True(t, errors.As(err, &pgErr), "expected a PostgreSQL error, got %v", err) + assert.Equal(t, "P0001", pgErr.Code) + assert.Equal(t, `Value ("foo") not recognized`, pgErr.Message) + assert.Contains(t, pgErr.Hint, "fields separated by space or tab. Values allowed") } func TestExecuteMigrationScript(t *testing.T) { assert.Error(t, pgengine.ExecuteMigrationScript(context.Background(), nil, "foo"), "File does not exist") diff --git a/internal/pgengine/sql/cron.sql b/internal/pgengine/sql/cron.sql index 96c9e2be..7df72657 100644 --- a/internal/pgengine/sql/cron.sql +++ b/internal/pgengine/sql/cron.sql @@ -48,9 +48,9 @@ BEGIN a_res := array_cat(a_res, a_range); ELSE RAISE EXCEPTION 'Value ("%") not recognized', a_element[i_index] - USING HINT = 'fields separated by space or tab.'+ - 'Values allowed: numbers (value list with ","), '+ - 'any value with "*", range of value with "-" and step values with "/"!'; + USING HINT = 'fields separated by space or tab. ' || + 'Values allowed: numbers (value list with ","), ' || + 'any value with "*", range of value with "-" and step values with "/"!'; END IF; END LOOP; SELECT diff --git a/internal/pgengine/sql/init.sql b/internal/pgengine/sql/init.sql index 1604049d..c813ddf2 100644 --- a/internal/pgengine/sql/init.sql +++ b/internal/pgengine/sql/init.sql @@ -31,4 +31,5 @@ VALUES (15, '00733 Add params column to timetable.execution_log table'), (16, '00792 Add ability to enable and disable tasks'), (17, '00797 Add indexes to timetable.execution_log'), - (18, '00820 Add timetable.secret store'); + (18, '00820 Add timetable.secret store'), + (19, '00850 Fix error hint in timetable.cron_split_to_arrays'); diff --git a/internal/pgengine/sql/migrations/00850.sql b/internal/pgengine/sql/migrations/00850.sql new file mode 100644 index 00000000..2cda64e0 --- /dev/null +++ b/internal/pgengine/sql/migrations/00850.sql @@ -0,0 +1,75 @@ +-- 00850 Fix error hint in timetable.cron_split_to_arrays +CREATE OR REPLACE FUNCTION timetable.cron_split_to_arrays( + cron text, + OUT mins integer[], + OUT hours integer[], + OUT days integer[], + OUT months integer[], + OUT dow integer[] +) RETURNS record AS $$ +DECLARE + a_element text[]; + i_index integer; + a_tmp text[]; + tmp_item text; + a_range int[]; + a_split text[]; + a_res integer[]; + max_val integer; + min_val integer; + dimensions constant text[] = '{"minutes", "hours", "days", "months", "days of week"}'; + allowed_ranges constant integer[][] = '{{0,59},{0,23},{1,31},{1,12},{0,7}}'; +BEGIN + a_element := regexp_split_to_array(cron, '\s+'); + FOR i_index IN 1..5 LOOP + a_res := NULL; + a_tmp := string_to_array(a_element[i_index],','); + FOREACH tmp_item IN ARRAY a_tmp LOOP + IF tmp_item ~ '^[0-9]+$' THEN -- normal integer + a_res := array_append(a_res, tmp_item::int); + ELSIF tmp_item ~ '^[*]+$' THEN -- '*' any value + a_range := array(select generate_series(allowed_ranges[i_index][1], allowed_ranges[i_index][2])); + a_res := array_cat(a_res, a_range); + ELSIF tmp_item ~ '^[0-9]+[-][0-9]+$' THEN -- '-' range of values + a_range := regexp_split_to_array(tmp_item, '-'); + a_range := array(select generate_series(a_range[1], a_range[2])); + a_res := array_cat(a_res, a_range); + ELSIF tmp_item ~ '^[0-9]+[\/][0-9]+$' THEN -- '/' step values + a_range := regexp_split_to_array(tmp_item, '/'); + a_range := array(select generate_series(a_range[1], allowed_ranges[i_index][2], a_range[2])); + a_res := array_cat(a_res, a_range); + ELSIF tmp_item ~ '^[0-9-]+[\/][0-9]+$' THEN -- '-' range of values and '/' step values + a_split := regexp_split_to_array(tmp_item, '/'); + a_range := regexp_split_to_array(a_split[1], '-'); + a_range := array(select generate_series(a_range[1], a_range[2], a_split[2]::int)); + a_res := array_cat(a_res, a_range); + ELSIF tmp_item ~ '^[*]+[\/][0-9]+$' THEN -- '*' any value and '/' step values + a_split := regexp_split_to_array(tmp_item, '/'); + a_range := array(select generate_series(allowed_ranges[i_index][1], allowed_ranges[i_index][2], a_split[2]::int)); + a_res := array_cat(a_res, a_range); + ELSE + RAISE EXCEPTION 'Value ("%") not recognized', a_element[i_index] + USING HINT = 'fields separated by space or tab. ' || + 'Values allowed: numbers (value list with ","), ' || + 'any value with "*", range of value with "-" and step values with "/"!'; + END IF; + END LOOP; + SELECT + ARRAY_AGG(x.val), MIN(x.val), MAX(x.val) INTO a_res, min_val, max_val + FROM ( + SELECT DISTINCT UNNEST(a_res) AS val ORDER BY val) AS x; + IF max_val > allowed_ranges[i_index][2] OR min_val < allowed_ranges[i_index][1] OR a_res IS NULL THEN + RAISE EXCEPTION '% is out of range % for %', tmp_item, allowed_ranges[i_index:i_index][:], dimensions[i_index]; + END IF; + CASE i_index + WHEN 1 THEN mins := a_res; + WHEN 2 THEN hours := a_res; + WHEN 3 THEN days := a_res; + WHEN 4 THEN months := a_res; + ELSE + dow := a_res; + END CASE; + END LOOP; + RETURN; +END; +$$ LANGUAGE PLPGSQL STRICT;