Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed issue where the password was being assigned to wrong config key #136

Closed
wants to merge 1 commit into from

Conversation

ap-in-git
Copy link

Previously when on DNS connection string when empty password was passed . It was being assigned to wrong config key.
For e.g host=localhost user=jack password= dbname=mydb port=5432 sslmode=disable used to generate password as dbname=mydb. This PR tries to fix this. Not sure if this would be right way to fix it though . All the tests were passing

@jackc
Copy link
Owner

jackc commented Sep 28, 2023

This seems to have broken TestParseConfigDSNWithTrailingEmptyEqualDoesNotPanic. Also, just FYI, this is the pgconn used with pgx v4 not the current v5.

@ap-in-git
Copy link
Author

I tried to fix TestParseConfigDSNWithTrailingEmptyEqualDoesNotPanic with the following piece of code

if len(s) == 0 || s[0] == ' ' && s[1] != '\'' { settings[key] = "" continue }

Then the final map of configs are generated correctly

Screenshot 2023-09-28 at 22 14 43

The problem is now I get different set of error because now parsePort method is returning an error because the port value method becomes empty string
Screenshot 2023-09-28 at 22 16 47

I wonder whether the test TestParseConfigDSNWithTrailingEmptyEqualDoesNotPanic should not panic on this case though

@jackc
Copy link
Owner

jackc commented Sep 30, 2023

Actually, on further research this is not really a bug. We try to match psql and the C library libpq.

https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING-KEYWORD-VALUE

To write an empty value, or a value containing spaces, surround it with single quotes

@jackc jackc closed this Sep 30, 2023
@ap-in-git
Copy link
Author

Ah, I see then shouldn't TestParseConfigDSNWithTrailingEmptyEqualDoesNotPanic panic?

@jackc
Copy link
Owner

jackc commented Sep 30, 2023

No, a bad conn string shouldn't be able to make the application panic. Though even calling it a bad string is not quite right. Spaces are allowed before and after the =. So there really is no way to know that password= port=5555 doesn't actually mean the password is port=5555.

@ap-in-git
Copy link
Author

Oh okay. Thanks anyway for amazing library. I just ran into the issue. I guess I will put empty single quote then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants