Skip to content
This repository has been archived by the owner on Feb 28, 2022. It is now read-only.

feat(html pipe): Handling 404 Errors #326

Closed
wants to merge 2 commits into from
Closed

feat(html pipe): Handling 404 Errors #326

wants to merge 2 commits into from

Conversation

ramboz
Copy link
Contributor

@ramboz ramboz commented May 10, 2019

Graceful error handling with custom pages and proper fallback

fix #227

Graceful error handling with custom pages and proper fallback

fix #227
@@ -97,6 +97,10 @@ async function fetch(
};
} catch (e) {
if (e && e.statusCode && e.statusCode === 404) {
if (path.match(/^\/?404\.(md|html)$/)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that we will first do a regular bail with the proper status code (i.e. 400, 500, etc.) and try to fetch the corresponding md file (400.md, 500.md, etc.).
If that is missing, we'll end up here again with a path that matches 404, and perform a safebail as fallback

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I would do this in an/the error handler at the end of the pipe, and not in fetch.

Add unit test to verify graceful handling of missing 404.md error page

fix #227
@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #326 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #326   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          46     46           
  Lines         965    969    +4     
=====================================
+ Hits          965    969    +4
Impacted Files Coverage Δ
src/html/fetch-markdown.js 100% <100%> (ø) ⬆️
src/helper.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9bb5ac...4c6533f. Read the comment docs.

@trieloff
Copy link
Contributor

In terms of traffic control, I'd like to merge #260 first, even if we still have to discuss through a couple of issues that would affect the handling of errors (and this PR)

@@ -97,6 +97,10 @@ async function fetch(
};
} catch (e) {
if (e && e.statusCode && e.statusCode === 404) {
if (path.match(/^\/?404\.(md|html)$/)) {
// Passing status 200 since we'll first have tried 400.html with a 404
return safebail(logger, `Could not find Markdown at ${options.uri}`, e, 200);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is IMO wrong: you must not return a 200. otherwise fastly doesn't know that the resource is missing and cannot fallback to static processing...which actually renders this entire feature useless :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

We can control the fallback, but it should definitely be a 404.

@tripodsan
Copy link
Contributor

since we do error handling now in the dispatcher, I'll close this for now.

@tripodsan tripodsan closed this Jul 18, 2019
@rofe rofe deleted the issues/227 branch November 28, 2019 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Idea: Handling 404 Errors
3 participants