Re: [pgAdmin4][patch]: RM #3090 pgadmin shows misleading "Query returned successfully" with incorrect SQL

From: Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
To: Joao De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>
Cc: Dave Page <dpage(at)pgadmin(dot)org>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: [pgAdmin4][patch]: RM #3090 pgadmin shows misleading "Query returned successfully" with incorrect SQL
Date: 2018-03-28 05:35:17
Message-ID: CANxoLDfctsa8gLahMW7azew-JgkVbzEkyZj=jBkPAyMH92=yvg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Hi Joao

On Tue, Mar 27, 2018 at 6:53 PM, Joao De Almeida Pereira <jdealmeidapereira
@pivotal.io> wrote:

> Hi Akshay,
>
> We were not trying to imply that your fix did not solve the problem, we
> were trying to understand the root cause of the problem.
>

As per my understanding root cause of the problem is "exception_obj.
diag.severity" variable was not decoded and used directly, when I debug the
code that variable contains the encoded string, so I have just use the
function "*self.decode_to_utf8()*" and it works. If you can see the code
below to this fix will have all the variables decoded to utf8.

>
> 1. We were not able to reproduce the problem
> We followed your directions in RM, removed your fix but we could not
> reproduce the problem. So we could not make sure that the application is in
> fact working. This maybe have been because we missed something.
> 2. The fix does not tackle the big problem
> From what we read on the RM the big problem is "when we throw an exception
> the front end is displaying the query successful message". Did you also
> understood that from the RM?
>
We believe this is the real problem behind the RM.
> Were you able replicate this behavior? If so we need to address the root
> of this.
>

Yes I understood that from RM, but as I mentioned in my previous email
that I haven't encounter that problem while trying to reproduce this. I got
"*Not connected to the server*..." error not the "Query returned
successfully". This bug has been log 2 months ago, I am not sure but it may
possible that we have fixed/change some logic because of that I have seen
"Not connected to the server......" error instead of "Query returned
successfully".

>
>
> As an aside sniping bugs is fine in some situations but as a general rule
> of thumb is a bad approach and creates a blob of code that no one can
> manage. A situation like this look like a very promising candidate for
> extraction/refactoring in our point of view. In which situation do you
> normally refactor or extract code?
>
> Thanks
> Joao
>
>
> On Tue, Mar 27, 2018, 1:31 AM Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
> wrote:
>
>> Hi Joao
>>
>> On Tue, Mar 27, 2018 at 12:01 AM, Joao De Almeida Pereira <
>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>
>>> Hello,
>>> We tried to reproduce the issue but we were not capable to reproduce it.
>>> What it is strange on the fix is that python is complaining about a
>>> different line then the one that was fixed. Maybe this is just a Python
>>> thing....
>>>
>>
>> I have mentioned the steps in RM and I have tried it on Ubuntu 16.
>> Python is complaining the exact line where I have fixed the logic.
>>
>> ex_diag_message = u"{0}: {1}".format(
>> self.decode_to_utf8(exception_obj.diag.severity), # exception_obj.diag.severity is not decoded before my fix.
>> self.decode_to_utf8(exception_obj.diag.message_primary)
>> )
>>
>>
>>
>>>
>>> I assume that the fix works, but I would love to see some tests to
>>> ensure it is working. Another issue that looks more problematic is the fact
>>> that, as per the Redmine issue, when an exception is thrown it sends back a
>>> Successful Query message. If this is the case then this fix doesn't look
>>> like it is enough to solve the problem.
>>>
>>
>> Yes it works. With the latest code I didn't see Successful Query
>> message, it is showing "Not connected to the server .....", but the stack
>> trace is same that was mentioned in the RM.
>>
>>>
>>> Thanks
>>> Victoria & Joao
>>>
>>> On Mon, Mar 26, 2018 at 9:00 AM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>
>>>> Thanks, applied.
>>>>
>>>> On Mon, Mar 26, 2018 at 11:43 AM, Akshay Joshi <
>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Hackers,
>>>>>
>>>>> Please find the attached patch to fix RM #3090 pgadmin shows
>>>>> misleading "Query returned successfully" with incorrect SQL.
>>>>>
>>>>> --
>>>>> *Akshay Joshi*
>>>>>
>>>>> *Sr. Software Architect *
>>>>>
>>>>>
>>>>>
>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Dave Page
>>>> Blog: http://pgsnake.blogspot.com
>>>> Twitter: @pgsnake
>>>>
>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>>
>>>
>>
>>
>> --
>> *Akshay Joshi*
>>
>> *Sr. Software Architect *
>>
>>
>>
>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 976-788-8246
>> <+91%2097678%2088246>*
>>
>

--
*Akshay Joshi*

*Sr. Software Architect *

*Phone: +91 20-3058-9517Mobile: +91 976-788-8246*

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Murtuza Zabuawala 2018-03-28 07:19:28 Re: [pgAdmin4][RM#3055] Allow user to sort the data in View data mode
Previous Message Wilhelm Wurzer 2018-03-28 04:17:40 Re: pgadmin healthcheck-url