Fix incorrect return value of error cases (#4803)

* Fix incorrect return value of error cases * Fix return type * Fix method does not return false on error * format code Co-authored-by: Yurun <[email protected]>
swoole · Aug 16, 2022 · d8f0ea3 · d8f0ea3
1 parent 4c65ec1
commit d8f0ea3
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 7 deletions.
diff --git a/ext-src/stubs/php_swoole_postgresql_coro.stub.php b/ext-src/stubs/php_swoole_postgresql_coro.stub.php
@@ -15,9 +15,9 @@ public function metaData(string $table_name): false|array {}
     class PostgreSQLStatement {
         public function execute(array $params = []): bool {}
         public function fetchAll(int $result_type = SW_PGSQL_ASSOC): false|array {}
-        public function affectedRows(): int {}
-        public function numRows(): int {}
-        public function fieldCount(): int {}
+        public function affectedRows(): false|int {}
+        public function numRows(): false|int {}
+        public function fieldCount(): false|int {}
         public function fetchObject(?int $row = 0, ?string $class_name = null, array $ctor_params = []): false|object {}
         public function fetchAssoc(?int $row = 0, int $result_type = SW_PGSQL_ASSOC): false|array {}
         public function fetchArray(?int $row = 0, int $result_type = SW_PGSQL_BOTH): false|array {}

diff --git a/ext-src/stubs/php_swoole_postgresql_coro_arginfo.h b/ext-src/stubs/php_swoole_postgresql_coro_arginfo.h
@@ -1,5 +1,5 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: ee056d9d6e3dce3bf2198fbc7fb6bdd1aaab5206 */
+ * Stub hash: 6564202509f445563bab1c57d5b7f635368fb1ad */
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_class_Swoole_Coroutine_PostgreSQL___construct, 0, 0, 0)
 ZEND_END_ARG_INFO()
@@ -37,7 +37,7 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_class_Swoole_Coroutine_PostgreSQ
 	ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, result_type, IS_LONG, 0, "SW_PGSQL_ASSOC")
 ZEND_END_ARG_INFO()
 
-ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Swoole_Coroutine_PostgreSQLStatement_affectedRows, 0, 0, IS_LONG, 0)
+ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_class_Swoole_Coroutine_PostgreSQLStatement_affectedRows, 0, 0, MAY_BE_FALSE|MAY_BE_LONG)
 ZEND_END_ARG_INFO()
 
 #define arginfo_class_Swoole_Coroutine_PostgreSQLStatement_numRows arginfo_class_Swoole_Coroutine_PostgreSQLStatement_affectedRows

diff --git a/ext-src/swoole_postgresql_coro.cc b/ext-src/swoole_postgresql_coro.cc
@@ -52,6 +52,7 @@ class Object {
         bool ignore_notices;
         bool log_notices;
         size_t stmt_counter;
+        bool request_success;
 
         bool yield(zval *_return_value, EventType event, double timeout);
         bool wait_write_ready();
@@ -538,10 +539,13 @@ static void connect_callback(PGObject *object, Reactor *reactor, Event *event) {
     }
 
     if (object->connected == 1) {
+        object->request_success = true;
         zend_update_property_null(swoole_postgresql_coro_ce, SW_Z8_OBJ_P(object->object), ZEND_STRL("error"));
         if (object->statement) {
             zend_update_property_null(swoole_postgresql_coro_statement_ce, SW_Z8_OBJ_P(object->object), ZEND_STRL("error"));
         }
+    } else {
+        object->request_success = false;
     }
     object->co->resume();
 }
@@ -715,6 +719,8 @@ static int query_result_parse(PGObject *object) {
             swoole_postgresql_coro_statement_ce, SW_Z8_OBJ_P(object->statement->object), ZEND_STRL("resultStatus"), status);
     }
 
+    object->request_success = (status == PGRES_COMMAND_OK || status == PGRES_TUPLES_OK);
+
     switch (status) {
     case PGRES_EMPTY_QUERY:
     case PGRES_BAD_RESPONSE:
@@ -771,6 +777,8 @@ static int prepare_result_parse(PGObject *object) {
         zend_update_property_long(swoole_postgresql_coro_statement_ce, SW_Z8_OBJ_P(object->statement->object), ZEND_STRL("resultStatus"), status);
     }
 
+    object->request_success = (status == PGRES_COMMAND_OK || status == PGRES_TUPLES_OK);
+
     switch (status) {
     case PGRES_EMPTY_QUERY:
     case PGRES_BAD_RESPONSE:
@@ -894,6 +902,9 @@ bool PGObject::yield(zval *_return_value, EventType event, double timeout) {
             }
         }
 
+        return false;
+    } else if (!request_success) {
+        ZVAL_FALSE(_return_value);
         return false;
     }
 

diff --git a/tests/init b/tests/init
@@ -64,13 +64,13 @@ Swoole\Coroutine\run(function () {
     $pgsql = new Swoole\Coroutine\PostgreSQL();
     $connected = $pgsql->connect(PGSQL_CONNECTION_STRING);
     if (!$connected) {
-        echo "[DB-init] Connect failed! Error#{$pgsql->error}: {$pgsql->notices}\n";
+        echo sprintf("[DB-init] Connect failed! Error#%s: %s", $pgsql->error, $pgsql->notices['sqlstate'] ?? ''), PHP_EOL;
         exit(1);
     }
     $sql_file = read_sql_file(__DIR__ . '/pgsql.sql');
     foreach ($sql_file as $line) {
         if (!$pgsql->query($line)) {
-            echo "[DB-init] Failed! Error#{$pgsql->error}: {$pgsql->notices}\n";
+            echo sprintf("[DB-init] Failed! Error#%s: %s", $pgsql->error, $pgsql->notices['sqlstate'] ?? ''), PHP_EOL;
             exit(1);
         }
     }

diff --git a/tests/swoole_pgsql_coro/error.phpt b/tests/swoole_pgsql_coro/error.phpt
@@ -0,0 +1,44 @@
+--TEST--
+swoole_pgsql_coro: error
+--SKIPIF--
+<?php require __DIR__ . '/../include/skipif.inc'; ?>
+--FILE--
+<?php
+require __DIR__ . '/../include/bootstrap.php';
+Swoole\Coroutine\run(function () {
+    $pgsql = new Swoole\Coroutine\PostgreSQL();
+    $connected = $pgsql->connect(PGSQL_CONNECTION_STRING);
+    Assert::true($connected, (string) $pgsql->error);
+
+    $stmt = $pgsql->query('SELECT * FROM not_exists;');
+    Assert::false($stmt, (string) $pgsql->error);
+
+    $stmt = $pgsql->prepare('SELECT * FROM not_exists;');
+    Assert::false($stmt, (string) $pgsql->error);
+
+    $stmt = $pgsql->prepare("INSERT INTO weather(city, temp_lo, temp_hi, prcp, date) VALUES ($1, $2, $3, $4, $5)  RETURNING id");
+    Assert::true(false !== $stmt, (string) $pgsql->error);
+
+    $result = $stmt->affectedRows();
+    Assert::false($result, (string) $stmt->error);
+
+    $result = $stmt->numRows();
+    Assert::false($result, (string) $stmt->error);
+
+    $result = $stmt->fieldCount();
+    Assert::false($result, (string) $stmt->error);
+
+    $result = $stmt->fetchObject();
+    Assert::false($result, (string) $stmt->error);
+
+    $result = $stmt->fetchAssoc();
+    Assert::false($result, (string) $stmt->error);
+
+    $result = $stmt->fetchArray();
+    Assert::false($result, (string) $stmt->error);
+
+    $result = $stmt->fetchRow();
+    Assert::false($result, (string) $stmt->error);
+});
+?>
+--EXPECT--