From ee56558869ce67f620c4f9886755c470272052bc Mon Sep 17 00:00:00 2001
From: dead-horse <dead_horse@qq.com>
Date: Mon, 13 Feb 2017 17:53:11 +0800
Subject: [PATCH 1/2] [BREAKING CHANGE] feat: remove notfound.enableRedirect

---
 app/middleware/notfound.js               |  9 +---
 config/config.default.js                 |  1 -
 docs/source/zh-cn/core/error-handling.md | 65 +++++++++++++++++++++---
 test/app/middleware/notfound.test.js     | 11 +---
 4 files changed, 62 insertions(+), 24 deletions(-)

diff --git a/app/middleware/notfound.js b/app/middleware/notfound.js
index f97562cd4e..d656430226 100644
--- a/app/middleware/notfound.js
+++ b/app/middleware/notfound.js
@@ -18,16 +18,11 @@ module.exports = options => {
       return;
     }
 
-    if (options.enableRedirect && options.pageUrl) {
+    if (options.pageUrl) {
       this.realStatus = 404;
       this.redirect(options.pageUrl);
       return;
     }
-    const title = '<h1>404 Not Found</h1>';
-    if (!options.enableRedirect && options.pageUrl) {
-      this.body = `${title}Because you are in a non-prod environment, you will be looking at this page, otherwise it will jump to ${options.pageUrl}`;
-    } else {
-      this.body = title;
-    }
+    this.body = '<h1>404 Not Found</h1>';
   };
 };
diff --git a/config/config.default.js b/config/config.default.js
index dc22505bed..254d766cd1 100644
--- a/config/config.default.js
+++ b/config/config.default.js
@@ -106,7 +106,6 @@ module.exports = appInfo => {
    */
   exports.notfound = {
     pageUrl: '',
-    enableRedirect: appInfo.env === 'prod',
   };
 
   /**
diff --git a/docs/source/zh-cn/core/error-handling.md b/docs/source/zh-cn/core/error-handling.md
index 96161646ee..0e096456b7 100644
--- a/docs/source/zh-cn/core/error-handling.md
+++ b/docs/source/zh-cn/core/error-handling.md
@@ -52,15 +52,15 @@ this.runInBackground(function* () {
 
 | 请求需求的格式 | 环境 | errorPageUrl 是否配置 | 返回内容 |
 |-------------|------|----------------------|--------|
-| html & text | local & unittest | - | onerror 自带的错误页面，展示详细的错误信息 |
-| html & text | 其他 | 是 | 重定向到 errorPageUrl |
-| html & text | 其他 | 否 | onerror 自带的没有错误信息的简单错误页（不推荐） |
-| json | local & unittest | - | json 对象，带详细的错误信息 |
-| json | 其他 | - | json 对象，不带详细的错误信息 |
+| HTML & TEXT | local & unittest | - | onerror 自带的错误页面，展示详细的错误信息 |
+| HTML & TEXT | 其他 | 是 | 重定向到 errorPageUrl |
+| HTML & TEXT | 其他 | 否 | onerror 自带的没有错误信息的简单错误页（不推荐） |
+| JSON | local & unittest | - | JSON 对象，带详细的错误信息 |
+| JSON | 其他 | - | json 对象，不带详细的错误信息 |
 
 ### errorPageUrl
 
-onerror 插件的配置中支持 errorPageUrl 属性，当配置了 errorPageUrl 时，一旦用户请求线上应用的 html 页面异常，就会重定向到这个地址。
+onerror 插件的配置中支持 errorPageUrl 属性，当配置了 errorPageUrl 时，一旦用户请求线上应用的 HTML 页面异常，就会重定向到这个地址。
 
 在 `config/config.default.js` 中
 
@@ -111,3 +111,56 @@ module.exports = {
   },
 };
 ```
+
+## 404
+
+框架并不会将服务端返回的 404 状态当做异常来处理，但是框架提供了当响应为 404 且没有返回 body 时的默认响应。
+
+- 当请求被框架判定为需要 JSON 格式的响应时，会返回一段 JSON：
+
+  ```json
+  { "message": "Not Found" }
+  ```
+
+- 当请求被框架判定为需要 HTML 格式的响应时，会返回一段 HTML：
+
+  ```html
+  <h1>404 Not Found</h1>
+  ```
+
+框架支持通过配置，将默认的 HTML 请求的 404 响应重定向到指定的页面。
+
+```js
+// config/config.default.js
+module.exports = {
+  notfound: {
+    pageUrl: '/404.html',
+  },
+};
+```
+
+### 自定义 404 响应
+
+在一些场景下，我们需要自定义服务器 404 时的响应，和自定义异常处理一样，我们也只需要加入一个中间件即可对 404 做统一处理：
+
+```js
+// app/middleware/notfound_handler.js
+module.exports = () => {
+  return function* (next) {
+    yield next;
+    if (this.status === 404 && !this.body) {
+      if (this.acceptJSON) this.body = { error: 'Not Found' };
+      else this.body = '<h1>Page Not Found</h1>';
+    }
+  };
+};
+```
+
+在配置中引入中间件：
+
+```js
+// config/config.default.js
+module.exports = {
+  middleware: [ 'notfoundHander' ],
+};
+```
diff --git a/test/app/middleware/notfound.test.js b/test/app/middleware/notfound.test.js
index 5d5f4c8ae7..ceb56e05e1 100644
--- a/test/app/middleware/notfound.test.js
+++ b/test/app/middleware/notfound.test.js
@@ -15,8 +15,7 @@ describe('test/app/middleware/notfound.test.js', () => {
 
   afterEach(mm.restore);
 
-  it('should 302 redirect to 404.html on production env', () => {
-    mm(app.config.notfound, 'enableRedirect', true);
+  it('should 302 redirect to 404.html', () => {
     return request(app.callback())
       .get('/test/404')
       .set('Accept', 'test/html')
@@ -24,13 +23,6 @@ describe('test/app/middleware/notfound.test.js', () => {
       .expect(302);
   });
 
-  it('should show 404 on dev env', () => {
-    return request(app.callback())
-      .get('/test/404')
-      .expect('<h1>404 Not Found</h1>Because you are in a non-prod environment, you will be looking at this page, otherwise it will jump to https://eggjs.org/404')
-      .expect(404);
-  });
-
   it('should 404 json response', () => {
     return request(app.callback())
       .get('/test/404.json?ctoken=404')
@@ -71,7 +63,6 @@ describe('test/app/middleware/notfound.test.js', () => {
 
     it('should 302 redirect to custom /404 on production env', done => {
       done = pedding(2, done);
-      mm(app.config.notfound, 'enableRedirect', true);
 
       request(app.callback())
         .get('/test/404')

From 436ebe819cb2fb3c7908eef65d88b447b566332a Mon Sep 17 00:00:00 2001
From: dead-horse <dead_horse@qq.com>
Date: Mon, 13 Feb 2017 18:20:24 +0800
Subject: [PATCH 2/2] feat: avoid circular redirects

---
 app/middleware/notfound.js           | 10 +++++++++-
 test/app/middleware/notfound.test.js | 30 +++++++++++++++++++---------
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/app/middleware/notfound.js b/app/middleware/notfound.js
index d656430226..daaf28705b 100644
--- a/app/middleware/notfound.js
+++ b/app/middleware/notfound.js
@@ -18,11 +18,19 @@ module.exports = options => {
       return;
     }
 
+    const notFoundHtml = '<h1>404 Not Found</h1>';
+
+    // notfound handler is unimplemented
+    if (options.pageUrl && this.path === options.pageUrl) {
+      this.body = `${notFoundHtml}<p><pre><code>config.notfound.pageUrl(${options.pageUrl})</code></pre> is unimplemented</p>`;
+      return;
+    }
+
     if (options.pageUrl) {
       this.realStatus = 404;
       this.redirect(options.pageUrl);
       return;
     }
-    this.body = '<h1>404 Not Found</h1>';
+    this.body = notFoundHtml;
   };
 };
diff --git a/test/app/middleware/notfound.test.js b/test/app/middleware/notfound.test.js
index ceb56e05e1..e5182d29e8 100644
--- a/test/app/middleware/notfound.test.js
+++ b/test/app/middleware/notfound.test.js
@@ -1,6 +1,5 @@
 'use strict';
 
-const pedding = require('pedding');
 const request = require('supertest');
 const mm = require('egg-mock');
 const utils = require('../../utils');
@@ -51,7 +50,7 @@ describe('test/app/middleware/notfound.test.js', () => {
       .expect(404);
   });
 
-  describe('app.404.url=/404', () => {
+  describe('config.notfound.pageUrl = "/404"', () => {
     let app;
     before(() => {
       app = utils.app('apps/notfound-custom-404');
@@ -61,19 +60,32 @@ describe('test/app/middleware/notfound.test.js', () => {
 
     afterEach(mm.restore);
 
-    it('should 302 redirect to custom /404 on production env', done => {
-      done = pedding(2, done);
-
-      request(app.callback())
+    it('should 302 redirect to custom /404 when required html', function* () {
+      yield request(app.callback())
         .get('/test/404')
         .set('Accept', 'test/html')
         .expect('Location', '/404')
-        .expect(302, done);
+        .expect(302);
 
-      request(app.callback())
+      yield request(app.callback())
         .get('/404')
         .expect('Hi, this is 404')
-        .expect(200, done);
+        .expect(200);
+    });
+
+    it('should not avoid circular redirects', function* () {
+      mm(app.config.notfound, 'pageUrl', '/notfound');
+
+      yield request(app.callback())
+        .get('/test/404')
+        .set('Accept', 'test/html')
+        .expect('Location', '/notfound')
+        .expect(302);
+
+      yield request(app.callback())
+        .get('/notfound')
+        .expect('<h1>404 Not Found</h1><p><pre><code>config.notfound.pageUrl(/notfound)</code></pre> is unimplemented</p>')
+        .expect(404);
     });
   });
 });