Skip to content

Make option parsing more conformant to libpq behaviour. #164

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

Merged
merged 1 commit into from
Oct 10, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ func (s *scanner) SkipSpaces() (rune, bool) {
func parseOpts(name string, o values) error {
s := newScanner(name)

top:
for {
var (
keyRunes, valRunes []rune
Expand All @@ -203,25 +202,25 @@ top:
for !unicode.IsSpace(r) && r != '=' {
keyRunes = append(keyRunes, r)
if r, ok = s.Next(); !ok {
break top
break
}
}

// Skip any whitespace if we're not at the = yet
if r != '=' {
if r, ok = s.SkipSpaces(); !ok {
break
}
r, ok = s.SkipSpaces()
}

// The current character should be =
if r != '=' {
if r != '=' || !ok {
return fmt.Errorf(`missing "=" after %q in connection info string"`, string(keyRunes))
}

// Skip any whitespace after the =
if r, ok = s.SkipSpaces(); !ok {
break top
// If we reach the end here, the last value is just an empty string as per libpq.
o.Set(string(keyRunes), "")
break
}

if r != '\'' {
Expand Down
21 changes: 14 additions & 7 deletions conn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -678,28 +678,35 @@ func TestParseOpts(t *testing.T) {
{"dbname=hello user= goodbye", values{"dbname": "hello", "user": "goodbye"}, true},
{"host=localhost password='correct horse battery staple'", values{"host": "localhost", "password": "correct horse battery staple"}, true},
{"dbname=データベース password=パスワード", values{"dbname": "データベース", "password": "パスワード"}, true},
{"dbname=hello user=''", values{"dbname": "hello", "user": ""}, true},
{"user='' dbname=hello", values{"dbname": "hello", "user": ""}, true},
// The last option value is an empty string if there's no non-whitespace after its =
{"dbname=hello user= ", values{"dbname": "hello", "user": ""}, true},

// The parser ignores spaces after = and interprets the next set of non-whitespace characters as the value.
{"user= password=foo", values{"user": "password=foo"}, true},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with this case now? Is it no longer relevant? Or do you think it was redundant to begin with?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two of them moved down to the cases below. blah= is actually valid and moved up to the tests above.

// The parser ignores trailing keys
{"user=foo blah", values{"user": "foo"}, true},
{"user=foo blah ", values{"user": "foo"}, true},
{"user=foo blah= ", values{"user": "foo"}, true},

// No '=' after the key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment applies to the unchanged line below, I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It applies to all four tests here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, I guess that's true. The URL format threw me off.

{"postgre://marko@internet", values{}, false},
{"dbname user=goodbye", values{}, false},
{"user=foo blah", values{}, false},
{"user=foo blah ", values{}, false},

// Unterminated quoted value
{"dbname=hello user='unterminated", values{}, false},
}

for _, test := range tests {
o := make(values)
err := parseOpts(test.in, o)
if err != nil && test.valid {

switch {
case err != nil && test.valid:
t.Errorf("%q got unexpected error: %s", test.in, err)
} else if err == nil && !reflect.DeepEqual(test.expected, o) {
case err == nil && test.valid && !reflect.DeepEqual(test.expected, o):
t.Errorf("%q got: %#v want: %#v", test.in, o, test.expected)
case err == nil && !test.valid:
t.Errorf("%q expected an error", test.in)
}
}
}
Expand Down