]> git.openstreetmap.org Git - nominatim.git/commitdiff
set exception handler by request format, not always HTML
authormarc tobias <mtmail@gmx.net>
Sun, 16 Sep 2018 15:16:42 +0000 (17:16 +0200)
committerMarc Tobias Metten <mtmail@gmx.net>
Wed, 3 Oct 2018 20:58:20 +0000 (22:58 +0200)
20 files changed:
lib/DatabaseError.php [new file with mode: 0644]
lib/init-website.php
lib/lib.php
lib/template/error-html.php [new file with mode: 0644]
lib/template/error-json.php [new file with mode: 0644]
lib/template/error-xml.php [new file with mode: 0644]
test/bdd/api/errors/formats.feature [new file with mode: 0644]
test/bdd/api/lookup/simple.feature
test/bdd/api/search/simple.feature
test/bdd/steps/queries.py
test/php/Nominatim/AddressDetailsTest.php
test/php/Nominatim/DatabaseErrorTest.php [new file with mode: 0644]
test/php/Nominatim/LibTest.php
test/php/Nominatim/StatusTest.php
test/php/Nominatim/TokenListTest.php
test/php/bootstrap.php
website/details.php
website/lookup.php
website/reverse.php
website/search.php

diff --git a/lib/DatabaseError.php b/lib/DatabaseError.php
new file mode 100644 (file)
index 0000000..2df331b
--- /dev/null
@@ -0,0 +1,32 @@
+<?php
+
+namespace Nominatim;
+
+class DatabaseError extends \Exception
+{
+
+    public function __construct($message, $code = 500, Exception $previous = null, $oSql)
+    {
+        parent::__construct($message, $code, $previous);
+        $this->oSql = $oSql;
+    }
+
+    public function __toString()
+    {
+        return __CLASS__ . ": [{$this->code}]: {$this->message}\n";
+    }
+
+    public function getSqlError()
+    {
+        return $this->oSql->getMessage();
+    }
+
+    public function getSqlDebugDump()
+    {
+        if (CONST_Debug) {
+            return var_export($this->oSql, true);
+        } else {
+            return $this->oSql->getUserInfo();
+        }
+    }
+}
index ca5214da07fd50530ae5abec74bb3cba1afa5e1c..ae2a5d360ae57dd439b29b46507bff99afdc0432 100644 (file)
@@ -2,6 +2,7 @@
 
 require_once('init.php');
 require_once('ParameterParser.php');
+require_once('DatabaseError.php');
 require_once(CONST_Debug ? 'DebugHtml.php' : 'DebugNone.php');
 
 /***************************************************************************
@@ -15,74 +16,51 @@ function chksql($oSql, $sMsg = 'Database request failed')
 {
     if (!PEAR::isError($oSql)) return $oSql;
 
-    header('HTTP/1.0 500 Internal Server Error');
-    header('Content-type: text/html; charset=utf-8');
-
-    $sSqlError = $oSql->getMessage();
-
-    echo <<<INTERNALFAIL
-<html>
-  <head><title>Internal Server Error</title></head>
-  <body>
-    <h1>Internal Server Error</h1>
-    <p>Nominatim has encountered an internal error while accessing the database.
-       This may happen because the database is broken or because of a bug in
-       the software. If you think it is a bug, feel free to report
-       it over on <a href="https://github.com/openstreetmap/Nominatim/issues">
-       Github</a>. Please include the URL that caused the problem and the
-       complete error details below.</p>
-    <p><b>Message:</b> $sMsg</p>
-    <p><b>SQL Error:</b> $sSqlError</p>
-    <p><b>Details:</b> <pre>
-INTERNALFAIL;
-
-    if (CONST_Debug) {
-        var_dump($oSql);
-    } else {
-        echo "<pre>\n".$oSql->getUserInfo().'</pre>';
-    }
+    throw new Nominatim\DatabaseError($sMsg, 500, null, $oSql);
+}
+
 
-    echo '</pre></p></body></html>';
-    exit;
+function userError($sMsg)
+{
+    throw new Exception($sMsg, 400);
 }
 
-function failInternalError($sError, $sSQL = false, $vDumpVar = false)
+
+function exception_handler_html($exception)
 {
-    header('HTTP/1.0 500 Internal Server Error');
-    header('Content-type: text/html; charset=utf-8');
-    echo '<html><body><h1>Internal Server Error</h1>';
-    echo '<p>Nominatim has encountered an internal error while processing your request. This is most likely because of a bug in the software.</p>';
-    echo '<p><b>Details:</b> '.$sError,'</p>';
-    echo '<p>Feel free to file an issue on <a href="https://github.com/openstreetmap/Nominatim/issues">Github</a>. ';
-    echo 'Please include the error message above and the URL you used.</p>';
-    if (CONST_Debug) {
-        echo '<hr><h2>Debugging Information</h2><br>';
-        if ($sSQL) {
-            echo '<h3>SQL query</h3><code>'.$sSQL.'</code>';
-        }
-        if ($vDumpVar) {
-            echo '<h3>Result</h3> <code>';
-            var_dump($vDumpVar);
-            echo '</code>';
-        }
-    }
-    echo "\n</body></html>\n";
-    exit;
+    http_response_code($exception->getCode());
+    header('Content-type: text/html; charset=UTF-8');
+    include(CONST_BasePath.'/lib/template/error-html.php');
 }
 
+function exception_handler_json($exception)
+{
+    http_response_code($exception->getCode());
+    header('Content-type: application/json; charset=utf-8');
+    include(CONST_BasePath.'/lib/template/error-json.php');
+}
 
-function userError($sError)
+function exception_handler_xml($exception)
 {
-    header('HTTP/1.0 400 Bad Request');
-    header('Content-type: text/html; charset=utf-8');
-    echo '<html><body><h1>Bad Request</h1>';
-    echo '<p>Nominatim has encountered an error with your request.</p>';
-    echo '<p><b>Details:</b> '.$sError.'</p>';
-    echo '<p>If you feel this error is incorrect feel file an issue on <a href="https://github.com/openstreetmap/Nominatim/issues">Github</a>. ';
-    echo 'Please include the error message above and the URL you used.</p>';
-    echo "\n</body></html>\n";
-    exit;
+    http_response_code($exception->getCode());
+    header('Content-type: text/xml; charset=utf-8');
+    echo '<?xml version="1.0" encoding="UTF-8" ?>'."\n";
+    include(CONST_BasePath.'/lib/template/error-xml.php');
+}
+
+
+function set_exception_handler_by_format($sFormat = 'html')
+{
+    if ($sFormat == 'html') {
+        set_exception_handler('exception_handler_html');
+    } elseif ($sFormat == 'xml') {
+        set_exception_handler('exception_handler_xml');
+    } else {
+        set_exception_handler('exception_handler_json');
+    }
 }
+// set a default
+set_exception_handler_by_format();
 
 
 /***************************************************************************
@@ -96,6 +74,6 @@ if (CONST_NoAccessControl) {
         header('Access-Control-Allow-Headers: '.$_SERVER['HTTP_ACCESS_CONTROL_REQUEST_HEADERS']);
     }
 }
-if ($_SERVER['REQUEST_METHOD'] == 'OPTIONS') exit;
+if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] == 'OPTIONS') exit;
 
 if (CONST_Debug) header('Content-type: text/html; charset=utf-8');
index a749cb6bd65842b5264f17ae2b23c0033aa1aa3e..cd23b06843ca550304a50924bb071be3108d8a8b 100644 (file)
@@ -61,6 +61,13 @@ function byImportance($a, $b)
 
 function javascript_renderData($xVal, $iOptions = 0)
 {
+    $sCallback = isset($_GET['json_callback']) ? $_GET['json_callback'] : '';
+    if ($sCallback && !preg_match('/^[$_\p{L}][$_\p{L}\p{Nd}.[\]]*$/u', $sCallback)) {
+        // Unset, we call javascript_renderData again during exception handling
+        unset($_GET['json_callback']);
+        throw new Exception('Invalid json_callback value', 400);
+    }
+
     $iOptions |= JSON_UNESCAPED_UNICODE;
     if (isset($_GET['pretty']) && in_array(strtolower($_GET['pretty']), array('1', 'true'))) {
         $iOptions |= JSON_PRETTY_PRINT;
@@ -68,16 +75,12 @@ function javascript_renderData($xVal, $iOptions = 0)
 
     $jsonout = json_encode($xVal, $iOptions);
 
-    if (!isset($_GET['json_callback'])) {
+    if ($sCallback) {
+        header('Content-Type: application/javascript; charset=UTF-8');
+        echo $_GET['json_callback'].'('.$jsonout.')';
+    } else {
         header('Content-Type: application/json; charset=UTF-8');
         echo $jsonout;
-    } else {
-        if (preg_match('/^[$_\p{L}][$_\p{L}\p{Nd}.[\]]*$/u', $_GET['json_callback'])) {
-            header('Content-Type: application/javascript; charset=UTF-8');
-            echo $_GET['json_callback'].'('.$jsonout.')';
-        } else {
-            header('HTTP/1.0 400 Bad Request');
-        }
     }
 }
 
diff --git a/lib/template/error-html.php b/lib/template/error-html.php
new file mode 100644 (file)
index 0000000..208237e
--- /dev/null
@@ -0,0 +1,61 @@
+<?php
+
+    $title = 'Internal Server Error';
+    if ( $exception->getCode() == 400 ) {
+        $title = 'Bad Request';
+    }
+?>
+<!DOCTYPE html>
+<html lang="en">
+<head>
+    <style>
+        em { font-weight: bold; font-family: monospace; color: #e00404; background-color: #ffeaea; }
+    </style>
+</head>
+<body>
+    <h1><?php echo $title ?></h1>
+    
+    <?php if (get_class($exception) == 'Nominatim\DatabaseError') { ?>
+
+        <p>Nominatim has encountered an internal error while accessing the database.
+           This may happen because the database is broken or because of a bug in
+           the software.</p>
+
+    <?php } else { ?>
+
+        <p>Nominatim has encountered an error with your request.</p>
+
+    <?php } ?>
+
+
+    <h3>Details</h3>
+
+    Uncaught exception <em><?php echo get_class($exception) ?></em>
+    with message <em><?php echo $exception->getMessage() ?></em>
+
+    <?php if (CONST_Debug) { ?>
+        <br>
+        thrown in <em><?php $exception->getFile() . '('. $exception->getLine() . ')' ?></em>.
+
+        <?php if (get_class($exception) == 'Nominatim\DatabaseError') { ?>
+
+            <h3>SQL Error</h3>
+            <em><?php echo $exception->getSqlError() ?></em>
+
+            <pre><?php echo $exception->getSqlDebugDump() ?></pre>
+
+        <?php } ?>
+
+        <h3>Stack trace</h3>
+        <pre><?php echo $exception->getTraceAsString() ?></pre>
+
+    <?php } ?>
+
+    <p>
+        If you feel this error is incorrect feel file an issue on
+        <a href="https://github.com/openstreetmap/Nominatim/issues">Github</a>.
+
+        Please include the error message above and the URL you used.
+    </p>
+</body>
+</html>
diff --git a/lib/template/error-json.php b/lib/template/error-json.php
new file mode 100644 (file)
index 0000000..81caa71
--- /dev/null
@@ -0,0 +1,11 @@
+<?php
+    $error = array(
+              'code' => $exception->getCode(),
+              'message' => $exception->getMessage()
+    );
+
+    if (CONST_Debug) {
+        $error['details'] = $exception->getFile() . '('. $exception->getLine() . ')';
+    }
+
+    echo javascript_renderData(array('error' => $error));
diff --git a/lib/template/error-xml.php b/lib/template/error-xml.php
new file mode 100644 (file)
index 0000000..a21ac19
--- /dev/null
@@ -0,0 +1,7 @@
+<error>
+    <code><?php echo $exception->getCode() ?></code>
+    <message><?php echo $exception->getMessage() ?></message>
+    <?php if (CONST_Debug) { ?>
+    <details><?php echo $exception->getFile() . '('. $exception->getLine() . ')' ?></details>
+    <?php } ?>
+</error>
\ No newline at end of file
diff --git a/test/bdd/api/errors/formats.feature b/test/bdd/api/errors/formats.feature
new file mode 100644 (file)
index 0000000..8a8e656
--- /dev/null
@@ -0,0 +1,13 @@
+@APIDB
+Feature: Places by osm_type and osm_id Tests
+    Simple tests for errors in various response formats.
+
+    Scenario Outline: Force error by providing too many ids
+        When sending <format> lookup query for N1,N2,N3,N4,N5,N6,N7,N8,N9,N10,N11,N12,N13,N14,N15,N16,N17,N18,N19,N20,N21,N22,N23,N24,N25,N26,N27,N28,N29,N30,N31,N32,N33,N34,N35,N36,N37,N38,N39,N40,N41,N42,N43,N44,N45,N46,N47,N48,N49,N50,N51
+        Then a <format> user error is returned
+
+    Examples:
+        | format  |
+        | xml     |
+        | json    |
+        | geojson |
index e29812e0123b15b4364afd506f1029ab65a744c3..660598db17352acb962ae94c2d7de9025397f253 100644 (file)
@@ -1,6 +1,6 @@
 @APIDB
 Feature: Places by osm_type and osm_id Tests
-    Simple tests for internal server errors and response format.
+    Simple tests for response format.
 
     Scenario Outline: address lookup for existing node, way, relation
         When sending <format> lookup query for N3284625766,W6065798,,R123924,X99,N0
index ca441258784c2fde1f468883d77f768fb833b145..c12635972dd29a48afb0cd9ed8f72b871da0334a 100644 (file)
@@ -194,7 +194,7 @@ Feature: Simple Tests
         When sending json search query "Tokyo"
             | param        | value |
             |json_callback | <data> |
-        Then a HTTP 400 is returned
+        Then a json user error is returned
 
     Examples:
       | data |
index df34b5cc0696fc17d156a5500fb4047cb03b5b8f..4d59b923ed3c39b9fee9f7556b6fe31ea7c49417 100644 (file)
@@ -494,6 +494,18 @@ def step_impl(context, fmt):
     context.execute_steps("Then a HTTP 200 is returned")
     eq_(context.response.format, fmt)
 
+@then(u'a (?P<fmt>\w+) user error is returned')
+def check_page_error(context, fmt):
+    context.execute_steps("Then a HTTP 400 is returned")
+    eq_(context.response.format, fmt)
+
+    if fmt == 'html':
+        assert_is_not_none(re.search(r'<html( |>).+</html>', context.response.page, re.DOTALL))
+    elif fmt == 'xml':
+        assert_is_not_none(re.search(r'<error>.+</error>', context.response.page, re.DOTALL))
+    else:
+        assert_is_not_none(re.search(r'({"error":)', context.response.page, re.DOTALL))
+
 @then(u'result header contains')
 def check_header_attr(context):
     for line in context.table:
index 62faf1a4b2d7caa27b0dd9cbcc819d65f2dc0457..b29d908862346cc1c03f3dd819b410f10b53f66a 100644 (file)
@@ -2,14 +2,10 @@
 
 namespace Nominatim;
 
+require_once(CONST_BasePath.'/lib/init-website.php');
 require_once(CONST_BasePath.'/lib/AddressDetails.php');
 
 
-function chksql($oSql, $sMsg = 'Database request failed')
-{
-    return $oSql;
-}
-
 class AddressDetailsTest extends \PHPUnit\Framework\TestCase
 {
 
diff --git a/test/php/Nominatim/DatabaseErrorTest.php b/test/php/Nominatim/DatabaseErrorTest.php
new file mode 100644 (file)
index 0000000..25b4aa0
--- /dev/null
@@ -0,0 +1,44 @@
+<?php
+
+namespace Nominatim;
+
+require_once(CONST_BasePath.'/lib/init-website.php');
+require_once(CONST_BasePath.'/lib/DatabaseError.php');
+
+class DatabaseErrorTest extends \PHPUnit\Framework\TestCase
+{
+
+    public function testSqlMessage()
+    {
+        $oSqlStub = $this->getMockBuilder(\DB_Error::class)
+                    ->setMethods(array('getMessage'))
+                    ->getMock();
+
+        $oSqlStub->method('getMessage')
+                ->willReturn('Unknown table.');
+
+        $oErr = new DatabaseError('Sql error', 123, null, $oSqlStub);
+        $this->assertEquals('Sql error', $oErr->getMessage());
+        $this->assertEquals(123, $oErr->getCode());
+        $this->assertEquals('Unknown table.', $oErr->getSqlError());
+
+        // causes a circular reference warning during dump
+        // $this->assertRegExp('/Mock_DB_Error/', $oErr->getSqlDebugDump());
+    }
+
+    public function testSqlObjectDump()
+    {
+        $oErr = new DatabaseError('Sql error', 123, null, array('one' => 'two'));
+        $this->assertRegExp('/two/', $oErr->getSqlDebugDump());
+    }
+
+    public function testChksqlThrows()
+    {
+        $this->expectException(DatabaseError::class);
+        $this->expectExceptionMessage('My custom error message');
+        $this->expectExceptionCode(500);
+
+        $oDB = new \DB_Error;
+        $this->assertEquals(false, chksql($oDB, 'My custom error message'));
+    }
+}
index 2891388d371b83976a8b50ae9dce379b916e47df..dbf8feca712a4ecf7d5d401baaa8f157760ef306 100644 (file)
@@ -2,6 +2,9 @@
 
 namespace Nominatim;
 
+require_once(CONST_BasePath.'/lib/lib.php');
+require_once(CONST_BasePath.'/lib/ClassTypes.php');
+
 class LibTest extends \PHPUnit\Framework\TestCase
 {
 
index 4f21706ecaf9274ce74740d750025152dc2c133a..eb4ad68aae25b2d84f9e844416c40efdff946878 100644 (file)
@@ -2,6 +2,7 @@
 
 namespace Nominatim;
 
+require_once(CONST_BasePath.'/lib/db.php');
 require_once(CONST_BasePath.'/lib/Status.php');
 
 
index fa1331e87b17d187fc0ebbd29b15e9c3b9567703..4016a839860294eb05bb2167fe002e243453a1b5 100644 (file)
@@ -2,8 +2,8 @@
 
 namespace Nominatim;
 
-require_once(CONST_BasePath.'/lib/db.php');
-require_once(CONST_BasePath.'/lib/cmd.php');
+// require_once(CONST_BasePath.'/lib/db.php');
+// require_once(CONST_BasePath.'/lib/cmd.php');
 require_once(CONST_BasePath.'/lib/TokenList.php');
 
 
index 0d4759622ff06f0ec8d6e4d09e2a673e4eb06a38..d6968717b5b46bb699b1eb1486d9d1c1327f3194 100644 (file)
@@ -1,2 +1,4 @@
 <?php
     @define('CONST_BasePath', '../..');
+    @define('CONST_Debug', true);
+    @define('CONST_NoAccessControl', false);
index c9e86312c9cc63a8c9da6954db45b214c9adff4e..81d643f052fd4c1438bca9711533957701c6ea09 100755 (executable)
@@ -11,6 +11,7 @@ ini_set('memory_limit', '200M');
 $oParams = new Nominatim\ParameterParser();
 
 $sOutputFormat = $oParams->getSet('format', array('html', 'json'), 'html');
+set_exception_handler_by_format($sOutputFormat);
 
 $aLangPrefOrder = $oParams->getPreferredLanguages();
 $sLanguagePrefArraySQL = 'ARRAY['.join(',', array_map('getDBQuoted', $aLangPrefOrder)).']';
index f09506a4c0de7ae1aef97afa22b38a4aecc9cae1..ec2a3a8a30f97880a4871d54d8539186b4a19832 100755 (executable)
@@ -12,6 +12,7 @@ $oParams = new Nominatim\ParameterParser();
 
 // Format for output
 $sOutputFormat = $oParams->getSet('format', array('xml', 'json', 'geojson'), 'xml');
+set_exception_handler_by_format($sOutputFormat);
 
 // Preferred language
 $aLangPrefOrder = $oParams->getPreferredLanguages();
index a7261802a40d57aff23145f58cbd594d42e58eaf..963f7500cf225ad0ceaece2e8f2604d9b59542c0 100755 (executable)
@@ -13,6 +13,7 @@ $oParams = new Nominatim\ParameterParser();
 
 // Format for output
 $sOutputFormat = $oParams->getSet('format', array('html', 'xml', 'json', 'jsonv2', 'geojson', 'geocodejson'), 'xml');
+set_exception_handler_by_format($sOutputFormat);
 
 // Preferred language
 $aLangPrefOrder = $oParams->getPreferredLanguages();
index 0ebf1814bcb129fafc63477b637fe5a5c52832cc..0b678caa65994c16145ac18c6141e6c9f11d4793 100755 (executable)
@@ -27,6 +27,7 @@ if (CONST_Search_ReversePlanForAll
 
 // Format for output
 $sOutputFormat = $oParams->getSet('format', array('html', 'xml', 'json', 'jsonv2', 'geojson', 'geocodejson'), 'html');
+set_exception_handler_by_format($sOutputFormat);
 
 $sForcedGeometry = ($sOutputFormat == 'html') ? 'geojson' : null;
 $oGeocode->loadParamArray($oParams, $sForcedGeometry);