Thread: [pgadmin-hackers][patch] Raise InternalServerError while retrievingtable DDL
[pgadmin-hackers][patch] Raise InternalServerError while retrievingtable DDL
From
Joao Pedro De Almeida Pereira
Date:
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.
Thanks
Joao & Sarah
Attachment
Ashesh, can you review/commit this please? On Thu, Mar 23, 2017 at 3:49 PM, Joao Pedro De Almeida Pereira <jdealmeidapereira@pivotal.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. > > > Thanks > Joao & Sarah > > > -- > Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.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
On Fri, Mar 24, 2017 at 5:07 PM, Dave Page <dpage@pgadmin.org> wrote:
We're already have out own version of 'internal_server_error', which will return value in JSON format.
Ashesh, can you review/commit this please?
On Thu, Mar 23, 2017 at 3:49 PM, Joao Pedro De Almeida Pereira
<jdealmeidapereira@pivotal.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@postgresql.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
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.
Tira & Joao
On Fri, Mar 24, 2017 at 9:15 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
On Fri, Mar 24, 2017 at 5:07 PM, Dave Page <dpage@pgadmin.org> wrote:Ashesh, can you review/commit this please?
On Thu, Mar 23, 2017 at 3:49 PM, Joao Pedro De Almeida Pereira
<jdealmeidapereira@pivotal.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@postgresql.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
On Fri, Mar 24, 2017 at 7:19 PM, Atira Odhner <aodhner@pivotal.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 & JoaoOn Fri, Mar 24, 2017 at 9:15 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote: On Fri, Mar 24, 2017 at 5:07 PM, Dave Page <dpage@pgadmin.org> wrote:Ashesh, can you review/commit this please?
On Thu, Mar 23, 2017 at 3:49 PM, Joao Pedro De Almeida Pereira
<jdealmeidapereira@pivotal.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@postgresql.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