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

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.

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