Skip to content

[query] Adding support to empty map or keyword fields in Query.Builder.where/2 #32

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.

Sign up for GitHub

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

Open
gullitmiranda wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[query] Adding support to empty map or keyword fields in Query.Builder.where/2 #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
49a7777
[connection] Fixing `Connection.Config.runtime/3` get first key inste…
gullitmiranda Dec 28, 2016
1081b1d
[connection] Adding support to env configurations to tests
gullitmiranda Dec 28, 2016
6c2e956
[query] Adding support to empty map or keyword fields in `Query.Build…
gullitmiranda Dec 28, 2016
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
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
# Changelog

## HEAD
Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

This one should probably be v0.15.0-dev to be in line with the history of the file.


- Enhancements
- [connection] Adding support to env configurations to tests
- [query] Adding support to empty map or keyword fields in `Query.Builder.where/2`
Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

All current changelog entries do not use any style of "tagging" for their entries. Would it be possible to reword the new entries to match the file?


- Bug fixes
- [connection] Fixing `Connection.Config.runtime/3` get first key instead of last

## v0.14.0 (2016-12-18)

- Enhancements
Expand Down
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,15 @@ not supported to write them to individual databases. The first point written
defines the database, other values are silently ignored!


## Contributing

##### Custom influxdb test connection
Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

It think this should be relocated under the already existing ### Testing headline. There are already some really basic instructions what is necessary to run the tests. Further information on customizing the test environment probably fit right in.


```
export INSTREAM_HOST=localhost
export INSTREAM_HTTP_PORT=8086
Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

Might it be a good alternative to not encourage exporting variables to the shell environment and instead use single invocation definition?

INSTREAM_HOST=localhost INSTREAM_HTTP_PORT=8086 mix test

However I think this may be completely irrelevant. Someone using a separate server (container, virtual machine, ...) to run the test against should be familiar with both ways.

Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

Looking at the variable names using exports might mess with a generic setup on the machine. But other than a note about predefined testing environment variables in the ### Connections section (referring to the testing section of the README) there should be nothing to worry about.

```

## License

[Apache License, Version 2.0](http://www.apache.org/licenses/LICENSE-2.0)
55 changes: 29 additions & 26 deletions config/test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,81 +5,84 @@ config :logger, :console,
format: "\n$time $metadata[$level] $levelpad$message\n",
metadata: [:query_time, :response_status]

common_host = [
Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

With common_host being available it might be good to also have a common_auth used in the same manner. Going all the way would also mean to use common_pool or similar to remove the repetitive pool configuration.

database: (System.get_env("INSTREAM_DATABASE") || "instream_test"),
Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

Setting the database as a default one should also clean up the tests individually passing one, i.e. Instream.ConnectionTest. Otherwise it would sometimes not work as expected.

host: System.get_env("INSTREAM_HOST") || "localhost",
port: System.get_env("INSTREAM_HTTP_PORT") || 8086,
]

config :instream, Instream.TestHelpers.Connection,
config :instream, Instream.TestHelpers.Connection, common_host ++ [
auth: [ username: "instream_test", password: "instream_test" ],
host: "localhost",
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ]
]

config :instream, Instream.TestHelpers.LogConnection,
config :instream, Instream.TestHelpers.LogConnection, common_host ++ [
auth: [ username: "instream_test", password: "instream_test" ],
host: "localhost",
pool: [ max_overflow: 0, size: 1 ],
port: 8086,
scheme: "http"
]


config :instream, Instream.TestHelpers.EnvConnection,
config :instream, Instream.TestHelpers.EnvConnection, common_host ++ [
auth: [ username: { :system, "INSTREAM_TEST_USERNAME" },
password: { :system, "INSTREAM_TEST_PASSWORD" } ],
host: { :system, "INSTREAM_TEST_HOST" },
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ]
]

config :instream, Instream.TestHelpers.UDPConnection,
config :instream, Instream.TestHelpers.UDPConnection, common_host ++ [
auth: [ username: "instream_test", password: "instream_test" ],
host: "localhost",
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ],
port_udp: 8089,
writer: Instream.Writer.UDP
]


config :instream, Instream.TestHelpers.AnonConnection,
host: "localhost",
config :instream, Instream.TestHelpers.AnonConnection, common_host ++ [
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ]
]

config :instream, Instream.TestHelpers.GuestConnection,
config :instream, Instream.TestHelpers.GuestConnection, common_host ++ [
auth: [ username: "instream_guest", password: "instream_guest" ],
host: "localhost",
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ]
]

config :instream, Instream.TestHelpers.InvalidConnection,
config :instream, Instream.TestHelpers.InvalidConnection, common_host ++ [
auth: [ username: "instream_test", password: "instream_invalid" ],
host: "localhost",
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ]
]

config :instream, Instream.TestHelpers.InvalidDbConnection,
config :instream, Instream.TestHelpers.InvalidDbConnection, common_host ++ [
auth: [ username: "instream_test", password: "instream_test" ],
database: "invalid_test_database",
host: "localhost",
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ]
]

config :instream, Instream.TestHelpers.NotFoundConnection,
config :instream, Instream.TestHelpers.NotFoundConnection, common_host ++ [
auth: [ username: "instream_not_found", password: "instream_not_found" ],
host: "localhost",
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ]
]

config :instream, Instream.TestHelpers.QueryAuthConnection,
config :instream, Instream.TestHelpers.QueryAuthConnection, common_host ++ [
auth: [ method: :query, username: "instream_test", password: "instream_test" ],
host: "localhost",
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ]
]

config :instream, Instream.TestHelpers.UnreachableConnection,
config :instream, Instream.TestHelpers.UnreachableConnection, [
host: "some.really.unreachable.host",
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ]
]


config :instream, Instream.TestHelpers.ConnectionWithOpts,
host: "localhost",
config :instream, Instream.TestHelpers.ConnectionWithOpts, common_host ++ [
loggers: [{ Instream.TestHelpers.NilLogger, :log, [] }],
pool: [ max_overflow: 0, size: 1 ],
http_opts: [ proxy: "http://invalidproxy" ]
]
Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

It really looks a lot cleaner when not copy-pasting stuff around 👍

1 change: 1 addition & 0 deletions lib/instream/connection/config.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ defmodule Instream.Connection.Config do
def runtime(otp_app, conn, keys) do
otp_app
|> Application.get_env(conn, [])
|> Keyword.new
Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

Apart from missing brackets is this really a necessary step? You defined this as a bug fix but from what I could reconstruct it might also be just called a configuration error.

The application configuration done using Mix.Config does not explicitly require a special format, but the most commonly used format is Keyword.t. That means it is possible to have one key defined multiple times with different values:

config :app, [ foo: :bar, foo: :baz ]

Calling Keyword.new([ foo: :bar, foo: :baz ]) guarantees the keys are unique and therefore overwrites the first value with the second. So the configuration value for :foo would be :baz. Your commit wording however said "get first key instead of last" and that seems like the complete opposite of what is actually happening here.

Can you provide a configuration that is problematic without adding this line? If there is a real bug I would extract it and create a patch release for just that fix.

|> maybe_fetch_deep(keys)
|> maybe_fetch_system()
|> maybe_use_default(keys)
Expand Down
3 changes: 3 additions & 0 deletions lib/instream/query/builder.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ defmodule Instream.Query.Builder do
Builds a `WHERE` query expression.
"""
@spec where(t, map) :: t
def where(query, nil ), do: query
def where(query, [] ), do: query
def where(query, fields) when fields == %{}, do: query
Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

I am unsure if this is the right place for this change. When only looking at some code I would expect the where function to always modify that part of the query. For example consider this situation:

query_limited = BuilderSeries |> Builder.from() |> Builder.where(%{ answer 42 })
query_unlimited = query_limited |> Builder.where(nil)

What would one expect the second query does? With your change it is the same, as passing an empty-ish value is a no-op. Before it would clear the currently defined where parameters of the query.

I think this behavior should be handled by Instream.Encoder.InfluxQL. Currently there is support for nil (that is, no WHERE clause) and filled maps. The above example would remove the WHERE statement from the query if it would have been set before. Empty maps are broken at the moment and definitely need a fix (to behave like having nil set).

Apart from that I think the contract should stick with maps. Allowing a Keyword.t here would break later in the encoder (append_where/2 calls Map.keys/1 explicitly). Having the possibility of duplicate keys could also create queries that would never return a result:

BuilderSeries
|> Builder.from()
|> Builder.where([ answer: 42, answer: 24 ])
|> InfluxQL.encode()

# "SELECT * FROM some_measurement WHERE answer=42 AND answer=24

The current QueryBuilder is completely oblivious of anything else than AND combinations. At least I find the time to attack the macro rewrite of that (see #18 for reference).

Copy link
Contributor

@jeffdeville jeffdeville May 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm in favor of this change, as it's consistent with the way query building works in other languages. The use case I'm thinking of is that of delegating query creation to code with different responsibilities. If one simply chooses not to alter the query, this allows them to do so.

If the way it works now is simply to clear what was there before, then there's no possibility of composition.

That said, I'm unclear as to what's going on with the other changes in the PR. If they're a concern, perhaps it could be split into 2 separate PR's?

def where(query, fields), do: set_argument(query, :where, fields)

@doc """
Expand Down
8 changes: 4 additions & 4 deletions mix.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
"excoveralls": {:hex, :excoveralls, "0.5.7", "5d26e4a7cdf08294217594a1b0643636accc2ad30e984d62f1d166f70629ff50", [:mix], [{:exjsx, "~> 3.0", [hex: :exjsx, optional: false]}, {:hackney, ">= 0.12.0", [hex: :hackney, optional: false]}]},
"exjsx": {:hex, :exjsx, "3.2.1", "1bc5bf1e4fd249104178f0885030bcd75a4526f4d2a1e976f4b428d347614f0f", [:mix], [{:jsx, "~> 2.8.0", [hex: :jsx, optional: false]}]},
"hackney": {:hex, :hackney, "1.6.3", "d489d7ca2d4323e307bedc4bfe684323a7bf773ecfd77938f3ee8074e488e140", [:mix, :rebar3], [{:certifi, "0.7.0", [hex: :certifi, optional: false]}, {:idna, "1.2.0", [hex: :idna, optional: false]}, {:metrics, "1.0.1", [hex: :metrics, optional: false]}, {:mimerl, "1.0.2", [hex: :mimerl, optional: false]}, {:ssl_verify_fun, "1.1.1", [hex: :ssl_verify_fun, optional: false]}]},
"idna": {:hex, :idna, "1.2.0", "ac62ee99da068f43c50dc69acf700e03a62a348360126260e87f2b54eced86b2", [], []},
"idna": {:hex, :idna, "1.2.0", "ac62ee99da068f43c50dc69acf700e03a62a348360126260e87f2b54eced86b2", [:rebar3], []},
"jsx": {:hex, :jsx, "2.8.1", "1453b4eb3615acb3e2cd0a105d27e6761e2ed2e501ac0b390f5bbec497669846", [:mix, :rebar3], []},
"metrics": {:hex, :metrics, "1.0.1", "25f094dea2cda98213cecc3aeff09e940299d950904393b2a29d191c346a8486", [], []},
"mimerl": {:hex, :mimerl, "1.0.2", "993f9b0e084083405ed8252b99460c4f0563e41729ab42d9074fd5e52439be88", [], []},
"metrics": {:hex, :metrics, "1.0.1", "25f094dea2cda98213cecc3aeff09e940299d950904393b2a29d191c346a8486", [:rebar3], []},
"mimerl": {:hex, :mimerl, "1.0.2", "993f9b0e084083405ed8252b99460c4f0563e41729ab42d9074fd5e52439be88", [:rebar3], []},
"poison": {:hex, :poison, "3.0.0", "625ebd64d33ae2e65201c2c14d6c85c27cc8b68f2d0dd37828fde9c6920dd131", [:mix], []},
"poolboy": {:hex, :poolboy, "1.5.1", "6b46163901cfd0a1b43d692657ed9d7e599853b3b21b95ae5ae0a777cf9b6ca8", [], []},
"poolboy": {:hex, :poolboy, "1.5.1", "6b46163901cfd0a1b43d692657ed9d7e599853b3b21b95ae5ae0a777cf9b6ca8", [:rebar], []},
Copy link
Owner

@mneudert mneudert Dec 31, 2016

Choose a reason for hiding this comment

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

This file should not be changed in this pull request as the dependency requirements have not changed.

"ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.1", "28a4d65b7f59893bc2c7de786dec1e1555bd742d336043fe644ae956c3497fbe", [:make, :rebar], []}}
6 changes: 4 additions & 2 deletions test/instream/connection/config_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,13 @@ defmodule Instream.Connection.ConfigTest do
test "system configuration connection" do
assert nil == EnvConnection.config([ :host ])

System.put_env("INSTREAM_TEST_HOST", "localhost")
host = System.get_env("INSTREAM_HOST") || "localhost"

System.put_env("INSTREAM_TEST_HOST", host)
System.put_env("INSTREAM_TEST_PASSWORD", "instream_test")
System.put_env("INSTREAM_TEST_USERNAME", "instream_test")

assert "localhost" == EnvConnection.config([ :host ])
assert host == EnvConnection.config([ :host ])
assert :pong == EnvConnection.ping()

System.delete_env("INSTREAM_TEST_HOST")
Expand Down
20 changes: 20 additions & 0 deletions test/instream/query/builder_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,26 @@ defmodule Instream.Query.BuilderTest do
assert query_select == "SELECT * FROM some_measurement"
end

test "SELECT * using WHERE with empty fields" do
keyword_query =
BuilderSeries
|> Builder.from()
|> Builder.where([])
|> InfluxQL.encode()

map_query =
BuilderSeries
|> Builder.from()
|> Builder.where(%{})
|> InfluxQL.encode()

should_query = "SELECT * FROM some_measurement"

assert keyword_query == map_query
assert keyword_query == should_query
assert map_query == should_query
end

test "SELECT * WHERE foo = bar" do
fields = %{ binary: "value", numeric: 42 }
query =
Expand Down