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

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?

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