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

Return errors in QueryRow properly #55

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

codemac
Copy link

@codemac codemac commented Feb 10, 2015

  • oracle.go: Handle QueryRow multiple return values
  • postgres.go: Handle QueryRow multiple return values
  • qbs.go: Return an error during QueryRow

This fixes a panic in the qbs.Count and qbs.ContainsValue implementation

Signed-off-by: Jeff Mickey [email protected]

* oracle.go: Handle QueryRow multiple return values
* postgres.go: Handle QueryRow multiple return values
* qbs.go: Return an error during QueryRow

This fixes a panic in the qbs.Count and qbs.ContainsValue implementation

Signed-off-by: Jeff Mickey <[email protected]>
@codemac
Copy link
Author

codemac commented Feb 10, 2015

See #54 for details of why this is necessary.

@coocood
Copy link
Owner

coocood commented Feb 10, 2015

Thank you for reporting the panic issue, but I can not merge this PR because it is an API change, ti will break the previous working code.

Can we just check if the row is nil and return 0 in the "Count" method?

@codemac
Copy link
Author

codemac commented Feb 10, 2015

Well, you certainly could, but I think the other function (doQueryRow, doQueryRows) all return errors as well, so this seems like a natural implementation. You don't document what QueryRow will return on error, and I think maintaining hidden behavior is a bad choice. Semantic change requiring two different code changes (an if row == nil or if err != nil) means you're changing the API either way.

You're more than welcome to make changes to your code however you wish, this took me very little time to fix.

@coocood
Copy link
Owner

coocood commented Feb 10, 2015

I just remembered that I didn't return error in "QueryRow" method because I follow the standard library "database.sql" package . So the error shoud be set in the "Row", and the "Row" should never be nil.

Would you please change it this way, then I will merge it as soon as possible.

Thank you.

@codemac
Copy link
Author

codemac commented Feb 10, 2015

On February 9, 2015 8:05:37 PM PST, Ewan Chou [email protected] wrote:

I just remembered that I didn't return error in "QueryRow" method
because I follow the standard library "database.sql" package . So the
error shoud be set in the "Row", and the "Row" should never be nil.

Would you please change it this way, then I will merge it as soon as
possible.

Thank you.


Reply to this email directly or view it on GitHub:
#55 (comment)

Ahhh that makes much more sense now that you explain it. Will change it accordingly.
// mickey

@codemac
Copy link
Author

codemac commented Feb 10, 2015

Problem - the err in sql.Row is unexported. I don't know how to set the error in Row. As long as a tx.Prepare is tied up in this API, I don't think it's possible without our own Row type.

@coocood
Copy link
Owner

coocood commented Feb 11, 2015

We can set the error via reflection.

@codemac
Copy link
Author

codemac commented Feb 11, 2015

It's unexported, you can't reflect on it... which struct are you talking about reflecting on? sql.Row.err is private to the stdlib. I think I'm misunderstanding the issue.

@coocood
Copy link
Owner

coocood commented Feb 11, 2015

Sorry, I didn't write go reflection for a whiel and confused it with JAVA reflection.

Then if we failed to prepare, we can just call db.QueryRow,, if tx is not nil, we call tx.QueryRow.

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