Broken RC file behaviours.

nestor-custodio commented 3 years ago

Description

I've identified 2 broken behaviours (detailed below) pertaining to how RC files are handled.

Scenario 1: in certain cases the RC file is moved

The Setup:

write config to "\~/.pgclirc"
"\~/.pgpass" contains "[host]:[port]:*:[user]:[pass]"

The Command:

/usr/bin/pgcli "postgres://[user]@[host]/[database]"

The Outcome: Note I cannot verify exactly what order some of the below items occur in, as I can only attest to what order I see output generated in.

a "\~/.config/pgcli" directory is created
the "\~/.pgclirc" file is moved (this is not a copy-delete two-step, for whatever that's worth) to "\~/.config/pgcli/config"

the following output is generated:

Config file (\~/.pgclirc) moved to new location /home/nestor/.config/pgcli/config

the config file is properly read and parsed without issue
prompt

The Problem:

the most superficial problem is that there is no reason for this whatsoever
the unnecessary and unexpected file move can break dotfile backup processes
pgcli will fail to start at all (with a PermissionError: [Errno 13]) if the process cannot create "\~/.config/pgcli" or write to "\~/.config/pgcli/config"

The Expectation:

no directories are created
no files are moved
the config file is properly read and parsed without issue
prompt

Scenario 2: in certain cases the RC file is moved and also replaced with the default config

Thinking that explicitly setting the --pgclirc option might make it clear that "this is where my config file is and this is where you should read it from, do nothing else please and thank you", I found a related (but somehow worse) problem.

The Setup:

write config to "\~/.pgclirc"
"\~/.pgpass" contains "[host]:[port]:*:[user]:[pass]"

The Command:

/usr/bin/pgcli --pgclirc="\~/.pgclirc" "postgres://[user]@[host]/[database]"

The Outcome: Note I cannot verify exactly what order some of the below items occur in, as I can only attest to what order I see output generated in.

a "\~/.config/pgcli" directory is created
the "\~/.pgclirc" file is moved (this is not a copy-delete two-step, for whatever that's worth) to "\~/.config/pgcli/config"

the following output is generated:

Config file (~/.pgclirc) moved to new location /home/nestor/.config/pgcli/config

the default config is written to "\~/.pgclirc"
the default config (as written to "\~/.pgclirc") is read and parsed
config-related errors are reported, depending on your system setup (e.g. no keyring backend is available but the default config attempts to read from a keyring)
prompt

The Problem:

all of the problems mentioned in Scenario 1 are also at play here
this is a more insidious problem than in Scenario 1, however, as dotfile backup processes will now continue to work with the (newly-written) "\~/.pgclirc" file, which no longer contains our config
now using the default config, pgcli will:
- additionally write "\~/.config/pgcli/log"
- additionally write "\~/.config/pgcli/history"
- present a (potentially) incorrect prompt
- potentially issue queries that adversely affect DB performance: e.g. You are a user who normally operates with multi_line = true and a low row_limit, and expect that entering ...
```
SELECT [complex query with many function calls] FROM [large table or complex series of joins]
WHERE ([condition on indexed field that VASTLY limits the resultset])
```
  ... will normally execute your query fairly quickly, but now the request is fired off to the DB on the first newline, before any WHERE is seen.
- I have not verified this myself, but I expect if pgcli had been invoked with -D/--dsn, this would fail as the loaded config dose not contain your "[alias_dsn]" entries.

The Expectation:

no directories are created
no files are moved
"\~/.pgclirc" is not reset to the default config
the config file is properly read and parsed without issue
prompt

Your Environment

[X] Please provide your OS and version information.

uname -s -r -o yields:

Linux 5.8.0-1038-gcp GNU/Linux

lsb_release --all yields:

No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 20.10
Release:        20.10
Codename:       groovy

[X] Please provide your CLI version.

/usr/bin/pgcli --version yields:
```
Version: 3.0.0
```
(This is the newest available version in my distro's repos. I have verified none of the above behaviours are changed by any commit made between the v3.0.0 release and the v3.1.0 release.)

[X] What is the output of pip freeze command.

pip freeze yields:

attrs==19.3.0
Automat==20.2.0
blinker==1.4
certifi==2020.4.5.1
chardet==3.0.4
click==7.1.2
cloud-init==21.2
colorama==0.4.3
command-not-found==0.3
configobj==5.0.6
constantly==15.1.0
cryptography==3.0
dbus-python==1.2.16
distro==1.5.0
distro-info===0.23ubuntu1
httplib2==0.18.1
humanize==2.5.0
hyperlink==19.0.0
idna==2.10
importlib-metadata==1.6.0
incremental==16.10.1
jeepney==0.4.3
Jinja2==2.11.2
jsonpatch==1.25
jsonpointer==2.0
jsonschema==3.2.0
keyring==21.3.0
language-selector==0.1
launchpadlib==1.10.13
lazr.restfulclient==0.14.2
lazr.uri==1.0.5
MarkupSafe==1.1.1
more-itertools==4.2.0
netifaces==0.10.4
oauthlib==3.1.0
pexpect==4.6.0
pgspecial==1.11.10
prompt-toolkit==3.0.6
psycopg2==2.8.5
pyasn1==0.4.8
pyasn1-modules==0.2.1
Pygments==2.3.1
PyGObject==3.38.0
PyHamcrest==1.9.0
PyJWT==1.7.1
PyMySQL==0.9.3
pyOpenSSL==19.1.0
pyrsistent==0.15.5
pyserial==3.4
python-apt==2.1.3+ubuntu1.4
python-debian==0.1.37
PyYAML==5.3.1
requests==2.23.0
requests-unixsocket==0.2.0
SecretStorage==3.1.2
service-identity==18.1.0
setproctitle==1.1.10
simplejson==3.17.0
six==1.15.0
sos==4.1
sqlparse==0.3.1
ssh-import-id==5.10
systemd-python==234
tabulate==0.8.6
terminaltables==3.1.0
Twisted==18.9.0
ubuntu-advantage-tools==27.0
ufw==0.36
unattended-upgrades==0.1
urllib3==1.25.9
wadllib==1.3.4
wcwidth==0.1.9
zipp==1.0.0
zope.interface==4.7.1

(Note I'm not sure if the above is actually useful or not, as I did not install pgcli via pip.)

amjith commented 3 years ago

Thanks for the detailed walk through of the multiple scenarios.

We don't use the home folder to keep our rc files. We only use ~/.config/pgcli/config location.

When pgcli was first created we were using ~/.pgclirc file and we were told this behavior of storing rc files in home folder is antiquated and we should really be using XDG_CONFIG_HOME location. Hence the change.

We copy the file from ~/.pgclirc to ~/.config/pgcli/config as a means of easing the transition for users so they don't have to start from scratch due to the sudden change in rc file location.

You point about rc file backup doesn't make sense to me. The rc file backup should backup the file located in ~/.config/pgcli/config location and not the ~/.pgclirc file.

nestor-custodio commented 3 years ago

Ah. If you're sticking to XDGBDS, that makes sense and I consider item 1 addressed.

I would still argue, however, that (a) the --pgclirc flag should be respected with no side effects, and that (b) --pgclirc="~/.pgclirc" resulting in a default config being loaded regardless of the file's original contents is a legitimate bug.

dbcli / pgcli