Propagate the request ID when issuing requests; rel v0.7.0

d00rman · d00rman · commit a92cccea9df8 · 2019-06-18T19:10:18.000+02:00
This commit changes the API of the Request object and the utility
functions used to contact the MW and REST APIs. The Request object now
has a `context` property that includes the request ID. Additionally, the
request now features an `issueRequest()` method which should be used
instead of `preq`. It augments the request by setting the user-agent and
x-request-id headers automatically, in this way propagating the request
ID further down the call chain.

In order to account for these changes, `mwApiGet()` and `restApiGet()`
have been changed to take the request as a parameter instead of the
application object.

Bug: T225711
diff --git a/lib/api-util.js b/lib/api-util.js
@@ -1,41 +1,43 @@
 'use strict';
 
-const preq = require('preq');
+const BBPromise = require('bluebird');
 const sUtil = require('./util');
 const Template = require('swagger-router').Template;
 const HTTPError = sUtil.HTTPError;
 
 /**
  * Calls the MW API with the supplied query as its body
- * @param {!Object} app the application object
- * @param {string} domain the domain to issue the request to
+ * @param {!Object} req the incoming request object
  * @param {?Object} query an object with all the query parameters for the MW API
+ * @param {?Object} headers additional headers to pass to the MW API
  * @return {!Promise} a promise resolving as the response object from the MW API
  */
-function mwApiGet(app, domain, query) {
+function mwApiGet(req, query, headers) {
 
+    const app = req.app;
     query = Object.assign({
         format: 'json',
         formatversion: 2
     }, query);
 
     const request = app.mwapi_tpl.expand({
         request: {
-            params: { domain },
-            headers: { 'user-agent': app.conf.user_agent },
+            params: { domain: req.params.domain },
+            headers: req.headers,
             query
         }
     });
+    Object.assign(request.headers, headers);
 
-    return preq(request).then((response) => {
+    return req.issueRequest(request).then((response) => {
         if (response.status < 200 || response.status > 399) {
             // there was an error when calling the upstream service, propagate that
-            throw new HTTPError({
+            return BBPromise.reject(new HTTPError({
                 status: response.status,
                 type: 'api_error',
                 title: 'MW API error',
                 detail: response.body
-            });
+            }));
         }
         return response;
     });
@@ -44,9 +46,8 @@ function mwApiGet(app, domain, query) {
 
 /**
  * Calls the REST API with the supplied domain, path and request parameters
- * @param {!Object} app the application object
- * @param {string} domain the domain to issue the request for
- * @param {!string} path the REST API path to contact without the leading slash
+ * @param {!Object} req the incoming request object
+ * @param {?string} path the REST API path to contact without the leading slash
  * @param {?Object} [restReq={}] the object containing the REST request details
  * @param {?string} [restReq.method=get] the request method
  * @param {?Object} [restReq.query={}] the query string to send, if any
@@ -55,22 +56,32 @@ function mwApiGet(app, domain, query) {
  * @return {!Promise} a promise resolving as the response object from the REST API
  *
  */
-function restApiGet(app, domain, path, restReq) {
+function restApiGet(req, path, restReq) {
 
+    const app = req.app;
+    if (path.constructor === Object) {
+        restReq = path;
+        path = undefined;
+    }
     restReq = restReq || {};
-    path = path[0] === '/' ? path.slice(1) : path;
-
-    const request = app.restbase_tpl.expand({
-        request: {
-            method: restReq.method,
-            params: { domain, path },
-            query: restReq.query,
-            headers: Object.assign({ 'user-agent': app.conf.user_agent }, restReq.headers),
-            body: restReq.body
-        }
-    });
+    restReq.method = restReq.method || 'get';
+    restReq.query = restReq.query || {};
+    restReq.headers = restReq.headers || {};
+    restReq.params = restReq.params || {};
+    restReq.params.path = path || restReq.params.path;
+    restReq.params.domain = restReq.params.domain || req.params.domain;
+    if (!restReq.params.path || !restReq.params.domain) {
+        return BBPromise.reject(new HTTPError({
+            status: 500,
+            type: 'internal_error',
+            title: 'Invalid internal call',
+            detail: 'domain and path need to be defined for the REST API call'
+        }));
+    }
+    restReq.params.path = restReq.params.path[0] === '/' ?
+        restReq.params.path.slice(1) : restReq.params.path;
 
-    return preq(request);
+    return req.issueRequest(app.restbase_tpl.expand({ request: restReq }));
 
 }
 
@@ -83,10 +94,9 @@ function setupApiTemplates(app) {
     // set up the MW API request template
     if (!app.conf.mwapi_req) {
         app.conf.mwapi_req = {
+            method: 'post',
             uri: 'http://{{domain}}/w/api.php',
-            headers: {
-                'user-agent': '{{user-agent}}'
-            },
+            headers: '{{request.headers}}',
             body: '{{ default(request.query, {}) }}'
         };
     }
diff --git a/lib/util.js b/lib/util.js
@@ -1,6 +1,7 @@
 'use strict';
 
 const BBPromise = require('bluebird');
+const preq = require('preq');
 const express = require('express');
 const uuidv1 = require('uuid/v1');
 const bunyan = require('bunyan');
@@ -86,14 +87,6 @@ function errForLog(err) {
 
 }
 
-/**
- * Generates a unique request ID.
- * @return {!string} the generated request ID
- */
-function generateRequestId() {
-    return uuidv1();
-}
-
 /**
  * Wraps all of the given router's handler functions with
  * promised try blocks so as to allow catching all errors,
@@ -239,12 +232,38 @@ function createRouter(opts) {
  */
 function initAndLogRequest(req, app) {
     req.headers = req.headers || {};
-    req.headers['x-request-id'] = req.headers['x-request-id'] || generateRequestId();
+    req.headers['x-request-id'] = req.headers['x-request-id'] || uuidv1();
     req.logger = app.logger.child({
         request_id: req.headers['x-request-id'],
         request: reqForLog(req, app.conf.log_header_whitelist)
     });
-    req.logger.log('trace/req', { msg: 'incoming request' });
+    req.context = { reqId: req.headers['x-request-id'] };
+    req.issueRequest = (request) => {
+        if (!(request.constructor === Object)) {
+            request = { uri: request };
+        }
+        if (request.url) {
+            request.uri = request.url;
+            delete request.url;
+        }
+        if (!request.uri) {
+            return BBPromise.reject(new HTTPError({
+                status: 500,
+                type: 'internal_error',
+                title: 'No request to issue',
+                detail: 'No request has been specified'
+            }));
+        }
+        request.method = request.method || 'get';
+        request.headers = request.headers || {};
+        Object.assign(request.headers, {
+            'user-agent': app.conf.user_agent,
+            'x-request-id': req.context.reqId
+        });
+        req.logger.log('trace/req', { msg: 'Outgoing request', out_request: request });
+        return preq(request);
+    };
+    req.logger.log('trace/req', { msg: 'Incoming request' });
 }
 
 module.exports = {
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "service-template-node",
-  "version": "0.6.0",
+  "version": "0.7.0",
   "description": "A blueprint for MediaWiki REST API services",
   "main": "./app.js",
   "scripts": {
@@ -22,7 +22,7 @@
     "service template",
     "MediaWiki"
   ],
-  "author": "Wikimedia Service Team <services@wikimedia.org>",
+  "author": "Wikimedia Service Team <services@lists.wikimedia.org>",
   "contributors": [],
   "license": "Apache-2.0",
   "bugs": {
@@ -33,14 +33,14 @@
     "bluebird": "^3.5.5",
     "body-parser": "^1.19.0",
     "bunyan": "^1.8.12",
-    "compression": "^1.7.3",
+    "compression": "^1.7.4",
     "domino": "^2.1.3",
     "express": "^4.17.1",
     "http-shutdown": "^1.2.1",
     "js-yaml": "^3.13.1",
-    "preq": "^0.5.8",
+    "preq": "^0.5.9",
     "service-runner": "^2.7.1",
-    "swagger-router": "^0.7.3",
+    "swagger-router": "^0.7.4",
     "swagger-ui-dist": "^3.22.3",
     "uuid": "^3.3.2"
   },
diff --git a/routes/empty.js.template b/routes/empty.js.template
@@ -2,7 +2,6 @@
 
 
 var BBPromise = require('bluebird');
-var preq = require('preq');
 var sUtil = require('../lib/util');
 
 // shortcut
diff --git a/routes/ex.js b/routes/ex.js
@@ -103,6 +103,23 @@ router.get('/err/manual/auth', (req, res) => {
 
 });
 
+/*
+ *  REQUEST EXAMPLES
+ */
+
+/**
+ * GET /req/uri
+ */
+router.get('/req/uri/:uri', (req, res) => {
+    // to issue an external request, use req.issueRequest
+    return req.issueRequest(req.params.uri)
+    .then((r) => {
+        res.status(r.status);
+        res.set(r.headers);
+        res.end(r.body);
+    });
+});
+
 module.exports = (appObj) => {
 
     return {
diff --git a/routes/v1.js b/routes/v1.js
@@ -13,11 +13,6 @@ const HTTPError = sUtil.HTTPError;
  */
 const router = sUtil.router();
 
-/**
- * The main application object reported when this module is require()d
- */
-let app;
-
 /**
  * GET /siteinfo{/prop}
  * Fetches site info for the given domain, optionally
@@ -44,7 +39,7 @@ router.get('/siteinfo/:prop?', (req, res) => {
     };
 
     // send it
-    return apiUtil.mwApiGet(app, req.params.domain, apiQuery)
+    return apiUtil.mwApiGet(req, apiQuery)
     // and then return the result to the caller
     .then((apiRes) => {
         // do we have to return only one prop?
@@ -79,14 +74,13 @@ router.get('/siteinfo/:prop?', (req, res) => {
 /**
  * A helper function that obtains the Parsoid HTML for a given title and
  * loads it into a domino DOM document instance.
- * @param {string} domain the domain to contact
- * @param {string} title the title of the page to get
+ * @param {!Object} req the incoming request
  * @return {Promise} a promise resolving as the HTML element object
  */
-function getBody(domain, title) {
+function getBody(req) {
 
     // get the page
-    return apiUtil.restApiGet(app, domain, `page/html/${title}`)
+    return apiUtil.restApiGet(req, `page/html/${req.params.title}`)
     .then((callRes) => {
         // and then load and parse the page
         return BBPromise.resolve(domino.createDocument(callRes.body));
@@ -101,7 +95,7 @@ function getBody(domain, title) {
 router.get('/page/:title', (req, res) => {
 
     // get the page's HTML directly
-    return getBody(req.params.domain, req.params.title)
+    return getBody(req)
     // and then return it
     .then((doc) => {
         res.status(200).type('html').end(doc.body.innerHTML);
@@ -116,7 +110,7 @@ router.get('/page/:title', (req, res) => {
 router.get('/page/:title/lead', (req, res) => {
 
     // get the page's HTML directly
-    return getBody(req.params.domain, req.params.title)
+    return getBody(req)
     // and then find the leading section and return it
     .then((doc) => {
         let leadSec = '';
@@ -139,8 +133,6 @@ router.get('/page/:title/lead', (req, res) => {
 
 module.exports = (appObj) => {
 
-    app = appObj;
-
     return {
         path: '/',
         api_version: 1,
diff --git a/spec.template.yaml b/spec.template.yaml
@@ -1,6 +1,6 @@
 openapi: 3.0.0
 info:
-  version: 0.6.0
+  version: 0.7.0
   title: WMF Node.JS Service Template
   description: A template for creating Node.JS services
   termsOfService: https://wikimediafoundation.org/wiki/Terms_of_Use
@@ -426,6 +426,20 @@ paths:
             status: 401
             headers:
               content-type: application/json
+  /ex/req/uri/{uri}:
+    get:
+      tags:
+        - Example
+        - Request issuing
+      description: Issues a request to the given URI
+      responses:
+        200:
+          description: OK
+      x-amples:
+        - title: Get example home page
+          request:
+            params:
+              uri: 'http://www.example.com'
 
 components:
   schemas:
@@ -470,4 +484,4 @@ components:
       schema:
         type: string
       description: |
-        Site info prop.
+        Site info prop.
