fix(grants)!: require schema for bare ALL selectors

docs/src/migrations/grants.md

@@ -60,21 +60,42 @@
 
 #### grant_options
 
-| Option            | Type                                       | Description                                 |
-| ----------------- | ------------------------------------------ | ------------------------------------------- |
-| `tables`          | [Name](/migrations/#type) or `array[Name]` | Names of tables                             |
-| `schema`          | `string`                                   | if tables ALL, then schema name is required |
-| `privileges`      | `array[TablePrivileges]` or `ALL`          | list of privileges                          |
-| `roles`           | [Name](/migrations/#type) or `array[Name]` | names of roles                              |
-| `withGrantOption` | `boolean`                                  | default `false`                             |
-| `cascade`         | `boolean`                                  | default `false`                             |
+| Option            | Type                                       | Description                                   |
+| ----------------- | ------------------------------------------ | --------------------------------------------- |
+| `tables`          | [Name](/migrations/#type) or `array[Name]` | Names of tables                               |
+| `schema`          | `string`                                   | Required with the bare string `tables: 'ALL'` |
+| `privileges`      | `array[TablePrivileges]` or `ALL`          | list of privileges                            |
+| `roles`           | [Name](/migrations/#type) or `array[Name]` | names of roles                                |
+| `withGrantOption` | `boolean`                                  | default `false`                               |
+| `cascade`         | `boolean`                                  | default `false`                               |
 
 Use `{ schema: 'app', name: 'foo' }` to select a schema-qualified table. The
 top-level `schema` option is used only with `tables: 'ALL'`; named selections
 ignore it. Unqualified names resolve through the connection's `search_path`,
 which the runner's `schema` option sets when supplied. The same selection rules
 apply to `revokeOnTables` and automatic grant reversal.
 
+The bare string `tables: 'ALL'` requires a top-level `schema`; omitting it
+throws an error before SQL is generated. To select a table literally named
+`ALL`, use `tables: { name: 'ALL' }` or a schema-qualified Name object.
+Arrays such as `tables: ['ALL', 'foo']` always select literal names, even
+when a top-level `schema` is present.
+
+```ts
+pgm.grantOnTables({
+  tables: 'ALL',
+  schema: 'app',
+  roles: 'reader',
+  privileges: 'SELECT',
+});
+
+pgm.grantOnTables({
+  tables: { name: 'ALL' },
+  roles: 'reader',
+  privileges: 'SELECT',
+});
+```
+
 ## Reverse Operation: `revokeOnTables`
 
 #### `pgm.revokeOnTables( revoke_options )`
@@ -93,7 +114,7 @@ apply to `revokeOnTables` and automatic grant reversal.
 | Option            | Type                                       | Description                                   |
 | ----------------- | ------------------------------------------ | --------------------------------------------- |
 | `tables`          | [Name](/migrations/#type) or `array[Name]` | Names of tables                               |
-| `schema`          | `string`                                   | if tables ALL, then schema name is required   |
+| `schema`          | `string`                                   | Required with the bare string `tables: 'ALL'` |
 | `privileges`      | `array[TablePrivileges]` or `ALL`          | list of privileges                            |
 | `roles`           | [Name](/migrations/#type) or `array[Name]` | names of roles                                |
 | `onlyGrantOption` | `boolean`                                  | Revoke only the grant option; default `false` |
@@ -137,12 +158,22 @@ pgm.grantOnSequences({
   schema: 'public',
   privileges: 'USAGE',
 });
+
+pgm.grantOnSequences({
+  roles: 'reader',
+  sequences: { name: 'ALL' },
+  privileges: 'USAGE',
+});
 ```
 
 Use `{ schema: 'myschema', name: 'my_table_id_seq' }` for a schema-qualified
 sequence. `sequences: 'ALL'` together with `schema` affects existing sequences
-only; it does not set default privileges for future sequences. Without `schema`,
-`'ALL'` is treated as a literal sequence name, as with `grantOnTables`.
+only; it does not set default privileges for future sequences. The bare string
+`sequences: 'ALL'` without a top-level `schema` throws an error before SQL is
+generated. Use `sequences: { name: 'ALL' }` for the literal sequence `ALL`, or
+`sequences: { schema: 'app', name: 'ALL' }` to qualify it. Arrays such as
+`sequences: ['ALL', 'foo']` always select literal names, even with a top-level
+`schema`. These rules also apply to `revokeOnSequences` and automatic reversal.
 
 The operation reverses automatically with `revokeOnSequences`. A schema-wide
 reversal revokes privileges on all sequences present when it runs, including

docs/src/upgrading.md

@@ -384,6 +384,22 @@ every table in that schema is intended; select individual tables when
 independently granted privileges must remain. See
 [Grant Operations](migrations/grants).
 
+#### Bare `ALL` grant selectors require `schema`
+
+`grantOnTables`, `revokeOnTables`, `grantOnSequences` and `revokeOnSequences`
+now throw an error when `tables: 'ALL'` or `sequences: 'ALL'` is supplied without
+a top-level `schema`. Previously, these calls targeted an object literally
+named `"ALL"`. This validation also applies to automatic grant reversal, so
+existing migrations using this form can fail during replay or rollback.
+
+For all existing objects in a schema, supply both options, for example
+`tables: 'ALL', schema: 'app'` or `sequences: 'ALL', schema: 'app'`.
+For a literal object named `ALL`, use `{ name: 'ALL' }` or
+`{ schema: 'app', name: 'ALL' }` instead. Explicit arrays such as `['ALL', 'foo']`
+continue to select literal names. Ordinary named selections are unchanged.
+Updating the library does not change privileges already stored in the database.
+See [Grant Operations](migrations/grants) for examples.
+
 #### Sequence options keep `0`
 
 `minvalue`, `maxvalue` and `start` in `createSequence`, `alterSequence` and

src/operations/grants/shared.ts

@@ -86,6 +86,14 @@ export function asTablesStr(
   options: AllTablesOptions | SomeTablesOptions,
   mOptions: MigrationOptions
 ): string {
+  // Only the bare string is a schema-wide selector; arrays contain literal names.
+  if (
+    options.tables === 'ALL' &&
+    (!('schema' in options) || options.schema === undefined)
+  ) {
+    throw new Error('The "schema" option is required when "tables" is "ALL"');
+  }
+
   return isAllTablesOptions(options)
     ? `ALL TABLES IN SCHEMA ${mOptions.literal(options.schema)}`
     : toArray(options.tables).map(mOptions.literal).join(', ');
@@ -95,6 +103,16 @@ export function asSequencesStr(
   options: AllSequencesOptions | SomeSequencesOptions,
   mOptions: MigrationOptions
 ): string {
+  // Only the bare string is a schema-wide selector; arrays contain literal names.
+  if (
+    options.sequences === 'ALL' &&
+    (!('schema' in options) || options.schema === undefined)
+  ) {
+    throw new Error(
+      'The "schema" option is required when "sequences" is "ALL"'
+    );
+  }
+
   return options.sequences === 'ALL' && 'schema' in options
     ? `ALL SEQUENCES IN SCHEMA ${mOptions.literal(options.schema)}`
     : `SEQUENCE ${toArray(options.sequences).map(mOptions.literal).join(', ')}`;

test/operations/grants/grantOnSequences.spec.ts

@@ -1,4 +1,5 @@
 import { describe, expect, it } from 'vitest';
+import type { GrantOnSequencesOptions, Name } from '../../../src';
 import { grantOnSequences } from '../../../src/operations/grants';
 import { options1, options2 } from '../../presetMigrationOptions';
 
@@ -28,23 +29,72 @@ describe('grantOnSequences', () => {
     );
   });
 
-  it('grants all privileges on all sequences in a quoted schema', () => {
-    expect(
-      grant({
-        sequences: 'ALL',
-        schema: 'app"schema',
-        roles: 'PUBLIC',
-        privileges: 'ALL',
-      })
-    ).toBe('GRANT ALL ON ALL SEQUENCES IN SCHEMA "app""schema" TO PUBLIC;');
-  });
+  it('grants and reverses all privileges on all sequences in a quoted schema', () => {
+    const grantOptions: GrantOnSequencesOptions = {
+      sequences: 'ALL',
+      schema: 'app"schema',
+      roles: 'PUBLIC',
+      privileges: 'ALL',
+    };
 
-  it('treats ALL as a sequence name when no schema is supplied', () => {
-    expect(
-      grant({ sequences: 'ALL', roles: 'PUBLIC', privileges: 'USAGE' })
-    ).toBe('GRANT USAGE ON SEQUENCE "ALL" TO PUBLIC;');
+    expect(grant(grantOptions)).toBe(
+      'GRANT ALL ON ALL SEQUENCES IN SCHEMA "app""schema" TO PUBLIC;'
+    );
+    expect(grant.reverse(grantOptions)).toBe(
+      'REVOKE ALL ON ALL SEQUENCES IN SCHEMA "app""schema" FROM PUBLIC;'
+    );
   });
 
+  it.each([{}, { schema: undefined }])(
+    'rejects bare ALL without a schema (%j), including reversal',
+    (schemaOptions) => {
+      const grantOptions: GrantOnSequencesOptions = {
+        sequences: 'ALL',
+        roles: 'reader',
+        privileges: 'USAGE',
+        ...schemaOptions,
+      };
+      const error = new Error(
+        'The "schema" option is required when "sequences" is "ALL"'
+      );
+
+      expect(() => grant(grantOptions)).toThrow(error);
+      expect(() => grant.reverse(grantOptions)).toThrow(error);
+    }
+  );
+
+  it.each([{}, { schema: 'other_schema' }])(
+    'preserves explicit ALL names and literal arrays (%j), including reversal',
+    (schemaOptions) => {
+      const selections: Array<{ sequences: Name | Name[]; sql: string }> = [
+        { sequences: { name: 'ALL' }, sql: '"ALL"' },
+        {
+          sequences: { schema: 'app"schema', name: 'ALL' },
+          sql: '"app""schema"."ALL"',
+        },
+        // Arrays are named selections, even with one element named ALL.
+        { sequences: ['ALL'], sql: '"ALL"' },
+        { sequences: ['ALL', 'foo'], sql: '"ALL", "foo"' },
+      ];
+
+      for (const { sequences, sql } of selections) {
+        const grantOptions: GrantOnSequencesOptions = {
+          sequences,
+          roles: 'reader',
+          privileges: 'USAGE',
+          ...schemaOptions,
+        };
+
+        expect(grant(grantOptions)).toBe(
+          `GRANT USAGE ON SEQUENCE ${sql} TO "reader";`
+        );
+        expect(grant.reverse(grantOptions)).toBe(
+          `REVOKE USAGE ON SEQUENCE ${sql} FROM "reader";`
+        );
+      }
+    }
+  );
+
   it('does not widen a named selection when options contain an extra schema', () => {
     const grantOptions = {
       sequences: 'ids',

test/operations/grants/grantOnTables.spec.ts

@@ -105,20 +105,55 @@ describe('operations', () => {
         );
       });
 
-      it('preserves the literal ALL name without a schema', () => {
-        const grantOptions: GrantOnTablesOptions = {
-          tables: 'ALL',
-          privileges: 'SELECT',
-          roles: 'reader',
-        };
+      it.each([{}, { schema: undefined }])(
+        'rejects bare ALL without a schema (%j), including reversal',
+        (schemaOptions) => {
+          const grantOptions: GrantOnTablesOptions = {
+            tables: 'ALL',
+            privileges: 'SELECT',
+            roles: 'reader',
+            ...schemaOptions,
+          };
+          const error = new Error(
+            'The "schema" option is required when "tables" is "ALL"'
+          );
 
-        expect(grantOnTablesFn(grantOptions)).toBe(
-          'GRANT SELECT ON "ALL" TO "reader";'
-        );
-        expect(grantOnTablesFn.reverse(grantOptions)).toBe(
-          'REVOKE SELECT ON "ALL" FROM "reader";'
-        );
-      });
+          expect(() => grantOnTablesFn(grantOptions)).toThrow(error);
+          expect(() => grantOnTablesFn.reverse(grantOptions)).toThrow(error);
+        }
+      );
+
+      it.each([{}, { schema: 'other_schema' }])(
+        'preserves explicit ALL names and literal arrays (%j), including reversal',
+        (schemaOptions) => {
+          const selections: Array<{ tables: Name | Name[]; sql: string }> = [
+            { tables: { name: 'ALL' }, sql: '"ALL"' },
+            {
+              tables: { schema: 'app"schema', name: 'ALL' },
+              sql: '"app""schema"."ALL"',
+            },
+            // Arrays are named selections, even with one element named ALL.
+            { tables: ['ALL'], sql: '"ALL"' },
+            { tables: ['ALL', 'foo'], sql: '"ALL", "foo"' },
+          ];
+
+          for (const { tables, sql } of selections) {
+            const grantOptions: GrantOnTablesOptions = {
+              tables,
+              privileges: 'SELECT',
+              roles: 'reader',
+              ...schemaOptions,
+            };
+
+            expect(grantOnTablesFn(grantOptions)).toBe(
+              `GRANT SELECT ON ${sql} TO "reader";`
+            );
+            expect(grantOnTablesFn.reverse(grantOptions)).toBe(
+              `REVOKE SELECT ON ${sql} FROM "reader";`
+            );
+          }
+        }
+      );
 
       it('uses configured identifier rendering for named selections with an extra schema', () => {
         const grantOptions = {

test/operations/grants/revokeOnSequences.spec.ts

@@ -1,4 +1,5 @@
 import { describe, expect, it } from 'vitest';
+import type { Name } from '../../../src';
 import { revokeOnSequences } from '../../../src/operations/grants';
 import { options1 } from '../../presetMigrationOptions';
 
@@ -40,6 +41,49 @@ describe('revokeOnSequences', () => {
     );
   });
 
+  it.each([{}, { schema: undefined }])(
+    'rejects bare ALL without a schema (%j)',
+    (schemaOptions) => {
+      expect(() =>
+        revoke({
+          sequences: 'ALL',
+          roles: 'reader',
+          privileges: 'USAGE',
+          ...schemaOptions,
+        })
+      ).toThrow(
+        new Error('The "schema" option is required when "sequences" is "ALL"')
+      );
+    }
+  );
+
+  it.each([{}, { schema: 'other_schema' }])(
+    'preserves explicit ALL names and literal arrays (%j)',
+    (schemaOptions) => {
+      const selections: Array<{ sequences: Name | Name[]; sql: string }> = [
+        { sequences: { name: 'ALL' }, sql: '"ALL"' },
+        {
+          sequences: { schema: 'app"schema', name: 'ALL' },
+          sql: '"app""schema"."ALL"',
+        },
+        // Arrays are named selections, even with one element named ALL.
+        { sequences: ['ALL'], sql: '"ALL"' },
+        { sequences: ['ALL', 'foo'], sql: '"ALL", "foo"' },
+      ];
+
+      for (const { sequences, sql } of selections) {
+        expect(
+          revoke({
+            sequences,
+            roles: 'reader',
+            privileges: 'USAGE',
+            ...schemaOptions,
+          })
+        ).toBe(`REVOKE USAGE ON SEQUENCE ${sql} FROM "reader";`);
+      }
+    }
+  );
+
   it('does not widen named selections when options contain an extra schema', () => {
     const revokeOptions = {
       sequences: [{ schema: 'app', name: 'ids' }, 'other_ids'],