fix(server): throw when encountering undefined positional arguments (…

…#958)

Fixes #951

src/postgraphile/__tests__/postgraphile-test.js

@@ -20,7 +20,7 @@ test('will use a connected client from the pool, the schemas, and options to cre
   createPostGraphileHttpRequestHandler.mockClear();
   const pgPool = new Pool();
   const schemas = [Symbol('schemas')];
-  const options = Symbol('options');
+  const options = { $options: Symbol('options') };
   await postgraphile(pgPool, schemas, options);
   expect(createPostGraphileSchema.mock.calls).toEqual([[pgPool, schemas, options]]);
 });
@@ -29,8 +29,7 @@ test('will use a connected client from the pool, the default schema, and options
   createPostGraphileSchema.mockClear();
   createPostGraphileHttpRequestHandler.mockClear();
   const pgPool = new Pool();
-  const options = Symbol('options');
-  const pgClient = { release: jest.fn() };
+  const options = { $options: Symbol('options') };
   await postgraphile(pgPool, options);
   expect(createPostGraphileSchema.mock.calls).toEqual([[pgPool, ['public'], options]]);
 });
@@ -83,3 +82,24 @@ test('will not error if jwtSecret is provided without jwtPgTypeIdentifier', asyn
   const pgPool = new Pool();
   expect(() => postgraphile(pgPool, [], { jwtSecret: 'test' })).not.toThrow();
 });
+
+test('will throw on undefined positional arguments', async () => {
+  const pgPool = new Pool();
+  const options = { $options: Symbol('options') };
+
+  createPostGraphileSchema.mockClear();
+  expect(() => postgraphile(pgPool, null, options)).not.toThrow();
+  expect(() => postgraphile(pgPool, options)).not.toThrow();
+  expect(createPostGraphileSchema.mock.calls).toEqual([
+    [pgPool, ['public'], options],
+    [pgPool, ['public'], options],
+  ]);
+
+  expect(() => postgraphile(null)).not.toThrow();
+  expect(() => postgraphile(pgPool, null)).not.toThrow();
+  expect(() => postgraphile(null, 'public')).not.toThrow();
+
+  expect(() => postgraphile(undefined)).toThrow();
+  expect(() => postgraphile(pgPool, undefined)).toThrow();
+  expect(() => postgraphile(undefined, 'public')).toThrow();
+});

src/postgraphile/postgraphile.ts

@@ -114,41 +114,40 @@ export default function postgraphile(
   // must process them with `pluginHook` before we can rely on them.
   let incomingOptions: PostGraphileOptions;
 
-  // If the second argument is undefined, use defaults for both `schema` and
-  // `incomingOptions`.
-  if (typeof schemaOrOptions === 'undefined') {
-    schema = 'public';
-    incomingOptions = {};
-  }
   // If the second argument is a string or array, it is the schemas so set the
   // `schema` value and try to use the third argument (or a default) for
   // `incomingOptions`.
-  else if (typeof schemaOrOptions === 'string' || Array.isArray(schemaOrOptions)) {
+  if (typeof schemaOrOptions === 'string' || Array.isArray(schemaOrOptions)) {
     schema = schemaOrOptions;
     incomingOptions = maybeOptions || {};
   }
-  // Otherwise the second argument is the incomingOptions so set `schema` to the
-  // default and `incomingOptions` to the second argument.
+  // If the second argument is null or an object then use default `schema`
+  // and set incomingOptions to second or third argument (or default).
+  else if (typeof schemaOrOptions === 'object') {
+    schema = 'public';
+    incomingOptions = schemaOrOptions || maybeOptions || {};
+  }
+  // Otherwise if the second argument is present it's invalid: throw an error.
+  else if (arguments.length > 1) {
+    throw new Error(
+      'The second argument to postgraphile was invalid... did you mean to set a schema?',
+    );
+  }
+  // No schema or options specified, use defaults.
   else {
     schema = 'public';
-    incomingOptions = schemaOrOptions;
+    incomingOptions = {};
+  }
+
+  if (typeof poolOrConfig === 'undefined' && arguments.length >= 1) {
+    throw new Error(
+      'The first argument to postgraphile was `undefined`... did you mean to set pool options?',
+    );
   }
 
   // Do some things with `poolOrConfig` so that in the end, we actually get a
   // Postgres pool.
-  const pgPool: Pool =
-    // If it is already a `Pool`, just use it.
-    poolOrConfig instanceof Pool || quacksLikePgPool(poolOrConfig)
-      ? (poolOrConfig as Pool)
-      : new Pool(
-          typeof poolOrConfig === 'string'
-            ? // Otherwise if it is a string, let us parse it to get a config to
-              // create a `Pool`.
-              parsePgConnectionString(poolOrConfig)
-            : // Finally, it must just be a config itself. If it is undefined, we
-              // will just use an empty config and let the defaults take over.
-              poolOrConfig || {},
-        );
+  const pgPool = toPgPool(poolOrConfig);
 
   pgPool.on('error', err => {
     /*
@@ -202,7 +201,27 @@ function constructorName(obj: mixed): string | null {
 }
 
 // tslint:disable-next-line no-any
-function quacksLikePgPool(pgConfig: any): boolean {
+function toPgPool(poolOrConfig: any): Pool {
+  if (quacksLikePgPool(poolOrConfig)) {
+    // If it is already a `Pool`, just use it.
+    return poolOrConfig;
+  }
+
+  return new Pool(
+    typeof poolOrConfig === 'string'
+      ? // Otherwise if it is a string, let us parse it to get a config to
+        // create a `Pool`.
+        parsePgConnectionString(poolOrConfig)
+      : // Finally, it must just be a config itself. If it is undefined, we
+        // will just use an empty config and let the defaults take over.
+        poolOrConfig || {},
+  );
+}
+
+// tslint:disable-next-line no-any
+function quacksLikePgPool(pgConfig: any): pgConfig is Pool {
+  if (pgConfig instanceof Pool) return true;
+
   // A diagnosis of exclusion
   if (!pgConfig || typeof pgConfig !== 'object') return false;
   if (constructorName(pgConfig) !== 'Pool' && constructorName(pgConfig) !== 'BoundPool')