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

allow to append comment to query #709

Merged
Merged
Show file tree
Hide file tree
Changes from 5 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
11 changes: 9 additions & 2 deletions lib/postgrex.ex
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,11 @@ defmodule Postgrex do
This is useful when using Postgrex against systems that do not support composite types
(default: `false`).

* `:comment` - When a binary string is provided, appends the given text as a comment to the
query. This can be useful for tracing purposes, such as when using SQLCommenter or similar
tools to track query performance and behavior. Note that including a comment disables query
caching since each query with a different comment is treated as unique (default: `nil`).

`Postgrex` uses the `DBConnection` library and supports all `DBConnection`
options like `:idle`, `:after_connect` etc. See `DBConnection.start_link/2`
for more information.
Expand Down Expand Up @@ -363,7 +368,8 @@ defmodule Postgrex do
{:ok, Postgrex.Query.t()} | {:error, Exception.t()}
def prepare(conn, name, statement, opts \\ []) do
query = %Query{name: name, statement: statement}
opts = Keyword.put(opts, :postgrex_prepare, true)
prepare? = Keyword.get(opts, :comment) == nil
opts = Keyword.put(opts, :postgrex_prepare, prepare?)
DBConnection.prepare(conn, query, opts)
end

Expand All @@ -373,7 +379,8 @@ defmodule Postgrex do
"""
@spec prepare!(conn, iodata, iodata, [option]) :: Postgrex.Query.t()
def prepare!(conn, name, statement, opts \\ []) do
opts = Keyword.put(opts, :postgrex_prepare, true)
prepare? = Keyword.get(opts, :comment) == nil
opts = Keyword.put(opts, :postgrex_prepare, prepare?)
DBConnection.prepare!(conn, %Query{name: name, statement: statement}, opts)
end

Expand Down
43 changes: 33 additions & 10 deletions lib/postgrex/protocol.ex
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ defmodule Postgrex.Protocol do
message:
"`:commit_comment` option cannot contain sequence \"*/\""
)
@comment_validation_error Postgrex.Error.exception(
message: "`:comment` option cannot contain sequence \"*/\""
)

defstruct sock: nil,
connection_id: nil,
Expand Down Expand Up @@ -347,8 +350,17 @@ defmodule Postgrex.Protocol do
status = new_status(opts, prepare: prepare)

case prepare do
true -> parse_describe_close(s, status, query)
false -> parse_describe_flush(s, status, query)
josevalim marked this conversation as resolved.
Show resolved Hide resolved
true ->
parse_describe_close(s, status, query)

false ->
comment = Keyword.get(opts, :comment)

if is_binary(comment) && String.contains?(comment, "*/") do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if is_binary(comment) && String.contains?(comment, "*/") do
if is_binary(comment) and String.contains?(comment, "*/") do

Copy link
Member

Choose a reason for hiding this comment

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

Let's move this validation to the client. There is no need to crash the connection on bad argument. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I just remove the whole if here?

Copy link
Member

Choose a reason for hiding this comment

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

You should move it to the client, to the Postgrex module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it there in the query function.
In theory the comment could be also an iolist but Currently we are getting CaseClauseError.
I think this is hard to validate for sql injection unless we first dump it to a string.

raise @comment_validation_error
else
parse_describe_flush(s, status, query, comment)
end
end
end

Expand Down Expand Up @@ -1418,9 +1430,9 @@ defmodule Postgrex.Protocol do
parse_describe(s, status, query)
end

defp parse_describe_flush(s, %{mode: :transaction} = status, query) do
defp parse_describe_flush(s, %{mode: :transaction} = status, query, comment) do
%{buffer: buffer} = s
msgs = parse_describe_msgs(query, [msg_flush()])
msgs = parse_describe_comment_msgs(query, comment, [msg_flush()])

with :ok <- msg_send(%{s | buffer: nil}, msgs, buffer),
{:ok, %Query{ref: ref} = query, %{postgres: postgres} = s, buffer} <-
Expand All @@ -1442,11 +1454,12 @@ defmodule Postgrex.Protocol do
defp parse_describe_flush(
%{postgres: :transaction, buffer: buffer} = s,
%{mode: :savepoint} = status,
query
query,
comment
) do
msgs =
[msg_query(statement: "SAVEPOINT postgrex_query")] ++
parse_describe_msgs(query, [msg_flush()])
parse_describe_comment_msgs(query, comment, [msg_flush()])

with :ok <- msg_send(%{s | buffer: nil}, msgs, buffer),
{:ok, _, %{buffer: buffer} = s} <- recv_transaction(s, status, buffer),
Expand All @@ -1466,7 +1479,7 @@ defmodule Postgrex.Protocol do
end
end

defp parse_describe_flush(%{postgres: postgres} = s, %{mode: :savepoint}, _)
defp parse_describe_flush(%{postgres: postgres} = s, %{mode: :savepoint}, _, _)
when postgres in [:idle, :error] do
transaction_error(s, postgres)
end
Expand Down Expand Up @@ -1593,6 +1606,16 @@ defmodule Postgrex.Protocol do
transaction_error(s, postgres)
end

defp parse_describe_comment_msgs(query, comment, tail) when is_binary(comment) do
statement = query.statement <> "/*#{comment}*/"
Copy link
Member

Choose a reason for hiding this comment

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

It seems we may no longer go wit hthis PR, but I wonder if this could be dealt as IO lists:

Suggested change
statement = query.statement <> "/*#{comment}*/"
statement = [query.statement, "/*", comment, "*/"]

query = %{query | statement: statement}
parse_describe_msgs(query, tail)
end

defp parse_describe_comment_msgs(query, _comment, tail) do
parse_describe_msgs(query, tail)
end

defp parse_describe_msgs(query, tail) do
%Query{name: name, statement: statement, param_oids: param_oids} = query
type_oids = param_oids || []
Expand Down Expand Up @@ -2079,7 +2102,7 @@ defmodule Postgrex.Protocol do

_ ->
# flush awaiting execute or declare
parse_describe_flush(s, status, query)
parse_describe_flush(s, status, query, nil)
end
end

Expand All @@ -2105,7 +2128,7 @@ defmodule Postgrex.Protocol do
defp handle_prepare_execute(%Query{name: ""} = query, params, opts, s) do
status = new_status(opts)

case parse_describe_flush(s, status, query) do
case parse_describe_flush(s, status, query, nil) do
{:ok, query, s} ->
bind_execute_close(s, status, query, params)

Expand Down Expand Up @@ -2396,7 +2419,7 @@ defmodule Postgrex.Protocol do
defp handle_prepare_bind(%Query{name: ""} = query, params, res, opts, s) do
status = new_status(opts)

case parse_describe_flush(s, status, query) do
case parse_describe_flush(s, status, query, nil) do
{:ok, query, s} ->
bind(s, status, query, params, res)

Expand Down
9 changes: 9 additions & 0 deletions test/query_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -1851,6 +1851,15 @@ defmodule QueryTest do
assert [["1", "2"], ["3", "4"]] = query("COPY (VALUES (1, 2), (3, 4)) TO STDOUT", [], opts)
end

test "comment", context do
assert [[123]] = query("select 123", [], comment: "query comment goes here")

assert_raise Postgrex.Error, fn ->
query("select 123", [], comment: "*/ DROP TABLE 123 --")
end
end

@tag :big_binary
Copy link
Contributor Author

@dkuku dkuku Oct 12, 2024

Choose a reason for hiding this comment

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

Makes problems when I log all queries in postgres - this inserts all the data in the log file, I need to have a way to skip it.

test "receive packet with remainder greater than 64MB", context do
# to ensure remainder is more than 64MB use 64MBx2+1
big_binary = :binary.copy(<<1>>, 128 * 1024 * 1024 + 1)
Expand Down
Loading