Re: [patch] Raise InternalServerError while retrieving table DDL

On Fri, Mar 24, 2017 at 7:19 PM, Atira Odhner <aodhner(at)pivotal(dot)io> wrote:

> When we were working on the DDL story, we found that some methods were
> returning the internal_server_error json, but the code that called those
> methods was not expecting that type of object. So, instead of returning
> that json to the user, the code would try to treat the json as a different
> type of object which often resulted in a different internal server error
> than the one that was intended.
>
> We decided to raise the error instead so that there would be no risk of
> the code interacting with the error object in an unexpected way, since it
> will raise up and skip over that code. Then, we added a handler for the
> error which returns the same type of internal_server_error which was
> intended originally. This ensures that the user sees the proper error
> messages.
>
I understand your concern about the Exception object.
But - what was the need to change the existing code, where it was working.

i.e.
*@@ -783,7 +785,7 @@ class TableView(PGChildNodeView, DataTypeReader,
VacuumSettings):*
* 25 status, result = self.conn.execute_dict(sql)*
* 26 •*
* 27 if not status:*
* 28 - return internal_server_error(errormsg=result)*
* 29 + raise InternalServerError(result)*
* 30 •*
* 31 for fk in result['rows']:*
* 32 •*

In above code (and, similar), why do we need that change at all?

-- Thanks, Ashesh

>
> Tira & Joao
>
> On Fri, Mar 24, 2017 at 9:15 AM, Ashesh Vashi <
> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> On Fri, Mar 24, 2017 at 5:07 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Ashesh, can you review/commit this please?
>>>
>>> On Thu, Mar 23, 2017 at 3:49 PM, Joao Pedro De Almeida Pereira
>>> <jdealmeidapereira(at)pivotal(dot)io> wrote:
>>> > Hi Hackers,
>>> >
>>> > While doing the DDL patch we found out that the code was not working
>>> > properly when errors happened during SQL execution:
>>> >
>>> > One example of this can be found in the file:
>>> >
>>> > web/pgadmin/browser/server_groups/servers/databases/schemas/
>>> tables/__init__.py
>>> >
>>> > The function '_formatter' that returns internal_server_error when an
>>> error
>>> > occur executing SQL
>>> > Nevertheless the 'sql' function uses the output of '_formatter' during
>>> the
>>> > execution without checking it.
>>> >
>>> > To solve this issue we raise an InternalServerError exception that we
>>> catch
>>> > in the 'sql' function instead of returning an error message.
>>>
>> Hi Joa & Sarah,
>>
>> I am not against putting the try..except block.
>>
>> We're already have out own version of 'internal_server_error', which will
>> return value in JSON format.
>> But - I did not understand the reason to change them with
>> 'werkzeug.exceptions.InternalServerError'.
>>
>> Can you please elaborate about that change?
>>
>>
>> -- Thanks, Ashesh
>>
>> >
>>> >
>>> > Thanks
>>> > Joao & Sarah
>>> >
>>> >
>>> > --
>>> > Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>>> > To make changes to your subscription:
>>> > http://www.postgresql.org/mailpref/pgadmin-hackers
>>> >
>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>