Skip to content

Commit

Permalink
fix(fetchtool, webhelper): handle asset requests with a 404 response
Browse files Browse the repository at this point in the history
FetchTool should resolve null for the data request if the request gets a 404 response. WebHelper
should properly handle this case and resolve null for the asset (instead of an asset skeleton with
null 'data' field) if the asset is nowhere to be found. WebHelper should also report errors
properly.

BREAKING CHANGE: The WebHelper was not previously resolving with null assets even though this was
the documented intended behavior of ScratchStorage.load

builds on and closes scratchfoundation#363 (thanks @AndreyNikitin!), closes scratchfoundation#156, might be related to scratchfoundation#367
  • Loading branch information
kchadha committed May 17, 2022
1 parent eca1123 commit 0df263f
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 11 deletions.
6 changes: 3 additions & 3 deletions src/FetchTool.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ class FetchTool {
get ({url, ...options}) {
return fetch(url, Object.assign({method: 'GET'}, options))
.then(result => {
if (result.ok) return result.arrayBuffer();
if (result.ok) return new Uint8Array(result.arrayBuffer());
if (result.status === 404) return null;
return Promise.reject(result.status);
})
.then(body => new Uint8Array(body));
});
}

/**
Expand Down
8 changes: 4 additions & 4 deletions src/ScratchStorage.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ class ScratchStorage {
* @param {string} assetId - The ID of the asset to fetch: a project ID, MD5, etc.
* @param {DataFormat} [dataFormat] - Optional: load this format instead of the AssetType's default.
* @return {Promise.<Asset>} A promise for the requested Asset.
* If the promise is resolved with non-null, the value is the requested asset or a fallback.
* If the promise is resolved with non-null, the value is the requested asset.
* If the promise is resolved with null, the desired asset could not be found with the current asset sources.
* If the promise is rejected, there was an error on at least one asset source. HTTP 404 does not count as an
* error here, but (for example) HTTP 403 does.
Expand All @@ -182,7 +182,7 @@ class ScratchStorage {
let helperIndex = 0;
let helper;
const tryNextHelper = err => {
if (err) {
if (err) { // Track the error, but continue looking
errors.push(err);
}

Expand All @@ -198,8 +198,8 @@ class ScratchStorage {
// TODO: maybe some types of error should prevent trying the next helper?
.catch(tryNextHelper);
} else if (errors.length > 0) {
// At least one thing went wrong and also we couldn't find the
// asset.
// We looked through all the helpers and couldn't find the asset, AND
// at least one thing went wrong while we were looking.
return Promise.reject(errors);
}

Expand Down
19 changes: 15 additions & 4 deletions src/WebHelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ class WebHelper extends Helper {
const errors = [];
const stores = this.stores.slice()
.filter(store => store.types.indexOf(assetType.name) >= 0);

// New empty asset but it doesn't have data yet
const asset = new Asset(assetType, assetId, dataFormat);

let tool = this.assetTool;
Expand All @@ -98,11 +100,14 @@ class WebHelper extends Helper {
}

let storeIndex = 0;
const tryNextSource = () => {
const tryNextSource = err => {
if (err) {
errors.push(err);
}
const store = stores[storeIndex++];

/** @type {UrlFunction} */
const reqConfigFunction = store.get;
const reqConfigFunction = store && store.get;

if (reqConfigFunction) {
const reqConfig = ensureRequestConfig(reqConfigFunction(asset));
Expand All @@ -111,7 +116,13 @@ class WebHelper extends Helper {
}

return tool.get(reqConfig)
.then(body => asset.setData(body, dataFormat))
.then(body => {
if (body) {
asset.setData(body, dataFormat);
return asset;
}
return tryNextSource();
})
.catch(tryNextSource);
} else if (errors.length > 0) {
return Promise.reject(errors);
Expand All @@ -121,7 +132,7 @@ class WebHelper extends Helper {
return Promise.resolve(null);
};

return tryNextSource().then(() => asset);
return tryNextSource();
}

/**
Expand Down

0 comments on commit 0df263f

Please sign in to comment.