-
Notifications
You must be signed in to change notification settings - Fork 81
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
Implement Go version of SQL Commenter #101
Conversation
This implements the SQL Commenter specification for affixing comments to a SQL query. Loosely inspired by url.Values [1] in the Go standard library. The specification doesn't fully describe the following situation, so I'll list the intended behavior of the Go implementation here: - Empty keys: omitted and don't appear in the comment string. - Keys with single quotes: this Go library uses the valid URL encoding of a single quote `%27` instead of escaping with a backslash. The spec has the confusing example: `name''` serialized to `name=\'\'`. Where did the equal sign come from? Also, the website shows fancy quotes, not plain ascii quotes. - Already URL encoded values: this Go library double-encodes the value, meaning the key-pair `a=%3D` is encoded as `a='%253D'`. The spec exhibit doesn't double-encode, instead preserving the original percent encoding. That seems inadvisable since the parsing a serialized sql comment won't result in the same original values. Fixes google#95. [1]: https://pkg.go.dev/net/url#Values
Thanks @jschaf for the PR. Will take a look today. |
So, we are mapping a map of values to the SQLCommenter format. Are we planning to add auto-instrumentation after this ? If we look at other sqlcommenter libraries (js-knex example), we are trying to achieve auto instrumentation i.e user need to just setup the library. Then the library (a set of middleware) will fetch the information like route and pass that to the SQLComment. |
There's at least two things to make auto-instrumentation work for HTTP requests:
Another issue is that lib/pq is deprecated for Postgres in favor of pgx. I think the only way to hook into a pgx.Conn is to wrap it, like option 1 in https://anymindgroup.com/news/tech-blog/15724/ To put my cards on the table, I only need the PR I've implemented since we have our own wrapper around pgx so I'm not likely to pick up any additional work. If that's a deal-breaker let me know and I can close this and someone can use it in the future for an auto-instrumented implementation. |
Hi, I also interested in the Go version for sqlcommenter, I discover that ent already has support for it, maybe the integration should be similar:
|
@jschaf I am thinking there is a value add to add this code as a library ( What do you think ? Thanks @artefactop, thats a good starting point to look. Will explore this. |
An alternative to the |
Hey guys, I am the author of Ent's |
Thanks @hedwigz! Sorry to miss this message. We are planning to start brainstorming about go-sqlcommenter after 2nd week of June. We will look at Maybe we will start a discussion on an issue in this repository. Will pull you guys there. Would that be fine ? |
I'm happy to move this wherever, but I think something like the following would be more idiomatic:
|
This implements the SQL Commenter specification for affixing comments to a
SQL query. Loosely inspired by url.Values 1 in the Go standard library.
The specification doesn't fully describe the following situations, so I'll
list the intended behavior of the Go implementation here:
Empty keys: omitted and don't appear in the comment string.
Keys with single quotes: this Go library uses the valid URL encoding
of a single quote
%27
instead of escaping with a backslash. Thespec has the confusing example:
name''
serialized toname=\'\'
.Where did the equal sign come from? Also, the website shows fancy
quotes, not plain ascii quotes.
Already URL encoded values: this Go library double-encodes the value,
meaning the key-pair
a=%3D
is encoded asa='%253D'
. The spec exhibitdoesn't double-encode, instead preserving the original percent encoding.
That seems inadvisable since the parsing a serialized sql comment won't
result in the same original values.
Fixes #95.