Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix large files failing to load. #1224

Merged
merged 4 commits into from
May 17, 2018
Merged

Fix large files failing to load. #1224

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2613a2c
Fix large files failing to load.
tech4him1 Apr 3, 2018
b066dd6
Handle large files fetched by path.
tech4him1 Apr 3, 2018
f6e9c81
cleanup
tech4him1 Apr 3, 2018
57b9ebd
typo
tech4him1 Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions src/backends/github/API.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import LocalForage from "Lib/LocalForage";
import { Base64 } from "js-base64";
import { uniq, initial, last, get, find } from "lodash";
import { uniq, initial, last, get, find, hasIn } from "lodash";
import { filterPromises, resolvePromiseProperties } from "Lib/promiseHelper";
import AssetProxy from "ValueObjects/AssetProxy";
import { SIMPLE, EDITORIAL_WORKFLOW, status } from "Constants/publishModes";
Expand Down Expand Up @@ -156,18 +156,33 @@ export default class API {
}

readFile(path, sha, branch = this.branch) {
const cache = sha ? LocalForage.getItem(`gh.${ sha }`) : Promise.resolve(null);
return cache.then((cached) => {
if (cached) { return cached; }

if (sha) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Benaiah I'm assuming if we know the SHA we should fetch by SHA first. Is that a problem at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there's some benefit to using the Contents API, e.g. differences in the response object, fetching by SHA make sense to me.

return this.getBlob(sha);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fetch the file by path if the SHA is invalid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the Contents API as a backup makes sense. I feel like there are edge cases like file name changes where this could cause confusion, but that seems pretty low risk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll need to go back and check -- I'm assuming that we can just check for a 404?

Copy link
Contributor Author

@tech4him1 tech4him1 May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although GitHub doesn't seem to ever do GC -- an invalid blob would be really weird. It would likely mean we were hashing the file differently than GitHub was on the server-side, which shouldn't happen.

} else {
return this.request(`${ this.repoURL }/contents/${ path }`, {
headers: { Accept: "application/vnd.github.VERSION.raw" },
params: { ref: branch },
cache: "no-store",
}).then((result) => {
if (sha) {
LocalForage.setItem(`gh.${ sha }`, result);
}).catch(error => {
if (hasIn(error, 'message.errors') && find(error.message.errors, { code: "too_large" })) {
const dir = path.split('/').slice(0, -1).join('/');
return this.listFiles(dir)
.then(files => files.find(file => file.path === path))
.then(file => getBlob(file.sha));
}
throw error;
});
}
}

getBlob(sha) {
return LocalForage.getItem(`gh.${sha}`).then(cached => {
if (cached) { return cached; }

return this.request(`${this.repoURL}/git/blobs/${sha}`, {
headers: { Accept: "application/vnd.github.VERSION.raw" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to drop this header as it's only documented for use with the Contents API (unless you experienced an issue that required it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is for use with the blobs API as well, see https://developer.github.com/v3/git/blobs/.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see it now 👍

}).then(result => {
LocalForage.setItem(`gh.${sha}`, result);
return result;
});
});
Expand Down