Merge pull request #1675 from kounoike/pr-handle-errors

Handle errors to show errors prettify and logging it.

src/main/scala/gitbucket/core/controller/ControllerBase.scala

@@ -26,6 +26,7 @@ import org.eclipse.jgit.lib.ObjectId
 import org.eclipse.jgit.revwalk.RevCommit
 import org.eclipse.jgit.treewalk._
 import org.apache.commons.io.IOUtils
+import org.slf4j.LoggerFactory
 
 /**
  * Provides generic features for controller implementations.
@@ -34,6 +35,8 @@ abstract class ControllerBase extends ScalatraFilter
   with ClientSideValidationFormSupport with JacksonJsonSupport with I18nSupport with FlashMapSupport with Validations
   with SystemSettingsService {
 
+  private val logger = LoggerFactory.getLogger(getClass)
+
   implicit val jsonFormats = gitbucket.core.api.JsonFormat.jsonFormats
 
   before("/api/v3/*") {
@@ -147,6 +150,20 @@ abstract class ControllerBase extends ScalatraFilter
       }
     }
 
+  error{
+    case e => {
+      logger.error(s"Catch unhandled error in request: ${request}", e)
+      if(request.hasAttribute(Keys.Request.Ajax)){
+        org.scalatra.InternalServerError()
+      } else if(request.hasAttribute(Keys.Request.APIv3)){
+        contentType = formats("json")
+        org.scalatra.InternalServerError(ApiError("Internal Server Error"))
+      } else {
+        org.scalatra.InternalServerError(gitbucket.core.html.error("Internal Server Error", Some(e)))
+      }
+    }
+  }
+
   override def url(path: String, params: Iterable[(String, Any)] = Iterable.empty,
                    includeContextPath: Boolean = true, includeServletPath: Boolean = true,
                    absolutize: Boolean = true, withSessionId: Boolean = true)

src/main/scala/gitbucket/core/controller/SystemSettingsController.scala

@@ -64,7 +64,8 @@ trait SystemSettingsControllerBase extends AccountManagementControllerBase {
         "ssl"                      -> trim(label("Enable SSL", optional(boolean()))),
         "keystore"                 -> trim(label("Keystore", optional(text())))
     )(Ldap.apply)),
-    "skinName" -> trim(label("AdminLTE skin name", text(required)))
+    "skinName" -> trim(label("AdminLTE skin name", text(required))),
+    "debug" -> trim(label("Debug", boolean()))
   )(SystemSettings.apply).verifying { settings =>
     Vector(
       if(settings.ssh && settings.baseUrl.isEmpty){

src/main/scala/gitbucket/core/service/SystemSettingsService.scala

@@ -55,6 +55,7 @@ trait SystemSettingsService {
         }
       }
       props.setProperty(SkinName, settings.skinName.toString)
+      props.setProperty(Debug, settings.debug.toString)
       using(new java.io.FileOutputStream(GitBucketConf)){ out =>
         props.store(out, null)
       }
@@ -113,7 +114,8 @@ trait SystemSettingsService {
         } else {
           None
         },
-        getValue(props, SkinName, "skin-blue")
+        getValue(props, SkinName, "skin-blue"),
+        getValue(props, Debug, false)
       )
     }
   }
@@ -139,7 +141,8 @@ object SystemSettingsService {
     smtp: Option[Smtp],
     ldapAuthentication: Boolean,
     ldap: Option[Ldap],
-    skinName: String){
+    skinName: String,
+    debug: Boolean){
     def baseUrl(request: HttpServletRequest): String = baseUrl.fold(request.baseUrl)(_.stripSuffix("/"))
 
     def sshAddress:Option[SshAddress] =
@@ -223,6 +226,7 @@ object SystemSettingsService {
   private val LdapSsl = "ldap.ssl"
   private val LdapKeystore = "ldap.keystore"
   private val SkinName = "skinName"
+  private val Debug = "debug"
 
   private def getValue[A: ClassTag](props: java.util.Properties, key: String, default: A): A = {
     getSystemProperty(key).getOrElse(getEnvironmentVariable(key).getOrElse {

src/main/twirl/gitbucket/core/error.scala.html

@@ -1,8 +1,22 @@
-@(title: String)(implicit context: gitbucket.core.controller.Context)
+@(title: String, e: Option[Throwable]=None)(implicit context: gitbucket.core.controller.Context)
 @gitbucket.core.html.main("Error"){
   <div class="content-wrapper main-center">
     <div class="content body">
       <h1>@title</h1>
+      @if(context.loginAccount.map{_.isAdmin}.getOrElse(false) || context.settings.debug){
+        @e.map { ex =>
+          <h2>@ex.getMessage</h2>
+          <table class="table table-condensed table-striped table-hover">
+            <tbody>
+            @ex.getStackTrace.map{ st =>
+            <tr><td>@st</td></tr>
+            }
+            </tbody>
+          </table>
+        }
+      } else {
+        <div>Please contact your administrator.</div>
+      }
     </div>
   </div>
-}
+}

src/test/scala/gitbucket/core/view/AvatarImageProviderSpec.scala

@@ -119,7 +119,8 @@ class AvatarImageProviderSpec extends FunSpec with MockitoSugar {
       smtp                     = None,
       ldapAuthentication       = false,
       ldap                     = None,
-      skinName                 = "skin-blue"
+      skinName                 = "skin-blue",
+      debug                    = false
     )
 
   /**