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

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 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