From 4da10b1409866f85bf74da92f294fed6bd123db1 Mon Sep 17 00:00:00 2001 From: Lachlan Kermode Date: Thu, 6 Dec 2018 15:19:01 +0000 Subject: [PATCH 1/2] 404s for only source, only tab, and bad resource --- src/api/index.js | 15 +++++++++++++-- src/copy/en.js | 6 ++++++ 2 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 src/copy/en.js diff --git a/src/api/index.js b/src/api/index.js index 06d961e..0fcfe04 100755 --- a/src/api/index.js +++ b/src/api/index.js @@ -1,5 +1,6 @@ import { version } from '../../package.json' import { Router } from 'express' +import copy from '../copy/en' export default ({ config, controller }) => { let api = Router() @@ -30,11 +31,21 @@ export default ({ config, controller }) => { .retrieve(req.params.source, req.params.tab, req.params.resource) .then(data => res.json(data)) .catch(err => - res.status(err.status || 501) - .send() + res.status(err.status || 404) + .send({ error: err }) ) }) + api.get('/:source', (req, res) => { + res.status(404) + .send({ error: copy.errors.onlySource }) + }) + + api.get('/:source/:tab', (req, res) => { + res.status(404) + .send({ error: copy.errors.onlyTab }) + }) + api.get('/update', (req, res) => { controller .update() diff --git a/src/copy/en.js b/src/copy/en.js new file mode 100644 index 0000000..d20da49 --- /dev/null +++ b/src/copy/en.js @@ -0,0 +1,6 @@ +export default { + errors: { + onlySource: 'You cannot query a source directly. The URL needs to be in the format /:source/:tab/:resource.', + onlyTab: 'You cannot query a tab directly. The URL needs to be in the format /:source/:tab/:resource.' + } +} From 72edac944ccad0e9ea469781c9ae15b1db030f5f Mon Sep 17 00:00:00 2001 From: Lachlan Kermode Date: Thu, 6 Dec 2018 16:22:10 +0000 Subject: [PATCH 2/2] centralise msgs in copy/en.js plus some other fixes --- src/api/index.js | 34 ++++++++++++----------- src/copy/en.js | 9 ++++++- src/lib/Controller.js | 16 ++++++----- src/lib/Fetcher.js | 3 ++- src/models/Interface.js | 6 +++++ src/models/StoreJson.js | 60 ++++++++++++++++++++--------------------- 6 files changed, 72 insertions(+), 56 deletions(-) diff --git a/src/api/index.js b/src/api/index.js index 0fcfe04..c3a069b 100755 --- a/src/api/index.js +++ b/src/api/index.js @@ -21,8 +21,8 @@ export default ({ config, controller }) => { .retrieveFrag(source, tab, resource, frag) .then(data => res.json(data)) .catch(err => - res.status(err.status || 501) - .send() + res.status(err.status || 404) + .send({ error: err.message }) ) }) @@ -32,20 +32,10 @@ export default ({ config, controller }) => { .then(data => res.json(data)) .catch(err => res.status(err.status || 404) - .send({ error: err }) + .send({ error: err.message }) ) }) - api.get('/:source', (req, res) => { - res.status(404) - .send({ error: copy.errors.onlySource }) - }) - - api.get('/:source/:tab', (req, res) => { - res.status(404) - .send({ error: copy.errors.onlyTab }) - }) - api.get('/update', (req, res) => { controller .update() @@ -55,11 +45,23 @@ export default ({ config, controller }) => { }) ) .catch(err => - res.json({ - error: err.message - }) + res.status(404) + .send({ error: err.message }) ) }) + // ERROR routes. Note that it is important that these come AFTER routes + // like /update, so that the regex does not greedily match these routes. + + api.get('/:source', (req, res) => { + res.status(404) + .send({ error: copy.errors.onlySource }) + }) + + api.get('/:source/:tab', (req, res) => { + res.status(404) + .send({ error: copy.errors.onlyTab }) + }) + return api } diff --git a/src/copy/en.js b/src/copy/en.js index d20da49..dd81a30 100644 --- a/src/copy/en.js +++ b/src/copy/en.js @@ -1,6 +1,13 @@ export default { errors: { + update: 'The server could not update. Check your API credentials and internet connection and try again.', onlySource: 'You cannot query a source directly. The URL needs to be in the format /:source/:tab/:resource.', - onlyTab: 'You cannot query a tab directly. The URL needs to be in the format /:source/:tab/:resource.' + onlyTab: 'You cannot query a tab directly. The URL needs to be in the format /:source/:tab/:resource.', + noSource: source => `The source ${source} is not available in this server.`, + noResource: prts => `The resource '${prts[2]}' does not exists in the tab '${prts[1]}' of the source '${prts[0]}'.`, + noFragment: prts => `Fragment index does not exist` + }, + success: { + update: 'All sources updated' } } diff --git a/src/lib/Controller.js b/src/lib/Controller.js index 8cc3c92..49077e5 100644 --- a/src/lib/Controller.js +++ b/src/lib/Controller.js @@ -1,3 +1,5 @@ +import copy from '../copy/en' + /** * Controller * @@ -23,7 +25,11 @@ class Controller { return this.fetchers[source].update() }) ).then(results => { - return 'All sources updated' + if (results.every(r => r)) { + return copy.success.update + } else { + throw new Error(copy.errors.update) + } }) } @@ -32,9 +38,7 @@ class Controller { const fetcher = this.fetchers[source] return fetcher.retrieve(tab, resource) } else { - return Promise.resolve().then(() => { - throw new Error(`Source ${source} not available.`) - }) + return Promise.reject(new Error(copy.errors.noResource(source))) } } @@ -43,9 +47,7 @@ class Controller { const fetcher = this.fetchers[source] return fetcher.retrieveFrag(tab, resource, frag) } else { - return Promise.resolve().then(() => { - throw new Error(`Source ${source} not available.`) - }) + return Promise.reject(new Error(copy.errors.noResource(source))) } } } diff --git a/src/lib/Fetcher.js b/src/lib/Fetcher.js index fe53f43..f22e7f0 100644 --- a/src/lib/Fetcher.js +++ b/src/lib/Fetcher.js @@ -111,7 +111,8 @@ class Fetcher { }) ) }) - .then(() => 'All tabs updated') + .then(() => true) + .catch(() => false) } save (tab, data) { diff --git a/src/models/Interface.js b/src/models/Interface.js index 1067e7d..60f98dd 100644 --- a/src/models/Interface.js +++ b/src/models/Interface.js @@ -2,6 +2,12 @@ /** * Model is a class whose sole responsibility is to save and load blueprints. * It allows for different storage mechanisms for different kinds of blueprints. + * + * ERRORS: + * When a load function fails, it should throw either: + * 1. A __ error if the resource doesn't exist on that sheet/tab. + * 2. A __ error if a fragment lookup fails because it doesn't exist. + * 3. A __ error if something else goes wrong. */ class Model { /** diff --git a/src/models/StoreJson.js b/src/models/StoreJson.js index fc91a1b..5b1360b 100644 --- a/src/models/StoreJson.js +++ b/src/models/StoreJson.js @@ -1,5 +1,6 @@ import fs from 'mz/fs' import { fmtSourceTitle } from '../lib/util' +import copy from '../copy/en' const STORAGE_DIRNAME = 'temp' @@ -22,39 +23,36 @@ class StoreJson { const fname = `${STORAGE_DIRNAME}/${parts[0]}__${parts[1]}__${ parts[2] }.json` - return fs - .exists(fname) - .then(isAvailable => { - if (isAvailable) return fs.readFile(fname, 'utf8') - else { - throw new Error('No resource exists') - } - }) - .then(data => JSON.parse(data)) - .then(data => { - if (parts.length === 3) { - // No lookup if the requested url doesn't have a fragment - return data - } else if (parts[2] === 'ids') { - // Do a lookup if fragment is included to filter a relevant item - // When the resource requested is 'ids' - const id = parseInt(parts[3]) - if (!isNaN(id) && id >= 0 && id < data.length) { - return data[id] + if (fs.existsSync(fname)) { + return fs.readFile(fname, 'utf8') + .then(data => JSON.parse(data)) + .then(data => { + if (parts.length === 3) { + // No lookup if the requested url doesn't have a fragment + return data + } else if (parts[2] === 'ids') { + // Do a lookup if fragment is included to filter a relevant item + // When the resource requested is 'ids' + const id = parseInt(parts[3]) + if (!isNaN(id) && id >= 0 && id < data.length) { + return data[id] + } else { + throw new Error(copy.errors.noFragment(parts)) + } } else { - throw new Error(`Fragment index does not exist`) + // Do a lookup if fragment is included to filter a relevant item + const index = parseInt(parts[3]) + if (!isNaN(index) && index >= 0 && index < data.length) { + console.log(data, index) + return data.filter((vl, idx) => idx === index)[0] + } else { + throw new Error(copy.errors.noFragment(parts)) + } } - } else { - // Do a lookup if fragment is included to filter a relevant item - const index = parseInt(parts[3]) - if (!isNaN(index) && index >= 0 && index < data.length) { - console.log(data, index) - return data.filter((vl, idx) => idx === index)[0] - } else { - throw new Error(`Fragment index does not exist`) - } - } - }) + }) + } else { + return Promise.reject(new Error(copy.errors.noResource(parts))) + } } // TODO: add method to build blueprint from data source