From 336089f8a7fd5d24c83deeaad3a49051d2204467 Mon Sep 17 00:00:00 2001
From: Kjell Winblad <kjellwinblad@gmail.com>
Date: Wed, 5 Jun 2024 15:53:02 +0200
Subject: [PATCH] fix: bug found by dialyzer and make test case cleaner

---
 .../test/emqx_bridge_pgsql_SUITE.erl          | 44 +++++++++++++++----
 apps/emqx_postgresql/src/emqx_postgresql.erl  | 24 +---------
 2 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/apps/emqx_bridge_pgsql/test/emqx_bridge_pgsql_SUITE.erl b/apps/emqx_bridge_pgsql/test/emqx_bridge_pgsql_SUITE.erl
index fb9341ddb..c6eb99f83 100644
--- a/apps/emqx_bridge_pgsql/test/emqx_bridge_pgsql_SUITE.erl
+++ b/apps/emqx_bridge_pgsql/test/emqx_bridge_pgsql_SUITE.erl
@@ -715,17 +715,39 @@ t_missing_table(Config) ->
     connect_and_create_table(Config),
     ok.
 
+%% We test that we can handle when the prepared statement with the channel
+%% name already exists in the connection instance when we try to make a new
+%% prepared statement. It is unknown in which scenario this can happen but it
+%% has been observed in a production log file.
+%% See:
+%% https://emqx.atlassian.net/browse/EEC-1036
 t_prepared_statement_exists(Config) ->
     Name = ?config(pgsql_name, Config),
     BridgeType = ?config(pgsql_bridge_type, Config),
+    emqx_common_test_helpers:on_exit(fun() ->
+        meck:unload()
+    end),
+    MeckOpts = [passthrough, no_link, no_history, non_strict],
+    meck:new(emqx_postgresql, MeckOpts),
+    InsertPrepStatementDupAndThenRemoveMeck =
+        fun(Conn, Key, SQL, List) ->
+            meck:passthrough([Conn, Key, SQL, List]),
+            meck:delete(
+                epgsql,
+                parse2,
+                4
+            ),
+            meck:passthrough([Conn, Key, SQL, List])
+        end,
+    meck:expect(
+        epgsql,
+        parse2,
+        InsertPrepStatementDupAndThenRemoveMeck
+    ),
     %% We should recover if the prepared statement name already exists in the
     %% driver
     ?check_trace(
         begin
-            ?inject_crash(
-                #{?snk_kind := pgsql_fake_prepare_statement_exists},
-                snabbkaffe_nemesis:recover_after(1)
-            ),
             ?assertMatch({ok, _}, create_bridge(Config)),
             ?retry(
                 _Sleep = 1_000,
@@ -742,13 +764,19 @@ t_prepared_statement_exists(Config) ->
             ok
         end
     ),
+    InsertPrepStatementDup =
+        fun(Conn, Key, SQL, List) ->
+            meck:passthrough([Conn, Key, SQL, List]),
+            meck:passthrough([Conn, Key, SQL, List])
+        end,
+    meck:expect(
+        epgsql,
+        parse2,
+        InsertPrepStatementDup
+    ),
     %% We should get status disconnected if removing already existing statment don't help
     ?check_trace(
         begin
-            ?inject_crash(
-                #{?snk_kind := pgsql_fake_prepare_statement_exists},
-                snabbkaffe_nemesis:recover_after(30)
-            ),
             ?assertMatch({ok, _}, create_bridge(Config)),
             ?retry(
                 _Sleep = 1_000,
diff --git a/apps/emqx_postgresql/src/emqx_postgresql.erl b/apps/emqx_postgresql/src/emqx_postgresql.erl
index 9553b3ceb..ea83b951e 100644
--- a/apps/emqx_postgresql/src/emqx_postgresql.erl
+++ b/apps/emqx_postgresql/src/emqx_postgresql.erl
@@ -661,7 +661,6 @@ prepare_sql_to_conn(
 ) when is_pid(Conn) ->
     LogMeta = #{msg => "postgresql_prepare_statement", name => Key, sql => SQL},
     ?SLOG(info, LogMeta),
-    test_maybe_inject_prepared_statement_already_exists(Conn, Key, SQL),
     case epgsql:parse2(Conn, Key, SQL, []) of
         {ok, Statement} ->
             prepare_sql_to_conn(Conn, Rest, Statements#{Key => Statement}, 0);
@@ -694,8 +693,8 @@ prepare_sql_to_conn(
             case epgsql:close(Conn, statement, Key) of
                 ok ->
                     ?SLOG(info, #{msg => "pqsql_closed_statement_successfully"});
-                {error, Error} ->
-                    ?SLOG(warning, #{msg => "pqsql_close_statement_failed", cause => Error})
+                {error, CloseError} ->
+                    ?SLOG(warning, #{msg => "pqsql_close_statement_failed", cause => CloseError})
             end,
             prepare_sql_to_conn(Conn, ToPrepare, Statements, Attempts + 1);
         {error, Error} ->
@@ -709,25 +708,6 @@ prepare_sql_to_conn(
             {error, export_error(TranslatedError)}
     end.
 
--ifdef(TEST).
-test_maybe_inject_prepared_statement_already_exists(Conn, Key, SQL) ->
-    try
-        %% In test we inject a crash in the following trace point to test the
-        %% scenario when the prepared statement already exists. It is unknkown
-        %% in which scenario this can happen but it has been observed in a
-        %% production log file. See:
-        %% https://emqx.atlassian.net/browse/EEC-1036
-        ?tp(pgsql_fake_prepare_statement_exists, #{})
-    catch
-        _:_ ->
-            epgsql:parse2(Conn, Key, SQL, [])
-    end,
-    ok.
--else.
-test_maybe_inject_prepared_statement_already_exists(_Conn, _Key, _SQL) ->
-    ok.
--endif.
-
 to_bin(Bin) when is_binary(Bin) ->
     Bin;
 to_bin(Atom) when is_atom(Atom) ->