Merge pull request #870 from koopjs/f/allow-no-callbacks

feat: allow no-callback getData and model.pull

.changeset/afraid-hornets-sleep.md

@@ -0,0 +1,8 @@
+---
+'@koopjs/koop-core': minor
+---
+
+- allow model getData, getLayer, and getCatalog methods to be used without a callback
+- allow model pull, pullLayer, and pullCatalog methods to be used without a callback
+- allow before and after functions without callback
+

.changeset/blue-readers-battle.md

@@ -0,0 +1,5 @@
+---
+'@koopjs/output-geoservices': minor
+---
+
+- use model.pull without callback

packages/core/src/data-provider/extend-model.js

@@ -1,10 +1,10 @@
 const { promisify } = require('util');
 const hasher = require('@sindresorhus/fnv1a');
 
-const before = (req, callback) => { callback(); };
-const after = (req, data, callback) => { callback(null, data); };
-const cacheRetrieveNoop = (key, options, callback) => { callback(); };
-const cacheInsertNoop = (key, options, data, callback) => { callback(); };
+const beforeNoop = async () => {};
+const afterNoop = async (req, data) => { return data; };
+const cacheRetrieveNoop = async () => {};
+const cacheInsertNoop = async () => {};
 
 module.exports = function extendModel ({ ProviderModel, namespace, logger, cache, authModule }, options = {}) {
   class Model extends ProviderModel {
@@ -18,34 +18,40 @@ module.exports = function extendModel ({ ProviderModel, namespace, logger, cache
     #getCatalog;
 
     constructor ({ logger, cache }, options) {
-
       super({ logger, log: logger }, options);
+
       // Provider constructor's may assign values to this.cache
       const modelCache = this.cache || options.cache || cache;
+
       this.#cacheTtl = options.cacheTtl;
       this.namespace = namespace;
       this.logger = logger;
-      this.#before = promisify(options.before || before);
-      this.#after = promisify(options.after || after);
-      this.#cacheRetrieve = promisify(modelCache?.retrieve || cacheRetrieveNoop).bind(modelCache);
-      this.#cacheInsert = promisify(modelCache?.insert || cacheInsertNoop).bind(modelCache);
-      this.#getProviderData = promisify(this.getData).bind(this);
-      this.#getLayer = this.getLayer ? promisify(this.getLayer).bind(this) : undefined;
-      this.#getCatalog = this.getCatalog ? promisify(this.getCatalog).bind(this) : undefined;
+      this.#before = this.#normalizeAndBindMethod(options.before || beforeNoop, 2);
+      this.#after = this.#normalizeAndBindMethod(options.after || afterNoop, 3);
+      this.#cacheRetrieve = this.#normalizeAndBindMethod(modelCache?.retrieve || cacheRetrieveNoop, 3, modelCache);
+      this.#cacheInsert = this.#normalizeAndBindMethod(modelCache?.insert || cacheInsertNoop, 4, modelCache);
+      this.#getProviderData = this.#normalizeAndBindMethod(this.getData, 2);
+      this.#getLayer = this.getLayer ? this.#normalizeAndBindMethod(this.getLayer, 2) : undefined;
+      this.#getCatalog = this.getCatalog ? this.#normalizeAndBindMethod(this.getCatalog, 2) : undefined;
+    }
+
+    #normalizeAndBindMethod (func, callbackArgumentIndex, context = this) {
+      return func?.length === callbackArgumentIndex ? promisify(func).bind(context) : func.bind(context);
     }
 
     async pull (req, callback) {
       const { error } = await this.#authorizeRequest(req);
       if (error) {
-        return callback(error);
+        return this.#handleReturn(callback, error);
       }
 
       const key = this.#createCacheKey(req);
 
       try {
         const cached = await this.#cacheRetrieve(key, {});
         if (shouldUseCache(cached)) {
-          return callback(null, cached);
+          this.logger.debug('fetched data from cache');
+          return this.#handleReturn(callback, null, cached);
         }
       } catch (err) {
         this.logger.debug(err);
@@ -54,35 +60,50 @@ module.exports = function extendModel ({ ProviderModel, namespace, logger, cache
       try {
         await this.#before(req);
         const providerGeojson = await this.#getProviderData(req);
-        const afterFuncGeojson = await this.#after(req, providerGeojson);
-        const { ttl = this.#cacheTtl } = afterFuncGeojson;
+        const modifiedGeojson = await this.#after(req, providerGeojson);
+        const { ttl = this.#cacheTtl } = modifiedGeojson;
         if (ttl) {
-          this.#cacheInsert(key, afterFuncGeojson, { ttl });
+          this.#cacheInsert(key, modifiedGeojson, { ttl });
         }
-        callback(null, afterFuncGeojson);
+
+        return this.#handleReturn(callback, null, modifiedGeojson);
       } catch (err) {
-        callback(err);
+        return this.#handleReturn(callback, err);
+      }
+    }
+
+    #handleReturn(callback, error, returnValue) {
+      if (callback) {
+        return callback(error, returnValue);
       }
+
+      if (error) {
+        throw error;
+      }
+
+      return returnValue;
     }
 
+
     // TODO: the pullLayer() and the pullCatalog() are very similar to the pull()
     // function. We may consider to merging them in the future.
     async pullLayer (req, callback) {
       const { error } = await this.#authorizeRequest(req);
       if (error) {
-        return callback(error);
+        return this.#handleReturn(callback, error);
       }
 
       if (!this.#getLayer) {
-        callback(new Error(`getLayer() method is not implemented in the ${this.namespace} provider.`));
+        return this.#handleReturn(callback, new Error(`getLayer() method is not implemented in the ${this.namespace} provider.`));
       }
 
       const key = `${this.#createCacheKey(req)}::layer`;
 
       try {
         const cached = await this.#cacheRetrieve(key, req.query);
         if (shouldUseCache(cached)) {
-          return callback(null, cached);
+          this.logger.debug('fetched data from cache');
+          return this.#handleReturn(callback, null, cached);
         }
       } catch (err) {
         this.logger.debug(err);
@@ -94,28 +115,29 @@ module.exports = function extendModel ({ ProviderModel, namespace, logger, cache
         if (ttl) {
           await this.#cacheInsert(key, data, { ttl });
         }
-        callback(null, data);
+        return this.#handleReturn(callback, null, data);
       } catch (err) {
-        callback(err);
+        return this.#handleReturn(callback, err);
       }
     }
 
     async pullCatalog (req, callback) {
       const { error } = await this.#authorizeRequest(req);
       if (error) {
-        return callback(error);
+        return this.#handleReturn(callback, error);
       }
 
       if (!this.#getCatalog) {
-        callback(new Error(`getCatalog() method is not implemented in the ${this.namespace} provider.`));
+        return this.#handleReturn(callback, new Error(`getCatalog() method is not implemented in the ${this.namespace} provider.`));
       }
 
       const key = `${this.#createCacheKey(req)}::catalog`;
 
       try {
         const cached = await this.#cacheRetrieve(key, req.query);
         if (shouldUseCache(cached)) {
-          return callback(null, cached);
+          this.logger.debug('fetched data from cache');
+          return this.#handleReturn(callback, null, cached);
         }
       } catch (err) {
         this.logger.debug(err);
@@ -127,9 +149,9 @@ module.exports = function extendModel ({ ProviderModel, namespace, logger, cache
         if (ttl) {
           this.#cacheInsert(key, data, { ttl });
         }
-        callback(null, data);
+        return this.#handleReturn(callback, null, data);
       } catch (err) {
-        callback(err);
+        return this.#handleReturn(callback, err);
       }
     }
 
@@ -169,10 +191,7 @@ module.exports = function extendModel ({ ProviderModel, namespace, logger, cache
     }
   }
 
-  // If provider does not have auth-methods, 
-  // check for global auth-module. if exists, use it, 
-  // otherwise use dummy methods
-
+  // If provider has auth methods use them, then use auth-module methods, otherwise dummy methods
   if (typeof ProviderModel.prototype.authorize !== 'function') {
     Model.prototype.authorize = typeof authModule?.authorize  === 'function' ? authModule.authorize : async () => {};
   }
@@ -185,24 +204,10 @@ module.exports = function extendModel ({ ProviderModel, namespace, logger, cache
     logger.warn('Use of "authenticationSpecification" is deprecated. It will be removed in a future release.');
     Model.prototype.authenticationSpecification =  authModule?.authenticationSpecification;
   }
-  // Add auth methods if auth plugin registered with Koop
-  // if (authModule) {
-  //   const {
-  //     authenticationSpecification,
-  //     authenticate,
-  //     authorize
-  //   } = authModule;
-
-  //   Model.prototype.authenticationSpecification = Object.assign({}, authenticationSpecification(namespace), { provider: namespace });
-  //   Model.prototype.authenticate = authenticate;
-  //   Model.prototype.authorize = authorize;
-  // }
 
   return new Model({ logger, cache }, options);
 };
 
-
-
 function shouldUseCache (cacheEntry) {
   // older cache plugins stored expiry time explicitly; all caches should move to returning empty if expired
   if (!cacheEntry) {