Hi Dave,
Please find updated patch, I have tested following scenarios in query tool,
1) To test if we highlight faulty syntax
SQL: select a from pg_roles;
2) To check duplicates in error messages.
- Open query tool
- Uncheck Auto-Commit and run below sql 3 times and you will get an error
SQL: select a from pg_roles;
3) To check proper error
SQL: --insert into pg_roles values(1);
4) To check duplicates in error messages.
SQL: insert into pg_roles values(1);
5) Tested RAISE notices from function.
6) Tested JS testcases
Please review and let me know if I missed anything.
Regards,
Murtuza
On Mon, Sep 18, 2017 at 8:20 PM, Dave Page <[email protected]> wrote:
> Hi
>
> On Mon, Sep 18, 2017 at 3:08 PM, Murtuza Zabuawala <
> [email protected]> wrote:
>
>> Hi Dave,
>>
>> Sorry my bad, I didn't check the backend code, I assumed that it is
>> coming from psycopg2 and so I was focusing it to remove from client side :(
>>
>> PFA updated patch.
>>
>
> I think it needs to be a bit smarter than that. Whilst it works well for
> the "empty query" message, it doesn't work well for errors that have full
> details. For instance, instead of:
>
> ========================================================
> ERROR: relation "pg_foo" does not exist
> LINE 1: select * from pg_foo
> ^
> ********** Error **********
>
> ERROR: relation "pg_foo" does not exist
> SQL state: 42P01
> Character: 15
> ========================================================
>
> We now get:
>
> ========================================================
> ERROR: ERROR: relation "pg_foo" does not exist
> LINE 1: select * from pg_foo
> ^
>
>
> ERROR: relation "pg_foo" does not exist
> SQL state: 42P01
> Character: 15
> ========================================================
>
> Done
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
diff --git a/web/pgadmin/utils/driver/psycopg2/__init__.py
b/web/pgadmin/utils/driver/psycopg2/__init__.py
index 11952e1..e9d312b 100644
--- a/web/pgadmin/utils/driver/psycopg2/__init__.py
+++ b/web/pgadmin/utils/driver/psycopg2/__init__.py
@@ -1537,6 +1537,22 @@ Failed to reset the connection to the server due to
following error:
resp.append(self.__notices.pop(0))
return resp
+ def decode_to_utf8(self, value):
+ """
+ This method will decode values to utf-8
+ Args:
+ value: String to be decode
+
+ Returns:
+ Decoded string
+ """
+ if hasattr(str, 'decode'):
+ try:
+ value = value.decode('utf-8')
+ except:
+ pass
+ return value
+
def _formatted_exception_msg(self, exception_obj, formatted_msg):
"""
This method is used to parse the psycopg2.Error object and returns the
@@ -1548,7 +1564,6 @@ Failed to reset the connection to the server due to
following error:
formatted_msg: if True then function return the formatted
exception message
"""
-
if exception_obj.pgerror:
errmsg = exception_obj.pgerror
elif exception_obj.diag.message_detail:
@@ -1556,60 +1571,72 @@ Failed to reset the connection to the server due to
following error:
else:
errmsg = str(exception_obj)
# errmsg might contains encoded value, lets decode it
- if hasattr(str, 'decode'):
- errmsg = errmsg.decode('utf-8')
+ errmsg = self.decode_to_utf8(errmsg)
# if formatted_msg is false then return from the function
if not formatted_msg:
return errmsg
- errmsg += u'********** Error **********\n\n'
+ # Do not append if error starts with `ERROR:` as most pg related
+ # error starts with `ERROR:`
+ if not errmsg.startswith(u'ERROR:'):
+ errmsg = u'ERROR: ' + errmsg + u'\n\n'
if exception_obj.diag.severity is not None \
and exception_obj.diag.message_primary is not None:
- errmsg += u"{}: {}".format(
+ ex_diag_message = u"{0}: {1}".format(
exception_obj.diag.severity,
- exception_obj.diag.message_primary.decode('utf-8') if
- hasattr(str, 'decode') else exception_obj.diag.message_primary)
-
+ self.decode_to_utf8(exception_obj.diag.message_primary)
+ )
+ # If both errors are different then only append it
+ if errmsg and ex_diag_message and \
+ ex_diag_message.strip().strip('\n').lower() not in \
+ errmsg.strip().strip('\n').lower():
+ errmsg += ex_diag_message
elif exception_obj.diag.message_primary is not None:
- errmsg += exception_obj.diag.message_primary.decode('utf-8') if \
- hasattr(str, 'decode') else exception_obj.diag.message_primary
+ message_primary = self.decode_to_utf8(
+ exception_obj.diag.message_primary
+ )
+ if message_primary.lower() not in errmsg.lower():
+ errmsg += message_primary
if exception_obj.diag.sqlstate is not None:
- if not errmsg[:-1].endswith('\n'):
+ if not errmsg.endswith('\n'):
errmsg += '\n'
errmsg += gettext('SQL state: ')
- errmsg += exception_obj.diag.sqlstate.decode('utf-8') if \
- hasattr(str, 'decode') else exception_obj.diag.sqlstate
+ errmsg += self.decode_to_utf8(exception_obj.diag.sqlstate)
if exception_obj.diag.message_detail is not None:
- if not errmsg[:-1].endswith('\n'):
- errmsg += '\n'
- errmsg += gettext('Detail: ')
- errmsg += exception_obj.diag.message_detail.decode('utf-8') if \
- hasattr(str, 'decode') else exception_obj.diag.message_detail
+ if 'Detail:'.lower() not in errmsg.lower():
+ if not errmsg.endswith('\n'):
+ errmsg += '\n'
+ errmsg += gettext('Detail: ')
+ errmsg += self.decode_to_utf8(
+ exception_obj.diag.message_detail
+ )
if exception_obj.diag.message_hint is not None:
- if not errmsg[:-1].endswith('\n'):
- errmsg += '\n'
- errmsg += gettext('Hint: ')
- errmsg += exception_obj.diag.message_hint.decode('utf-8') if \
- hasattr(str, 'decode') else exception_obj.diag.message_hint
+ if 'Hint:'.lower() not in errmsg.lower():
+ if not errmsg.endswith('\n'):
+ errmsg += '\n'
+ errmsg += gettext('Hint: ')
+ errmsg += self.decode_to_utf8(exception_obj.diag.message_hint)
if exception_obj.diag.statement_position is not None:
- if not errmsg[:-1].endswith('\n'):
- errmsg += '\n'
- errmsg += gettext('Character: ')
- errmsg += exception_obj.diag.statement_position.decode('utf-8') if
\
- hasattr(str, 'decode') else
exception_obj.diag.statement_position
+ if 'Character:'.lower() not in errmsg.lower():
+ if not errmsg.endswith('\n'):
+ errmsg += '\n'
+ errmsg += gettext('Character: ')
+ errmsg += self.decode_to_utf8(
+ exception_obj.diag.statement_position
+ )
if exception_obj.diag.context is not None:
- if not errmsg[:-1].endswith('\n'):
- errmsg += '\n'
- errmsg += gettext('Context: ')
- errmsg += exception_obj.diag.context.decode('utf-8') if \
- hasattr(str, 'decode') else exception_obj.diag.context
+ if 'Context:'.lower() not in errmsg.lower():
+ if not errmsg.endswith('\n'):
+ errmsg += '\n'
+ errmsg += gettext('Context: ')
+ errmsg += self.decode_to_utf8(exception_obj.diag.context)
return errmsg
diff --git a/web/regression/javascript/history/query_history_spec.jsx
b/web/regression/javascript/history/query_history_spec.jsx
index 0a96244..494eed9 100644
--- a/web/regression/javascript/history/query_history_spec.jsx
+++ b/web/regression/javascript/history/query_history_spec.jsx
@@ -383,6 +383,45 @@ describe('QueryHistory', () => {
expect(queryDetail.at(0).text()).toContain('third sql statement');
});
});
+
+ describe('when a fourth SQL query is executed', () => {
+ beforeEach(() => {
+ historyCollection.add({
+ query: 'fourth sql statement',
+ start_time: new Date(2017, 12, 12, 1, 33, 5, 99),
+ status: false,
+ row_affected: 0,
+ total_time: '26 msec',
+ message: 'ERROR: unexpected error from fourth sql message',
+ });
+
+ queryEntries = historyWrapper.find(QueryHistoryEntry);
+ });
+
+ it('displays fourth query SQL in the right pane', () => {
+ expect(queryDetail.at(0).text()).toContain('Error Message unexpected
error from fourth sql message');
+ });
+ });
+
+ describe('when a fifth SQL query is executed', () => {
+ beforeEach(() => {
+ historyCollection.add({
+ query: 'fifth sql statement',
+ start_time: new Date(2017, 12, 12, 1, 33, 5, 99),
+ status: false,
+ row_affected: 0,
+ total_time: '26 msec',
+ message: 'unknown error',
+ });
+
+ queryEntries = historyWrapper.find(QueryHistoryEntry);
+ });
+
+ it('displays fourth query SQL in the right pane', () => {
+ expect(queryDetail.at(0).text()).toContain('Error Message unknown
error');
+ });
+ });
+
});
describe('when several days of queries were executed', () => {
