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

On Mon, Sep 16, 2019 at 2:18 PM Murtuza Zabuawala <
[email protected]> wrote:

>
> On Mon, Sep 16, 2019 at 1:54 PM Dave Page <[email protected]> wrote:
>
>>
>>
>> On Mon, Sep 16, 2019 at 8:45 AM Murtuza Zabuawala <
>> [email protected]> wrote:
>>
>>>
>>>
>>>
>>> On Mon, Sep 16, 2019 at 12:38 PM Neel Patel <[email protected]>
>>> wrote:
>>>
>>>> Hi,
>>>>
>>>> On Mon, Sep 16, 2019 at 12:20 PM Murtuza Zabuawala <
>>>> [email protected]> wrote:
>>>>
>>>>> On Mon, Sep 16, 2019 at 12:14 PM Neel Patel <
>>>>> [email protected]> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Mon, Sep 16, 2019 at 12:05 PM Murtuza Zabuawala <
>>>>>> [email protected]> wrote:
>>>>>>
>>>>>>> Hi Akshay,
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Sep 16, 2019 at 11:26 AM Akshay Joshi <
>>>>>>> [email protected]> wrote:
>>>>>>>
>>>>>>>> Hi Murtuza
>>>>>>>>
>>>>>>>> On Sun, Sep 15, 2019 at 7:11 PM Murtuza Zabuawala <
>>>>>>>> [email protected]> wrote:
>>>>>>>>
>>>>>>>>> Hi Akshay,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Fri, Sep 13, 2019 at 3:34 PM Akshay Joshi <
>>>>>>>>> [email protected]> wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Fri, Sep 13, 2019 at 3:01 PM Murtuza Zabuawala <
>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Sep 13, 2019 at 2:32 PM Akshay Joshi <
>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi Murtuza
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Sep 13, 2019 at 1:40 PM Murtuza Zabuawala <
>>>>>>>>>>>> [email protected]> 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.
>
Murtuza,

Let's not waste time as Dave suggested, it has low priority.
Also - it does not have much impact on the functionality.

Create a case with low priority, and more on.

-- Ashesh

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 <
>>>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Looks good to me.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Fri, Sep 13, 2019 at 8:05 AM Akshay Joshi <
>>>>>>>>>>>>>> [email protected]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Can someone please review this.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Mon, 9 Sep, 2019, 18:06 Akshay Joshi, <
>>>>>>>>>>>>>>> [email protected]> 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
>>
>