Postgres dialect parse password with spaces (#775) #779

lunny · 2017-11-17T09:36:37Z

daniellee commented

2017-11-17 09:36:37 +00:00

(Migrated from github.com)

Fixes #775 This fixes two problems - the postgres dialect can parse a password with spaces. If an error occurs the Parse func returns an error instead of panicking.

There are some problems still however:

The Parse function cannot handle symbols like # in a password but we handle that in Grafana (where I found this bug) by escaping everything before creating the connection string.

The code seems to be based on code from pq from 5 year ago. The current version of pq does a lot more checks when parsing connection strings to handle all these cases - see this PR that fixed the panic problem in pq.

Fixes #775 This fixes two problems - the postgres dialect can parse a password with spaces. If an error occurs the Parse func returns an error instead of panicking. There are some problems still however: - The Parse function cannot handle symbols like `#` in a password but we handle that in Grafana (where I found this bug) by escaping everything before creating the connection string. The code seems to be based on code from pq from 5 year ago. The current version of pq does a lot more checks when parsing connection strings to handle all these cases - see this [PR](https://github.com/lib/pq/pull/133) that fixed the panic problem in pq.

daniellee commented

2017-11-17 13:57:19 +00:00

(Migrated from github.com)

Working on fixing the coverage tests - not sure exactly why are failing yet.

daniellee commented

2017-11-17 14:07:07 +00:00

(Migrated from github.com)

Aha, was a little too aggressive with removing code. Needs to be able to parse opts and not just urls.

codecov-io commented

2017-11-17 14:35:27 +00:00

(Migrated from github.com)

Codecov Report

Merging #779 into master will increase coverage by 0.35%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #779      +/-   ##
==========================================
+ Coverage   51.37%   51.72%   +0.35%     
==========================================
  Files          39       39              
  Lines        8605     8585      -20     
==========================================
+ Hits         4421     4441      +20     
+ Misses       3693     3653      -40     
  Partials      491      491

Impacted Files	Coverage Δ
dialect_postgres.go	`76.56% <66.66%> (+10.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e88774...f773a25. Read the comment docs.

# [Codecov](https://codecov.io/gh/go-xorm/xorm/pull/779?src=pr&el=h1) Report > Merging [#779](https://codecov.io/gh/go-xorm/xorm/pull/779?src=pr&el=desc) into [master](https://codecov.io/gh/go-xorm/xorm/commit/4e88774eeef97e733a5a94c6ac4adacf8f4ddbd1?src=pr&el=desc) will **increase** coverage by `0.35%`. > The diff coverage is `66.66%`. [![Impacted file tree graph](https://codecov.io/gh/go-xorm/xorm/pull/779/graphs/tree.svg?token=yB5nO1krEe&width=650&height=150&src=pr)](https://codecov.io/gh/go-xorm/xorm/pull/779?src=pr&el=tree) ```diff @@ Coverage Diff @@ ## master #779 +/- ## ========================================== + Coverage 51.37% 51.72% +0.35% ========================================== Files 39 39 Lines 8605 8585 -20 ========================================== + Hits 4421 4441 +20 + Misses 3693 3653 -40 Partials 491 491 ``` | [Impacted Files](https://codecov.io/gh/go-xorm/xorm/pull/779?src=pr&el=tree) | Coverage Δ | | |---|---|---| | [dialect\_postgres.go](https://codecov.io/gh/go-xorm/xorm/pull/779?src=pr&el=tree#diff-ZGlhbGVjdF9wb3N0Z3Jlcy5nbw==) | `76.56% <66.66%> (+10.38%)` | :arrow_up: | ------ [Continue to review full report at Codecov](https://codecov.io/gh/go-xorm/xorm/pull/779?src=pr&el=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/go-xorm/xorm/pull/779?src=pr&el=footer). Last update [4e88774...f773a25](https://codecov.io/gh/go-xorm/xorm/pull/779?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

daniellee commented

2017-11-17 15:24:00 +00:00

(Migrated from github.com)

@lunny Changes made. Is that what you were thinking?