From 85cb6798890a8b57a0a7e62629890f137ab08690 Mon Sep 17 00:00:00 2001 From: James Hadfield Date: Tue, 29 Aug 2023 15:13:37 +1200 Subject: [PATCH 1/2] Improve data fetching error handling The previous code would (depending on the sidecar file in question) catch fetch errors and print console errors, and then also attempt to load the (non-existant) data, resulting in more errors. I believe this was due to promise chains such as: promise.then() .catch() .then() .catch() and the (mis)understanding that if the first catch clause was hit then the entire chain would terminate. The approach implemented here also lets us differentiate between fetching + parsing errors, which is a subtle but nice improvement to the warning notifications. (Added comments are also relevant for understanding intended behaviour.) --- src/actions/loadData.js | 59 ++++++++++++++----- .../narrativeEditor/useDatasetFetch.js | 53 +++++++++-------- src/util/exceptions.js | 6 ++ src/util/serverInteraction.js | 4 +- 4 files changed, 81 insertions(+), 41 deletions(-) diff --git a/src/actions/loadData.js b/src/actions/loadData.js index 3e408679f..bab18c1ca 100644 --- a/src/actions/loadData.js +++ b/src/actions/loadData.js @@ -8,7 +8,7 @@ import { parseMeasurementsJSON, loadMeasurements } from "./measurements"; import { fetchJSON, fetchWithErrorHandling } from "../util/serverInteraction"; import { warningNotification, errorNotification } from "./notifications"; import { parseMarkdownNarrativeFile } from "../util/parseNarrative"; -import { NoContentError } from "../util/exceptions"; +import { NoContentError, FetchError} from "../util/exceptions"; import { parseMarkdown } from "../util/parseMarkdown"; import { updateColorByWithRootSequenceData } from "../actions/colors"; import { explodeTree } from "./tree"; @@ -283,47 +283,78 @@ Dataset.prototype.fetchMain = function fetchMain() { .then((res) => res.json()); }; Dataset.prototype.fetchSidecars = async function fetchSidecars() { + /** + * If deemed appropriate, fetch sidecars and store the resulting promise as `this.`. + * The returned promise will (eventually) be processed by chaining further `then` clauses via + * the `loadSidecars()` prototype. Because this may happen some time in the future, or even + * not at all, the promises created here should not reject in order to avoid the browser's default + * unhandled promise rejection logging. If the fetch fails we instead resolve to an Error object + * and and it is the responsibility of code which uses these promises to react appropriately. + */ const mainJson = await this.main; if (!mainJson) throw new Error("Cannot fetch sidecar files since the main JSON didn't succeed."); if (mainJson.meta.panels && mainJson.meta.panels.includes("frequencies") && !this.tipFrequencies) { this.tipFrequencies = fetchJSON(this.apiCalls.tipFrequencies) - .catch((err) => { - console.error("Failed to fetch frequencies", err.message); - }); + .catch((reason) => Promise.resolve(reason)) } + if (!this.rootSequence) { + // Note that the browser may log a GET error if the above 404s this.rootSequence = fetchJSON(this.apiCalls.rootSequence) - .catch(() => {}); // it's not unexpected to be missing the root-sequence JSON + .catch((reason) => Promise.resolve(reason)) } + if (mainJson.meta.panels && mainJson.meta.panels.includes("measurements") && !this.measurements) { this.measurements = fetchJSON(this.apiCalls.measurements) .then((json) => parseMeasurementsJSON(json)) - .catch((err) => { - console.error("Failed to fetch and parse measurements collections", err.message); - }); + .catch((reason) => Promise.resolve(reason)) } }; Dataset.prototype.loadSidecars = function loadSidecars(dispatch) { - // Helper function to load (dispatch) the visualisation of sidecar files + /* Helper function to load (dispatch) the visualisation of sidecar files. + `this.` will be undefined (if the request was never made) + or a promise which may resolve to the parsed JSON data, + or reject with a suitable error. + */ if (this.tipFrequencies) { this.tipFrequencies + .then((data) => { + if (data instanceof Error) throw data; + return data + }) .then((data) => dispatch(loadFrequencies(data))) - .catch(() => { - dispatch(warningNotification({message: "Failed to fetch frequencies"})); + .catch((reason) => { + console.error(reason) + const message = `Failed to ${reason instanceof FetchError ? 'fetch' : 'parse'} tip frequencies`; + dispatch(warningNotification({message})); }); } if (this.rootSequence) { this.rootSequence.then((data) => { + if (data instanceof Error) throw data; + return data + }).then((data) => { dispatch({type: types.SET_ROOT_SEQUENCE, data}); dispatch(updateColorByWithRootSequenceData()); - }); + }).catch((reason) => { + if (reason instanceof FetchError) { + // no console error message as root sequence sidecars are often not present + return + } + console.error(reason); + dispatch(warningNotification({message: "Failed to parse root sequence JSON"})); + }) } if (this.measurements) { this.measurements + .then((data) => { + if (data instanceof Error) throw data; + return data + }) .then((data) => dispatch(loadMeasurements(data))) .catch((err) => { - const errorMessage = "Failed to load measurements collections"; + const errorMessage = `Failed to ${err instanceof FetchError ? 'fetch' : 'parse'} measurements collections`; console.error(errorMessage, err.message); dispatch(warningNotification({message: errorMessage})); // Hide measurements panel @@ -333,6 +364,6 @@ Dataset.prototype.loadSidecars = function loadSidecars(dispatch) { }); } }; -Dataset.prototype.fetchAvailable = async function fetchSidecars() { +Dataset.prototype.fetchAvailable = async function fetchAvailable() { this.available = fetchJSON(this.apiCalls.getAvailable); }; diff --git a/src/components/narrativeEditor/useDatasetFetch.js b/src/components/narrativeEditor/useDatasetFetch.js index 3f9bd0586..3c146f4fe 100644 --- a/src/components/narrativeEditor/useDatasetFetch.js +++ b/src/components/narrativeEditor/useDatasetFetch.js @@ -1,6 +1,7 @@ import {useEffect, useReducer, useRef} from "react"; import { useDispatch } from 'react-redux'; import { CACHE_JSONS } from "../../actions/types"; +import { FetchError } from "../../util/exceptions"; /** * The auspice architecture includes a cache of datasets (main + sidecars) @@ -73,41 +74,43 @@ async function fetchDatasetAndSidecars(name, dataset, dispatchDatasetResponses) try { await dataset.fetchSidecars(); - /** fetchSidecars sets up promises for the sidecar files _after_ waiting - * for the main dataset to arrive (resolve), but it doesn't wait for the - * sidecar promises to resolve. - * - * The contents of the sidecars are not validated (further than simple JSON validation - * via res.JSON(). Neither are they dispatched -- this only matters if the debugger is - * actually visualising the dataset when the promise resolves (as if its resolved when the - * dataset viz loads then it'll access the promise), which is an edge case we don't need - * to address (yet). - * TODO the above isn't true! The sidecar file doesn't load :( + /** fetchSidecars conditionally sets up promises for the sidecar files at + * `dataset[sidecarName]` (this is not set if the main dataset indicates + * that the sidecar file should not be fetched). + * The promises will _always_ resolve, but the resolved value may be an error. + * If the resolved value is not an error, the sidecar may still be invalid, + * but this is not currently known until it is loaded (`loadSidecars()`). */ - /* rootSequence is always attempted, and resolves to undefined on fetch failure or the JSON on success. */ - dispatchDatasetResponses({name, datasetType: 'rootSeq', status: 'inProgress'}); - dataset.rootSequence.then((rootSeqData) => { - if (rootSeqData) { - dispatchDatasetResponses({name, datasetType: 'rootSeq', status: 'success'}); - } else { - dispatchDatasetResponses({name, datasetType: 'rootSeq', status: 'Warning - could not fetch the root sequence sidecar file. This is not necessarily a problem!'}); - } - }); + /* ----- root sequence sidecar JSON ------- */ + if (dataset.rootSequence) { + dispatchDatasetResponses({name, datasetType: 'rootSeq', status: 'inProgress'}); + dataset.rootSequence.then((rootSeqData) => { + if (rootSeqData instanceof FetchError) { + dispatchDatasetResponses({name, datasetType: 'rootSeq', status: "Warning - root sequence JSON isn't available (this is not necessarily a problem!)"}); + } else if (rootSeqData instanceof Error) { + dispatchDatasetResponses({name, datasetType: 'rootSeq', status: `Error - root sequence JSON exists but is invalid: "${rootSeqData.message}"`}); + } else { + dispatchDatasetResponses({name, datasetType: 'rootSeq', status: 'success'}); + } + }); + } else { + // TODO -- this indicates the root-sequence was inlined & we should improve the status message + // (default status message: not-attempted) + } - /* tipFrequencies is NOT fetched unless the dataset asks to display it. If attempted, - it resolves to undefined on fetch failure or the JSON on success. */ + /* ----- tip frequencies sidecar JSON ------- */ if (dataset.tipFrequencies) { dispatchDatasetResponses({name, datasetType: 'frequencies', status: 'inProgress'}); dataset.tipFrequencies.then((tipFreqData) => { - if (tipFreqData) { - dispatchDatasetResponses({name, datasetType: 'frequencies', status: 'success'}); + if (tipFreqData instanceof Error) { + dispatchDatasetResponses({name, datasetType: 'frequencies', status: `Error - the dataset requested a tipFrequencies sidecar JSON however the following error was raised: "${tipFreqData.message}"`}); } else { - dispatchDatasetResponses({name, datasetType: 'frequencies', status: 'Error - the dataset requested a tipFrequencies sidecar but the fetch failed'}); + dispatchDatasetResponses({name, datasetType: 'frequencies', status: 'success'}); } }); } else { - // TODO -- expand on status messaging here. + // TODO -- expand on status messaging here (default status message: not-attempted) } diff --git a/src/util/exceptions.js b/src/util/exceptions.js index 0401739cb..1a6dfabce 100644 --- a/src/util/exceptions.js +++ b/src/util/exceptions.js @@ -4,6 +4,12 @@ export class NoContentError extends Error { } } +export class FetchError extends Error { + constructor(...params) { + super(...params); + } +} + /** * Thrown when a download produces an empty Newick tree. * Usually caused by users trying to download multiple subtrees that do not diff --git a/src/util/serverInteraction.js b/src/util/serverInteraction.js index 84cefe6c0..010787c4b 100644 --- a/src/util/serverInteraction.js +++ b/src/util/serverInteraction.js @@ -1,4 +1,4 @@ -import {NoContentError} from "./exceptions"; +import {FetchError, NoContentError} from "./exceptions"; export const fetchWithErrorHandling = async (path) => { const res = await fetch(path); @@ -7,7 +7,7 @@ export const fetchWithErrorHandling = async (path) => { if (res.status === 204) { throw new NoContentError(); } - throw new Error(`${await res.text()} (${res.statusText})`); + throw new FetchError(`${path} ${await res.text()} (${res.statusText})`); } return res; }; From 471f457cfebc041389bf190e6338da6f77689930 Mon Sep 17 00:00:00 2001 From: James Hadfield Date: Tue, 29 Aug 2023 15:23:22 +1200 Subject: [PATCH 2/2] Allow inline root sequence data For smaller genomes it's a nicer experience to inline the root sequence data to avoid having to manage an extra sidecar file, with the downside being an (often negligible) increase in file size. This commit implements this by continuing our approach of conditionally fetching sidecars based on the data present in the main JSON. For full context, see discussions in Slack including Related Augur PR --- src/actions/loadData.js | 2 +- src/actions/recomputeReduxState.js | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/actions/loadData.js b/src/actions/loadData.js index bab18c1ca..c87c90128 100644 --- a/src/actions/loadData.js +++ b/src/actions/loadData.js @@ -299,7 +299,7 @@ Dataset.prototype.fetchSidecars = async function fetchSidecars() { .catch((reason) => Promise.resolve(reason)) } - if (!this.rootSequence) { + if (!mainJson.root_sequence && !this.rootSequence) { // Note that the browser may log a GET error if the above 404s this.rootSequence = fetchJSON(this.apiCalls.rootSequence) .catch((reason) => Promise.resolve(reason)) diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js index 9c3ce592e..e1168b1e7 100644 --- a/src/actions/recomputeReduxState.js +++ b/src/actions/recomputeReduxState.js @@ -733,6 +733,11 @@ const createMetadataStateFromJSON = (json) => { if (json.meta.panels) { metadata.panels = json.meta.panels; } + if (json.root_sequence) { + /* A dataset may set the root sequence inline (i.e. within the main dataset JSON), which + we capture here. Alternatively it may be a sidecar JSON file */ + metadata.rootSequence = json.root_sequence; + } if (json.meta.display_defaults) { metadata.displayDefaults = {}; const jsonKeyToAuspiceKey = {