subject:"\[pgAdmin4\]\[Patch\]\: Fix the logic to extract the error in Query tool"

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

2017-09-19 Thread Murtuza Zabuawala

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  wrote:

> Hi
>
> On Mon, Sep 18, 2017 at 3:08 PM, Murtuza Zabuawala <
> murtuza.zabuaw...@enterprisedb.com> 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

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

2017-09-18 Thread Dave Page

Hi

On Mon, Sep 18, 2017 at 3:08 PM, Murtuza Zabuawala <
murtuza.zabuaw...@enterprisedb.com> 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



-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

2017-09-18 Thread Murtuza Zabuawala

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.

--
Regards,
Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


On Mon, Sep 18, 2017 at 7:19 PM, Dave Page  wrote:

> Hi
>
> On Mon, Sep 18, 2017 at 2:20 PM, Murtuza Zabuawala  enterprisedb.com> wrote:
>
>> Hi Dave,
>>
>> Please disregard my previous patch and instead attaching updated patch.
>>
>> In my previous patch I used `let` keyword instead of `var` for defining
>> variable, for consistency & backward compatibility I have used `var` in my
>> latest patch.
>>
>
> That seems like an odd way to fix this - we put in * Error * in
> the backend, then remove it in the front end.
>
> I think it would be better to ensure it's formatted sensibly at the backed
> to begin with.
>
> --
> 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..f22a697 100644
--- a/web/pgadmin/utils/driver/psycopg2/__init__.py
+++ b/web/pgadmin/utils/driver/psycopg2/__init__.py
@@ -1563,7 +1563,7 @@ Failed to reset the connection to the server due to 
following error:
 if not formatted_msg:
 return errmsg
 
-errmsg += u'** Error **\n\n'
+errmsg = u'ERROR: ' + errmsg + u'\n\n'
 
 if exception_obj.diag.severity is not None \
 and exception_obj.diag.message_primary is not None:
diff --git a/web/regression/javascript/history/query_history_spec.jsx 
b/web/regression/javascript/history/query_history_spec.jsx
index 0a96244..92e5174 100644
--- a/web/regression/javascript/history/query_history_spec.jsx
+++ b/web/regression/javascript/history/query_history_spec.jsx
@@ -383,6 +383,25 @@ 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 several days of queries were executed', () => {

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

2017-09-18 Thread Murtuza Zabuawala

Hi Dave,

Please disregard my previous patch and instead attaching updated patch.

In my previous patch I used `let` keyword instead of `var` for defining
variable, for consistency & backward compatibility I have used `var` in my
latest patch.

--
Regards,
Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[image: https://community.postgresrocks.net/]


On Mon, Sep 18, 2017 at 6:37 PM, Murtuza Zabuawala <
murtuza.zabuaw...@enterprisedb.com> wrote:

> Hi Dave,
>
> PFA patch.
>
> On Mon, Sep 18, 2017 at 4:34 PM, Dave Page  wrote:
>
>> Hi
>>
>> On Mon, Sep 18, 2017 at 10:54 AM, Murtuza Zabuawala <
>> murtuza.zabuaw...@enterprisedb.com> wrote:
>>
>>> Hi,
>>>
>>> PFA minor patch to fix the issue where logic to extract the error using
>>> RegEX from error message was incorrect in Query tool(History tab).
>>> RM#2700
>>>
>>
>> Thanks - applied, but
>>
>> - Could you please add some JS tests to ensure parseErrorMessage
>> continues to work as it should.
>>
> Done
>
>
>>
>> - I'm not happy with the fact that we still display:
>>
>> can't execute an empty query ** Error **
>>
>> Can we not make that look more like a real error message? At the very
>> least something like:
>>
>> Error: can't execute an empty query
>>
> Done
>
>
>>
>>
>>>
>>> Another minor issue which I observed on login page is that close button
>>> on alert is little misaligned(screenshot attached).
>>>
>>>
>> Also applied - thanks!
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
diff --git a/web/pgadmin/static/jsx/history/detail/history_error_message.jsx 
b/web/pgadmin/static/jsx/history/detail/history_error_message.jsx
index af77a90..352f229 100644
--- a/web/pgadmin/static/jsx/history/detail/history_error_message.jsx
+++ b/web/pgadmin/static/jsx/history/detail/history_error_message.jsx
@@ -14,6 +14,16 @@ import Shapes from '../../react_shapes';
 export default class HistoryErrorMessage extends React.Component {
 
   parseErrorMessage(message) {
+/*
+ * Below regex pattern will match `** Error **` string in 
message
+ * and if we found it then we will remove it to make the error clear.
+ */
+var psycopg_error_pattern = /[\*]{10}[\w|\s]+[\*]{10}/i;
+if(psycopg_error_pattern.test(message)) {
+  message = message.replace(psycopg_error_pattern, '');
+}
+
+// Extract relevant error from message if pattern is found else return 
message
 return message.match(/ERROR:\s*([^\n\r]*)/i) ?
message.match(/ERROR:\s*([^\n\r]*)/i)[1] :
message;
diff --git a/web/regression/javascript/history/query_history_spec.jsx 
b/web/regression/javascript/history/query_history_spec.jsx
index 0a96244..118b9aa 100644
--- a/web/regression/javascript/history/query_history_spec.jsx
+++ b/web/regression/javascript/history/query_history_spec.jsx
@@ -383,6 +383,44 @@ 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: '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('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, 34, 5, 99),
+status: false,
+row_affected: 0,
+total_time: '26 msec',
+message: 'testing unknown exception** Error ** 
using regex',
+  });
+
+  queryEntries = historyWrapper.find(QueryHistoryEntry);
+});
+
+it('displays fifth query SQL in the right pane', () => {
+  expect(queryDetail.at(0).text()).toContain('Error Message testing 
unknown exception using regex');
+});
+  });
 });
 
 describe('when several days of queries were executed', () => {

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

2017-09-18 Thread Murtuza Zabuawala

Hi Dave,

PFA patch.

On Mon, Sep 18, 2017 at 4:34 PM, Dave Page  wrote:

> Hi
>
> On Mon, Sep 18, 2017 at 10:54 AM, Murtuza Zabuawala  enterprisedb.com> wrote:
>
>> Hi,
>>
>> PFA minor patch to fix the issue where logic to extract the error using
>> RegEX from error message was incorrect in Query tool(History tab).
>> RM#2700
>>
>
> Thanks - applied, but
>
> - Could you please add some JS tests to ensure parseErrorMessage continues
> to work as it should.
>
Done


>
> - I'm not happy with the fact that we still display:
>
> can't execute an empty query ** Error **
>
> Can we not make that look more like a real error message? At the very
> least something like:
>
> Error: can't execute an empty query
>
Done


>
>
>>
>> Another minor issue which I observed on login page is that close button
>> on alert is little misaligned(screenshot attached).
>>
>>
> Also applied - thanks!
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
diff --git a/web/pgadmin/static/jsx/history/detail/history_error_message.jsx 
b/web/pgadmin/static/jsx/history/detail/history_error_message.jsx
index af77a90..63f5e14 100644
--- a/web/pgadmin/static/jsx/history/detail/history_error_message.jsx
+++ b/web/pgadmin/static/jsx/history/detail/history_error_message.jsx
@@ -14,6 +14,16 @@ import Shapes from '../../react_shapes';
 export default class HistoryErrorMessage extends React.Component {
 
   parseErrorMessage(message) {
+/*
+ * Below regex pattern will match `** Error **` string in 
message
+ * and if we found it then we will remove it to make the error clear.
+ */
+let psycopg_error_pattern = /[\*]{10}[\w|\s]+[\*]{10}/i;
+if(psycopg_error_pattern.test(message)) {
+  message = message.replace(psycopg_error_pattern, '');
+}
+
+// Extract relevant error from message if pattern is found else return 
message
 return message.match(/ERROR:\s*([^\n\r]*)/i) ?
message.match(/ERROR:\s*([^\n\r]*)/i)[1] :
message;
diff --git a/web/regression/javascript/history/query_history_spec.jsx 
b/web/regression/javascript/history/query_history_spec.jsx
index 0a96244..118b9aa 100644
--- a/web/regression/javascript/history/query_history_spec.jsx
+++ b/web/regression/javascript/history/query_history_spec.jsx
@@ -383,6 +383,44 @@ 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: '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('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, 34, 5, 99),
+status: false,
+row_affected: 0,
+total_time: '26 msec',
+message: 'testing unknown exception** Error ** 
using regex',
+  });
+
+  queryEntries = historyWrapper.find(QueryHistoryEntry);
+});
+
+it('displays fifth query SQL in the right pane', () => {
+  expect(queryDetail.at(0).text()).toContain('Error Message testing 
unknown exception using regex');
+});
+  });
 });
 
 describe('when several days of queries were executed', () => {

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

2017-09-18 Thread Dave Page

Hi

On Mon, Sep 18, 2017 at 10:54 AM, Murtuza Zabuawala <
murtuza.zabuaw...@enterprisedb.com> wrote:

> Hi,
>
> PFA minor patch to fix the issue where logic to extract the error using
> RegEX from error message was incorrect in Query tool(History tab).
> RM#2700
>

Thanks - applied, but

- Could you please add some JS tests to ensure parseErrorMessage continues
to work as it should.

- I'm not happy with the fact that we still display:

can't execute an empty query ** Error **

Can we not make that look more like a real error message? At the very least
something like:

Error: can't execute an empty query


>
> Another minor issue which I observed on login page is that close button on
> alert is little misaligned(screenshot attached).
>
>
Also applied - thanks!

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

2017-09-18 Thread Murtuza Zabuawala

Hi,

PFA minor patch to fix the issue where logic to extract the error using
RegEX from error message was incorrect in Query tool(History tab).
RM#2700

Another minor issue which I observed on login page is that close button on
alert is little misaligned(screenshot attached).

--
Regards,
Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[image: https://community.postgresrocks.net/]

diff --git a/web/pgadmin/static/jsx/history/detail/history_error_message.jsx 
b/web/pgadmin/static/jsx/history/detail/history_error_message.jsx
index 0b5e27b..af77a90 100644
--- a/web/pgadmin/static/jsx/history/detail/history_error_message.jsx
+++ b/web/pgadmin/static/jsx/history/detail/history_error_message.jsx
@@ -14,7 +14,9 @@ import Shapes from '../../react_shapes';
 export default class HistoryErrorMessage extends React.Component {
 
   parseErrorMessage(message) {
-return message.match(/ERROR:\s*([^\n\r]*)/i)[1];
+return message.match(/ERROR:\s*([^\n\r]*)/i) ?
+   message.match(/ERROR:\s*([^\n\r]*)/i)[1] :
+   message;
   }
 
   render() {
@@ -27,4 +29,4 @@ export default class HistoryErrorMessage extends 
React.Component {
 
 HistoryErrorMessage.propTypes = {
   historyEntry: Shapes.historyDetail,
-};
\ No newline at end of file
+};
diff --git a/web/pgadmin/static/css/bootstrap.overrides.css 
b/web/pgadmin/static/css/bootstrap.overrides.css
index 82c2f4e..73b4fc7 100755
--- a/web/pgadmin/static/css/bootstrap.overrides.css
+++ b/web/pgadmin/static/css/bootstrap.overrides.css
@@ -1341,3 +1341,8 @@ body {
 .override_label_class_font_size {
   font-size: inherit !important;
 }
+
+/* To align 'X' in alert on login page */
+.alert-dismissable, .alert-dismissible {
+  padding-right: 35px !important;
+}

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

Re: [pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

[pgAdmin4][Patch]: Fix the logic to extract the error in Query tool

7 matches

Site Navigation

Mail list logo

Footer information