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

Jump to bottom

fix: prevent totalCommitsFetch error result from being cached #2177

Closed
wants to merge 2 commits into from
Closed

fix: prevent totalCommitsFetch error result from being cached #2177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3123e68
fix: prevent totalCommitsFetch error result from being cached
rickstaa Oct 10, 2022
6c2d175
Merge branch 'master' into prevent_total_fetch_zero_cache
rickstaa Jan 10, 2023
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
21 changes: 14 additions & 7 deletions api/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,25 +48,32 @@ export default async (req, res) => {
}

try {
const stats = await fetchStats(
const statsResp = await fetchStats(
username,
parseBoolean(count_private),
parseBoolean(include_all_commits),
parseArray(exclude_repo),
);
const stats = statsResp.stats;

const cacheSeconds = clampValue(
parseInt(cache_seconds || CONSTANTS.FOUR_HOURS, 10),
CONSTANTS.FOUR_HOURS,
CONSTANTS.ONE_DAY,
);

res.setHeader(
"Cache-Control",
`max-age=${
cacheSeconds / 2
}, s-maxage=${cacheSeconds}, stale-while-revalidate=${CONSTANTS.ONE_DAY}`,
);
if (statsResp.success) {
res.setHeader(
"Cache-Control",
`max-age=${
cacheSeconds / 2
}, s-maxage=${cacheSeconds}, stale-while-revalidate=${
CONSTANTS.ONE_DAY
}`,
);
} else {
res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache unsuccessful responses.
Copy link
Owner

Choose a reason for hiding this comment

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

Good idea, but I'm just worried that this will make it so that vercel will need to execute more fresh invocations causing increased load when PATs get exhausted.

For example say we get 10k fresh requests per day, and we cache 70% of the requests, if PATs get exhausted we will render & cache the error card and after 4hour everything will go back to normal.

But if we do this, all those 10k requests need to be executed by vercel even though they will all respond with error card.

I think ideal solution would be to decrease the cache seconds on errors: instead of no-cache let's do 1h for error responses (1h since cache will invalidate the same time as PATs get revalidate)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is 100% true I overlooked that fact. Let's close this for now.

Copy link
Collaborator Author

@rickstaa rickstaa Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@anuraghazra In that case, we should also change the following code back so that it caches the error:

github-readme-stats/api/wakatime.js

Line 84 in 0ff426d

return res.send(renderError(err.message, err.secondaryMessage));

github-readme-stats/api/top-langs.js

Line 81 in 0ff426d

res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.

github-readme-stats/api/pin.js

Line 83 in 0ff426d

res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.

github-readme-stats/api/index.js

Line 96 in 0ff426d

res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.

These were merged some time ago and might also be depleting the PATs. However, I suspect this catch block is only triggered when something goes wrong in our code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The fetcher handles errors using throw as well as trycatch, so that means that errors with the API as well as with our code should trigger the trycatch in ./api/index.js, right?

Trying it out in the browser with a random (nonexistent) username gives a no-cache header, so you're right; this should be changed back, since it has the same effect as whatever the PRs trying to do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Zo-Bro-23 I changed the code. Please review #2448. 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we close this PR now since #2448 does the same thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This pull request handles errors from the REST API, which will write 0 commits to the card when it fails. While the other pull request handles other errors. I will update this one when #2448 is merged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I understand, API errors will trigger Javascript errors, which means that what the other PR does will apply to API errors too.

github-readme-stats/src/fetchers/stats-fetcher.js

Lines 215 to 218 in 4b17300

throw new CustomError(
"Something went while trying to retrieve the stats data using the GraphQL API.",
CustomError.GRAPHQL_ERROR,
);

The fetcher is throwing a Javascript error, so GraphQL errors should trigger the trycatch block too right? Or am I missing something?

}

return res.send(
renderStatsCard(stats, {
Expand Down
64 changes: 38 additions & 26 deletions src/fetchers/stats-fetcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ const fetcher = (variables, token) => {
* Fetch first 100 repositories for a given username.
*
* @param {import('axios').AxiosRequestHeaders} variables Fetcher variables.
* @param {string} token GitHub token.
* @returns {Promise<import('../common/types').StatsFetcherResponse>} Repositories fetcher response.
* @param {string} token Github token.
* @returns {Promise<import('../common/types').RepositoriesFetcherResponse>} Repositories fetcher response.
*/
const repositoriesFetcher = (variables, token) => {
return request(
Expand Down Expand Up @@ -95,46 +95,54 @@ const repositoriesFetcher = (variables, token) => {
);
};

/**
* Fetch all commits for a given username.
*
* @param {import('axios').AxiosRequestHeaders} variables Fetcher variables.
* @param {string} token Github token.
* @returns {Promise<import('../common/types').TotalCommitsFetcherResponse>} Total commits fetcher response.
*
* @see https://developer.github.com/v3/search/#search-commits.
*/
const totalCommitsFetcher = (variables, token) => {
return axios({
method: "get",
url: `https://api.github.com/search/commits?q=author:${variables.login}`,
headers: {
"Content-Type": "application/json",
Accept: "application/vnd.github.cloak-preview",
Authorization: `token ${token}`,
},
});
};

/**
* Fetch all the commits for all the repositories of a given username.
*
* @param {*} username GitHub username.
* @returns {Promise<number>} Total commits.
* @param {*} username Github username.
* @returns {Promise<Object>} Object containing the total number of commits and a success boolean.
*
* @description Done like this because the GitHub API does not provide a way to fetch all the commits. See
* #92#issuecomment-661026467 and #211 for more information.
*/
const totalCommitsFetcher = async (username) => {
const fetchTotalCommits = async (username) => {
if (!githubUsernameRegex.test(username)) {
logger.log("Invalid username");
return 0;
return { totalCommits: 0, success: false };
}

// https://developer.github.com/v3/search/#search-commits
const fetchTotalCommits = (variables, token) => {
return axios({
method: "get",
url: `https://api.github.com/search/commits?q=author:${variables.login}`,
headers: {
"Content-Type": "application/json",
Accept: "application/vnd.github.cloak-preview",
Authorization: `token ${token}`,
},
});
};

try {
let res = await retryer(fetchTotalCommits, { login: username });
let res = await retryer(totalCommitsFetcher, { login: username });
let total_count = res.data.total_count;
if (!!total_count && !isNaN(total_count)) {
return res.data.total_count;
return { totalCommits: res.data.total_count, success: true };
}
} catch (err) {
logger.log(err);
}
// just return 0 if there is something wrong so that
// we don't break the whole app
return 0;
return { totalCommits: 0, success: false };
};

/**
Expand Down Expand Up @@ -183,7 +191,7 @@ const totalStarsFetcher = async (username, repoToHide) => {
* @param {string} username GitHub username.
* @param {boolean} count_private Include private contributions.
* @param {boolean} include_all_commits Include all commits.
* @returns {Promise<import("./types").StatsData>} Stats data.
* @returns {Promise<Object>} Object containing the stats data a success boolean that can be used to handle the caching behavior.
*/
const fetchStats = async (
username,
Expand Down Expand Up @@ -244,9 +252,13 @@ const fetchStats = async (
stats.totalCommits = user.contributionsCollection.totalCommitContributions;

// if include_all_commits then just get that,
// since totalCommitsFetcher already sends totalCommits no need to +=
// NOTE: Since fetchTotalCommits already sends totalCommits no need to +=.
let totalCommitsResp;
let success = true;
if (include_all_commits) {
stats.totalCommits = await totalCommitsFetcher(username);
totalCommitsResp = await fetchTotalCommits(username);
stats.totalCommits = totalCommitsResp.totalCommits;
success = totalCommitsResp.success;
}

// if count_private then add private commits to totalCommits so far.
Expand All @@ -271,7 +283,7 @@ const fetchStats = async (
issues: stats.totalIssues,
});

return stats;
return { stats, success };
};

export { fetchStats };
Expand Down
35 changes: 30 additions & 5 deletions tests/fetchStats.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ afterEach(() => {

describe("Test fetchStats", () => {
it("should fetch correct stats", async () => {
let stats = await fetchStats("anuraghazra");
let { stats } = await fetchStats("anuraghazra");
const rank = calculateRank({
totalCommits: 100,
totalRepos: 5,
Expand Down Expand Up @@ -140,7 +140,7 @@ describe("Test fetchStats", () => {
.onPost("https://api.github.com/graphql")
.replyOnce(200, repositoriesWithZeroStarsData);

let stats = await fetchStats("anuraghazra");
let { stats } = await fetchStats("anuraghazra");
const rank = calculateRank({
totalCommits: 100,
totalRepos: 5,
Expand Down Expand Up @@ -172,7 +172,7 @@ describe("Test fetchStats", () => {
});

it("should fetch and add private contributions", async () => {
let stats = await fetchStats("anuraghazra", true);
let { stats } = await fetchStats("anuraghazra", true);
const rank = calculateRank({
totalCommits: 150,
totalRepos: 5,
Expand Down Expand Up @@ -201,7 +201,7 @@ describe("Test fetchStats", () => {
.onGet("https://api.github.com/search/commits?q=author:anuraghazra")
.reply(200, { total_count: 1000 });

let stats = await fetchStats("anuraghazra", true, true);
let { stats } = await fetchStats("anuraghazra", true, true);
const rank = calculateRank({
totalCommits: 1050,
totalRepos: 5,
Expand All @@ -225,12 +225,37 @@ describe("Test fetchStats", () => {
});
});

it("should return `true` success boolean when 'include_all_commits' is `false`", async () => {
let { success } = await fetchStats("anuraghazra", true, false);
expect(success).toStrictEqual(true);
});

it("should return `true` success boolean when 'include_all_commits' is `true` and total commits were fetched successfully", async () => {
mock
.onGet("https://api.github.com/search/commits?q=author:anuraghazra")
.reply(200, { total_count: 1000 });

let { success } = await fetchStats("anuraghazra", true, true);
expect(success).toStrictEqual(true);
});

it("should return `false` success boolean when 'include_all_commits' is `true` and total commits could not be fetched", async () => {
mock
.onGet("https://api.github.com/search/commits?q=author:anuraghazra")
.reply(404, "Could not fetch total commits");

let { success } = await fetchStats("anuraghazra", true, true);
expect(success).toStrictEqual(false);
});

it("should exclude stars of the `test-repo-1` repository", async () => {
mock
.onGet("https://api.github.com/search/commits?q=author:anuraghazra")
.reply(200, { total_count: 1000 });

let stats = await fetchStats("anuraghazra", true, true, ["test-repo-1"]);
let { stats } = await fetchStats("anuraghazra", true, true, [
"test-repo-1",
]);
const rank = calculateRank({
totalCommits: 1050,
totalRepos: 5,
Expand Down