Improving error handling

Author: andrewshell

.node-version

@@ -1 +1 @@
-12
+14

DEVNOTES.md

@@ -20,3 +20,14 @@ I should make sure I return the same response if possible.
 <notifyResult success="false" msg="The following parameters were missing from the request body: notifyProcedure, port, path, protocol." />
 ```
 
+I think I check for all of these except notifyProcedure.
+
+## Update README docs
+
+Fully spec out XML-RPC endpoints with code samples.
+
+## Build freestanding rssCloud testing tool
+
+Hopefully this can be based on the test suite I'm creating now, but it will be a hosted app that has a dashboard where you can put in the details for an rssCloud server. It mocks the RSS feed and aggregator endpoints so it can direct the real rssCloud server at resources it controls. This way it can run through a series of tests with red/greed lights to see if each test passes.
+
+Split out rssCloud.root functionality vs my extended functionality.

README.md

@@ -1,4 +1,4 @@
-# rssCloud Server
+# rssCloud Server v2
 
 [![MIT License](https://img.shields.io/badge/license-MIT-brightgreen.svg)](https://github.com/rsscloud/rsscloud-server/blob/2.x/LICENSE.md)
 [![rssCloud Server 2.x](https://circleci.com/gh/rsscloud/rsscloud-server/tree/2.x.svg?style=shield)](https://circleci.com/gh/rsscloud/rsscloud-server/tree/2.x)

controllers/ping.js

@@ -35,10 +35,14 @@
     }
 
     router.post('/', urlencodedParser, function (req, res) {
-        const params = parsePingParams(req);
-        ping(params.url)
-            .then(result => processResponse(req, res, result))
-            .catch(err => handleError(req, res, err));
+        try {
+            const params = parsePingParams.rest(req);
+            ping(params.url)
+                .then(result => processResponse(req, res, result))
+                .catch(err => handleError(req, res, err));
+        } catch (err) {
+            return handleError(req, res, err);
+        }
     });
 
     module.exports = router;

controllers/rpc2.js

@@ -5,6 +5,7 @@
         express = require('express'),
         parseRpcRequest = require('../services/parse-rpc-request'),
         parseNotifyParams = require('../services/parse-notify-params'),
+        parsePingParams = require('../services/parse-ping-params'),
         pleaseNotify = require('../services/please-notify'),
         ping = require('../services/ping'),
         router = new express.Router(),
@@ -25,35 +26,44 @@
     }
 
     function handleError(req, res, err) {
-        // console.error(err);
+        console.error(err);
         processResponse(req, res, rpcReturnFault(4, err.message));
     }
 
     router.post('/', textParser, function (req, res) {
+        let params;
         parseRpcRequest(req)
             .then(request => {
                 switch (request.methodName) {
                 case 'rssCloud.hello':
-                    console.log(request.params[0]);
                     processResponse(req, res, rpcReturnSuccess(true));
                     break;
                 case 'rssCloud.pleaseNotify':
-                    const params = parseNotifyParams.rpc(req, request.params);
-                    pleaseNotify(
-                        params.notifyProcedure,
-                        params.apiurl,
-                        params.protocol,
-                        params.urlList,
-                        params.diffDomain
-                    )
-                        .then(result => processResponse(req, res, rpcReturnSuccess(result.success)))
-                        .catch(err => handleError(req, res, err));
+                    try {
+                        params = parseNotifyParams.rpc(req, request.params);
+                        pleaseNotify(
+                            params.notifyProcedure,
+                            params.apiurl,
+                            params.protocol,
+                            params.urlList,
+                            params.diffDomain
+                        )
+                            .then(result => processResponse(req, res, rpcReturnSuccess(result.success)))
+                            .catch(err => handleError(req, res, err));
+                    } catch (err) {
+                        handleError(req, res, err);
+                    }
                     break;
                 case 'rssCloud.ping':
-                    // Dave's rssCloud server always returns true whether it succeeded or not
-                    ping(request.params[0])
-                        .then(result => processResponse(req, res, rpcReturnSuccess(result.success)))
-                        .catch(err => processResponse(req, res, rpcReturnSuccess(true)));
+                    try {
+                        params = parsePingParams.rpc(req, request.params);
+                        // Dave's rssCloud server always returns true whether it succeeded or not
+                        ping(params.url)
+                            .then(result => processResponse(req, res, rpcReturnSuccess(result.success)))
+                            .catch(err => processResponse(req, res, rpcReturnSuccess(true)));
+                    } catch (err) {
+                        handleError(req, res, err);
+                    }
                     break;
                 default:
                     handleError(

services/app-messages.js

@@ -13,6 +13,10 @@
             ping: {
                 tooRecent: 'Can\'t accept the request because the minimum seconds between pings is %s and you pinged us %s seconds ago.',
                 readResource: 'The ping was cancelled because there was an error reading the resource at URL %s.'
+            },
+            rpc: {
+                notEnoughParams: 'Can\'t call "%s" because there aren\'t enough parameters.',
+                tooManyParams: 'Can\'t call "%s" because there are too many parameters.'
             }
         },
         log: {

services/parse-notify-params.js

@@ -106,6 +106,12 @@
         let params = {},
             parts = {};
 
+        if (5 > rpcParams.length) {
+            throw new Error(sprintf(appMessages.error.rpc.notEnoughParams, 'pleaseNotify'));
+        } else if (6 < rpcParams.length) {
+            throw new Error(sprintf(appMessages.error.rpc.tooManyParams, 'pleaseNotify'));
+        }
+
         if (validProtocol(rpcParams[3])) {
             params.protocol = rpcParams[3];
         }

services/parse-ping-params.js

@@ -4,7 +4,7 @@
     const appMessages = require('./app-messages'),
         sprintf = require('sprintf-js').sprintf;
 
-    function parsePingParams(req) {
+    function rest(req) {
         var s = '',
             params = {};
 
@@ -20,5 +20,19 @@
         }
     }
 
-    module.exports = parsePingParams;
+    function rpc(req, rpcParams) {
+        let params = {};
+
+        if (1 > rpcParams.length) {
+            throw new Error(sprintf(appMessages.error.rpc.notEnoughParams, 'ping'));
+        } else if (1 < rpcParams.length) {
+            throw new Error(sprintf(appMessages.error.rpc.tooManyParams, 'ping'));
+        }
+
+        params.url = rpcParams[0];
+
+        return params;
+    }
+
+    module.exports = { rest, rpc };
 }());

test/ping.js

@@ -6,14 +6,21 @@ const chai = require("chai"),
     mock = require("./mock"),
     mongodb = require("./mongodb"),
     xmlrpc = require("davexmlrpc"),
-    rpcReturnSuccess = require('../services/rpc-return-success');
+    rpcReturnSuccess = require('../services/rpc-return-success'),
+    rpcReturnFault = require('../services/rpc-return-fault');
 
 chai.use(chaiHttp);
 chai.use(chaiXml);
 
 function ping(pingProtocol, resourceUrl, returnFormat) {
     if ('XML-RPC' === pingProtocol) {
-        const rpctext = xmlrpc.buildCall('rssCloud.ping', [resourceUrl], 'xml');
+        let rpctext;
+        if (null == resourceUrl) {
+            rpctext = xmlrpc.buildCall('rssCloud.ping', [], 'xml');
+            console.log(rpctext);
+        } else {
+            rpctext = xmlrpc.buildCall('rssCloud.ping', [resourceUrl], 'xml');
+        }
 
         return chai
             .request(SERVER_URL)
@@ -30,7 +37,11 @@ function ping(pingProtocol, resourceUrl, returnFormat) {
             req.set('accept', 'application/json');
         }
 
-        return req.send({ url: resourceUrl });
+        if (null == resourceUrl) {
+            return req.send({});
+        } else {
+            return req.send({ url: resourceUrl });
+        }
     }
 }
 
@@ -151,6 +162,46 @@ for (const pingProtocol of ['XML-RPC', 'REST']) {
             }
         });
 
+        it('should reject a ping with a missing url', async () => {
+            const feedPath = '/rss.xml',
+                pingPath = '/feedupdated',
+                resourceUrl = null;
+
+            let apiurl = ('http-post' === protocol ? mock.serverUrl : mock.secureServerUrl) + pingPath,
+                notifyProcedure = false;
+
+            if ('xml-rpc' === protocol) {
+                apiurl = mock.serverUrl + '/RPC2';
+                notifyProcedure = 'river.feedUpdated';
+            }
+
+            mock.route('GET', feedPath, 404, 'Not Found');
+            mock.route('POST', pingPath, 200, 'Thanks for the update! :-)');
+            mock.rpc(notifyProcedure, rpcReturnSuccess(true));
+
+            let res = await ping(pingProtocol, resourceUrl, returnFormat);
+
+            expect(res).status(200);
+
+            if ('XML-RPC' === pingProtocol) {
+                expect(res.text).xml.equal(rpcReturnFault(4, 'Can\'t call "ping" because there aren\'t enough parameters.'));
+            } else {
+                if ('JSON' === returnFormat) {
+                    expect(res.body).deep.equal({ success: false, msg: `The following parameters were missing from the request body: url.` });
+                } else {
+                    expect(res.text).xml.equal(`<result success="false" msg="The following parameters were missing from the request body: url."/>`);
+                }
+            }
+
+            expect(mock.requests.GET).property(feedPath).lengthOf(0, `Should not GET ${feedPath}`);
+
+            if ('xml-rpc' === protocol) {
+                expect(mock.requests.RPC2).property(notifyProcedure).lengthOf(0, `Should not XML-RPC call ${notifyProcedure}`);
+            } else {
+                expect(mock.requests.POST).property(pingPath).lengthOf(0, `Should not POST ${pingPath}`);
+            }
+        });
+
         it('should accept a ping for unchanged resource', async () => {
             const feedPath = '/rss.xml',
                 pingPath = '/feedupdated',

views/docs.handlebars

@@ -2,7 +2,7 @@
 <html>
 <head>
     <meta charset="utf-8">
-    <title>rssCloud: Log</title>
+    <title>rssCloud v2: Documentation</title>
     <link href="/css/bootstrap.min.css" rel="stylesheet">
     <link href="/css/font-awesome.min.css" rel="stylesheet">
 </head>