Re: [pgAdmin4][Patch]: Foreign Data Wrapper

Hi Dave,

Thank you for the reviewing the patch.
Please find inline comments and attached updated patch file after fixing
all the comments.

Do let us know in case of any comments.

Thanks,
Neel Patel

On Fri, Mar 4, 2016 at 10:19 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Tue, Feb 23, 2016 at 5:24 PM, Neel Patel <neel(dot)patel(at)enterprisedb(dot)com>
> wrote:
>
>> Hi,
>>
>> Please find attached patch file for the following three nodes.
>>
>> - Foreign Data Wrappers
>> - Foreign Servers
>> - User Mapping
>>
>> With this patch, we have implemented "Dependencies", "Dependent" tab and
>> proper comments has been added.
>>
>> Do review it and let us know for any comments.
>>
>
> This seems to be nearly ready now. Some feedback below - note that a
> couple of the issues may be caused by infrastructure code, in which case
> please do fix them, but feel free to put them in a different patch:
>
> - When adding a User Mapping, you cannot specify an empty option, e.g. a
> blank password.
>

Fixed. User will not be able to save with blank password. "Save" button
will be disabled and error message will be displayed.

>
> - When granting USAGE to PUBLIC on a foreign server, the WITH GRANT OPTION
> is set, despite not being selected. e.g. adding "U" permissions for
> "public" results in:
>
> GRANT ALL ON FOREIGN SERVER redis_server TO public;
> GRANT ALL ON FOREIGN SERVER redis_server TO public WITH GRANT OPTION;
>

Fixed.

>
> - Why all the extra blank lines in this SQL? I would expect to see only 2,
> between the ALTER/COMMENT/GRANT sections.
>
> ====
> ALTER SERVER redis_server
> VERSION 'Fooo';
>
> COMMENT ON SERVER redis_server
> IS 'Redis Server x';
>
>
>
>
>
>
> GRANT ALL ON FOREIGN SERVER redis_server TO public;
> GRANT ALL ON FOREIGN SERVER redis_server TO public WITH GRANT OPTION;
>
>
> ====
>

Fixed.

>
> - Error messages from the server are not displayed properly - e.g:
>
> invalid option "server"
> HINT: Valid options in this context are: <none>
>
> Is displayed as:
>
> invalid option "server" HINT: Valid options in this context are:
>
> (Notice the lack of "<none>")
>

Here we are displaying the generic error message text received from
database server. We can not add <none> to message text but "HINT" will be
displayed in new line.

e.g. If you execute below command in PG 9.5 through "plpgsql" then database
server will give the error message without valid context.

CREATE FOREIGN DATA WRAPPER test_fdw VALIDATOR postgresql_fdw_validator
OPTIONS (server '123');

Error message will be as below from database server.

ERROR: invalid option "server"
HINT: Valid options in this context are:

> - A foreign server object is not listed as being dependent upon the FDW
> it's defined as using (but, pgAdmin 3 gets this wrong too).
>

Fixed.

>
> - The "Options" tabs have a hint message of "Please enter some value!",
> whether the focus is in the Option or Value field. Please fix to display:
>
> Please enter an option name.
>
> or
>
> Please enter a value.
>
> Based on the current focus. Note also that there should not be an
> exclamation mark.
>

Fixed.

>
> Thanks!
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>