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;