-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add a Render() function which returns a valid query #3
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about including this in the library, since people may use it to directly pass to SQL. We then need to be 100% sure it isn't escapable in all sql backends.
Alternatively we could include a Render(escape func(interface{}) string)
. We should be able to implement that cleanly directly using the fmt package (and avoid uses of strings.Replace/etc)
Is the purpose of this for observability somewhere?
switch arg := arg.(type) { | ||
case string: | ||
return fmt.Sprintf("'%s'", escapeQuotes(arg)), nil | ||
case *pq.StringArray: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than pulling in the pq
dependency, can you just do type check against interface{Value()(interface{}, error)}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea 💡
|
||
// TODO support other BindVars such as `?`. | ||
|
||
// TODO there must be a better way to do this, perhaps using `q.fmt`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup using the formatter is likely the way to do this.
I've been using this locally behind a debug flag while working on Rockskip. I'm not 100% sure the escape function is safe (so I removed the code that ran the query automatically). I didn't expect this to get merged without a lot more work, and it should've been a draft to begin with. |
Check this out:
That prints this:
Ready to copy-n-paste into
psql
, orEXPLAIN ANALYZE ....
and print the timing info for local dev.This was the bare minimum necessary to work for my pursposes, but there's a lot left to do in order to work in general for all
BindVar
s, drivers, and argument types.