Re: [pgAdmin][RM4642] port should not be mandatory when a service is provided

On Mon, Sep 16, 2019 at 1:54 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:

>
>
> On Mon, Sep 16, 2019 at 8:45 AM Murtuza Zabuawala <
> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>
>>
>>
>>
>> On Mon, Sep 16, 2019 at 12:38 PM Neel Patel <neel(dot)patel(at)enterprisedb(dot)com>
>> wrote:
>>
>>> Hi,
>>>
>>> On Mon, Sep 16, 2019 at 12:20 PM Murtuza Zabuawala <
>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>
>>>> On Mon, Sep 16, 2019 at 12:14 PM Neel Patel <
>>>> neel(dot)patel(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> On Mon, Sep 16, 2019 at 12:05 PM Murtuza Zabuawala <
>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi Akshay,
>>>>>>
>>>>>>
>>>>>> On Mon, Sep 16, 2019 at 11:26 AM Akshay Joshi <
>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi Murtuza
>>>>>>>
>>>>>>> On Sun, Sep 15, 2019 at 7:11 PM Murtuza Zabuawala <
>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Akshay,
>>>>>>>>
>>>>>>>>
>>>>>>>> On Fri, Sep 13, 2019 at 3:34 PM Akshay Joshi <
>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Fri, Sep 13, 2019 at 3:01 PM Murtuza Zabuawala <
>>>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On Fri, Sep 13, 2019 at 2:32 PM Akshay Joshi <
>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi Murtuza
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Sep 13, 2019 at 1:40 PM Murtuza Zabuawala <
>>>>>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> We are making assumption here that all user's service file has
>>>>>>>>>>>> port & username inside them.
>>>>>>>>>>>> What if they are not present?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 5432 is the default port and the system user is the default
>>>>>>>>>>> user to connect.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Despite default values we had frontend validation on those fields
>>>>>>>>>> from the beginning [Not specific to service file but in general].
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Validations are still there until the user provides the
>>>>>>>>> service field.
>>>>>>>>>
>>>>>>>>> [image: Screenshot 2019-09-13 at 3.18.56 PM.png] [image:
>>>>>>>>> Screenshot 2019-09-13 at 3.19.15 PM.png]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> If port and username are not provided in the service file then
>>>>>>>>>>> the user can specify that in the dialog itself.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I know user can provide in the dialog itself but we are not
>>>>>>>>>> validating that service file contains port and/or username information in
>>>>>>>>>> it & still we allow to save the server, I think if we are removing the
>>>>>>>>>> validation from frontend then we should verify it in the backend if the
>>>>>>>>>> service file contains required fields or not and throw an error before
>>>>>>>>>> saving the server.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Why we should read the service file and check for the
>>>>>>>>> validation, as I mentioned if port and username is not provided in the
>>>>>>>>> service file then it will try to connect to 5432 with username as the
>>>>>>>>> system user.
>>>>>>>>>
>>>>>>>>
>>>>>>>> As per my knowledge, default port has nothing to do with service
>>>>>>>> field, if we remove all the validation for port field and do not provide
>>>>>>>> any value to port field when saving the server in the pgAdmin4 (whether the
>>>>>>>> service field is provided or not), Psycopg2 will still use 5432 port when
>>>>>>>> connecting to DB server because that is from libpq.
>>>>>>>>
>>>>>>>
>>>>>>> Agreed.
>>>>>>>
>>>>>>>>
>>>>>>>> Despite of libpq's default port value before this change we had
>>>>>>>> validation around port field and it was compulsory field in pgAdmin4 before
>>>>>>>> user can save the server in pgAdmin4, Now lets say I have cleared/removed
>>>>>>>> the default port field value from pgAdmin4 GUI and I provided service file
>>>>>>>> in which port field is not present, we are still allowing user to save the
>>>>>>>> server, which was not the case earlier as we were not allowing user to add
>>>>>>>> or save the server which does not have port value.
>>>>>>>>
>>>>>>>
>>>>>>> As I told earlier validation for the port field is still there
>>>>>>> until the user will provide the *service* name. I am assuming the
>>>>>>> user of the service file should aware of the parameters provided in the
>>>>>>> service file. Consider a case where users need to change the database
>>>>>>> server port and username for some reason then why we should bound the user
>>>>>>> to update the same information (port and username) in two places one in
>>>>>>> service file and another on server dialog?
>>>>>>>
>>>>>>
>>>>>> I think you are getting me wrong, What I am suggesting is, if the
>>>>>> port value is not present on GUI as well as in the service file then we
>>>>>> should not allow user to save the server, port should be present in either
>>>>>> on GUI or in service file, if it is not then we should notify user to
>>>>>> provide correct port value at least via GUI before saving the server.
>>>>>>
>>>>>
>>>>> I am not agree to read the service file by pgadmin4 as we are adding
>>>>> unnecessary complexity. As we are already providing the user to add the
>>>>> port if it is not provided in service file. Think about other client
>>>>> application like "psql". If you have not provided port, it will connect to
>>>>> default port.
>>>>>
>>>>
>>>> So if we are depending on default port with service file, why we had
>>>> validation for port in pgAdmin4 GUI from the beginning?
>>>>
>>>
>>> At the beginning, we may not have thought about it. Without adding port
>>> and username, it is connecting to default port and system user. We should
>>> have add those in help message to user under control. You should have
>>> raised those valid points during service file implementation. After service
>>> implementation, we are asking user to add the port and username even though
>>> it is already added in service file which is useless.
>>>
>>
>> @Neel,
>> Please check above comments, I am no where suggesting the port/username
>> value to be entered twice, I'm only suggesting to add validation if the
>> port value is not present on both GUI & in Service file then we notify user
>> about it and ask user to enter it from GUI at least but as Akshay suggested
>> service file can present at multiple locations so we can live with it :)
>>
>
> libpq reads the service file, not us. Unless we write a parser for it, we
> cannot validate it.
>
We don't require to write parser, Python's inbuilt module configparser
supports INI file format.
The only challenge I see is to pick the correct file location as Akshay
suggested.

> We should validate the port, unless a service file is specified.
>
>
>>
>>
>> - But still I think we can display the port used for the connection from
>> pg_settings table on the properties panel if server is connected & it using
>> service file for connection and user has removed the port value from GUI.
>>
>
> We could do that, but I think there are more important things on the todo
> list right now.
>

>
>>
>> @Akshay,
>> What do you think?
>>
>>
>> Regards,
>> Murtuza
>>
>>
>>
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>> - Also I would like to suggest one minor enhancement that if server
>>>>>> is connected using service file then we can show the port used for the
>>>>>> connection from pg_settings table, with current implementation is not
>>>>>> showing the port value on properties panel.
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> If I remember correctly those validation were left intentional
>>>>>>>>>>>> when we added service field just to make sure that we connect to DB server
>>>>>>>>>>>> from pgAdmin4 regardless of those information present in service file.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I personally think that behaviour is wrong if my service
>>>>>>>>>>> file has the correct port number then why should the user enter all those
>>>>>>>>>>> information again, and what if the user provides the wrong port number from
>>>>>>>>>>> server dialog, it trys to connect to the wrong port.
>>>>>>>>>>>
>>>>>>>>>>> One small suggestion such comments should come early as I
>>>>>>>>>>> have sent the patch 4 days ago not after the patch has been committed.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Sorry I missed the earlier email, I only saw today morning with
>>>>>>>>>> your reminder email.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Murtuza
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Murtuza Zabuawala
>>>>>>>>>>>> EnterpriseDB: http://www.enterprisedb.com
>>>>>>>>>>>> The Enterprise PostgreSQL Company
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Sep 13, 2019 at 12:09 PM Aditya Toshniwal <
>>>>>>>>>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Looks good to me.
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Fri, Sep 13, 2019 at 8:05 AM Akshay Joshi <
>>>>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Can someone please review this.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Mon, 9 Sep, 2019, 18:06 Akshay Joshi, <
>>>>>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Hackers,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Attached is the patch to fix the RM #4642 "port should not
>>>>>>>>>>>>>>> be mandatory when a service is provided". *Cyril Jouve* has
>>>>>>>>>>>>>>> sent the initial patch, but that only removes the validation on GUI, when
>>>>>>>>>>>>>>> we save the server properties without port and username backend throws an
>>>>>>>>>>>>>>> error because port and username is *NOT NULL* columns in
>>>>>>>>>>>>>>> the server table in SQLite.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I have removed the *NOT NULL *constraint from the port and
>>>>>>>>>>>>>>> username. The maintenance database is required as our whole connection
>>>>>>>>>>>>>>> logic is based on that, so I have added the NOT NULL constraint for that
>>>>>>>>>>>>>>> column.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Please review it.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>> *Thanks & Regards*
>>>>>>>>>>>>>>> *Akshay Joshi*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *Sr. Software Architect*
>>>>>>>>>>>>>>> *EnterpriseDB Software India Private Limited*
>>>>>>>>>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Thanks and Regards,
>>>>>>>>>>>>> Aditya Toshniwal
>>>>>>>>>>>>> Software Engineer | EnterpriseDB India | Pune
>>>>>>>>>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> *Thanks & Regards*
>>>>>>>>>>> *Akshay Joshi*
>>>>>>>>>>>
>>>>>>>>>>> *Sr. Software Architect*
>>>>>>>>>>> *EnterpriseDB Software India Private Limited*
>>>>>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> *Thanks & Regards*
>>>>>>>>> *Akshay Joshi*
>>>>>>>>>
>>>>>>>>> *Sr. Software Architect*
>>>>>>>>> *EnterpriseDB Software India Private Limited*
>>>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> *Thanks & Regards*
>>>>>>> *Akshay Joshi*
>>>>>>>
>>>>>>> *Sr. Software Architect*
>>>>>>> *EnterpriseDB Software India Private Limited*
>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>
>>>>>>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>