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

sql/pgwire: explicitly handle 0, 1 or n format codes #5783

Merged
merged 1 commit into from Apr 4, 2016

Conversation

dt
Copy link
Member

@dt dt commented Mar 31, 2016

refactored slightly to explicitly handle the cases allowed by the spec and check for a few
invalid cases that we were previously allowing (eg 0 < format codes < params).

Unfortunately hard to write a test with current setup because of lib/pq, but verified by using the failing php example.

Fixes #5758.


This change is Reviewable

@bdarnell
Copy link
Member

We can't easily test this from go but we should still be able to add it to acceptance/php_test.go.

@tamird
Copy link
Contributor

tamird commented Mar 31, 2016

:lgtm:

agree we need an acceptance test that tickles this.


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


sql/pgwire/v3.go, line 496 [r1] (raw file):
link to it: http://www.postgresql.org/docs/current/static/protocol-message-formats.html

also, do we use multiline comments like this anywhere else?


sql/pgwire/v3.go, line 509 [r1] (raw file):
this should be a switch now, and the 0 case can just be a no-op


sql/pgwire/v3.go, line 563 [r1] (raw file):
ditto. also in both cases we need to give context explaining what the docs are referring to when they say "This"


sql/pgwire/v3.go, line 577 [r1] (raw file):
also a switch


Comments from the review on Reviewable.io

@tbg
Copy link
Member

tbg commented Mar 31, 2016

Thanks for fixing this so quickly! cc @jseldess

@dt
Copy link
Member Author

dt commented Mar 31, 2016

Hm, maybe we should make all the examples acceptance tests.


Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


sql/pgwire/v3.go, line 496 [r1] (raw file):
Done.


sql/pgwire/v3.go, line 509 [r1] (raw file):
Done.


sql/pgwire/v3.go, line 563 [r1] (raw file):
Done.


sql/pgwire/v3.go, line 577 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@bdarnell
Copy link
Member

We need to find some solution for testing the examples; adding them to the acceptance tests would be one way to do it. See cockroachdb/docs#182

@tamird
Copy link
Contributor

tamird commented Apr 1, 2016

LGTM with the test


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


sql/pgwire/v3.go, line 565 [r2] (raw file):
To match the above:

"From the docs on number of result-column format codes to bind:"


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Apr 1, 2016

:lgtm:


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


acceptance/php_test.go, line 41 [r3] (raw file):
separate this with a blank line?


Comments from the review on Reviewable.io

refactored slightly to explicitly handle the cases allowed by the spec and check for a few
invalid cases that we were previously allowing (eg 0 < format codes < params).

Fixes cockroachdb#5758.
@dt dt merged commit 858ced4 into cockroachdb:master Apr 4, 2016
@dt dt deleted the formatcodes branch April 7, 2016 17:07
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.

None yet

4 participants